diff mbox

[RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

Message ID 1343696537-2564-1-git-send-email-computersforpeace@gmail.com
State New, archived
Headers show

Commit Message

Brian Norris July 31, 2012, 1:02 a.m. UTC 
The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
silently masks off at least one flag that might be set by the driver
(NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly others.

Really, as long as driver writers exercise a small amount of care with NAND_*
options, this mask is not necessary at all. Thus, kill it.

From Huang Shijie:

"I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
it works fine, thanks."

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Tested-by: Huang Shijie <shijie8@gmail.com>
Cc: Marek Vasut <marex@denx.de>
---
Hello Artem/David,

GPMI NAND has needed this patch for some time, and I think it was agreed
on by a few. I'm resending to get acknowledgment from a maintainer.

 drivers/mtd/nand/nand_base.c | 7 ++-----
 include/linux/mtd/nand.h     | 3 ---
 2 files changed, 2 insertions(+), 8 deletions(-)

Comments

Marek Vasut July 31, 2012, 4:17 a.m. UTC | #1 
Dear Brian Norris,

> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> silently masks off at least one flag that might be set by the driver
> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
> others.
> 
> Really, as long as driver writers exercise a small amount of care with
> NAND_* options, this mask is not necessary at all. Thus, kill it.
> 
> From Huang Shijie:
> 
> "I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
> it works fine, thanks."
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Tested-by: Huang Shijie <shijie8@gmail.com>

Tested-by: Marek Vasut <marex@denx.de>

Sorry for the slow reply. Tested it on denx m28evk board.

> Cc: Marek Vasut <marex@denx.de>
> ---
> Hello Artem/David,
> 
> GPMI NAND has needed this patch for some time, and I think it was agreed
> on by a few. I'm resending to get acknowledgment from a maintainer.

[...]

Best regards,
Marek Vasut
Matthieu CASTET July 31, 2012, 7:33 a.m. UTC | #2 
Hi,

for ONFI flash (like this micron one) the information should be extracted form
the ONFI table (programs_per_page IIRC)

This should be better than relying on the SOC driver for setting this flags.

Does the gpmi driver set this flag because it do not support partial write ?
In this case why it doesn't set  chip->ecc.steps to 1 ?

Matthieu


Brian Norris a écrit :
> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> silently masks off at least one flag that might be set by the driver
> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly others.
> 
> Really, as long as driver writers exercise a small amount of care with NAND_*
> options, this mask is not necessary at all. Thus, kill it.
> 
>>From Huang Shijie:
> 
> "I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
> it works fine, thanks."
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Tested-by: Huang Shijie <shijie8@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
> Hello Artem/David,
> 
> GPMI NAND has needed this patch for some time, and I think it was agreed
> on by a few. I'm resending to get acknowledgment from a maintainer.
> 
>  drivers/mtd/nand/nand_base.c | 7 ++-----
>  include/linux/mtd/nand.h     | 3 ---
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ead301a..996cc48 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2909,8 +2909,6 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  	if (le16_to_cpu(p->features) & 1)
>  		*busw = NAND_BUSWIDTH_16;
>  
> -	chip->options &= ~NAND_CHIPOPTIONS_MSK;
> -
>  	pr_info("ONFI flash detected\n");
>  	return 1;
>  }
> @@ -3074,9 +3072,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  			mtd->erasesize <<= ((id_data[3] & 0x03) << 1);
>  		}
>  	}
> -	/* Get chip options, preserve non chip based options */
> -	chip->options &= ~NAND_CHIPOPTIONS_MSK;
> -	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
> +	/* Get chip options */
> +	chip->options |= type->options;
>  
>  	/*
>  	 * Check if chip is not a Samsung device. Do not clear the
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 6dce5a7..eeb7015 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -206,9 +206,6 @@ typedef enum {
>  #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \
>  					&& (chip->page_shift > 9))
>  
> -/* Mask to zero out the chip options, which come from the id table */
> -#define NAND_CHIPOPTIONS_MSK	0x0000ffff
> -
>  /* Non chip related options */
>  /* This option skips the bbt scan during initialization. */
>  #define NAND_SKIP_BBTSCAN	0x00010000
Marek Vasut July 31, 2012, 1:20 p.m. UTC | #3 
Dear Matthieu CASTET,

