| Message ID | 1320459844-18243-1-git-send-email-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
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
> 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
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
> 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 ;-)
> 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 ?
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
> 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?
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
> 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
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 */
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(-)