Patchwork [v3,1/3] mtd: nand: omap2: Update nerrors using ecc.strength

login
register
mail settings
Submitter Philip, Avinash
Date Nov. 29, 2012, 11:46 a.m.
Message ID <1354189595-12784-2-git-send-email-avinashphilip@ti.com>
Download mbox | patch
Permalink /patch/202717/
State New
Headers show

Comments

Philip, Avinash - Nov. 29, 2012, 11:46 a.m.
Update number of errors using nand ecc strength.
Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 359293e... 7e61dac... M	drivers/mtd/nand/omap2.c
 drivers/mtd/nand/omap2.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Sekhar Nori - Dec. 5, 2012, 12:03 p.m.
Hi Avinash,

On 11/29/2012 5:16 PM, Philip, Avinash wrote:
> Update number of errors using nand ecc strength.
> Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX

Can you please describe why the original method of setting nerrors was
incorrect? Was it causing any issues in any particular configuration?
Mentioning this will help maintainers assign priority to your patch. If
this is a real bug affecting existing platforms, then there is a chance
this patch can get into v3.7 (or at least into v3.8-rc1).

Like Peter who commented on this before, I am not a fan of using macros
for self-describing constants - especially when you end up using the
same numbers inside the macro name itself. No need to resend any thing
just for this, you can wait to see if the maintainers are okay with it.

Thanks,
Sekhar

> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> ---
> :100644 100644 359293e... 7e61dac... M	drivers/mtd/nand/omap2.c
>  drivers/mtd/nand/omap2.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 359293e..7e61dac 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -118,6 +118,9 @@
>  
>  #define OMAP24XX_DMA_GPMC		4
>  
> +#define BCH8_MAX_ERROR		8	/* upto 8 bit correctable */
> +#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
> +
>  /* oob info generated runtime depending on ecc algorithm and layout selected */
>  static struct nand_ecclayout omap_oobinfo;
>  /* Define some generic bad / good block scan pattern which are used
> @@ -1042,7 +1045,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
>  	struct nand_chip *chip = mtd->priv;
>  	u32 val;
>  
> -	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
> +	nerrors = info->nand.ecc.strength;
>  	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
>  	nsectors = 1;
>  	/*
> @@ -1219,13 +1222,14 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
>  	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>  						   mtd);
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH8
> -	const int hw_errors = 8;
> +	const int hw_errors = BCH8_MAX_ERROR;
>  #else
> -	const int hw_errors = 4;
> +	const int hw_errors = BCH4_MAX_ERROR;
>  #endif
>  	info->bch = NULL;
>  
> -	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ? 8 : 4;
> +	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
> +		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
>  	if (max_errors != hw_errors) {
>  		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
>  		       max_errors, hw_errors);
>
Philip, Avinash - Dec. 5, 2012, 12:43 p.m.
On Wed, Dec 05, 2012 at 17:33:37, Nori, Sekhar wrote:
> Hi Avinash,
> 
> On 11/29/2012 5:16 PM, Philip, Avinash wrote:
> > Update number of errors using nand ecc strength.
> > Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX
> 
> Can you please describe why the original method of setting nerrors was
> incorrect? Was it causing any issues in any particular configuration?

It affects the reusability of the code. For example BCH8 with AM335x RBL 
compatibility requires  14 bytes instead of 13 byte. So setting nerrors
with
	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
will break am335x RBL compatibility.

> Mentioning this will help maintainers assign priority to your patch. If
> this is a real bug affecting existing platforms, then there is a chance
> this patch can get into v3.7 (or at least into v3.8-rc1).
> 
> Like Peter who commented on this before, I am not a fan of using macros
> for self-describing constants - especially when you end up using the
> same numbers inside the macro name itself. No need to resend any thing
> just for this, you can wait to see if the maintainers are okay with it.

Yes I agree. But the same constants are used in multiple places here.
So usage of macro helps in readability & code debugging.

Thanks
Avinash
 
> 
> Thanks,
> Sekhar
> 
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > ---
> > :100644 100644 359293e... 7e61dac... M	drivers/mtd/nand/omap2.c
> >  drivers/mtd/nand/omap2.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 359293e..7e61dac 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -118,6 +118,9 @@
> >  
> >  #define OMAP24XX_DMA_GPMC		4
> >  
> > +#define BCH8_MAX_ERROR		8	/* upto 8 bit correctable */
> > +#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
> > +
> >  /* oob info generated runtime depending on ecc algorithm and layout selected */
> >  static struct nand_ecclayout omap_oobinfo;
> >  /* Define some generic bad / good block scan pattern which are used
> > @@ -1042,7 +1045,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> >  	struct nand_chip *chip = mtd->priv;
> >  	u32 val;
> >  
> > -	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
> > +	nerrors = info->nand.ecc.strength;
> >  	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
> >  	nsectors = 1;
> >  	/*
> > @@ -1219,13 +1222,14 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
> >  	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> >  						   mtd);
> >  #ifdef CONFIG_MTD_NAND_OMAP_BCH8
> > -	const int hw_errors = 8;
> > +	const int hw_errors = BCH8_MAX_ERROR;
> >  #else
> > -	const int hw_errors = 4;
> > +	const int hw_errors = BCH4_MAX_ERROR;
> >  #endif
> >  	info->bch = NULL;
> >  
> > -	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ? 8 : 4;
> > +	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
> > +		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
> >  	if (max_errors != hw_errors) {
> >  		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
> >  		       max_errors, hw_errors);
> > 
>
Sekhar Nori - Dec. 7, 2012, 10:40 a.m.
On 12/5/2012 6:13 PM, Philip, Avinash wrote:
> On Wed, Dec 05, 2012 at 17:33:37, Nori, Sekhar wrote:
>> Hi Avinash,
>>
>> On 11/29/2012 5:16 PM, Philip, Avinash wrote:
>>> Update number of errors using nand ecc strength.
>>> Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX
>>
>> Can you please describe why the original method of setting nerrors was
>> incorrect? Was it causing any issues in any particular configuration?
> 
> It affects the reusability of the code. For example BCH8 with AM335x RBL 
> compatibility requires  14 bytes instead of 13 byte. So setting nerrors
> with
> 	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
> will break am335x RBL compatibility.

This should be summarized in the patch description so the motivation is
clear to those who read the history later.

Thanks,
Sekhar
Philip, Avinash - Dec. 10, 2012, 6:44 a.m.
On Fri, Dec 07, 2012 at 16:10:04, Nori, Sekhar wrote:
> On 12/5/2012 6:13 PM, Philip, Avinash wrote:
> > On Wed, Dec 05, 2012 at 17:33:37, Nori, Sekhar wrote:
> >> Hi Avinash,
> >>
> >> On 11/29/2012 5:16 PM, Philip, Avinash wrote:
> >>> Update number of errors using nand ecc strength.
> >>> Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX
> >>
> >> Can you please describe why the original method of setting nerrors was
> >> incorrect? Was it causing any issues in any particular configuration?
> > 
> > It affects the reusability of the code. For example BCH8 with AM335x RBL 
> > compatibility requires  14 bytes instead of 13 byte. So setting nerrors
> > with
> > 	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
> > will break am335x RBL compatibility.
> 
> This should be summarized in the patch description so the motivation is
> clear to those who read the history later.

Ok I will add the details in patch description in next version.

Thanks
Avinash

> 
> Thanks,
> Sekhar
>

Patch

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 359293e..7e61dac 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -118,6 +118,9 @@ 
 
 #define OMAP24XX_DMA_GPMC		4
 
+#define BCH8_MAX_ERROR		8	/* upto 8 bit correctable */
+#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
+
 /* oob info generated runtime depending on ecc algorithm and layout selected */
 static struct nand_ecclayout omap_oobinfo;
 /* Define some generic bad / good block scan pattern which are used
@@ -1042,7 +1045,7 @@  static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
 	struct nand_chip *chip = mtd->priv;
 	u32 val;
 
-	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
+	nerrors = info->nand.ecc.strength;
 	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
 	nsectors = 1;
 	/*
@@ -1219,13 +1222,14 @@  static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 						   mtd);
 #ifdef CONFIG_MTD_NAND_OMAP_BCH8
-	const int hw_errors = 8;
+	const int hw_errors = BCH8_MAX_ERROR;
 #else
-	const int hw_errors = 4;
+	const int hw_errors = BCH4_MAX_ERROR;
 #endif
 	info->bch = NULL;
 
-	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ? 8 : 4;
+	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
+		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
 	if (max_errors != hw_errors) {
 		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
 		       max_errors, hw_errors);