[U-Boot] [PATCH] nand: fix reading after switching ecc

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Jeroen Hofstee]

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;
-- 
1.8.3.2

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Nikita Kiryanov]

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>

-- 
Regards,
Nikita.

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Gupta, Pekon]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Scott Wood]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Jeroen Hofstee]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Scott Wood]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Jeroen Hofstee]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Scott Wood]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Jeroen Hofstee]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Scott Wood]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Jeroen Hofstee]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Scott Wood]

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

[U-Boot] [PATCH] nand: fix reading after switching ecc

[Gupta, Pekon]

>From: Jeroen Hofstee [mailto:jeroen at 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

[U-Boot] [PATCH v2] nand, gpmc: fix reading after switching ecc

[Jeroen Hofstee]

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 clear the flag when switching ecc 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>
---
version 2:
  - clear the flag from the omap_gpmc specific omap_nand_switch_ecc
---
 drivers/mtd/nand/omap_gpmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 790d538..389c4de 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -933,6 +933,7 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength)
 	mtd = &nand_info[nand_curr_device];
 	nand = mtd->priv;
 	nand->options |= NAND_OWN_BUFFERS;
+	nand->options &= ~NAND_SUBPAGE_READ;
 	/* Setup the ecc configurations again */
 	if (hardware) {
 		if (eccstrength == 1) {
-- 
1.8.3.2

[U-Boot] [U-Boot, v2] nand, gpmc: fix reading after switching ecc

[Tom Rini]

On Wed, Jan 15, 2014 at 05:58:54PM +0100, 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 clear the flag when switching ecc 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140117/22e57f6e/attachment.pgp>