Re: [PATCH v5 16/28] mtd: rawnand: timings: Use default values for tPROG_max and tBERS_max

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Miquel,

Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote on Wed, 27 May 2020
09:17:58 +0200:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Tue, 26 May
> 2020 23:25:30 +0200:
> 
> > On Tue, 26 May 2020 21:17:13 +0200
> > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >   
> > > The ONFI parameter page of a chip might define more fine grained
> > > tPROG_max and tBERS_max. When we do not have this information, we
> > > default to the highest possible values (they are maxima anyway).
> > > 
> > > There is no point setting these fields at runtime, so explicitly move
> > > these defaults to the main ONFI SDR timings structure.    
> > 
> > Okay, now I see why you're doing that: you want to return the mode 0
> > entry directly, and it makes sense, but it should be explained in the
> > commit message.  
> 
> Okay I will mention it in the other commit message, and perhaps move
> the comment about them as well so that it does not get lost.
> 
> I can also assign these timings manually for mode 0 in the helper you
> requested.

Actually I don't like this idea which would mean changing the content
of the timings structure itself...

I would prefer to move these 4 values to the structure (as I already
do) plus moving the comment explaining the derivations.

> 
> What do you prefer?
> 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > > ---
> > >  drivers/mtd/nand/raw/nand_timings.c | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_timings.c b/drivers/mtd/nand/raw/nand_timings.c
> > > index c2286a75d134..52ee83e75646 100644
> > > --- a/drivers/mtd/nand/raw/nand_timings.c
> > > +++ b/drivers/mtd/nand/raw/nand_timings.c
> > > @@ -20,6 +20,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 20000,
> > >  			.tALS_min = 50000,
> > > @@ -63,6 +65,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 10000,
> > >  			.tALS_min = 25000,
> > > @@ -106,6 +110,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 10000,
> > >  			.tALS_min = 15000,
> > > @@ -149,6 +155,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 5000,
> > >  			.tALS_min = 10000,
> > > @@ -192,6 +200,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 5000,
> > >  			.tALS_min = 10000,
> > > @@ -235,6 +245,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
> > >  		.timings.sdr = {
> > >  			.tCCS_min = 500000,
> > >  			.tR_max = 200000000,
> > > +			.tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > > +			.tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX,
> > >  			.tADL_min = 400000,
> > >  			.tALH_min = 5000,
> > >  			.tALS_min = 10000,
> > > @@ -359,18 +371,6 @@ int onfi_fill_data_interface(struct nand_chip *chip,
> > >  
> > >  		/* nanoseconds -> picoseconds */
> > >  		timings->tCCS_min = 1000UL * onfi->tCCS;
> > > -	} else {
> > > -		struct nand_sdr_timings *timings = &iface->timings.sdr;
> > > -		/*
> > > -		 * For non-ONFI chips we use the highest possible value for
> > > -		 * tPROG and tBERS. tR and tCCS will take the default values
> > > -		 * precised in the ONFI specification for timing mode 0,
> > > -		 * respectively 200us and 500ns.
> > > -		 */
> > > -
> > > -		/* microseconds -> picoseconds */
> > > -		timings->tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX;
> > > -		timings->tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX;
> > >  	}
> > >  
> > >  	return 0;    
> >   


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