> Hi,
> 
> for ONFI flash (like this micron one) the information should be extracted
> form the ONFI table (programs_per_page IIRC)
> 
> This should be better than relying on the SOC driver for setting this
> flags.
> 
> Does the gpmi driver set this flag because it do not support partial write
> ?

Yes

> In this case why it doesn't set  chip->ecc.steps to 1 ?

Can you elabore how exactly will that help please?

> Matthieu
> 
> Brian Norris a écrit :
> > The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> > silently masks off at least one flag that might be set by the driver
> > (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
> > others.
> > 
> > Really, as long as driver writers exercise a small amount of care with
> > NAND_* options, this mask is not necessary at all. Thus, kill it.
> > 
> >>From Huang Shijie:
> > "I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
> > it works fine, thanks."
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > Tested-by: Huang Shijie <shijie8@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> > Hello Artem/David,
> > 
> > GPMI NAND has needed this patch for some time, and I think it was agreed
> > on by a few. I'm resending to get acknowledgment from a maintainer.
> > 
> >  drivers/mtd/nand/nand_base.c | 7 ++-----
> >  include/linux/mtd/nand.h     | 3 ---
> >  2 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index ead301a..996cc48 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2909,8 +2909,6 @@ static int nand_flash_detect_onfi(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > 
> >  	if (le16_to_cpu(p->features) & 1)
> >  	
> >  		*busw = NAND_BUSWIDTH_16;
> > 
> > -	chip->options &= ~NAND_CHIPOPTIONS_MSK;
> > -
> > 
> >  	pr_info("ONFI flash detected\n");
> >  	return 1;
> >  
> >  }
> > 
> > @@ -3074,9 +3072,8 @@ static struct nand_flash_dev
> > *nand_get_flash_type(struct mtd_info *mtd,
> > 
> >  			mtd->erasesize <<= ((id_data[3] & 0x03) << 1);
> >  		
> >  		}
> >  	
> >  	}
> > 
> > -	/* Get chip options, preserve non chip based options */
> > -	chip->options &= ~NAND_CHIPOPTIONS_MSK;
> > -	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
> > +	/* Get chip options */
> > +	chip->options |= type->options;
> > 
> >  	/*
> >  	
> >  	 * Check if chip is not a Samsung device. Do not clear the
> > 
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 6dce5a7..eeb7015 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -206,9 +206,6 @@ typedef enum {
> > 
> >  #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \
> >  
> >  					&& (chip->page_shift > 9))
> > 
> > -/* Mask to zero out the chip options, which come from the id table */
> > -#define NAND_CHIPOPTIONS_MSK	0x0000ffff
> > -
> > 
> >  /* Non chip related options */
> >  /* This option skips the bbt scan during initialization. */
> >  #define NAND_SKIP_BBTSCAN	0x00010000

Best regards,
Marek Vasut
Matthieu CASTET July 31, 2012, 1:28 p.m. UTC | #4 
Hi Marek,

Marek Vasut a écrit :
> Dear Matthieu CASTET,
> 
>> Hi,
>>
>> for ONFI flash (like this micron one) the information should be extracted
>> form the ONFI table (programs_per_page IIRC)
>>
>> This should be better than relying on the SOC driver for setting this
>> flags.
>>
>> Does the gpmi driver set this flag because it do not support partial write
>> ?
> 
> Yes
> 
>> In this case why it doesn't set  chip->ecc.steps to 1 ?
> 
> Can you elabore how exactly will that help please?
> 
If you look at the nand_base.c, you will see that mtd->subpage_sft = 0 if
NAND_NO_SUBPAGE_WRITE flags is set or chip->ecc.steps == 1 [1].


Matthieu



