Skip to contentSkip to conversation

nand: fix reading after switching ecc

PATCH New Jeroen Hofstee 6 participants 15 messages Jan 11, 2014 – Jan 17, 2014

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Jeroen Hofstee

author
#0 lore raw

PATCH 1/1 nand: fix reading after switching ecc

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---
 drivers/mtd/nand/nand_base.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mtd/nand/nand_base.c +2 -0
index 1ce55fd..0762a19 100644
@@ -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;
Signature · 1 line
1.8.3.2
#1 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Hi Jeroen,

Quoted 21 lines · 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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---
  drivers/mtd/nand/nand_base.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/mtd/nand/nand_base.c +0 -0
index 1ce55fd..0762a19 100644

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3361

Quoted 8 lines
drivers/mtd/nand/nand_base.c +2 -0
@@ -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 <[email protected]>
Tested-by Nikita Kiryanov <[email protected]>
Signature · 2 lines
Regards,
Nikita.
#2 lore raw

PATCH 1/1 nand: fix reading after switching ecc

In reply to Jeroen Hofstee

Hi Jeroen,

Quoted 21 lines

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
drivers/mtd/nand/nand_base.c | 2 ++

1 file changed, 2 insertions(+)
drivers/mtd/nand/nand_base.c +0 -0
index 1ce55fd..0762a19 100644

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3359

Quoted 6 lines
drivers/mtd/nand/nand_base.c +2 -0
@@ -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

#3 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 18 lines · Pekon

Hi Jeroen,

Quoted 16 lines

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
drivers/mtd/nand/nand_base.c | 2 ++

1 file changed, 2 insertions(+)
Quoted 10 lines

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3358

Quoted 10 lines
drivers/mtd/nand/nand_base.c +2 -0
index 1ce55fd..0762a19 100644
@@ -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.

Quoted 2 lines

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

author
#4 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Hello Scott, Pekon,

Quoted 17 lines · Scott Wood
Quoted 17 lines
Quoted 17 lines

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
drivers/mtd/nand/nand_base.c | 2 ++

1 file changed, 2 insertions(+)
Quoted 12 lines
Quoted 10 lines

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3358

Quoted 10 lines
drivers/mtd/nand/nand_base.c +2 -0
index 1ce55fd..0762a19 100644
@@ -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...

Quoted 4 lines
Quoted 2 lines

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

#5 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 33 lines · Jeroen Hofstee

Hello Scott, Pekon,

Quoted 17 lines · Scott Wood
Quoted 17 lines
Quoted 17 lines

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
drivers/mtd/nand/nand_base.c | 2 ++

1 file changed, 2 insertions(+)
Quoted 12 lines
Quoted 10 lines

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3358

Quoted 10 lines
drivers/mtd/nand/nand_base.c +2 -0
index 1ce55fd..0762a19 100644
@@ -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.

Quoted 6 lines

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

author
#6 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 37 lines · Scott Wood
Quoted 30 lines · Jeroen Hofstee

Hello Scott, Pekon,

Quoted 15 lines · Scott Wood
Quoted 15 lines
Quoted 15 lines

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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
drivers/mtd/nand/nand_base.c | 2 ++

1 file changed, 2 insertions(+)
Quoted 12 lines
Quoted 10 lines

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 3358

Quoted 10 lines
drivers/mtd/nand/nand_base.c +2 -0
index 1ce55fd..0762a19 100644
@@ -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.

Quoted 6 lines

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...

Quoted 3 lines

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.

Quoted 3 lines

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

#7 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 8 lines · Jeroen Hofstee
Quoted 2 lines · Scott Wood

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

author
#8 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 5 lines · Scott Wood
Quoted 4 lines · Jeroen Hofstee
Quoted 2 lines · Scott Wood

All the other cleanup required to change ECC modes is handled by the OMAP driver; why shouldn't this be as well?

Quoted 7 lines
Quoted 3 lines

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

#9 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 9 lines · Jeroen Hofstee
Quoted 8 lines · Scott Wood
Quoted 7 lines · Jeroen Hofstee
Quoted 2 lines · Scott Wood

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.

Quoted 7 lines
Quoted 4 lines

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

#10 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 5 lines · Scott Wood

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

#11 lore raw

PATCH 1/1 nand: fix reading after switching ecc

Quoted 8 lines · Jeroen Hofstee
Quoted 5 lines · Scott Wood

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

#12 lore raw

PATCH 1/1 nand: fix reading after switching ecc

In reply to Jeroen Hofstee
Quoted 1 line

From: Jeroen Hofstee [mailto:jeroen at myspectrum.nl]

[...]

Quoted 5 lines

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

author
#13 lore raw

PATCH v2 1/1 nand, gpmc: fix reading after switching ecc

In reply to 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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

---
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(+)
drivers/mtd/nand/omap_gpmc.c +1 -0
index 790d538..389c4de 100644
@@ -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) {
Signature · 1 line
1.8.3.2
#14 lore raw

PATCH v2 1/1 nand, gpmc: fix reading after switching ecc

Quoted 11 lines · 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 <[email protected]>
Cc Pekon Gupta <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
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>