Hi Miquel,
> -----Original Message-----
> From: Miquel Raynal [mailto:[email protected]]
> Sent: Thursday, June 28, 2018 12:45 PM
> To: Naga Sureshkumar Relli <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected];
> [email protected]; Michal Simek <[email protected]>
> Subject: Re: [[LINUX PATCH v10] 4/4] mtd: rawnand: pl353: Add basic driver
> for arm
> pl353 smc nand interface
>
> Hi Naga,
>
> > > > +/**
> > > > + * pl353_nand_read_page_hwecc - Hardware ECC based page read function
> > > > + * @mtd: Pointer to the mtd info structure
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @buf: Pointer to the buffer to store read data
> > > > + * @oob_required: Caller requires OOB data read to chip->oob_poi
> > > > + * @page: Page number to read
> > > > + *
> > > > + * This functions reads data and checks the data integrity by
> > > > +comparing hardware
> > > > + * generated ECC values and read ECC values from spare area.
> > > > + *
> > > > + * Return: 0 always and updates ECC operation status in to MTD
> > > > structure
> > > > + */
> > > > +static int pl353_nand_read_page_hwecc(struct mtd_info *mtd,
> > > > + struct nand_chip *chip,
> > > > + u8 *buf, int oob_required, int
> > > > page) {
> > > > + int i, stat, eccsize = chip->ecc.size;
> > > > + int eccbytes = chip->ecc.bytes;
> > > > + int eccsteps = chip->ecc.steps;
> > > > + u8 *p = buf;
> > > > + u8 *ecc_calc = chip->ecc.calc_buf;
> > > > + u8 *ecc = chip->ecc.code_buf;
> > > > + unsigned int max_bitflips = 0;
> > > > + u8 *oob_ptr;
> > > > + u32 ret;
> > > > + unsigned long data_phase_addr;
> > > > + struct pl353_nand_info *xnfc =
> > > > + container_of(chip, struct pl353_nand_info, chip);
> > > > + unsigned long nand_offset = (unsigned long
> > > > +__force)xnfc->nand_base;
> > > > +
> > > > + pl353_prepare_cmd(mtd, chip, page, 0, NAND_CMD_READ0,
> > > > + NAND_CMD_READSTART, 1);
> > > > + ndelay(100);
> > >
> > > What is this delay for?
> > We have seen failures with out this delay, with older code.
> > But i will check this by removing this delay, in this new driver.
>
> Please check all of them. We should get rid of random delays like that.
> Either there is something to poll, or there is a specific value to use (you
> can get them from the
> SDR interface structure).
>
> [...]
>
> > > > +
> > > > + if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> > > > + return -EINVAL;
> > >
> > > Why?
> > It is similar to
> > if (chipnr < 0)
> > return 0;
>
> Mmmmmh, no?
>
> (return 0) != (return -EINVAL)
>
> The core is asking you to check if the controller driver support particular
> timings (usually
> ONFI modes 1-5). Returning an error means "I only support the slowest
> timings" which, I
> suppose, is wrong. Please fix this and compare the speeds.
I tried updating the driver as per your comments.
But I am facing an issue here.
The part I am using is http://www.cypress.com/file/207521/download.
This part doesn't support get/set features. But the controller supports it.
In this case, the frame work is doing like this
If chip supports set_features, then it issues the ONFI_FEATURE_ADDR_TIMING_MODE
other wise not.
In our case it won't and then next it simply changes the controller mode. Hence
both are in different timing mode and not
Able to communicate with the nand flash device.
https://elixir.bootlin.com/linux/v4.18-rc3/source/drivers/mtd/nand/raw/nand_base.c#L1285
Am I missing something?
Could you please help on this.
The code snippet is like this
tatic int pl353_setup_data_interface(struct mtd_info *mtd, int csline,
const struct nand_data_interface *conf)
{
sdr = nand_get_sdr_timings(conf);
if (IS_ERR(sdr)) {
return PTR_ERR(sdr);
}
if (csline == NAND_DATA_IFACE_CHECK_ONLY) {
return 0;
}
}
Thanks,
Naga Sureshkumar Relli.