[1]
    /* Allow subpage writes up to ecc.steps. Not possible for MLC flash */
    if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
        !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
        switch (chip->ecc.steps) {
        case 2:
            mtd->subpage_sft = 1;
            break;
        case 4:
        case 8:
        case 16:
            mtd->subpage_sft = 2;
            break;
        }
    }
    chip->subpagesize = mtd->writesize >> mtd->subpage_sft;
Marek Vasut July 31, 2012, 1:59 p.m. UTC | #5 
Dear Matthieu CASTET,

> Hi Marek,
> 
> Marek Vasut a écrit :
> > Dear Matthieu CASTET,
> > 
> >> Hi,
> >> 
> >> for ONFI flash (like this micron one) the information should be
> >> extracted form the ONFI table (programs_per_page IIRC)
> >> 
> >> This should be better than relying on the SOC driver for setting this
> >> flags.
> >> 
> >> Does the gpmi driver set this flag because it do not support partial
> >> write ?
> > 
> > Yes
> > 
> >> In this case why it doesn't set  chip->ecc.steps to 1 ?
> > 
> > Can you elabore how exactly will that help please?
> 
> If you look at the nand_base.c, you will see that mtd->subpage_sft = 0 if
> NAND_NO_SUBPAGE_WRITE flags is set or chip->ecc.steps == 1 [1].

Ok, this is what I saw coming ... this is yet another hole in the design and I 
see only undefined behavior. So if default: branch started returning an error, 
this whole code will break again.


> [1]
>     /* Allow subpage writes up to ecc.steps. Not possible for MLC flash */
>     if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
>         !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
>         switch (chip->ecc.steps) {
>         case 2:
>             mtd->subpage_sft = 1;
>             break;
>         case 4:
>         case 8:
>         case 16:
>             mtd->subpage_sft = 2;
>             break;
>         }
>     }
>     chip->subpagesize = mtd->writesize >> mtd->subpage_sft;

Best regards,
Marek Vasut
Matthieu CASTET July 31, 2012, 2:09 p.m. UTC | #6 
Marek Vasut a écrit :
> Dear Matthieu CASTET,
> 
>> Hi Marek,
>>
>> Marek Vasut a écrit :
>>> Dear Matthieu CASTET,
>>>
>>>> Hi,
>>>>
>>>> for ONFI flash (like this micron one) the information should be
>>>> extracted form the ONFI table (programs_per_page IIRC)
>>>>
>>>> This should be better than relying on the SOC driver for setting this
>>>> flags.
>>>>
>>>> Does the gpmi driver set this flag because it do not support partial
>>>> write ?
>>> Yes
>>>
>>>> In this case why it doesn't set  chip->ecc.steps to 1 ?
>>> Can you elabore how exactly will that help please?
>> If you look at the nand_base.c, you will see that mtd->subpage_sft = 0 if
>> NAND_NO_SUBPAGE_WRITE flags is set or chip->ecc.steps == 1 [1].
> 
> Ok, this is what I saw coming ... this is yet another hole in the design and I 
> see only undefined behavior. So if default: branch started returning an error, 
> this whole code will break again.
Do you see any reason why chip->ecc.steps == 1 will return an error ?
This will break drivers.

The behavior match the comment : "Allow subpage writes up to ecc.steps"

Matthieu
> 
> 
>> [1]
>>     /* Allow subpage writes up to ecc.steps. Not possible for MLC flash */
>>     if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
>>         !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
>>         switch (chip->ecc.steps) {
>>         case 2:
>>             mtd->subpage_sft = 1;
>>             break;
>>         case 4:
>>         case 8:
>>         case 16:
>>             mtd->subpage_sft = 2;
>>             break;
>>         }
>>     }
>>     chip->subpagesize = mtd->writesize >> mtd->subpage_sft;
Marek Vasut July 31, 2012, 2:12 p.m. UTC | #7 
Dear Matthieu CASTET,

