Re: [PATCH v2 13/22] mtd: rawnand: Pass the CS line to be selected in struct nand_operation

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hi Janusz,

On Sat, 10 Nov 2018 02:30:04 +0100
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Hi Boris,
> 
> On Tuesday, November 6, 2018 4:08:01 PM CET Boris Brezillon wrote:
> > In order to deprecate the ->select_chip hook we need to pass the CS
> > line a NAND operations are targeting. This is done through the
> > addition of a cs field to the nand_operation struct.
> > 
> > We also need to keep track of the currently selected target to
> > properly initialize op->cs, hence the ->cur_cs field addition to the
> > nand_chip struct.
> > 
> > Note that op->cs is not assigned in nand_exec_op() because we might
> > rework the way we execute NAND operations in the future (adopt a
> > queuing mechanism instead of the serialization we have right now).
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > Changes in v2:
> > - Initialize ->cur_cs
> > ---
> >  drivers/mtd/nand/raw/internals.h  |  3 +++
> >  drivers/mtd/nand/raw/nand_base.c  | 39 +++++++++++++++++--------------
> >  drivers/mtd/nand/raw/nand_hynix.c |  4 ++--
> >  include/linux/mtd/rawnand.h       | 11 ++++++++-
> >  4 files changed, 37 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> > index 6e2f61fbc5f0..b62728d5884b 100644
> > --- a/drivers/mtd/nand/raw/internals.h
> > +++ b/drivers/mtd/nand/raw/internals.h
> > @@ -101,6 +101,9 @@ static inline int nand_exec_op(struct nand_chip *chip,
> >  	if (!chip->exec_op)
> >  		return -ENOTSUPP;
> >  
> > +	if (WARN_ON(op->cs >= chip->numchips))
> > +		return -EINVAL;  
> 
> This needs a fix to nand_scan_ident() like the following:
> 
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5034,10 +5034,11 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
>  
>  	/* Set the default functions */
>  	nand_set_defaults(chip);
>  
>  	/* Read the flash type */
> +	chip->numchips = 1;

Oh, you're right. Actually it should be

	chip->numchips = maxchips;

and should be done in patch 12.

>  	ret = nand_detect(chip, table);
>  	if (ret) {
>  		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
>  			pr_warn("No NAND device found\n");
>  		nand_deselect_target(chip);
> 
> 
> Otherwise nand_detect() fails for me in nand_exec_op() called from the very 
> first nand_reset_op():
> 
>     WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/internals.h:104 nand_reset_op+0x100/0x17c
>     ...
>     nand: No NAND device found
> 
> With that issue fixed one way or another, you can add my:
> 
> 	Tested-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> 
> to your whole series.

Thanks for testing.

> 
> Thanks,
> Janusz
> 
> 
> PS.
> I'm ready to submit a series which converts ams-delta-nand to use GPIO 
> API for data I/O.  My only concerns is which branch should I rebase it on.

I'd say nand/next, unless you have dependencies on new GPIO stuff that
are not in 4.20-rc1.

Regards,

Boris