RE: [[LINUX PATCH v10] 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Naga Sureshkumar Relli]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal@xxxxxxxxxxx]
> Sent: Thursday, June 28, 2018 12:45 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: boris.brezillon@xxxxxxxxxxx; richard@xxxxxx; dwmw2@xxxxxxxxxxxxx;
> computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; f.fainelli@xxxxxxxxx;
> mmayer@xxxxxxxxxxxx; rogerq@xxxxxx; ladis@xxxxxxxxxxxxxx; ada@xxxxxxxxxxx;
> honghui.zhang@xxxxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> nagasureshkumarrelli@xxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>
> 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.