> Marek Vasut a écrit :
> > Dear Matthieu CASTET,
> > 
> >> Hi Marek,
> >> 
> >> Marek Vasut a écrit :
> >>> Dear Matthieu CASTET,
> >>> 
> >>>> Hi,
> >>>> 
> >>>> for ONFI flash (like this micron one) the information should be
> >>>> extracted form the ONFI table (programs_per_page IIRC)
> >>>> 
> >>>> This should be better than relying on the SOC driver for setting this
> >>>> flags.
> >>>> 
> >>>> Does the gpmi driver set this flag because it do not support partial
> >>>> write ?
> >>> 
> >>> Yes
> >>> 
> >>>> In this case why it doesn't set  chip->ecc.steps to 1 ?
> >>> 
> >>> Can you elabore how exactly will that help please?
> >> 
> >> If you look at the nand_base.c, you will see that mtd->subpage_sft = 0
> >> if NAND_NO_SUBPAGE_WRITE flags is set or chip->ecc.steps == 1 [1].
> > 
> > Ok, this is what I saw coming ... this is yet another hole in the design
> > and I see only undefined behavior. So if default: branch started
> > returning an error, this whole code will break again.
> 
> Do you see any reason why chip->ecc.steps == 1 will return an error ?
> This will break drivers.

It'd be enough if mtd->subpage_sft was inited to some other value. So either add 
"case 1: mtd->subpage_sft = 0; break;" or go with this patch. Either way is fine 
by me.

> The behavior match the comment : "Allow subpage writes up to ecc.steps"
> 
> Matthieu
> 
> >> [1]
> >> 
> >>     /* Allow subpage writes up to ecc.steps. Not possible for MLC flash
> >>     */ if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
> >>     
> >>         !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
> >>         switch (chip->ecc.steps) {
> >>         
> >>         case 2:
> >>             mtd->subpage_sft = 1;
> >>             break;
> >>         
> >>         case 4:
> >>         case 8:
> >>         
> >>         case 16:
> >>             mtd->subpage_sft = 2;
> >>             break;
> >>         
> >>         }
> >>     
> >>     }
> >>     chip->subpagesize = mtd->writesize >> mtd->subpage_sft;

Best regards,
Marek Vasut
Scott Wood July 31, 2012, 4:11 p.m. UTC | #8 
On 07/31/2012 02:33 AM, Matthieu CASTET wrote:
> Hi,
> 
> for ONFI flash (like this micron one) the information should be extracted form
> the ONFI table (programs_per_page IIRC)
> 
> This should be better than relying on the SOC driver for setting this flags.

This is for cases where the constraint is the controller, not the chip.

> Does the gpmi driver set this flag because it do not support partial write ?
> In this case why it doesn't set  chip->ecc.steps to 1 ?

Why is it better to lie about ECC geometry than to just say "subpage
writes aren't supported"?  Does/will the ECC geometry get used by upper
layers in evaluating the number of corrected bitflips?

-Scott
Huang Shijie Aug. 1, 2012, 2:21 a.m. UTC | #9 
于 2012年08月01日 00:11, Scott Wood 写道:
> Why is it better to lie about ECC geometry than to just say "subpage
> writes aren't supported"?  Does/will the ECC geometry get used by upper
yes. I think it's better to provide an official method to let the driver 
say: " i do not support the subpage writes."

Huang Shijie
Matthieu CASTET Aug. 1, 2012, 1:05 p.m. UTC | #10 
Hi Scott,

Scott Wood a écrit :
> On 07/31/2012 02:33 AM, Matthieu CASTET wrote:
>> Hi,
>>
>> for ONFI flash (like this micron one) the information should be extracted form
>> the ONFI table (programs_per_page IIRC)
>>
>> This should be better than relying on the SOC driver for setting this flags.
> 
> This is for cases where the constraint is the controller, not the chip.
> 
>> Does the gpmi driver set this flag because it do not support partial write ?
>> In this case why it doesn't set  chip->ecc.steps to 1 ?
> 
> Why is it better to lie about ECC geometry than to just say "subpage
> writes aren't supported"?  Does/will the ECC geometry get used by upper
> layers in evaluating the number of corrected bitflips?
If it is not because of ecc geometry, why the controller doesn't support subpage
writes ?



