Re: [PATCH v5 14/28] mtd: rawnand: timings: Add a helper to find the closest ONFI mode

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

Hi Boris,

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

> On Tue, 26 May 2020 21:17:11 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Vendors are allowed to provide their own set of timings. In this case,
> > we provide a way to derive the "closest" timing mode so that, if the
> > NAND controller does not support tweaking these parameters, it will be
> > able to configure itself anyway.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/internals.h    |  2 ++
> >  drivers/mtd/nand/raw/nand_timings.c | 49 +++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> > index dc84e3b55d48..c7357ae86eeb 100644
> > --- a/drivers/mtd/nand/raw/internals.h
> > +++ b/drivers/mtd/nand/raw/internals.h
> > @@ -88,6 +88,8 @@ int onfi_fill_data_interface(struct nand_chip *chip,
> >  			     struct nand_data_interface *iface,
> >  			     enum nand_data_interface_type type,
> >  			     int timing_mode);
> > +unsigned int
> > +onfi_find_closest_sdr_mode(const 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_timings.c b/drivers/mtd/nand/raw/nand_timings.c
> > index ce6bb87db2e8..3c44c4b90536 100644
> > --- a/drivers/mtd/nand/raw/nand_timings.c
> > +++ b/drivers/mtd/nand/raw/nand_timings.c
> > @@ -273,6 +273,55 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> >  	},
> >  };
> >  
> > +/**
> > + * onfi_find_closest_sdr_mode - Derive the closest ONFI SDR timing mode given a
> > + *                              set of timings
> > + * @spec_timings: the timings to challenge
> > + */
> > +unsigned int
> > +onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings)
> > +{
> > +	const struct nand_sdr_timings *onfi_timings;
> > +	int mode;
> > +
> > +	for (mode = ARRAY_SIZE(onfi_sdr_timings) - 1; mode > 0; mode--) {
> > +		onfi_timings = &onfi_sdr_timings[mode].timings.sdr;
> > +
> > +		if (spec_timings->tCCS_min > onfi_timings->tCCS_min ||
> > +		    spec_timings->tADL_min > onfi_timings->tADL_min ||
> > +		    spec_timings->tALH_min > onfi_timings->tALH_min ||
> > +		    spec_timings->tALS_min > onfi_timings->tALS_min ||
> > +		    spec_timings->tAR_min > onfi_timings->tAR_min ||
> > +		    spec_timings->tCEH_min > onfi_timings->tCEH_min ||
> > +		    spec_timings->tCH_min > onfi_timings->tCH_min ||
> > +		    spec_timings->tCLH_min > onfi_timings->tCLH_min ||
> > +		    spec_timings->tCLR_min > onfi_timings->tCLR_min ||
> > +		    spec_timings->tCLS_min > onfi_timings->tCLS_min ||
> > +		    spec_timings->tCOH_min > onfi_timings->tCOH_min ||
> > +		    spec_timings->tCS_min > onfi_timings->tCS_min ||
> > +		    spec_timings->tDH_min > onfi_timings->tDH_min ||
> > +		    spec_timings->tDS_min > onfi_timings->tDS_min ||
> > +		    spec_timings->tIR_min > onfi_timings->tIR_min ||
> > +		    spec_timings->tRC_min > onfi_timings->tRC_min ||
> > +		    spec_timings->tREH_min > onfi_timings->tREH_min ||
> > +		    spec_timings->tRHOH_min > onfi_timings->tRHOH_min ||
> > +		    spec_timings->tRHW_min > onfi_timings->tRHW_min ||
> > +		    spec_timings->tRLOH_min > onfi_timings->tRLOH_min ||
> > +		    spec_timings->tRP_min > onfi_timings->tRP_min ||
> > +		    spec_timings->tRR_min > onfi_timings->tRR_min ||
> > +		    spec_timings->tWC_min > onfi_timings->tWC_min ||
> > +		    spec_timings->tWH_min > onfi_timings->tWH_min ||
> > +		    spec_timings->tWHR_min > onfi_timings->tWHR_min ||
> > +		    spec_timings->tWP_min > onfi_timings->tWP_min ||
> > +		    spec_timings->tWW_min > onfi_timings->tWW_min)  
> 
> Just nitpicking but you could just do the opposite test ('<=' instead of
> '>' and '&&' instead of '||') and return mode directly, so you don't  
> need this continue.

I did it because I wanted the continue; statement to be called as soon
as one test was "true" (ie. wrong in this case) but actually it would
work the same with a <= and a &&, so I'll switch.

> I didn't check if all the _min timings were tested, but assuming that's
> the case, you can add

I copy/pasted the entire structure definition so unless I messed with
it, it should be fine.

> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> > +			continue;
> > +
> > +		return mode;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * onfi_fill_data_interface - Initialize a data interface from a given ONFI mode
> >   * @chip: The NAND chip  
> 


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