[PATCH] mtd: add a new macro about the subpage write

[PATCH] mtd: add a new macro about the subpage write

[Huang Shijie]

From: Huang Shijie <shijie8@gmail.com>

The nand chip may does not support the subpage writes, while the nand
controller may also does not support the subpage writes too.

Now, the nand controller drivers use the NAND_NO_SUBPAGE_WRITE(0x00000200)
to tell the MTD layer that they do not support the subpage writes. But
this flag could be masked off in nand_get_flash_type():
	....................................................
	/* Get chip options, preserve non chip based options */
	chip->options &= ~NAND_CHIPOPTIONS_MSK;
	....................................................

So in nand_scan_tail(), there is a risk that the subpage write could be
enabled by mistake:
	....................................................
	if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
	    !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
	....................................................

Absolutely, this is what we do not want.

So add a new macro NAND_CONTROLLER_NO_SUBPAGE_WRITE, which is only used
by the nand controller, and it could not be masked off.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/docg4.c               |    2 +-
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    2 +-
 drivers/mtd/nand/nand_base.c           |    3 ++-
 include/linux/mtd/nand.h               |    3 +++
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index a225e49..85e74b4 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -1193,7 +1193,7 @@ static void __init init_mtd_structs(struct mtd_info *mtd)
 	nand->ecc.prepad = 8;
 	nand->ecc.bytes	= 8;
 	nand->ecc.strength = DOCG4_T;
-	nand->options = NAND_BUSWIDTH_16 | NAND_NO_SUBPAGE_WRITE;
+	nand->options = NAND_BUSWIDTH_16 | NAND_CONTROLLER_NO_SUBPAGE_WRITE;
 	nand->IO_ADDR_R = nand->IO_ADDR_W = doc->virtadr + DOC_IOSPACE_DATA;
 	nand->controller = &nand->hwcontrol;
 	spin_lock_init(&nand->controller->lock);
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index b821517..5a90df1 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1556,7 +1556,7 @@ static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->scan_bbt		= gpmi_scan_bbt;
 	chip->badblock_pattern	= &gpmi_bbt_descr;
 	chip->block_markbad	= gpmi_block_markbad;
-	chip->options		|= NAND_NO_SUBPAGE_WRITE;
+	chip->options		|= NAND_CONTROLLER_NO_SUBPAGE_WRITE;
 	chip->ecc.mode		= NAND_ECC_HW;
 	chip->ecc.size		= 1;
 	chip->ecc.strength	= 8;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d47586c..c58df65 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3453,7 +3453,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 	chip->ecc.total = chip->ecc.steps * chip->ecc.bytes;
 
 	/* Allow subpage writes up to ecc.steps. Not possible for MLC flash */
