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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Naga,

[...]

> > > > > +
> > > > > +	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.

Yes because some NAND parts are broken and do not support set feature
while working naturally at an higher rate than the slowest default one.

> In our case it won't and then next it simply changes the controller mode. Hence both are in different timing mode and not

If you look at the table 3.4 of your NAND part specification, you can
see that it supports mode 0-4. Hence the core will suppose it can
switch the controller timings to mode 4. Now, is your controller able
to support these speeds? Maybe it only supports non-EDO modes (0-3)
and mode 4 cannot be achieved?

> 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.

Thanks,
Miquèl