Re: [RFC 03/12] mtd: nand: use a static data_interface in the nand_chip structure

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hi Boris,

> > Change the data_interface field from the nand_chip structure,
> > convert the pointer to a static structure.  
> 
> You mean "embed a nand_data_interface object". Not sure the static
> specifier can be used to describe what you do.

Ok.

> > Also remove the nand_get_default_data_interface() function that
> > become useless and rename the onfi_init_data_interface() by
> > nand_fill_data_interface(),  
> 
> I know I'm the one who suggested to rename this function, but I'm not
> so sure it's a good idea in the end, and I don't like the new
> prototype where you drop the iface_type parameter.

No problem, I will just rename it "onfi_fill_data_interface" because
there is no more real "initialization", it is just a dumb function
that fills the structure with the appropriate timings (I found the
name too close to "nand_init_data_interface" that really does
something). But ok, let's keep the iface_type parameter.

> 
> > which is a more appropriate name because
> > applied timings are ONFI, no matter if the NAND actually is one.  
> 
> Well, this is actually a good reason to keep the onfi_ prefix. A NAND
> driver can decide that its NAND is close to an existing timing mode
> and re-use this onfi_init_data_interface(), but maybe someday we'll
> have drivers filling the nand_data_interface object with their own
> (non-ONFI) timings.
> 
> > 
> > This is needed before passing to ->exec_op() to avoid any race
> > that  
> 
> It's not really a race, it's just that you need some timings during
> NAND detection, and the core is currently creating the
> nand_data_interface object after the detection step (in
> nand_scan_tail()).

I changed the commit message.


> > @@ -1107,7 +1107,6 @@ static int nand_wait(struct mtd_info *mtd,
> > struct nand_chip *chip) static int nand_reset_data_interface(struct
> > nand_chip *chip, int chipnr) {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > -	const struct nand_data_interface *conf;
> >  	int ret;
> >  
> >  	if (!chip->setup_data_interface)
> > @@ -1127,8 +1126,8 @@ static int nand_reset_data_interface(struct
> > nand_chip *chip, int chipnr)
> >  	 * timings to timing mode 0.
> >  	 */
> >  
> > -	conf = nand_get_default_data_interface();
> > -	ret = chip->setup_data_interface(mtd, chipnr, conf);
> > +	nand_fill_data_interface(chip, 0);
> > +	ret = chip->setup_data_interface(mtd, chipnr,
> > &chip->data_interface); if (ret)
> >  		pr_err("Failed to configure data interface to SDR
> > timing mode 0\n"); 
> 
> So now you're stuck in timing mode 0 as soon as you have nand_reset()
> called.
> 
> Actually, what we should do is:
> 
> 1/ at the beginning of nand_scan_ident(), call
>    onfi_init_data_interface(chip, &chip->data_interface,
> NAND_SDR_IFACE, 0) (or a wrapper that does that) so that
> data_interface is initialized with the appropriate timings for
> reset/detection operations 2/ after detection, find the best iface
> type and timing mode we can use (what's currently done in
> nand_init_data_interface()) 3/ in nand_reset(), you should save
> what's in chip->data_interface, in a local variable, then apply
> SDR+timing-mode-0 and finally restore the previous settings (the one
> you stored in your local variable) after the RESET op.
> 
> This way you're guaranteed to always end up with the best data iface
> settings after a RESET.

Clear explanation, this is corrected.

Thank you,
Miquèl