| Message ID | 1389447588-17529-1-git-send-email-jeroen@myspectrum.nl |
|---|---|
| State | Superseded |
| Headers | show |
Hi Jeroen, On 01/11/2014 03:39 PM, Jeroen Hofstee wrote: > The omap_gpmc allows switching ecc at runtime. Since > the NAND_SUBPAGE_READ flag is only set, it is kept when > switching to hw ecc, which is not correct. This leads to > calling chip->ecc.read_subpage which is not a valid > pointer. Therefore also clear the flag so reading in > hw mode works again. > > Cc: Scott Wood <scottwood@freescale.com> > Cc: Pekon Gupta <pekon@ti.com> > Cc: Nikita Kiryanov <nikita@compulab.co.il> > Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> > --- > drivers/mtd/nand/nand_base.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 1ce55fd..0762a19 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) > /* Large page NAND with SOFT_ECC should support subpage reads */ > if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) > chip->options |= NAND_SUBPAGE_READ; > + else > + chip->options &= ~NAND_SUBPAGE_READ; > > /* Fill in remaining MTD driver data */ > mtd->type = MTD_NANDFLASH; > Tested on cm_t35. Acked-by: Nikita Kiryanov <nikita@compulab.co.il> Tested-by: Nikita Kiryanov <nikita@compulab.co.il>
Hi Jeroen, > >The omap_gpmc allows switching ecc at runtime. Since >the NAND_SUBPAGE_READ flag is only set, it is kept when >switching to hw ecc, which is not correct. This leads to >calling chip->ecc.read_subpage which is not a valid >pointer. Therefore also clear the flag so reading in >hw mode works again. > >Cc: Scott Wood <scottwood@freescale.com> >Cc: Pekon Gupta <pekon@ti.com> >Cc: Nikita Kiryanov <nikita@compulab.co.il> >Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> >--- > drivers/mtd/nand/nand_base.c | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >index 1ce55fd..0762a19 100644 >--- a/drivers/mtd/nand/nand_base.c >+++ b/drivers/mtd/nand/nand_base.c >@@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) > /* Large page NAND with SOFT_ECC should support subpage reads */ > if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) > chip->options |= NAND_SUBPAGE_READ; >+ else >+ chip->options &= ~NAND_SUBPAGE_READ; > I don't think it's good to add OMAP specific changes to nand_base.c. It's better if you can add this as part of omap_select_ecc_scheme() in omap_gpmc.c with regards, pekon
On Mon, 2014-01-13 at 08:29 +0000, Gupta, Pekon wrote: > Hi Jeroen, > > > > >The omap_gpmc allows switching ecc at runtime. Since > >the NAND_SUBPAGE_READ flag is only set, it is kept when > >switching to hw ecc, which is not correct. This leads to > >calling chip->ecc.read_subpage which is not a valid > >pointer. Therefore also clear the flag so reading in > >hw mode works again. > > > >Cc: Scott Wood <scottwood@freescale.com> > >Cc: Pekon Gupta <pekon@ti.com> > >Cc: Nikita Kiryanov <nikita@compulab.co.il> > >Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> > >--- > > drivers/mtd/nand/nand_base.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > >diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >index 1ce55fd..0762a19 100644 > >--- a/drivers/mtd/nand/nand_base.c > >+++ b/drivers/mtd/nand/nand_base.c > >@@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) > > /* Large page NAND with SOFT_ECC should support subpage reads */ > > if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) > > chip->options |= NAND_SUBPAGE_READ; > >+ else > >+ chip->options &= ~NAND_SUBPAGE_READ; NACK; this breaks NAND_SUBPAGE_READ with hardware ECC for drivers that support it. > I don't think it's good to add OMAP specific changes to nand_base.c. > It's better if you can add this as part of omap_select_ecc_scheme() in omap_gpmc.c Yes, clear it from the OMAP switching code if OMAP can't do subpage reads with hardware ECC. -Scott
Hello Scott, Pekon, On 01/13/2014 07:18 PM, Scott Wood wrote: > >> >>> The omap_gpmc allows switching ecc at runtime. Since >>> the NAND_SUBPAGE_READ flag is only set, it is kept when >>> switching to hw ecc, which is not correct. This leads to >>> calling chip->ecc.read_subpage which is not a valid >>> pointer. Therefore also clear the flag so reading in >>> hw mode works again. >>> >>> Cc: Scott Wood <scottwood@freescale.com> >>> Cc: Pekon Gupta <pekon@ti.com> >>> Cc: Nikita Kiryanov <nikita@compulab.co.il> >>> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> >>> --- >>> drivers/mtd/nand/nand_base.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index 1ce55fd..0762a19 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) >>> /* Large page NAND with SOFT_ECC should support subpage reads */ >>> if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) >>> chip->options |= NAND_SUBPAGE_READ; >>> + else >>> + chip->options &= ~NAND_SUBPAGE_READ; > NACK; this breaks NAND_SUBPAGE_READ with hardware ECC for drivers that > support it. There is something to argue in favour of that in general, but there are no such drivers in u-boot at the moment though (well at least not doing it on purpose). I don't mind moving it to gpmc, but for correctness, it doesn't break things at the moment... >> I don't think it's good to add OMAP specific changes to nand_base.c. >> It's better if you can add this as part of omap_select_ecc_scheme() in omap_gpmc.c > Yes, clear it from the OMAP switching code if OMAP can't do subpage > reads with hardware ECC. The gpmc will fail in hw ecc mode when trying to do subpage reads. Pekon any suggestion for the elm mode, or should this bit just be cleared unconditionally? Regards, Jeroen
On Tue, 2014-01-14 at 21:11 +0100, Jeroen Hofstee wrote: > Hello Scott, Pekon, > > On 01/13/2014 07:18 PM, Scott Wood wrote: > > > >> > >>> The omap_gpmc allows switching ecc at runtime. Since > >>> the NAND_SUBPAGE_READ flag is only set, it is kept when > >>> switching to hw ecc, which is not correct. This leads to > >>> calling chip->ecc.read_subpage which is not a valid > >>> pointer. Therefore also clear the flag so reading in > >>> hw mode works again. > >>> > >>> Cc: Scott Wood <scottwood@freescale.com> > >>> Cc: Pekon Gupta <pekon@ti.com> > >>> Cc: Nikita Kiryanov <nikita@compulab.co.il> > >>> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> > >>> --- > >>> drivers/mtd/nand/nand_base.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >>> index 1ce55fd..0762a19 100644 > >>> --- a/drivers/mtd/nand/nand_base.c > >>> +++ b/drivers/mtd/nand/nand_base.c > >>> @@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) > >>> /* Large page NAND with SOFT_ECC should support subpage reads */ > >>> if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) > >>> chip->options |= NAND_SUBPAGE_READ; > >>> + else > >>> + chip->options &= ~NAND_SUBPAGE_READ; > > NACK; this breaks NAND_SUBPAGE_READ with hardware ECC for drivers that > > support it. > > There is something to argue in favour of that in general, but there are > no such drivers in u-boot at the moment though (well at least not doing > it on > purpose). I don't mind moving it to gpmc, but for correctness, it > doesn't break > things at the moment... It doesn't matter if there are any such drivers at the moment. It's a bad change, and a gratuitous difference from Linux with which we share this common code. All the other cleanup required to change ECC modes is handled by the OMAP driver; why shouldn't this be as well? -Scott
On 01/14/2014 09:21 PM, Scott Wood wrote: > On Tue, 2014-01-14 at 21:11 +0100, Jeroen Hofstee wrote: >> Hello Scott, Pekon, >> >> On 01/13/2014 07:18 PM, Scott Wood wrote: >>>>> The omap_gpmc allows switching ecc at runtime. Since >>>>> the NAND_SUBPAGE_READ flag is only set, it is kept when >>>>> switching to hw ecc, which is not correct. This leads to >>>>> calling chip->ecc.read_subpage which is not a valid >>>>> pointer. Therefore also clear the flag so reading in >>>>> hw mode works again. >>>>> >>>>> Cc: Scott Wood <scottwood@freescale.com> >>>>> Cc: Pekon Gupta <pekon@ti.com> >>>>> Cc: Nikita Kiryanov <nikita@compulab.co.il> >>>>> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> >>>>> --- >>>>> drivers/mtd/nand/nand_base.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>>>> index 1ce55fd..0762a19 100644 >>>>> --- a/drivers/mtd/nand/nand_base.c >>>>> +++ b/drivers/mtd/nand/nand_base.c >>>>> @@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) >>>>> /* Large page NAND with SOFT_ECC should support subpage reads */ >>>>> if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) >>>>> chip->options |= NAND_SUBPAGE_READ; >>>>> + else >>>>> + chip->options &= ~NAND_SUBPAGE_READ; >>> NACK; this breaks NAND_SUBPAGE_READ with hardware ECC for drivers that >>> support it. >> There is something to argue in favour of that in general, but there are >> no such drivers in u-boot at the moment though (well at least not doing >> it on >> purpose). I don't mind moving it to gpmc, but for correctness, it >> doesn't break >> things at the moment... > It doesn't matter if there are any such drivers at the moment. It's a > bad change, and a gratuitous difference from Linux with which we share > this common code. of course it does matter, you can chose not to support certain features, like subpage reads, interrupts, power management etc in a bootloader. As I said, moving it to gpmc is fine with me, so lets not making a long discussion of it. > > All the other cleanup required to change ECC modes is handled by the > OMAP driver; why shouldn't this be as well? If there are more architectures, who think it is brilliant to switch ecc it will become a mesh if not coped with in general. And yes omap might be the exception for now, so lets put it there. Regards, Jeroen
On Tue, 2014-01-14 at 21:38 +0100, Jeroen Hofstee wrote: > On 01/14/2014 09:21 PM, Scott Wood wrote: > > All the other cleanup required to change ECC modes is handled by the > > OMAP driver; why shouldn't this be as well? > > > If there are more architectures, who think it is brilliant to switch ecc > it will become a mesh if not coped with in general. And yes omap might > be the exception for now, so lets put it there. Perhaps, but solving it in general should not happen via changes like this, but rather some sort of reset function that gives you a clean slate. -Scott
On 01/14/2014 09:41 PM, Scott Wood wrote: > On Tue, 2014-01-14 at 21:38 +0100, Jeroen Hofstee wrote: >> On 01/14/2014 09:21 PM, Scott Wood wrote: >>> All the other cleanup required to change ECC modes is handled by the >>> OMAP driver; why shouldn't this be as well? >> >> If there are more architectures, who think it is brilliant to switch ecc >> it will become a mesh if not coped with in general. And yes omap might >> be the exception for now, so lets put it there. > Perhaps, but solving it in general should not happen via changes like > this, but rather some sort of reset function that gives you a clean > slate. > I have no problem with a function call to nand_base resetting this flag, at least nand_base keeps in control of its own flag. I'll send you a patch for it. Regards, Jeroen
On Tue, 2014-01-14 at 21:56 +0100, Jeroen Hofstee wrote: > On 01/14/2014 09:41 PM, Scott Wood wrote: > > On Tue, 2014-01-14 at 21:38 +0100, Jeroen Hofstee wrote: > >> On 01/14/2014 09:21 PM, Scott Wood wrote: > >>> All the other cleanup required to change ECC modes is handled by the > >>> OMAP driver; why shouldn't this be as well? > >> > >> If there are more architectures, who think it is brilliant to switch ecc > >> it will become a mesh if not coped with in general. And yes omap might > >> be the exception for now, so lets put it there. > > Perhaps, but solving it in general should not happen via changes like > > this, but rather some sort of reset function that gives you a clean > > slate. > > > I have no problem with a function call to nand_base resetting this > flag, at least nand_base keeps in control of its own flag. I'll send > you a patch for it. I meant a function that resets everything that might have been set automatically by the previous nand_scan_tail(), not just one flag. -Scott
On 01/14/2014 10:05 PM, Scott Wood wrote: > > I meant a function that resets everything that might have been set > automatically by the previous nand_scan_tail(), not just one flag. > > Well I am talking about a single bit bricking my board. Not causing a problem otherwise, besides you falling about it. Regards, Jeroen
On Tue, 2014-01-14 at 22:15 +0100, Jeroen Hofstee wrote: > On 01/14/2014 10:05 PM, Scott Wood wrote: > > > > I meant a function that resets everything that might have been set > > automatically by the previous nand_scan_tail(), not just one flag. > > > > > > Well I am talking about a single bit bricking my board. I thought you already said you were OK handling that in the OMAP driver, along with all the other stuff it does to fix things up? I was responding to your comment about eventually having proper support for ECC changing in common code, not suggesting this as a short term fix. -Scott
>From: Jeroen Hofstee [mailto:jeroen@myspectrum.nl] [...] > >The gpmc will fail in hw ecc mode when trying to do subpage reads. Pekon any >suggestion for the elm mode, or should this bit just be cleared >unconditionally? > There are two reasons for not supporting sub-page feature in OMAP platforms: (1) ELM h/w engine has limitation that it can support ecc-corrections only in junks of 512Bytes. Now some flash devices (like page-size=4K) have sub-page = 4k/4 = 1k. Thus, it becomes trivial (though not impossible) to support sub-page. Hence, for now sub-page feature has been marked as 'un-supported' for OMAP. (2) Also, we found that sub-page feature was not giving much benefit, except improving data-packing density on given NAND to some extent. Like in UBIFS, having sub-page feature packs 'erase-header' and volume-id-header' into single page, thereby saving 1 page for every erase-block. Memory Saved = (1x page-size)/ block-size = 2k / 128k = 1/64 ~ 1.5 % (raw calculations). Hence, in summary, yes you can un-conditionally clear the sub-page read bit. with regards, pekon
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 1ce55fd..0762a19 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3354,6 +3354,8 @@ int nand_scan_tail(struct mtd_info *mtd) /* Large page NAND with SOFT_ECC should support subpage reads */ if ((chip->ecc.mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) chip->options |= NAND_SUBPAGE_READ; + else + chip->options &= ~NAND_SUBPAGE_READ; /* Fill in remaining MTD driver data */ mtd->type = MTD_NANDFLASH;
The omap_gpmc allows switching ecc at runtime. Since the NAND_SUBPAGE_READ flag is only set, it is kept when switching to hw ecc, which is not correct. This leads to calling chip->ecc.read_subpage which is not a valid pointer. Therefore also clear the flag so reading in hw mode works again. Cc: Scott Wood <scottwood@freescale.com> Cc: Pekon Gupta <pekon@ti.com> Cc: Nikita Kiryanov <nikita@compulab.co.il> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> --- drivers/mtd/nand/nand_base.c | 2 ++ 1 file changed, 2 insertions(+)