diff mbox series

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

Message ID 20220404195427.8871-2-han.xu@nxp.com
State Changes Requested
Headers show
Series [v3,1/4] mtd: rawnand: gpmi: Refactor bch geometry settings function | expand

Commit Message

Han Xu April 4, 2022, 7:54 p.m. UTC 
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@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(-)

Comments

Miquel Raynal April 5, 2022, 7:28 a.m. UTC | #1 
Hi Han,

han.xu@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@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?

> +	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
Han Xu April 8, 2022, 10:05 p.m. UTC | #2 
On 22/04/05 09:28AM, Miquel Raynal wrote:
> Hi Han,
> 
> han.xu@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@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.

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
Miquel Raynal April 11, 2022, 7:30 a.m. UTC | #3 
Hi Han,

han.xu@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@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@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
diff mbox series

Patch

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)
 {
+	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 */
+	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;
 }
 
 /*