Re: [PATCH v2 1/2] mtd: nand: Check ONFI timings have been acked by the chip

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

Hi Boris,

On Mon, 12 Feb 2018 13:36:05 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Fri, 19 Jan 2018 10:25:46 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > Choosing ONFI timings when ->onfi_set/get_features() calls are supported
> > by the NAND chip is a matter of reading the chip's ONFI parameter page
> > and telling the chip the chosen mode (between all of the supported ones)
> > with ->onfi_set_feature().
> > 
> > Add a check on whether the chip "acked" the timing mode or not.
> > 
> > This can be a problem for NAND chips that do not follow entirely the
> > ONFI specification. These chips actually support other modes than
> > "mode 0", but do not update the parameter page once a timing mode has
> > been selected. This issue will be addressed in another patch that will
> > add the feature to overwrite NAND chips features within the parameter
> > page, from the NAND chip driver.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 41 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 96c97588e1ba..e209a65a17e0 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1231,15 +1231,47 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
> >  			chip->onfi_timing_mode_default,
> >  		};
> >  
> > +		chip->select_chip(mtd, chipnr);
> >  		ret = chip->onfi_set_features(mtd, chip,
> >  				ONFI_FEATURE_ADDR_TIMING_MODE,
> >  				tmode_param);
> > +		chip->select_chip(mtd, -1);
> >  		if (ret)
> > -			goto err;
> > +			return ret;
> >  	}
> >  
> >  	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
> > -err:
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (chip->onfi_version &&
> > +	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
> > +	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
> > +		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {};
> > +
> > +		chip->select_chip(mtd, chipnr);
> > +		ret = chip->onfi_get_features(mtd, chip,
> > +					      ONFI_FEATURE_ADDR_TIMING_MODE,
> > +					      tmode_param);
> > +		chip->select_chip(mtd, -1);
> > +		if (ret)
> > +			goto err_reset_chip;
> > +
> > +		if (tmode_param[0] != chip->onfi_timing_mode_default) {
> > +			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
> > +				chip->onfi_timing_mode_default);
> > +			goto err_reset_chip;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +err_reset_chip:
> > +	onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0);
> > +	chip->select_chip(mtd, chipnr);
> > +	nand_reset_op(chip);
> > +	chip->select_chip(mtd, -1);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -2738,10 +2770,8 @@ int nand_reset(struct nand_chip *chip, int chipnr)
> >  	if (ret)
> >  		return ret;
> >  
> > -	chip->select_chip(mtd, chipnr);
> >  	chip->data_interface = saved_data_intf;
> >  	ret = nand_setup_data_interface(chip, chipnr);
> > -	chip->select_chip(mtd, -1);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -6571,10 +6601,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  
> >  	/* Enter fastest possible mode on all dies. */
> >  	for (i = 0; i < chip->numchips; i++) {
> > -		chip->select_chip(mtd, i);
> >  		ret = nand_setup_data_interface(chip, i);
> > -		chip->select_chip(mtd, -1);  
> 
> Can you split this patch in 2: one that moves the ->select_chip() calls
> to nand_setup_data_interface() and a second one that calls
> ->get_features() to make sure the new timing mode has been applied.  

Done. I will send a new series with these patches plus the one that
simplify GPMI timings derivation (untouched).

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com