[PATCH v3 2/4] mtd: rawnand: gpmi: Add strict ecc strength check

[Miquel Raynal]

Hi Han,

han.xu at nxp.com wrote on Fri, 8 Apr 2022 17:05:41 -0500:

> On 22/04/05 09:28AM, Miquel Raynal wrote:
> > Hi Han,
> > 
> > han.xu at nxp.com wrote on Mon,  4 Apr 2022 14:54:25 -0500:
> >   
> > > Add strict ecc strength check in gpmi_check_ecc() function, which is
> > > same as nand_ecc_is_strong_enough() did. It will check both correct bits
> > > and correct bits per byte to ensure it meets chip required ecc strength.
> > > 
> > > Signed-off-by: Han Xu <han.xu at nxp.com>
> > > 
> > > ---
> > > Changes since v2:
> > >  - split the ecc check to a single patch
> > > ---
> > > ---
> > >  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 24 ++++++++++++++++++++--
> > >  1 file changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > index 4144d5937103..9a37f8cc663e 100644
> > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > @@ -238,9 +238,14 @@ static void gpmi_dump_info(struct gpmi_nand_data *this)
> > >  		geo->block_mark_bit_offset);
> > >  }
> > >  
> > > -static inline bool gpmi_check_ecc(struct gpmi_nand_data *this)
> > > +static bool gpmi_check_ecc(struct gpmi_nand_data *this)  
> > 
> > This change should be in a separate commit.
> >   
> > >  {
> > > +	struct nand_chip *chip = &this->nand;
> > > +	struct mtd_info *mtd = nand_to_mtd(&this->nand);
> > >  	struct bch_geometry *geo = &this->bch_geometry;
> > > +	const struct nand_ecc_props *requirements =
> > > +		nanddev_get_ecc_requirements(&chip->base);
> > > +	int corr, ds_corr;
> > >  
> > >  	/* Do the sanity check. */
> > >  	if (GPMI_IS_MXS(this)) {
> > > @@ -248,7 +253,22 @@ static inline bool gpmi_check_ecc(struct gpmi_nand_data *this)
> > >  		if (geo->gf_len == 14)
> > >  			return false;
> > >  	}
> > > -	return geo->ecc_strength <= this->devdata->bch_max_ecc_strength;
> > > +
> > > +	if (geo->ecc_strength > this->devdata->bch_max_ecc_strength)
> > > +		return false;
> > > +
> > > +	/* check ecc strength, same as nand_ecc_is_strong_enough() did */  
> > 
> > Why not using nand_ecc_is_strong_enough() in this case?  
> 
> Hi Miquèl,
> 
> 1. current nand_ecc_is_strong_enough() compared required ecc with conf ecc, but
> I only see conf was initialized in some spi-nand and sw ecc drivers. Not sure if
> it's a issue or someting missed in the gpmi driver? On my side,
> nand_ecc_is_strong_enough() just returned.

It was indeed meant to be used primarily by the new ECC engine
abstraction, but it's best not to duplicate this logic over and over
again, so if filling a nand_ecc_props structure is all it takes to be
able to use this helper, it's probably worth trying?

> 2. I remembered that nand_ecc_is_strong_enough() used to compare required ecc
> with nand_ecc_ctrl ecc strength, but even in this case, I don't know if it's a
> good idea just directly call this function. Usually the driver looking for a
> proper bch setting and get the ecc strength/step, at last set the nand_ecc_ctrl
> . So at this moment, it's not ready to use nand_ecc_is_strong_enough().
> >   
> > > +	if (requirements->step_size) {
> > > +		corr = mtd->writesize * geo->ecc_strength /
> > > +		       geo->ecc_chunk_size;
> > > +		ds_corr = mtd->writesize * requirements->strength /
> > > +			  requirements->step_size;
> > > +		if (corr < ds_corr ||
> > > +		    geo->ecc_strength < requirements->strength)
> > > +			return false;
> > > +	}
> > > +
> > > +	return true;
> > >  }
> > >  
> > >  /*  
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl