Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

Huang Shijie
2012-07-14T02:02:10Z

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
>