| Message ID | 1341293533-2214-1-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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/ >
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