-	if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
+	if (!(chip->options &
+		(NAND_NO_SUBPAGE_WRITE | NAND_CONTROLLER_NO_SUBPAGE_WRITE)) &&
 	    !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
 		switch (chip->ecc.steps) {
 		case 2:
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 57977c6..cd62dd8 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -226,6 +226,9 @@ typedef enum {
 /* Chip may not exist, so silence any errors in scan */
 #define NAND_SCAN_SILENT_NODEV	0x00040000
 
+/* The nand controller does not support the subpage write. */
+#define NAND_CONTROLLER_NO_SUBPAGE_WRITE	0x00080000
+
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */
 #define NAND_CONTROLLER_ALLOC	0x80000000
-- 
1.7.0.4

Re: [PATCH] mtd: add a new macro about the subpage write

[Brian Norris]

Hi Huang,

On Mon, Jul 2, 2012 at 10:32 PM, Huang Shijie <b32955@freescale.com> wrote:
> The nand chip may does not support the subpage writes, while the nand
> controller may also does not support the subpage writes too.
>
> Now, the nand controller drivers use the NAND_NO_SUBPAGE_WRITE(0x00000200)
> to tell the MTD layer that they do not support the subpage writes. But
> this flag could be masked off in nand_get_flash_type():
>         ....................................................
>         /* Get chip options, preserve non chip based options */
>         chip->options &= ~NAND_CHIPOPTIONS_MSK;
>         ....................................................
...

It looks like you're finally getting around to solving your
NO_SUBPAGE_WRITE problem. I believe the discussion was left hanging a
few months ago. In that discussion, I'm beginning to agree with Scott
Wood [1]; I don't see a good reason not to just kill the
NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
perform a few sanity tests, I think it'd be safe.

If somebody has a good reason not to remove NAND_CHIPOPTIONS_MSK,
though, I'd Ack this patch.

Regards,
Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-January/039144.html

Re: [PATCH] mtd: add a new macro about the subpage write

[Huang Shijie]

Hi Brian:
> Wood [1]; I don't see a good reason not to just kill the
> NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
> perform a few sanity tests, I think it'd be safe.
>
I think it's more clear in logic to add this new macro:
    The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do 
no support the subpage write;
    The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand 
controller such as gpmi nand.

But I will be happy if you submit a patch to fix this issue by removing 
the NAND_CHIPOPTIONS_MSK.

thanks
Huang Shijie

Re: [PATCH] mtd: add a new macro about the subpage write

[Marek Vasut]

Dear Huang Shijie,

> Hi Brian:
> > Wood [1]; I don't see a good reason not to just kill the
> > NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
> > perform a few sanity tests, I think it'd be safe.
> 
> I think it's more clear in logic to add this new macro:
>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do
> no support the subpage write;
>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
> controller such as gpmi nand.

It's not clearer at all. It's just more error-prone.

> But I will be happy if you submit a patch to fix this issue by removing
> the NAND_CHIPOPTIONS_MSK.

I'd be happy if the GPMI NAND driver was properly fixed, 6 months after 
reporting this bug, which is quite critical as UBI doesn't work because of that 
and it's being silently ignored.

> thanks
> Huang Shijie

Best regards,
Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write

[Huang Shijie]

On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Huang Shijie,
>
>> Hi Brian:
>> > Wood [1]; I don't see a good reason not to just kill the
>> > NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
>> > perform a few sanity tests, I think it'd be safe.
>>
>> I think it's more clear in logic to add this new macro:
>>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do
>> no support the subpage write;
>>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
>> controller such as gpmi nand.
>
> It's not clearer at all. It's just more error-prone.
ok, thanks. But I do not how to fix it now. I hope some one could give a patch.

Best Regards
Huang Shijie

>
>> But I will be happy if you submit a patch to fix this issue by removing
>> the NAND_CHIPOPTIONS_MSK.
>
> I'd be happy if the GPMI NAND driver was properly fixed, 6 months after
> reporting this bug, which is quite critical as UBI doesn't work because of that
> and it's being silently ignored.
>
>> thanks
>> Huang Shijie
>
> Best regards,
> Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write

[Marek Vasut]

Dear Huang Shijie,

> On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Huang Shijie,
> > 
> >> Hi Brian:
> >> > Wood [1]; I don't see a good reason not to just kill the
> >> > NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
> >> > perform a few sanity tests, I think it'd be safe.
> >> 
> >> I think it's more clear in logic to add this new macro:
> >>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do
> >> 
> >> no support the subpage write;
> >> 
> >>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
> >> 
> >> controller such as gpmi nand.
> > 
> > It's not clearer at all. It's just more error-prone.
> 
> ok, thanks. But I do not how to fix it now. I hope some one could give a
> patch.

Why not remove the mask?

> Best Regards
> Huang Shijie
> 
> >> But I will be happy if you submit a patch to fix this issue by removing
> >> the NAND_CHIPOPTIONS_MSK.
> > 
> > I'd be happy if the GPMI NAND driver was properly fixed, 6 months after
> > reporting this bug, which is quite critical as UBI doesn't work because
> > of that and it's being silently ignored.
> > 
> >> thanks
> >> Huang Shijie
> > 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write

[Huang Shijie]

On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Huang Shijie,
>
>> On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Huang Shijie,
>> >
>> >> Hi Brian:
>> >> > Wood [1]; I don't see a good reason not to just kill the
>> >> > NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
>> >> > perform a few sanity tests, I think it'd be safe.
>> >>
>> >> I think it's more clear in logic to add this new macro:
>> >>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do
>> >>
>> >> no support the subpage write;
>> >>
>> >>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
>> >>
>> >> controller such as gpmi nand.
>> >
>> > It's not clearer at all. It's just more error-prone.
>>
>> ok, thanks. But I do not how to fix it now. I hope some one could give a
>> patch.
>
> Why not remove the mask?
>

I do not understand why this line was added here,  was it added on purpose?
so I am not sure whether we can just remove this line.


Best Regards
Huang Shijie

Re: [PATCH] mtd: add a new macro about the subpage write

[Marek Vasut]

Dear Huang Shijie,

> On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Huang Shijie,
> > 
> >> On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
> >> > Dear Huang Shijie,
> >> > 
> >> >> Hi Brian:
> >> >> > Wood [1]; I don't see a good reason not to just kill the
> >> >> > NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
> >> >> > perform a few sanity tests, I think it'd be safe.
> >> >> 
> >> >> I think it's more clear in logic to add this new macro:
> >> >>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which
> >> >>     do
> >> >> 
> >> >> no support the subpage write;
> >> >> 
> >> >>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
> >> >> 
> >> >> controller such as gpmi nand.
> >> > 
> >> > It's not clearer at all. It's just more error-prone.
> >> 
> >> ok, thanks. But I do not how to fix it now. I hope some one could give a
> >> patch.
> > 
> > Why not remove the mask?
> 
> I do not understand why this line was added here,  was it added on purpose?
> so I am not sure whether we can just remove this line.

Investigate, you're the author and you have an unresolved bug sitting there for 
a while ;-)

Check where the masked values are used and if they has to be masked at all, if 
it can't be done in some easier way.

> Best Regards
> Huang Shijie

Best regards,
Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write

[Scott Wood]

On 07/13/2012 11:08 AM, Huang Shijie wrote:
> On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
>> Dear Huang Shijie,
>>
>>> On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
>>>> Dear Huang Shijie,
>>>>
>>>>> Hi Brian:
>>>>>> Wood [1]; I don't see a good reason not to just kill the
>>>>>> NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
>>>>>> perform a few sanity tests, I think it'd be safe.
>>>>>
>>>>> I think it's more clear in logic to add this new macro:
>>>>>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which do
>>>>>
>>>>> no support the subpage write;
>>>>>
>>>>>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
>>>>>
>>>>> controller such as gpmi nand.
>>>>
>>>> It's not clearer at all. It's just more error-prone.
>>>
>>> ok, thanks. But I do not how to fix it now. I hope some one could give a
>>> patch.
>>
>> Why not remove the mask?
>>
> 
> I do not understand why this line was added here,  was it added on purpose?
> so I am not sure whether we can just remove this line.

If whoever wanted that line to be there cared enough, they could have
justified it with a comment (in the code, in the changelog, or in one of
these threads).  We can't just let cruft sit there (or worse, produce
more cruft to work around existing cruft) just because we don't know
exactly what the original author was thinking.  It appears to just have
been a misguided attempt at enforcing any given option to come from only
one place.

-Scott

Re: [PATCH] mtd: add a new macro about the subpage write

[Marek Vasut]

Dear Scott Wood,

> On 07/13/2012 11:08 AM, Huang Shijie wrote:
> > On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
> >> Dear Huang Shijie,
> >> 
> >>> On Fri, Jul 13, 2012 at 6:35 AM, Marek Vasut <marex@denx.de> wrote:
> >>>> Dear Huang Shijie,
> >>>> 
> >>>>> Hi Brian:
> >>>>>> Wood [1]; I don't see a good reason not to just kill the
> >>>>>> NAND_CHIPOPTIONS_MSK instead of adding more flags. As long as we
> >>>>>> perform a few sanity tests, I think it'd be safe.
> >>>>> 
> >>>>> I think it's more clear in logic to add this new macro:
> >>>>>     The NAND_NO_SUBPAGE_WRITE can be used only by the MLC nands which
> >>>>>     do
> >>>>> 
> >>>>> no support the subpage write;
> >>>>> 
> >>>>>     The NAND_CONTROLLER_NO_SUBPAGE_WRITE only used by the nand
> >>>>> 
> >>>>> controller such as gpmi nand.
> >>>> 
> >>>> It's not clearer at all. It's just more error-prone.
> >>> 
> >>> ok, thanks. But I do not how to fix it now. I hope some one could give
> >>> a patch.
> >> 
> >> Why not remove the mask?
> > 
> > I do not understand why this line was added here,  was it added on
> > purpose? so I am not sure whether we can just remove this line.
> 
> If whoever wanted that line to be there cared enough, they could have
> justified it with a comment (in the code, in the changelog, or in one of
> these threads).  We can't just let cruft sit there (or worse, produce
> more cruft to work around existing cruft) just because we don't know
> exactly what the original author was thinking.  It appears to just have
> been a misguided attempt at enforcing any given option to come from only
> one place.