Matthieu
Scott Wood Aug. 1, 2012, 4:15 p.m. UTC | #11 
On 08/01/2012 08:05 AM, Matthieu CASTET wrote:
> Hi Scott,
> 
> Scott Wood a écrit :
>> On 07/31/2012 02:33 AM, Matthieu CASTET wrote:
>>> Hi,
>>>
>>> for ONFI flash (like this micron one) the information should be extracted form
>>> the ONFI table (programs_per_page IIRC)
>>>
>>> This should be better than relying on the SOC driver for setting this flags.
>>
>> This is for cases where the constraint is the controller, not the chip.
>>
>>> Does the gpmi driver set this flag because it do not support partial write ?
>>> In this case why it doesn't set  chip->ecc.steps to 1 ?
>>
>> Why is it better to lie about ECC geometry than to just say "subpage
>> writes aren't supported"?  Does/will the ECC geometry get used by upper
>> layers in evaluating the number of corrected bitflips?
> If it is not because of ecc geometry, why the controller doesn't support subpage
> writes ?

I can't answer for GPMI, but in the case of Freescale eLBC/IFC, the
controller only does ECC when you do a full page transaction -- but the
ECC is still done in steps, which is relevant for bitflip thresholds.

-Scott
Brian Norris Aug. 2, 2012, 1:08 a.m. UTC | #12 
On Wed, Aug 1, 2012 at 9:15 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 08/01/2012 08:05 AM, Matthieu CASTET wrote:
>> Scott Wood a écrit :
>>> On 07/31/2012 02:33 AM, Matthieu CASTET wrote:
>>>> Hi,
>>>>
>>>> for ONFI flash (like this micron one) the information should be extracted form
>>>> the ONFI table (programs_per_page IIRC)
>>>>
>>>> This should be better than relying on the SOC driver for setting this flags.
>>>
>>> This is for cases where the constraint is the controller, not the chip.
>>>
>>>> Does the gpmi driver set this flag because it do not support partial write ?
>>>> In this case why it doesn't set  chip->ecc.steps to 1 ?
>>>
>>> Why is it better to lie about ECC geometry than to just say "subpage
>>> writes aren't supported"?  Does/will the ECC geometry get used by upper
>>> layers in evaluating the number of corrected bitflips?
>> If it is not because of ecc geometry, why the controller doesn't support subpage
>> writes ?
>
> I can't answer for GPMI, but in the case of Freescale eLBC/IFC, the
> controller only does ECC when you do a full page transaction -- but the
> ECC is still done in steps, which is relevant for bitflip thresholds.

My controller (out-of-tree driver) *can* support subpage writes, but
disabling them gives performance benefits and allows stronger ECC
modes. My driver already performs a kind of hack by enabling
NAND_NO_SUBPAGE_WRITE in between nand_scan_ident() and
nand_scan_tail().

I agree that removing the mask allows a direct way of stating that the
controller does not support subpage writes, a property which is
orthogonal to the concept of ECC step size.

Brian
diff mbox

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ead301a..996cc48 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2909,8 +2909,6 @@  static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	if (le16_to_cpu(p->features) & 1)
 		*busw = NAND_BUSWIDTH_16;
 
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-
 	pr_info("ONFI flash detected\n");
 	return 1;
 }
@@ -3074,9 +3072,8 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 			mtd->erasesize <<= ((id_data[3] & 0x03) << 1);
 		}
 	}
-	/* Get chip options, preserve non chip based options */
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+	/* Get chip options */
+	chip->options |= type->options;
 
 	/*
 	 * Check if chip is not a Samsung device. Do not clear the
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6dce5a7..eeb7015 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -206,9 +206,6 @@  typedef enum {
 #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \
 					&& (chip->page_shift > 9))
 
-/* Mask to zero out the chip options, which come from the id table */
-#define NAND_CHIPOPTIONS_MSK	0x0000ffff
-
 /* Non chip related options */
 /* This option skips the bbt scan during initialization. */
 #define NAND_SKIP_BBTSCAN	0x00010000