Message ID | 1343696537-2564-1-git-send-email-computersforpeace@gmail.com |
---|---|
State | New, archived |
Headers | show |
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
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
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
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;
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
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;
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
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
于 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
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
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
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 --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