I think it was there to allow having two different chips on the same NAND bus 
... or something. But this is just a guess. Anyway, it proved irrelevant, so 
let's drop it.

> -Scott

Best regards,
Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write

[Brian Norris]

On Fri, Jul 13, 2012 at 9:53 AM, Marek Vasut <marex@denx.de> wrote:
>> On 07/13/2012 11:08 AM, Huang Shijie wrote:
>> > On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
>> >> Why not remove the mask?
>> >
>> > I do not understand why this line was added here,  was it added on
>> > purpose? so I am not sure whether we can just remove this line.
>>
>> If whoever wanted that line to be there cared enough, they could have
>> justified it with a comment (in the code, in the changelog, or in one of
>> these threads).  We can't just let cruft sit there (or worse, produce
>> more cruft to work around existing cruft) just because we don't know
>> exactly what the original author was thinking.  It appears to just have
>> been a misguided attempt at enforcing any given option to come from only
>> one place.
>
> I think it was there to allow having two different chips on the same NAND bus
> ... or something. But this is just a guess. Anyway, it proved irrelevant, so
> let's drop it.

I agree. Then review/test/ack my patch that I just sent :)

Brian

Re: [PATCH] mtd: add a new macro about the subpage write

[Marek Vasut]

Dear Brian Norris,

> On Fri, Jul 13, 2012 at 9:53 AM, Marek Vasut <marex@denx.de> wrote:
> >> On 07/13/2012 11:08 AM, Huang Shijie wrote:
> >> > On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut <marex@denx.de> wrote:
> >> >> Why not remove the mask?
> >> > 
> >> > I do not understand why this line was added here,  was it added on
> >> > purpose? so I am not sure whether we can just remove this line.
> >> 
> >> If whoever wanted that line to be there cared enough, they could have
> >> justified it with a comment (in the code, in the changelog, or in one of
> >> these threads).  We can't just let cruft sit there (or worse, produce
> >> more cruft to work around existing cruft) just because we don't know
> >> exactly what the original author was thinking.  It appears to just have
> >> been a misguided attempt at enforcing any given option to come from only
> >> one place.
> > 
> > I think it was there to allow having two different chips on the same NAND
> > bus ... or something. But this is just a guess. Anyway, it proved
> > irrelevant, so let's drop it.
> 
> I agree. Then review/test/ack my patch that I just sent :)

It's in the queue, as I'm really interested to see this finally fixed.

> Brian

Best regards,
Marek Vasut

Re: [PATCH] mtd: add a new macro about the subpage write (REPORT SPAM)

[William F.]

Em 13-07-2012 14:10, Marek Vasut escreveu:
> Dear Brian Norris,
>
>> On Fri, Jul 13, 2012 at 9:53 AM, Marek Vasut<marex@denx.de>  wrote:
>>>> On 07/13/2012 11:08 AM, Huang Shijie wrote:
>>>>> On Fri, Jul 13, 2012 at 11:40 AM, Marek Vasut<marex@denx.de>  wrote:
>>>>>> Why not remove the mask?
>>>>> I do not understand why this line was added here,  was it added on
>>>>> purpose? so I am not sure whether we can just remove this line.
>>>> If whoever wanted that line to be there cared enough, they could have
>>>> justified it with a comment (in the code, in the changelog, or in one of
>>>> these threads).  We can't just let cruft sit there (or worse, produce
>>>> more cruft to work around existing cruft) just because we don't know
>>>> exactly what the original author was thinking.  It appears to just have
>>>> been a misguided attempt at enforcing any given option to come from only
>>>> one place.
>>> I think it was there to allow having two different chips on the same NAND
>>> bus ... or something. But this is just a guess. Anyway, it proved
>>> irrelevant, so let's drop it.
>> I agree. Then review/test/ack my patch that I just sent :)
> It's in the queue, as I'm really interested to see this finally fixed.
>
>> Brian
> Best regards,
> Marek Vasut
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>