NAND: Let NAND_NO_SUBPAGE_WRITE through

PATCH New Marek Vasut 4 participants 10 messages Nov 5, 2011 – Nov 11, 2011

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Marek Vasut

author

2011-11-05 02:24 UTC #0 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.

This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>

The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>

Signed-off-by Marek Vasut <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Scott Wood <[email protected]>

Cc Veli-Pekka Peltola <[email protected]>

---
 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mtd/nand/nand_base.c +11 -0

index 6aac6a2..7ecd5a3 100644
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 {
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
+	int no_subpage = 0;
 
 	/* Select the device */
 	chip->select_chip(mtd, 0);

@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (!ret)
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);
 
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+
 	/* Get chip options, preserve non chip based options */
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
 
+	if (no_subpage)
+		chip->options |= NAND_NO_SUBPAGE_WRITE;
+
 	/*
 	 * Set chip as a default. Board drivers can override it, if necessary
 	 */

Signature · 1 line

1.7.6.3

Stefano Babic

2011-11-06 09:01 UTC #1 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 4 lines · Marek Vasut

There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.

Hi Marek,

Quoted 2 lines

This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>

Right, there was some discussion also comparing what it is done on the Linux Kernel. There is a discrepancy for the usage of the NAND_NO_SUBPAGE_WRITE, because it belongs to the chip options, but in this case (and in the patch I submitted) it is used as "controller" option. Same case here.

Quoted 21 lines

The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>

 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mtd/nand/nand_base.c +1 -0

index 6aac6a2..7ecd5a3 100644
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 {
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
+	int no_subpage = 0;
 
 	/* Select the device */
 	chip->select_chip(mtd, 0);

drivers/mtd/nand/nand_base.c static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2622

Quoted 10 lines

drivers/mtd/nand/nand_base.c +7 -0

@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (!ret)
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);
 
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+

The patch does not convince me. This relies only if the SUBPAGE_WRITE is supported by the chip, and this is half problem. What happens if we have a SLC (that generally supports subpage mode) connected to a controller that does not support it ? NAND_NO_SUBPAGE_WRITE is not set, but the controller can't manage it.
I think your patch fix only a particular case, and it is not general enough.

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Marek Vasut

author

2011-11-06 12:43 UTC #2 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 7 lines · Marek Vasut

Quoted 3 lines
There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.
Hi Marek,

Quoted 8 lines

Quoted 2 lines
This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>
Right, there was some discussion also comparing what it is done on the Linux Kernel. There is a discrepancy for the usage of the NAND_NO_SUBPAGE_WRITE, because it belongs to the chip options, but in this case (and in the patch I submitted) it is used as "controller" option. Same case here.

Yes sure, but the controller driver sets it as a chip option as there's no "controller" option field and I don't want to introduce new definition consuming one bit in the top half of chip option.

Quoted 32 lines

The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>

 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mtd/nand/nand_base.c +1 -0

index 6aac6a2..7ecd5a3 100644
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd,
 
 {
 
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
 
+	int no_subpage = 0;
 
 	/* Select the device */
 	chip->select_chip(mtd, 0);

@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd,
 
 	if (!ret)
 
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);

Quoted 15 lines

drivers/mtd/nand/nand_base.c static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2622
Quoted 7 lines
drivers/mtd/nand/nand_base.c +7 -0
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+
The patch does not convince me. This relies only if the SUBPAGE_WRITE is supported by the chip, and this is half problem. What happens if we have a SLC (that generally supports subpage mode) connected to a controller that does not support it ? NAND_NO_SUBPAGE_WRITE is not set, but the controller can't manage it.
I think your patch fix only a particular case, and it is not general enough.

No, the controller must set NAND_NO_SUBPAGE_WRITE as a chip option in its driver to force it so it should work for both cases.

Cheers

Stefano Babic

2011-11-06 14:54 UTC #3 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 4 lines · Marek Vasut

Yes sure, but the controller driver sets it as a chip option as there's no "controller" option field and I don't want to introduce new definition consuming one bit in the top half of chip option.

Let's see how evolve the discussion - allowing to mix chip and controller options was

Quoted 3 lines

No, the controller must set NAND_NO_SUBPAGE_WRITE as a chip option in its driver to force it so it should work for both cases.

As far as I remember (not very well, I admit..), the only way for the driver to set them was in the board_nand_init(), but these options are overwritten when the flash is detected, making NAND_NO_SUBPAGE_WRITE worthless. Am I wrong ?

Stefano

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Marek Vasut

author

2011-11-06 15:16 UTC #4 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 7 lines · Marek Vasut

Quoted 3 lines
Yes sure, but the controller driver sets it as a chip option as there's no "controller" option field and I don't want to introduce new definition consuming one bit in the top half of chip option.
Let's see how evolve the discussion - allowing to mix chip and controller options was

Exactly, I consider this patch more as a workaround, so figuring out a proper solution would be awesome. Though I'm quite sure there is a bug in the MTD layer.

Quoted 9 lines

Quoted 3 lines
No, the controller must set NAND_NO_SUBPAGE_WRITE as a chip option in its driver to force it so it should work for both cases.
As far as I remember (not very well, I admit..), the only way for the driver to set them was in the board_nand_init(), but these options are overwritten when the flash is detected, making NAND_NO_SUBPAGE_WRITE worthless. Am I wrong ?

This is what this patch works around ;-)

Marek Vasut

author

2011-11-10 17:02 UTC #5 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

In reply to Marek Vasut

Quoted 42 lines

