| Message ID | 1342196904-13850-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | bf7a01bf7987b63b121d572b240c132ec44129c4 |
| Headers | show |
Dear Brian Norris, > The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It > silently masks off at least one flag that might be set by the driver > (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly > others. > > Really, as long as driver writers exercise a small amount of care with > NAND_* options, this mask is not necessary at all; it was only here to > prevent certain options from accidentally being set by the driver. But the > original thought turns out to be a bad idea occasionally. Thus, kill it. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Cc: Huang Shijie <shijie8@gmail.com> > Cc: Marek Vasut <marex@denx.de> [...] Maybe we should simply split it into two sets of flags to make it clear -- chip ones and controller ones ? Best regards, Marek Vasut
On 07/13/2012 12:12 PM, Marek Vasut wrote: > Dear Brian Norris, > >> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It >> silently masks off at least one flag that might be set by the driver >> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly >> others. >> >> Really, as long as driver writers exercise a small amount of care with >> NAND_* options, this mask is not necessary at all; it was only here to >> prevent certain options from accidentally being set by the driver. But the >> original thought turns out to be a bad idea occasionally. Thus, kill it. >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> Cc: Huang Shijie <shijie8@gmail.com> >> Cc: Marek Vasut <marex@denx.de> > [...] > > Maybe we should simply split it into two sets of flags to make it clear -- chip > ones and controller ones ? Isn't that what we're trying to get rid of? -Scott
On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote: > On 07/13/2012 12:12 PM, Marek Vasut wrote: >>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It >>> silently masks off at least one flag that might be set by the driver >>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly >>> others. >>> >>> Really, as long as driver writers exercise a small amount of care with >>> NAND_* options, this mask is not necessary at all; it was only here to >>> prevent certain options from accidentally being set by the driver. But the >>> original thought turns out to be a bad idea occasionally. Thus, kill it. >>> >>> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >>> Cc: Huang Shijie <shijie8@gmail.com> >>> Cc: Marek Vasut <marex@denx.de> >> [...] >> >> Maybe we should simply split it into two sets of flags to make it clear -- chip >> ones and controller ones ? > > Isn't that what we're trying to get rid of? I thought so. Brian
Em 13-07-2012 14:19, Scott Wood escreveu: > On 07/13/2012 12:12 PM, Marek Vasut wrote: >> Dear Brian Norris, >> >>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It >>> silently masks off at least one flag that might be set by the driver >>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly >>> others. >>> >>> Really, as long as driver writers exercise a small amount of care with >>> NAND_* options, this mask is not necessary at all; it was only here to >>> prevent certain options from accidentally being set by the driver. But the >>> original thought turns out to be a bad idea occasionally. Thus, kill it. >>> >>> Signed-off-by: Brian Norris<computersforpeace@gmail.com> >>> Cc: Huang Shijie<shijie8@gmail.com> >>> Cc: Marek Vasut<marex@denx.de> >> [...] >> >> Maybe we should simply split it into two sets of flags to make it clear -- chip >> ones and controller ones ? > Isn't that what we're trying to get rid of? > > -Scott > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
Em 13-07-2012 14:12, Marek Vasut escreveu: > Dear Brian Norris, > >> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It >> silently masks off at least one flag that might be set by the driver >> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly >> others. >> >> Really, as long as driver writers exercise a small amount of care with >> NAND_* options, this mask is not necessary at all; it was only here to >> prevent certain options from accidentally being set by the driver. But the >> original thought turns out to be a bad idea occasionally. Thus, kill it. >> >> Signed-off-by: Brian Norris<computersforpeace@gmail.com> >> Cc: Huang Shijie<shijie8@gmail.com> >> Cc: Marek Vasut<marex@denx.de> > [...] > > Maybe we should simply split it into two sets of flags to make it clear -- chip > ones and controller ones ? > > Best regards, > Marek Vasut > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > . >
Dear Brian Norris, > On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote: > > On 07/13/2012 12:12 PM, Marek Vasut wrote: > >>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. > >>> It silently masks off at least one flag that might be set by the > >>> driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and > >>> possibly others. > >>> > >>> Really, as long as driver writers exercise a small amount of care with > >>> NAND_* options, this mask is not necessary at all; it was only here to > >>> prevent certain options from accidentally being set by the driver. But > >>> the original thought turns out to be a bad idea occasionally. Thus, > >>> kill it. > >>> > >>> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > >>> Cc: Huang Shijie <shijie8@gmail.com> > >>> Cc: Marek Vasut <marex@denx.de> > >> > >> [...] > >> > >> Maybe we should simply split it into two sets of flags to make it clear > >> -- chip ones and controller ones ? > > > > Isn't that what we're trying to get rid of? Sure, but aren't there controller specific properties and chip-specific properties? And than even chip-specific state? > I thought so. > > Brian Best regards, Marek Vasut
On 07/13/2012 12:35 PM, Marek Vasut wrote: > Dear Brian Norris, > >> On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote: >>> On 07/13/2012 12:12 PM, Marek Vasut wrote: >>>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. >>>>> It silently masks off at least one flag that might be set by the >>>>> driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and >>>>> possibly others. >>>>> >>>>> Really, as long as driver writers exercise a small amount of care with >>>>> NAND_* options, this mask is not necessary at all; it was only here to >>>>> prevent certain options from accidentally being set by the driver. But >>>>> the original thought turns out to be a bad idea occasionally. Thus, >>>>> kill it. >>>>> >>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >>>>> Cc: Huang Shijie <shijie8@gmail.com> >>>>> Cc: Marek Vasut <marex@denx.de> >>>> >>>> [...] >>>> >>>> Maybe we should simply split it into two sets of flags to make it clear >>>> -- chip ones and controller ones ? >>> >>> Isn't that what we're trying to get rid of? > > Sure, but aren't there controller specific properties and chip-specific > properties? Yes, but there are some properties than can be specified by either one. If you're proposing three sets of options (chip, controller, or both), or duplicating certain options between the sets and then checking both everywhere, I don't see how that's an improvement. > And than even chip-specific state? Not sure what you mean here. -Scott
Dear Scott Wood, > On 07/13/2012 12:35 PM, Marek Vasut wrote: > > Dear Brian Norris, > > > >> On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote: > >>> On 07/13/2012 12:12 PM, Marek Vasut wrote: > >>>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real > >>>>> bugs. It silently masks off at least one flag that might be set by > >>>>> the driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver > >>>>> and possibly others. > >>>>> > >>>>> Really, as long as driver writers exercise a small amount of care > >>>>> with NAND_* options, this mask is not necessary at all; it was only > >>>>> here to prevent certain options from accidentally being set by the > >>>>> driver. But the original thought turns out to be a bad idea > >>>>> occasionally. Thus, kill it. > >>>>> > >>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > >>>>> Cc: Huang Shijie <shijie8@gmail.com> > >>>>> Cc: Marek Vasut <marex@denx.de> > >>>> > >>>> [...] > >>>> > >>>> Maybe we should simply split it into two sets of flags to make it > >>>> clear -- chip ones and controller ones ? > >>> > >>> Isn't that what we're trying to get rid of? > > > > Sure, but aren't there controller specific properties and chip-specific > > properties? > > Yes, but there are some properties than can be specified by either one. > If you're proposing three sets of options (chip, controller, or both), > or duplicating certain options between the sets and then checking both > everywhere, I don't see how that's an improvement. Ok, you're right here, screw that and let's do a single set ;-) > > And than even chip-specific state? > > Not sure what you mean here. > > -Scott Best regards, Marek Vasut
On Fri, Jul 13, 2012 at 12:28 PM, Brian Norris <computersforpeace@gmail.com> wrote: > The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It > silently masks off at least one flag that might be set by the driver > (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly > others. > > Really, as long as driver writers exercise a small amount of care with > NAND_* options, this mask is not necessary at all; it was only here to > prevent certain options from accidentally being set by the driver. But the > original thought turns out to be a bad idea occasionally. Thus, kill it. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Cc: Huang Shijie <shijie8@gmail.com> > Cc: Marek Vasut <marex@denx.de> > --- > Huang, please test this with your driver. > I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA. it works fine, thanks. Tested-by: Huang Shijie <shijie8@gmail.com> > Artem, > > If this is acceptable, should it go to -stable, since it fixes some major > gpmi-nand breakage? It probably won't apply directly, since I've touched some > code around it recently. > > drivers/mtd/nand/nand_base.c | 7 ++----- > include/linux/mtd/nand.h | 3 --- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 98ba46e..6652d1f 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2909,8 +2909,6 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > if (le16_to_cpu(p->features) & 1) > *busw = NAND_BUSWIDTH_16; > > - chip->options &= ~NAND_CHIPOPTIONS_MSK; > - > pr_info("ONFI flash detected\n"); > return 1; > } > @@ -3074,9 +3072,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > mtd->erasesize <<= ((id_data[3] & 0x03) << 1); > } > } > - /* Get chip options, preserve non chip based options */ > - chip->options &= ~NAND_CHIPOPTIONS_MSK; > - chip->options |= type->options & NAND_CHIPOPTIONS_MSK; > + /* Get chip options */ > + chip->options |= type->options; > > /* > * Check if chip is not a Samsung device. Do not clear the > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 6dce5a7..eeb7015 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -206,9 +206,6 @@ typedef enum { > #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \ > && (chip->page_shift > 9)) > > -/* Mask to zero out the chip options, which come from the id table */ > -#define NAND_CHIPOPTIONS_MSK 0x0000ffff > - > /* Non chip related options */ > /* This option skips the bbt scan during initialization. */ > #define NAND_SKIP_BBTSCAN 0x00010000 > -- > 1.7.11.1 >
Bump. On Fri, Jul 13, 2012 at 7:02 PM, Huang Shijie <shijie8@gmail.com> wrote: > On Fri, Jul 13, 2012 at 12:28 PM, Brian Norris > <computersforpeace@gmail.com> wrote: >> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It >> silently masks off at least one flag that might be set by the driver >> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly >> others. >> >> Really, as long as driver writers exercise a small amount of care with >> NAND_* options, this mask is not necessary at all; it was only here to >> prevent certain options from accidentally being set by the driver. But the >> original thought turns out to be a bad idea occasionally. Thus, kill it. >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> Cc: Huang Shijie <shijie8@gmail.com> >> Cc: Marek Vasut <marex@denx.de> >> --- >> Huang, please test this with your driver. >> > I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA. > it works fine, thanks. > > Tested-by: Huang Shijie <shijie8@gmail.com> Any other Acks? Or acceptance from Artem/David? Brian
On Fri, 2012-07-13 at 09:28 -0700, Brian Norris wrote: > The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It > silently masks off at least one flag that might be set by the driver > (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly > others. > > Really, as long as driver writers exercise a small amount of care with > NAND_* options, this mask is not necessary at all; it was only here to > prevent certain options from accidentally being set by the driver. But the > original thought turns out to be a bad idea occasionally. Thus, kill it. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Pushed to l2-mtd.git, thanks. > If this is acceptable, should it go to -stable, since it fixes some major > gpmi-nand breakage? It probably won't apply directly, since I've touched some > code around it recently. Done.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 98ba46e..6652d1f 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2909,8 +2909,6 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, if (le16_to_cpu(p->features) & 1) *busw = NAND_BUSWIDTH_16; - chip->options &= ~NAND_CHIPOPTIONS_MSK; - pr_info("ONFI flash detected\n"); return 1; } @@ -3074,9 +3072,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, mtd->erasesize <<= ((id_data[3] & 0x03) << 1); } } - /* Get chip options, preserve non chip based options */ - chip->options &= ~NAND_CHIPOPTIONS_MSK; - chip->options |= type->options & NAND_CHIPOPTIONS_MSK; + /* Get chip options */ + chip->options |= type->options; /* * Check if chip is not a Samsung device. Do not clear the diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 6dce5a7..eeb7015 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -206,9 +206,6 @@ typedef enum { #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \ && (chip->page_shift > 9)) -/* Mask to zero out the chip options, which come from the id table */ -#define NAND_CHIPOPTIONS_MSK 0x0000ffff - /* Non chip related options */ /* This option skips the bbt scan during initialization. */ #define NAND_SKIP_BBTSCAN 0x00010000
The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It silently masks off at least one flag that might be set by the driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly others. Really, as long as driver writers exercise a small amount of care with NAND_* options, this mask is not necessary at all; it was only here to prevent certain options from accidentally being set by the driver. But the original thought turns out to be a bad idea occasionally. Thus, kill it. Signed-off-by: Brian Norris <computersforpeace@gmail.com> Cc: Huang Shijie <shijie8@gmail.com> Cc: Marek Vasut <marex@denx.de> --- Huang, please test this with your driver. Artem, If this is acceptable, should it go to -stable, since it fixes some major gpmi-nand breakage? It probably won't apply directly, since I've touched some code around it recently. drivers/mtd/nand/nand_base.c | 7 ++----- include/linux/mtd/nand.h | 3 --- 2 files changed, 2 insertions(+), 8 deletions(-)