Re: [PATCH v5 21/28] mtd: rawnand: Introduce nand_choose_best_sdr_timings()

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

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Tue, 26 May
2020 23:50:03 +0200:

> On Tue, 26 May 2020 21:17:18 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Extract the logic out of nand_choose_data_interface() to create a
> > public helper that can be reused by manufacturer drivers. Add the
> > possibility to provide a specific set of timings.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/internals.h |   3 +
> >  drivers/mtd/nand/raw/nand_base.c | 104 +++++++++++++++++++------------
> >  2 files changed, 66 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> > index c7357ae86eeb..5c49c4d1e231 100644
> > --- a/drivers/mtd/nand/raw/internals.h
> > +++ b/drivers/mtd/nand/raw/internals.h
> > @@ -90,6 +90,9 @@ int onfi_fill_data_interface(struct nand_chip *chip,
> >  			     int timing_mode);
> >  unsigned int
> >  onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings);
> > +int nand_choose_best_sdr_timings(struct nand_chip *chip,
> > +				 struct nand_data_interface *iface,
> > +				 struct nand_sdr_timings *spec_timings);
> >  int nand_get_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
> >  int nand_set_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
> >  int nand_read_page_raw_notsupp(struct nand_chip *chip, u8 *buf,
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 6a7edd2e1357..b2e833a8ab9d 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -1003,61 +1003,83 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
> >  	return ret;
> >  }
> >  
> > +/**
> > + * nand_choose_best_sdr_timings - Pick up the best SDR timings that both the
> > + *                                NAND controller and the NAND chip support
> > + * @chip: the NAND chip
> > + * @iface: the data interface (can eventually be updated)
> > + * @spec_timings: specific timings, when not fitting the ONFI specification
> > + *
> > + * If specific timings are provided, use them. Otherwise, try to retrieve
> > + * supported timing modes from ONFI information. Finally, if the NAND chip does
> > + * not follow the ONFI specification, rely on the ->default_timing_mode
> > + * specified in the nand_ids table.
> > + */
> > +int nand_choose_best_sdr_timings(struct nand_chip *chip,
> > +				 struct nand_data_interface *iface,
> > +				 struct nand_sdr_timings *spec_timings)
> > +{
> > +	const struct nand_controller_ops *ops = chip->controller->ops;
> > +	int best_mode = 0, onfi_modes, mode, ret;
> > +
> > +	iface->type = NAND_SDR_IFACE;
> > +
> > +	if (spec_timings) {
> > +		iface->timings.sdr = *spec_timings;
> > +		iface->timings.mode = onfi_find_closest_sdr_mode(spec_timings);
> > +
> > +		/* Verify the controller supports the requested interface */
> > +		ret = ops->setup_data_interface(chip,
> > +						NAND_DATA_IFACE_CHECK_ONLY,
> > +						iface);
> > +		if (!ret)
> > +			return ret;
> > +
> > +		/* Fallback to slower modes */
> > +		best_mode = iface->timings.mode;
> > +	} else {
> > +		if (chip->parameters.onfi) {  
> 
> onfi_modes var should be declared here.

Ok

> 
> > +			onfi_modes = chip->parameters.onfi->async_timing_mode;
> > +			best_mode = fls(onfi_modes) - 1;
> > +		} else {
> > +			best_mode = chip->onfi_timing_mode_default;
> > +		}
> > +	}
> > +
> > +
> > +	for (mode = best_mode; mode >= 0; mode--) {
> > +		ret = onfi_fill_data_interface(chip, iface, NAND_SDR_IFACE,
> > +					       mode);
> > +		if (ret)
> > +			continue;  
> 
> In practice onfi_fill_data_interface() will only return an error if you
> pass a type that's not SDR (not the case here, or a mode that's outside
> the SDR mode bounds). Maybe we should just clamp the value/warn when
> that happens and make onfi_fill_data_interface() a void function.

Agreed, I'll call WARN_ON() in it and return void. This function should
"never" fail.

> 
> > +
> > +		ret = ops->setup_data_interface(chip,
> > +						NAND_DATA_IFACE_CHECK_ONLY,
> > +						iface);
> > +		if (!ret)
> > +			break;  
> 
> 			return 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * nand_choose_data_interface - find the best data interface and timings
> >   * @chip: The NAND chip
> >   *
> >   * Find the best data interface and NAND timings supported by the chip
> >   * and the driver.
> > - * First tries to retrieve supported timing modes from ONFI information,
> > - * and if the NAND chip does not support ONFI, relies on the
> > - * ->onfi_timing_mode_default specified in the nand_ids table. After this
> > - * function nand_chip->data_interface is initialized with the best timing mode
> > - * available.
> > + * After this function nand_chip->data_interface is initialized with the best
> > + * timing mode available.
> >   *
> >   * Returns 0 for success or negative error code otherwise.
> >   */
> >  static int nand_choose_data_interface(struct nand_chip *chip)
> >  {
> > -	int modes, mode, ret;
> > -
> >  	if (!nand_controller_has_setup_data_iface(chip))
> >  		return 0;
> >  
> > -	/*
> > -	 * First try to identify the best timings from ONFI parameters and
> > -	 * if the NAND does not support ONFI, fallback to the default ONFI
> > -	 * timing mode.
> > -	 */
> > -	if (chip->parameters.onfi) {
> > -		modes = chip->parameters.onfi->async_timing_mode;
> > -	} else {
> > -		if (!chip->onfi_timing_mode_default)
> > -			return 0;
> > -
> > -		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> > -	}
> > -
> > -	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > -		ret = onfi_fill_data_interface(chip, &chip->data_interface,
> > -					       NAND_SDR_IFACE, mode);
> > -		if (ret)
> > -			continue;
> > -
> > -		/*
> > -		 * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
> > -		 * controller supports the requested timings.
> > -		 */
> > -		ret = chip->controller->ops->setup_data_interface(chip,
> > -						 NAND_DATA_IFACE_CHECK_ONLY,
> > -						 &chip->data_interface);
> > -		if (!ret) {
> > -			chip->onfi_timing_mode_default = mode;
> > -			break;
> > -		}
> > -	}
> > -
> > -	return 0;
> > +	return nand_choose_best_sdr_timings(chip, &chip->data_interface, NULL);
> >  }
> >  
> >  /**  
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/