* [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
@ 2012-07-31 1:02 Brian Norris
2012-07-31 4:17 ` Marek Vasut
2012-07-31 7:33 ` Matthieu CASTET
0 siblings, 2 replies; 13+ messages in thread
From: Brian Norris @ 2012-07-31 1:02 UTC (permalink / raw)
To: Artem Bityutskiy
Cc: Scott Wood, David Woodhouse, Marek Vasut, Brian Norris, linux-mtd
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
--
1.7.11.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 1:02 [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver Brian Norris
@ 2012-07-31 4:17 ` Marek Vasut
2012-07-31 7:33 ` Matthieu CASTET
1 sibling, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2012-07-31 4:17 UTC (permalink / raw)
To: Brian Norris; +Cc: Scott Wood, David Woodhouse, linux-mtd, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 1:02 [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver Brian Norris
2012-07-31 4:17 ` Marek Vasut
@ 2012-07-31 7:33 ` Matthieu CASTET
2012-07-31 13:20 ` Marek Vasut
2012-07-31 16:11 ` Scott Wood
1 sibling, 2 replies; 13+ messages in thread
From: Matthieu CASTET @ 2012-07-31 7:33 UTC (permalink / raw)
To: Brian Norris
Cc: Scott Wood, Marek Vasut, David Woodhouse, linux-mtd, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 7:33 ` Matthieu CASTET
@ 2012-07-31 13:20 ` Marek Vasut
2012-07-31 13:28 ` Matthieu CASTET
2012-07-31 16:11 ` Scott Wood
1 sibling, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2012-07-31 13:20 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Scott Wood, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 13:20 ` Marek Vasut
@ 2012-07-31 13:28 ` Matthieu CASTET
2012-07-31 13:59 ` Marek Vasut
0 siblings, 1 reply; 13+ messages in thread
From: Matthieu CASTET @ 2012-07-31 13:28 UTC (permalink / raw)
To: Marek Vasut
Cc: Scott Wood, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 13:28 ` Matthieu CASTET
@ 2012-07-31 13:59 ` Marek Vasut
2012-07-31 14:09 ` Matthieu CASTET
0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2012-07-31 13:59 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Scott Wood, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 13:59 ` Marek Vasut
@ 2012-07-31 14:09 ` Matthieu CASTET
2012-07-31 14:12 ` Marek Vasut
0 siblings, 1 reply; 13+ messages in thread
From: Matthieu CASTET @ 2012-07-31 14:09 UTC (permalink / raw)
To: Marek Vasut
Cc: Scott Wood, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 14:09 ` Matthieu CASTET
@ 2012-07-31 14:12 ` Marek Vasut
0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2012-07-31 14:12 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Scott Wood, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 7:33 ` Matthieu CASTET
2012-07-31 13:20 ` Marek Vasut
@ 2012-07-31 16:11 ` Scott Wood
2012-08-01 2:21 ` Huang Shijie
2012-08-01 13:05 ` Matthieu CASTET
1 sibling, 2 replies; 13+ messages in thread
From: Scott Wood @ 2012-07-31 16:11 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Marek Vasut, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 16:11 ` Scott Wood
@ 2012-08-01 2:21 ` Huang Shijie
2012-08-01 13:05 ` Matthieu CASTET
1 sibling, 0 replies; 13+ messages in thread
From: Huang Shijie @ 2012-08-01 2:21 UTC (permalink / raw)
To: Scott Wood
Cc: Marek Vasut, Artem Bityutskiy, Matthieu CASTET, linux-mtd,
Brian Norris, David Woodhouse
于 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-07-31 16:11 ` Scott Wood
2012-08-01 2:21 ` Huang Shijie
@ 2012-08-01 13:05 ` Matthieu CASTET
2012-08-01 16:15 ` Scott Wood
1 sibling, 1 reply; 13+ messages in thread
From: Matthieu CASTET @ 2012-08-01 13:05 UTC (permalink / raw)
To: Scott Wood
Cc: Marek Vasut, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-08-01 13:05 ` Matthieu CASTET
@ 2012-08-01 16:15 ` Scott Wood
2012-08-02 1:08 ` Brian Norris
0 siblings, 1 reply; 13+ messages in thread
From: Scott Wood @ 2012-08-01 16:15 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Marek Vasut, linux-mtd, Brian Norris, David Woodhouse, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver
2012-08-01 16:15 ` Scott Wood
@ 2012-08-02 1:08 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-08-02 1:08 UTC (permalink / raw)
To: Scott Wood
Cc: Marek Vasut, linux-mtd, David Woodhouse, Matthieu CASTET,
Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-08-02 1:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-31 1:02 [PATCH RESEND] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver Brian Norris
2012-07-31 4:17 ` Marek Vasut
2012-07-31 7:33 ` Matthieu CASTET
2012-07-31 13:20 ` Marek Vasut
2012-07-31 13:28 ` Matthieu CASTET
2012-07-31 13:59 ` Marek Vasut
2012-07-31 14:09 ` Matthieu CASTET
2012-07-31 14:12 ` Marek Vasut
2012-07-31 16:11 ` Scott Wood
2012-08-01 2:21 ` Huang Shijie
2012-08-01 13:05 ` Matthieu CASTET
2012-08-01 16:15 ` Scott Wood
2012-08-02 1:08 ` Brian Norris
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.