There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.
This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>
The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Scott Wood <[email protected]>
Cc Veli-Pekka Peltola <[email protected]>
---
 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
drivers/mtd/nand/nand_base.c +8 -0
index 6aac6a2..7ecd5a3 100644
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, {
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
+	int no_subpage = 0;
 
 	/* Select the device */
 	chip->select_chip(mtd, 0);
@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, if (!ret)
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);
 
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+

drivers/mtd/nand/nand_base.c static const struct nand_flash_dev · line 2632

Quoted 10 lines

drivers/mtd/nand/nand_base.c +3 -0

 	/* Get chip options, preserve non chip based options */
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
 
+	if (no_subpage)
+		chip->options |= NAND_NO_SUBPAGE_WRITE;
+
 	/*
 	 * Set chip as a default. Board drivers can override it, if necessary
 	 */

Scott, ping ?

Scott Wood

2011-11-10 17:10 UTC #6 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 46 lines · Marek Vasut

Quoted 46 lines
There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.
This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>
The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Scott Wood <[email protected]>
Cc Veli-Pekka Peltola <[email protected]>
---
 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
drivers/mtd/nand/nand_base.c +8 -0
index 6aac6a2..7ecd5a3 100644
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, {
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
+	int no_subpage = 0;
 
 	/* Select the device */
 	chip->select_chip(mtd, 0);
@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, if (!ret)
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);
 
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+
 	/* Get chip options, preserve non chip based options */
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
 

Quoted 8 lines

drivers/mtd/nand/nand_base.c static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2632
Quoted 6 lines
drivers/mtd/nand/nand_base.c +3 -0
+	if (no_subpage)
+		chip->options |= NAND_NO_SUBPAGE_WRITE;
+
 	/*
 	 * Set chip as a default. Board drivers can override it, if necessary
 	 */
Scott, ping ?

We could do that I guess, but couldn't we also just get rid of NAND_CHIPOPTIONS_MSK?

-Scott

Marek Vasut

author

2011-11-10 17:44 UTC #7 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 64 lines · Marek Vasut

Quoted 56 lines
Quoted 56 lines
There is a problem reported that the NAND_NO_SUBPAGE_WRITE, set by some drivers, is silently ignored by NAND core. This causes UBI to malfunction on these drivers, because UBI tries to use subpage writes.
This was discussed already with no conclusion, see thread:
Message-Id: <[email protected]>
The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with UBI on the MX28 CPU:
Message-ID: <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Scott Wood <[email protected]>
Cc Veli-Pekka Peltola <[email protected]>
---
 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/nand_base.c index 6aac6a2..7ecd5a3 100644
drivers/mtd/nand/nand_base.c +1 -0
@@ -2548,6 +2548,7 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, {
 
 	int ret, maf_idx;
 	int tmp_id, tmp_manf;
 
+	int no_subpage = 0;
 
drivers/mtd/nand/nand_base.c +10 -0
 	/* Select the device */
 	chip->select_chip(mtd, 0);
@@ -2612,10 +2613,20 @@ static const struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd, if (!ret)
 
 		nand_flash_detect_non_onfi(mtd, chip, type, &busw);
 
+	/*
+	 * If the controller is incapable of subpage writes, force no subpage
+	 * writes. This has to be done here, otherwise UBI will complain.
+	 */
+	if (chip->options & NAND_NO_SUBPAGE_WRITE)
+		no_subpage = 1;
+
 
 	/* Get chip options, preserve non chip based options */
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
 
+	if (no_subpage)
+		chip->options |= NAND_NO_SUBPAGE_WRITE;
+
 
Quoted 6 lines
drivers/mtd/nand/nand_base.c static const struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2632
Quoted 4 lines
drivers/mtd/nand/nand_base.c +0 -0
 	/*
 
 	 * Set chip as a default. Board drivers can override it, if necessary
 	 */
Scott, ping ?

Quoted 4 lines

We could do that I guess, but couldn't we also just get rid of NAND_CHIPOPTIONS_MSK?
-Scott

Actually, maybe we should fix this in Linux first. Is the GPMI-NAND driver for i.MX28 applied already to mainline linux-mtd?

Veli-Pekka Peltola

2011-11-11 12:52 UTC #8 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 2 lines · Marek Vasut

Actually, maybe we should fix this in Linux first. Is the GPMI-NAND driver for i.MX28 applied already to mainline linux-mtd?

Yes, it is in 3.2-rc1:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=tree;f=drivers/mtd/nand/gpmi-nand

I have tried to figure out why it works. The driver sets same NAND_NO_SUBPAGE_WRITE flag but it is also dropped in nand_base. One difference is: chip->ecc.size is set to 1 instead of 512.

--
Veli-Pekka Peltola

Marek Vasut

author

2011-11-11 17:50 UTC #9 lore raw

PATCH 1/1 NAND: Let NAND_NO_SUBPAGE_WRITE through

Quoted 11 lines · Marek Vasut

Quoted 2 lines
Actually, maybe we should fix this in Linux first. Is the GPMI-NAND driver for i.MX28 applied already to mainline linux-mtd?
Yes, it is in 3.2-rc1:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=tree;f=drive
rs/mtd/nand/gpmi-nand
I have tried to figure out why it works. The driver sets same NAND_NO_SUBPAGE_WRITE flag but it is also dropped in nand_base. One difference is: chip->ecc.size is set to 1 instead of 512.

This is good, I'll look into it later today or tomorrow I hope. Thanks for your great help!