[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

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

This was discussed already with no conclusion, see thread:
Message-Id: <1302372335-30232-6-git-send-email-sbabic@denx.de>

The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with
UBI on the MX28 CPU:
Message-ID: <4EB3E4EA.9080509@bluegiga.com>

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Veli-Pekka Peltola <veli-pekka.peltola@bluegiga.com>
---
 drivers/mtd/nand/nand_base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 6aac6a2..7ecd5a3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -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
 	 */
-- 
1.7.6.3

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Stefano Babic]

On 11/05/2011 03:24 AM, Marek Vasut wrote:
> 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,

> This was discussed already with no conclusion, see thread:
> Message-Id: <1302372335-30232-6-git-send-email-sbabic@denx.de>

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.

> 
> The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble with
> UBI on the MX28 CPU:
> Message-ID: <4EB3E4EA.9080509@bluegiga.com>

>  drivers/mtd/nand/nand_base.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 6aac6a2..7ecd5a3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -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;
> +

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

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

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Marek Vasut]

> On 11/05/2011 03:24 AM, Marek Vasut wrote:
> > 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,
> 
> > This was discussed already with no conclusion, see thread:
> > Message-Id: <1302372335-30232-6-git-send-email-sbabic@denx.de>
> 
> 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.
> 
> > The bug was recently retriggered by Veli-Pekka Peltola, causing him
> > trouble with UBI on the MX28 CPU:
> > Message-ID: <4EB3E4EA.9080509@bluegiga.com>
> > 
> >  drivers/mtd/nand/nand_base.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 6aac6a2..7ecd5a3 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -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;
> > +
> 
> 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

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Stefano Babic]

On 11/06/2011 01:43 PM, Marek Vasut wrote:
> 
> 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

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

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

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Marek Vasut]

> On 11/06/2011 01:43 PM, Marek Vasut wrote:
> > 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.

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

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[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.
> 
> This was discussed already with no conclusion, see thread:
> Message-Id: <1302372335-30232-6-git-send-email-sbabic@denx.de>
> 
> The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble
> with UBI on the MX28 CPU:
> Message-ID: <4EB3E4EA.9080509@bluegiga.com>
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Veli-Pekka Peltola <veli-pekka.peltola@bluegiga.com>
> ---
>  drivers/mtd/nand/nand_base.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 6aac6a2..7ecd5a3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -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
>  	 */

Scott, ping ?

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Scott Wood]

On Thu, Nov 10, 2011 at 06:02:36PM +0100, Marek Vasut wrote:
> > 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: <1302372335-30232-6-git-send-email-sbabic@denx.de>
> > 
> > The bug was recently retriggered by Veli-Pekka Peltola, causing him trouble
> > with UBI on the MX28 CPU:
> > Message-ID: <4EB3E4EA.9080509@bluegiga.com>
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Cc: Veli-Pekka Peltola <veli-pekka.peltola@bluegiga.com>
> > ---
> >  drivers/mtd/nand/nand_base.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 6aac6a2..7ecd5a3 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -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
> >  	 */
> 
> Scott, ping ?

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

-Scott

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Marek Vasut]

> On Thu, Nov 10, 2011 at 06:02:36PM +0100, Marek Vasut wrote:
> > > 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: <1302372335-30232-6-git-send-email-sbabic@denx.de>
> > > 
> > > The bug was recently retriggered by Veli-Pekka Peltola, causing him
> > > trouble with UBI on the MX28 CPU:
> > > Message-ID: <4EB3E4EA.9080509@bluegiga.com>
> > > 
> > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: Scott Wood <scottwood@freescale.com>
> > > Cc: Veli-Pekka Peltola <veli-pekka.peltola@bluegiga.com>
> > > ---
> > > 
> > >  drivers/mtd/nand/nand_base.c |   11 +++++++++++
> > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c
> > > b/drivers/mtd/nand/nand_base.c index 6aac6a2..7ecd5a3 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -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
> > >  	 */
> > 
> > Scott, ping ?
> 
> 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?

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Veli-Pekka Peltola]

On 11/10/2011 07:44 PM, Marek Vasut wrote:
> 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

[U-Boot] [PATCH] NAND: Let NAND_NO_SUBPAGE_WRITE through

[Marek Vasut]

> On 11/10/2011 07:44 PM, Marek Vasut wrote:
> > 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!

M