diff mbox series

mtd: rawnand: check nand support for cache reads

Message ID 20230922100116.145090-1-r.czerwinski@pengutronix.de
State New
Headers
Series mtd: rawnand: check nand support for cache reads |

Commit Message

Rouven Czerwinski Sept. 22, 2023, 10:01 a.m. UTC 
  Both the JEDEC and ONFI specification say that read cache sequential
support is an optional command. This means that we not only need to
check whether the individual controller implements the command, we also
need to check the parameter pages for both ONFI and JEDEC NAND flashes
before enabling sequential cache reads.

This fixes support for NAND flashes which don't support enabling cache
reads, i.e. Samsung K9F4G08U0F or Toshiba TC58NVG0S3HTA00.

Sequential cache reads are no only available for ONFI and JEDEC devices,
if individual vendors implement this, it needs to be enabled per vendor.

Tested on i.MX6Q with a Samsung NAND flash chip that doesn't support
sequential reads.

Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")

Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
---
@Martin, Måns:
I would appreciate if you could test this on your hardware.

@Miguel:
I didn't have the time to test this on ONFI/JEDEC devices with support
yet, I'd be fine if you hold off merging this.

 drivers/mtd/nand/raw/nand_base.c  | 3 +++
 drivers/mtd/nand/raw/nand_jedec.c | 3 +++
 drivers/mtd/nand/raw/nand_onfi.c  | 3 +++
 include/linux/mtd/jedec.h         | 3 +++
 include/linux/mtd/onfi.h          | 1 +
 include/linux/mtd/rawnand.h       | 1 +
 6 files changed, 14 insertions(+)

Comments

Martin Hundebøll Sept. 22, 2023, 10:04 a.m. UTC | #1 
On Fri, 2023-09-22 at 12:01 +0200, Rouven Czerwinski wrote:
> Both the JEDEC and ONFI specification say that read cache sequential
> support is an optional command. This means that we not only need to
> check whether the individual controller implements the command, we
> also
> need to check the parameter pages for both ONFI and JEDEC NAND
> flashes
> before enabling sequential cache reads.
> 
> This fixes support for NAND flashes which don't support enabling
> cache
> reads, i.e. Samsung K9F4G08U0F or Toshiba TC58NVG0S3HTA00.
> 
> Sequential cache reads are no only available for ONFI and JEDEC
> devices,
> if individual vendors implement this, it needs to be enabled per
> vendor.
> 
> Tested on i.MX6Q with a Samsung NAND flash chip that doesn't support
> sequential reads.
> 
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache
> reads")
> 
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> @Martin, Måns:
> I would appreciate if you could test this on your hardware.
> 
> @Miguel:
> I didn't have the time to test this on ONFI/JEDEC devices with
> support
> yet, I'd be fine if you hold off merging this.
> 
>  drivers/mtd/nand/raw/nand_base.c  | 3 +++
>  drivers/mtd/nand/raw/nand_jedec.c | 3 +++
>  drivers/mtd/nand/raw/nand_onfi.c  | 3 +++
>  include/linux/mtd/jedec.h         | 3 +++
>  include/linux/mtd/onfi.h          | 1 +
>  include/linux/mtd/rawnand.h       | 1 +
>  6 files changed, 14 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index d4b55155aeae..1fcac403cee6 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5110,6 +5110,9 @@ static void
> rawnand_check_cont_read_support(struct nand_chip *chip)
>  {
>         struct mtd_info *mtd = nand_to_mtd(chip);
>  
> +       if (!chip->parameters.supports_read_cache)
> +               return;
> +
>         if (chip->read_retries)
>                 return;
>  
> diff --git a/drivers/mtd/nand/raw/nand_jedec.c
> b/drivers/mtd/nand/raw/nand_jedec.c
> index 836757717660..e6ecbc4b2493 100644
> --- a/drivers/mtd/nand/raw/nand_jedec.c
> +++ b/drivers/mtd/nand/raw/nand_jedec.c
> @@ -94,6 +94,9 @@ int nand_jedec_detect(struct nand_chip *chip)
>                 goto free_jedec_param_page;
>         }
>  
> +       if (p->opt_cmd[0] & JEDEC_OPT_CMD_READ_CACHE)
> +               chip->parameters.supports_read_cache;

Missing ` = true` here ?

> +
>         memorg->pagesize = le32_to_cpu(p->byte_per_page);
>         mtd->writesize = memorg->pagesize;
>  
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c
> b/drivers/mtd/nand/raw/nand_onfi.c
> index f15ef90aec8c..bf79bf2b5866 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -303,6 +303,9 @@ int nand_onfi_detect(struct nand_chip *chip)
>                            ONFI_FEATURE_ADDR_TIMING_MODE, 1);
>         }
>  
> +       if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_READ_CACHE)
> +               chip->parameters.supports_read_cache;

And here?

// Martin
> +
>         onfi = kzalloc(sizeof(*onfi), GFP_KERNEL);
>         if (!onfi) {
>                 ret = -ENOMEM;
> diff --git a/include/linux/mtd/jedec.h b/include/linux/mtd/jedec.h
> index 0b6b59f7cfbd..56047a4e54c9 100644
> --- a/include/linux/mtd/jedec.h
> +++ b/include/linux/mtd/jedec.h
> @@ -21,6 +21,9 @@ struct jedec_ecc_info {
>  /* JEDEC features */
>  #define JEDEC_FEATURE_16_BIT_BUS       (1 << 0)
>  
> +/* JEDEC Optional Commands */
> +#define JEDEC_OPT_CMD_READ_CACHE       BIT(1)
> +
>  struct nand_jedec_params {
>         /* rev info and features block */
>         /* 'J' 'E' 'S' 'D'  */
> diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
> index a7376f9beddf..55ab2e4d62f9 100644
> --- a/include/linux/mtd/onfi.h
> +++ b/include/linux/mtd/onfi.h
> @@ -55,6 +55,7 @@
>  #define ONFI_SUBFEATURE_PARAM_LEN      4
>  
>  /* ONFI optional commands SET/GET FEATURES supported? */
> +#define ONFI_OPT_CMD_READ_CACHE                BIT(1)
>  #define ONFI_OPT_CMD_SET_GET_FEATURES  BIT(2)
>  
>  struct nand_onfi_params {
> diff --git a/include/linux/mtd/rawnand.h
> b/include/linux/mtd/rawnand.h
> index 90a141ba2a5a..766856fcaba8 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -233,6 +233,7 @@ struct nand_parameters {
>         /* Generic parameters */
>         const char *model;
>         bool supports_set_get_features;
> +       bool supports_read_cache;
>         DECLARE_BITMAP(set_feature_list, ONFI_FEATURE_NUMBER);
>         DECLARE_BITMAP(get_feature_list, ONFI_FEATURE_NUMBER);
>
Rouven Czerwinski Sept. 22, 2023, 10:07 a.m. UTC | #2 
On Fri, 2023-09-22 at 12:04 +0200, Martin Hundebøll wrote:
> On Fri, 2023-09-22 at 12:01 +0200, Rouven Czerwinski wrote:
> > Both the JEDEC and ONFI specification say that read cache
> > sequential
> > support is an optional command. This means that we not only need to
> > check whether the individual controller implements the command, we
> > also
> > need to check the parameter pages for both ONFI and JEDEC NAND
> > flashes
> > before enabling sequential cache reads.
> > 
> > This fixes support for NAND flashes which don't support enabling
> > cache
> > reads, i.e. Samsung K9F4G08U0F or Toshiba TC58NVG0S3HTA00.
> > 
> > Sequential cache reads are no only available for ONFI and JEDEC
> > devices,
> > if individual vendors implement this, it needs to be enabled per
> > vendor.
> > 
> > Tested on i.MX6Q with a Samsung NAND flash chip that doesn't
> > support
> > sequential reads.
> > 
> > Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache
> > reads")
> > 
> > Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> > ---
> > @Martin, Måns:
> > I would appreciate if you could test this on your hardware.
> > 
> > @Miguel:
> > I didn't have the time to test this on ONFI/JEDEC devices with
> > support
> > yet, I'd be fine if you hold off merging this.
> > 
> >  drivers/mtd/nand/raw/nand_base.c  | 3 +++
> >  drivers/mtd/nand/raw/nand_jedec.c | 3 +++
> >  drivers/mtd/nand/raw/nand_onfi.c  | 3 +++
> >  include/linux/mtd/jedec.h         | 3 +++
> >  include/linux/mtd/onfi.h          | 1 +
> >  include/linux/mtd/rawnand.h       | 1 +
> >  6 files changed, 14 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index d4b55155aeae..1fcac403cee6 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5110,6 +5110,9 @@ static void
> > rawnand_check_cont_read_support(struct nand_chip *chip)
> >  {
> >         struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > +       if (!chip->parameters.supports_read_cache)
> > +               return;
> > +
> >         if (chip->read_retries)
> >                 return;
> >  
> > diff --git a/drivers/mtd/nand/raw/nand_jedec.c
> > b/drivers/mtd/nand/raw/nand_jedec.c
> > index 836757717660..e6ecbc4b2493 100644
> > --- a/drivers/mtd/nand/raw/nand_jedec.c
> > +++ b/drivers/mtd/nand/raw/nand_jedec.c
> > @@ -94,6 +94,9 @@ int nand_jedec_detect(struct nand_chip *chip)
> >                 goto free_jedec_param_page;
> >         }
> >  
> > +       if (p->opt_cmd[0] & JEDEC_OPT_CMD_READ_CACHE)
> > +               chip->parameters.supports_read_cache;
> 
> Missing ` = true` here ?
> 
> > +
> >         memorg->pagesize = le32_to_cpu(p->byte_per_page);
> >         mtd->writesize = memorg->pagesize;
> >  
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c
> > b/drivers/mtd/nand/raw/nand_onfi.c
> > index f15ef90aec8c..bf79bf2b5866 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -303,6 +303,9 @@ int nand_onfi_detect(struct nand_chip *chip)
> >                            ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> >         }
> >  
> > +       if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_READ_CACHE)
> > +               chip->parameters.supports_read_cache;
> 
> And here?
> 
> // Martin

Argh, totally. This should still fix your device, but cause performance
regressions on devices supporting cached sequential reads.
Unfortunately I don't have hardware to test this.

Fixed locally, I'll send a v2 later.

> > +
> >         onfi = kzalloc(sizeof(*onfi), GFP_KERNEL);
> >         if (!onfi) {
> >                 ret = -ENOMEM;
> > diff --git a/include/linux/mtd/jedec.h b/include/linux/mtd/jedec.h
> > index 0b6b59f7cfbd..56047a4e54c9 100644
> > --- a/include/linux/mtd/jedec.h
> > +++ b/include/linux/mtd/jedec.h
> > @@ -21,6 +21,9 @@ struct jedec_ecc_info {
> >  /* JEDEC features */
> >  #define JEDEC_FEATURE_16_BIT_BUS       (1 << 0)
> >  
> > +/* JEDEC Optional Commands */
> > +#define JEDEC_OPT_CMD_READ_CACHE       BIT(1)
> > +
> >  struct nand_jedec_params {
> >         /* rev info and features block */
> >         /* 'J' 'E' 'S' 'D'  */
> > diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
> > index a7376f9beddf..55ab2e4d62f9 100644
> > --- a/include/linux/mtd/onfi.h
> > +++ b/include/linux/mtd/onfi.h
> > @@ -55,6 +55,7 @@
> >  #define ONFI_SUBFEATURE_PARAM_LEN      4
> >  
> >  /* ONFI optional commands SET/GET FEATURES supported? */
> > +#define ONFI_OPT_CMD_READ_CACHE                BIT(1)
> >  #define ONFI_OPT_CMD_SET_GET_FEATURES  BIT(2)
> >  
> >  struct nand_onfi_params {
> > diff --git a/include/linux/mtd/rawnand.h
> > b/include/linux/mtd/rawnand.h
> > index 90a141ba2a5a..766856fcaba8 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -233,6 +233,7 @@ struct nand_parameters {
> >         /* Generic parameters */
> >         const char *model;
> >         bool supports_set_get_features;
> > +       bool supports_read_cache;
> >         DECLARE_BITMAP(set_feature_list, ONFI_FEATURE_NUMBER);
> >         DECLARE_BITMAP(get_feature_list, ONFI_FEATURE_NUMBER);
> >  
> 
>
kernel test robot Sept. 22, 2023, 11:20 a.m. UTC | #3 
Hi Rouven,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc2 next-20230921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rouven-Czerwinski/mtd-rawnand-check-nand-support-for-cache-reads/20230922-180317
base:   linus/master
patch link:    https://lore.kernel.org/r/20230922100116.145090-1-r.czerwinski%40pengutronix.de
patch subject: [PATCH] mtd: rawnand: check nand support for cache reads
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230922/202309221938.wpGSSUjJ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230922/202309221938.wpGSSUjJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309221938.wpGSSUjJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/mtd/nand/raw/nand_onfi.c: In function 'nand_onfi_detect':
>> drivers/mtd/nand/raw/nand_onfi.c:307:33: warning: statement with no effect [-Wunused-value]
     307 |                 chip->parameters.supports_read_cache;
         |                 ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
--
   drivers/mtd/nand/raw/nand_jedec.c: In function 'nand_jedec_detect':
>> drivers/mtd/nand/raw/nand_jedec.c:98:33: warning: statement with no effect [-Wunused-value]
      98 |                 chip->parameters.supports_read_cache;
         |                 ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~


vim +307 drivers/mtd/nand/raw/nand_onfi.c

   140	
   141	/*
   142	 * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
   143	 */
   144	int nand_onfi_detect(struct nand_chip *chip)
   145	{
   146		struct nand_device *base = &chip->base;
   147		struct mtd_info *mtd = nand_to_mtd(chip);
   148		struct nand_memory_organization *memorg;
   149		struct nand_onfi_params *p = NULL, *pbuf;
   150		struct onfi_params *onfi;
   151		bool use_datain = false;
   152		int onfi_version = 0;
   153		char id[4];
   154		int i, ret, val;
   155		u16 crc;
   156	
   157		memorg = nanddev_get_memorg(&chip->base);
   158	
   159		/* Try ONFI for unknown chip or LP */
   160		ret = nand_readid_op(chip, 0x20, id, sizeof(id));
   161		if (ret || strncmp(id, "ONFI", 4))
   162			return 0;
   163	
   164		/* ONFI chip: allocate a buffer to hold its parameter page */
   165		pbuf = kzalloc((sizeof(*pbuf) * ONFI_PARAM_PAGES), GFP_KERNEL);
   166		if (!pbuf)
   167			return -ENOMEM;
   168	
   169		if (!nand_has_exec_op(chip) || chip->controller->supported_op.data_only_read)
   170			use_datain = true;
   171	
   172		for (i = 0; i < ONFI_PARAM_PAGES; i++) {
   173			if (!i)
   174				ret = nand_read_param_page_op(chip, 0, &pbuf[i],
   175							      sizeof(*pbuf));
   176			else if (use_datain)
   177				ret = nand_read_data_op(chip, &pbuf[i], sizeof(*pbuf),
   178							true, false);
   179			else
   180				ret = nand_change_read_column_op(chip, sizeof(*pbuf) * i,
   181								 &pbuf[i], sizeof(*pbuf),
   182								 true);
   183			if (ret) {
   184				ret = 0;
   185				goto free_onfi_param_page;
   186			}
   187	
   188			crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254);
   189			if (crc == le16_to_cpu(pbuf[i].crc)) {
   190				p = &pbuf[i];
   191				break;
   192			}
   193		}
   194	
   195		if (i == ONFI_PARAM_PAGES) {
   196			const void *srcbufs[ONFI_PARAM_PAGES];
   197			unsigned int j;
   198	
   199			for (j = 0; j < ONFI_PARAM_PAGES; j++)
   200				srcbufs[j] = pbuf + j;
   201	
   202			pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
   203			nand_bit_wise_majority(srcbufs, ONFI_PARAM_PAGES, pbuf,
   204					       sizeof(*pbuf));
   205	
   206			crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)pbuf, 254);
   207			if (crc != le16_to_cpu(pbuf->crc)) {
   208				pr_err("ONFI parameter recovery failed, aborting\n");
   209				goto free_onfi_param_page;
   210			}
   211			p = pbuf;
   212		}
   213	
   214		if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
   215		    chip->manufacturer.desc->ops->fixup_onfi_param_page)
   216			chip->manufacturer.desc->ops->fixup_onfi_param_page(chip, p);
   217	
   218		/* Check version */
   219		val = le16_to_cpu(p->revision);
   220		if (val & ONFI_VERSION_2_3)
   221			onfi_version = 23;
   222		else if (val & ONFI_VERSION_2_2)
   223			onfi_version = 22;
   224		else if (val & ONFI_VERSION_2_1)
   225			onfi_version = 21;
   226		else if (val & ONFI_VERSION_2_0)
   227			onfi_version = 20;
   228		else if (val & ONFI_VERSION_1_0)
   229			onfi_version = 10;
   230	
   231		if (!onfi_version) {
   232			pr_info("unsupported ONFI version: %d\n", val);
   233			goto free_onfi_param_page;
   234		}
   235	
   236		sanitize_string(p->manufacturer, sizeof(p->manufacturer));
   237		sanitize_string(p->model, sizeof(p->model));
   238		chip->parameters.model = kstrdup(p->model, GFP_KERNEL);
   239		if (!chip->parameters.model) {
   240			ret = -ENOMEM;
   241			goto free_onfi_param_page;
   242		}
   243	
   244		memorg->pagesize = le32_to_cpu(p->byte_per_page);
   245		mtd->writesize = memorg->pagesize;
   246	
   247		/*
   248		 * pages_per_block and blocks_per_lun may not be a power-of-2 size
   249		 * (don't ask me who thought of this...). MTD assumes that these
   250		 * dimensions will be power-of-2, so just truncate the remaining area.
   251		 */
   252		memorg->pages_per_eraseblock =
   253				1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
   254		mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
   255	
   256		memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
   257		mtd->oobsize = memorg->oobsize;
   258	
   259		memorg->luns_per_target = p->lun_count;
   260		memorg->planes_per_lun = 1 << p->interleaved_bits;
   261	
   262		/* See erasesize comment */
   263		memorg->eraseblocks_per_lun =
   264			1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
   265		memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
   266		memorg->bits_per_cell = p->bits_per_cell;
   267	
   268		if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
   269			chip->options |= NAND_BUSWIDTH_16;
   270	
   271		if (p->ecc_bits != 0xff) {
   272			struct nand_ecc_props requirements = {
   273				.strength = p->ecc_bits,
   274				.step_size = 512,
   275			};
   276	
   277			nanddev_set_ecc_requirements(base, &requirements);
   278		} else if (onfi_version >= 21 &&
   279			(le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
   280	
   281			/*
   282			 * The nand_flash_detect_ext_param_page() uses the
   283			 * Change Read Column command which maybe not supported
   284			 * by the chip->legacy.cmdfunc. So try to update the
   285			 * chip->legacy.cmdfunc now. We do not replace user supplied
   286			 * command function.
   287			 */
   288			nand_legacy_adjust_cmdfunc(chip);
   289	
   290			/* The Extended Parameter Page is supported since ONFI 2.1. */
   291			if (nand_flash_detect_ext_param_page(chip, p))
   292				pr_warn("Failed to detect ONFI extended param page\n");
   293		} else {
   294			pr_warn("Could not retrieve ONFI ECC requirements\n");
   295		}
   296	
   297		/* Save some parameters from the parameter page for future use */
   298		if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
   299			chip->parameters.supports_set_get_features = true;
   300			bitmap_set(chip->parameters.get_feature_list,
   301				   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
   302			bitmap_set(chip->parameters.set_feature_list,
   303				   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
   304		}
   305	
   306		if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_READ_CACHE)
 > 307			chip->parameters.supports_read_cache;
Miquel Raynal Sept. 22, 2023, 2:04 p.m. UTC | #4 
Hi Rouven,

Thanks a lot for the investigation and the patch!

r.czerwinski@pengutronix.de wrote on Fri, 22 Sep 2023 12:01:13 +0200:

Would you mind changing the title to
"mtd: rawnand: Ensure the nand chip supports cached reads"

> Both the JEDEC and ONFI specification say that read cache sequential
> support is an optional command.

I clearly overlooked that part, just checking the set/get_features()
entries as usual, good catch.

> This means that we not only need to
> check whether the individual controller implements the command, we also

The controller itself does not implement the command, but may or may
not support it (can you please update the sentence?).

> need to check the parameter pages for both ONFI and JEDEC NAND flashes
> before enabling sequential cache reads.
> 
> This fixes support for NAND flashes which don't support enabling cache
> reads, i.e. Samsung K9F4G08U0F or Toshiba TC58NVG0S3HTA00.
> 
> Sequential cache reads are no only available for ONFI and JEDEC devices,
> if individual vendors implement this, it needs to be enabled per vendor.

Agreed.

> Tested on i.MX6Q with a Samsung NAND flash chip that doesn't support
> sequential reads.
> 
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> 

Please remove this empty line and instead add:

Cc: stable@vger.kernel.org

> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> @Martin, Måns:
> I would appreciate if you could test this on your hardware.

That would me much appreciated!

I also added Alexander who also had troubles with this patchset, could
you check on your setup if that solves the issue?

> @Miguel:
> I didn't have the time to test this on ONFI/JEDEC devices with support
> yet, I'd be fine if you hold off merging this.

Of course. I was about to send a revert but that looks a promising fix,
let's see how it goes.

> 
>  drivers/mtd/nand/raw/nand_base.c  | 3 +++
>  drivers/mtd/nand/raw/nand_jedec.c | 3 +++
>  drivers/mtd/nand/raw/nand_onfi.c  | 3 +++
>  include/linux/mtd/jedec.h         | 3 +++
>  include/linux/mtd/onfi.h          | 1 +
>  include/linux/mtd/rawnand.h       | 1 +
>  6 files changed, 14 insertions(+)
> 

Thanks,
Miquèl
kernel test robot Sept. 29, 2023, 5 p.m. UTC | #5 
Hi Rouven,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rouven-Czerwinski/mtd-rawnand-check-nand-support-for-cache-reads/20230922-180317
base:   linus/master
patch link:    https://lore.kernel.org/r/20230922100116.145090-1-r.czerwinski%40pengutronix.de
patch subject: [PATCH] mtd: rawnand: check nand support for cache reads
config: mips-qi_lb60_defconfig (https://download.01.org/0day-ci/archive/20230930/202309300033.zf83jfBL-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230930/202309300033.zf83jfBL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309300033.zf83jfBL-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mtd/nand/raw/nand_onfi.c:307:20: warning: expression result unused [-Wunused-value]
     307 |                 chip->parameters.supports_read_cache;
         |                 ~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~
   1 warning generated.
--
>> drivers/mtd/nand/raw/nand_jedec.c:98:20: warning: expression result unused [-Wunused-value]
      98 |                 chip->parameters.supports_read_cache;
         |                 ~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +307 drivers/mtd/nand/raw/nand_onfi.c

   140	
   141	/*
   142	 * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
   143	 */
   144	int nand_onfi_detect(struct nand_chip *chip)
   145	{
   146		struct nand_device *base = &chip->base;
   147		struct mtd_info *mtd = nand_to_mtd(chip);
   148		struct nand_memory_organization *memorg;
   149		struct nand_onfi_params *p = NULL, *pbuf;
   150		struct onfi_params *onfi;
   151		bool use_datain = false;
   152		int onfi_version = 0;
   153		char id[4];
   154		int i, ret, val;
   155		u16 crc;
   156	
   157		memorg = nanddev_get_memorg(&chip->base);
   158	
   159		/* Try ONFI for unknown chip or LP */
   160		ret = nand_readid_op(chip, 0x20, id, sizeof(id));
   161		if (ret || strncmp(id, "ONFI", 4))
   162			return 0;
   163	
   164		/* ONFI chip: allocate a buffer to hold its parameter page */
   165		pbuf = kzalloc((sizeof(*pbuf) * ONFI_PARAM_PAGES), GFP_KERNEL);
   166		if (!pbuf)
   167			return -ENOMEM;
   168	
   169		if (!nand_has_exec_op(chip) || chip->controller->supported_op.data_only_read)
   170			use_datain = true;
   171	
   172		for (i = 0; i < ONFI_PARAM_PAGES; i++) {
   173			if (!i)
   174				ret = nand_read_param_page_op(chip, 0, &pbuf[i],
   175							      sizeof(*pbuf));
   176			else if (use_datain)
   177				ret = nand_read_data_op(chip, &pbuf[i], sizeof(*pbuf),
   178							true, false);
   179			else
   180				ret = nand_change_read_column_op(chip, sizeof(*pbuf) * i,
   181								 &pbuf[i], sizeof(*pbuf),
   182								 true);
   183			if (ret) {
   184				ret = 0;
   185				goto free_onfi_param_page;
   186			}
   187	
   188			crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254);
   189			if (crc == le16_to_cpu(pbuf[i].crc)) {
   190				p = &pbuf[i];
   191				break;
   192			}
   193		}
   194	
   195		if (i == ONFI_PARAM_PAGES) {
   196			const void *srcbufs[ONFI_PARAM_PAGES];
   197			unsigned int j;
   198	
   199			for (j = 0; j < ONFI_PARAM_PAGES; j++)
   200				srcbufs[j] = pbuf + j;
   201	
   202			pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
   203			nand_bit_wise_majority(srcbufs, ONFI_PARAM_PAGES, pbuf,
   204					       sizeof(*pbuf));
   205	
   206			crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)pbuf, 254);
   207			if (crc != le16_to_cpu(pbuf->crc)) {
   208				pr_err("ONFI parameter recovery failed, aborting\n");
   209				goto free_onfi_param_page;
   210			}
   211			p = pbuf;
   212		}
   213	
   214		if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
   215		    chip->manufacturer.desc->ops->fixup_onfi_param_page)
   216			chip->manufacturer.desc->ops->fixup_onfi_param_page(chip, p);
   217	
   218		/* Check version */
   219		val = le16_to_cpu(p->revision);
   220		if (val & ONFI_VERSION_2_3)
   221			onfi_version = 23;
   222		else if (val & ONFI_VERSION_2_2)
   223			onfi_version = 22;
   224		else if (val & ONFI_VERSION_2_1)
   225			onfi_version = 21;
   226		else if (val & ONFI_VERSION_2_0)
   227			onfi_version = 20;
   228		else if (val & ONFI_VERSION_1_0)
   229			onfi_version = 10;
   230	
   231		if (!onfi_version) {
   232			pr_info("unsupported ONFI version: %d\n", val);
   233			goto free_onfi_param_page;
   234		}
   235	
   236		sanitize_string(p->manufacturer, sizeof(p->manufacturer));
   237		sanitize_string(p->model, sizeof(p->model));
   238		chip->parameters.model = kstrdup(p->model, GFP_KERNEL);
   239		if (!chip->parameters.model) {
   240			ret = -ENOMEM;
   241			goto free_onfi_param_page;
   242		}
   243	
   244		memorg->pagesize = le32_to_cpu(p->byte_per_page);
   245		mtd->writesize = memorg->pagesize;
   246	
   247		/*
   248		 * pages_per_block and blocks_per_lun may not be a power-of-2 size
   249		 * (don't ask me who thought of this...). MTD assumes that these
   250		 * dimensions will be power-of-2, so just truncate the remaining area.
   251		 */
   252		memorg->pages_per_eraseblock =
   253				1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
   254		mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
   255	
   256		memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
   257		mtd->oobsize = memorg->oobsize;
   258	
   259		memorg->luns_per_target = p->lun_count;
   260		memorg->planes_per_lun = 1 << p->interleaved_bits;
   261	
   262		/* See erasesize comment */
   263		memorg->eraseblocks_per_lun =
   264			1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
   265		memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
   266		memorg->bits_per_cell = p->bits_per_cell;
   267	
   268		if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
   269			chip->options |= NAND_BUSWIDTH_16;
   270	
   271		if (p->ecc_bits != 0xff) {
   272			struct nand_ecc_props requirements = {
   273				.strength = p->ecc_bits,
   274				.step_size = 512,
   275			};
   276	
   277			nanddev_set_ecc_requirements(base, &requirements);
   278		} else if (onfi_version >= 21 &&
   279			(le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
   280	
   281			/*
   282			 * The nand_flash_detect_ext_param_page() uses the
   283			 * Change Read Column command which maybe not supported
   284			 * by the chip->legacy.cmdfunc. So try to update the
   285			 * chip->legacy.cmdfunc now. We do not replace user supplied
   286			 * command function.
   287			 */
   288			nand_legacy_adjust_cmdfunc(chip);
   289	
   290			/* The Extended Parameter Page is supported since ONFI 2.1. */
   291			if (nand_flash_detect_ext_param_page(chip, p))
   292				pr_warn("Failed to detect ONFI extended param page\n");
   293		} else {
   294			pr_warn("Could not retrieve ONFI ECC requirements\n");
   295		}
   296	
   297		/* Save some parameters from the parameter page for future use */
   298		if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
   299			chip->parameters.supports_set_get_features = true;
   300			bitmap_set(chip->parameters.get_feature_list,
   301				   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
   302			bitmap_set(chip->parameters.set_feature_list,
   303				   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
   304		}
   305	
   306		if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_READ_CACHE)
 > 307			chip->parameters.supports_read_cache;
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d4b55155aeae..1fcac403cee6 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5110,6 +5110,9 @@  static void rawnand_check_cont_read_support(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
+	if (!chip->parameters.supports_read_cache)
+		return;
+
 	if (chip->read_retries)
 		return;
 
diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
index 836757717660..e6ecbc4b2493 100644
--- a/drivers/mtd/nand/raw/nand_jedec.c
+++ b/drivers/mtd/nand/raw/nand_jedec.c
@@ -94,6 +94,9 @@  int nand_jedec_detect(struct nand_chip *chip)
 		goto free_jedec_param_page;
 	}
 
+	if (p->opt_cmd[0] & JEDEC_OPT_CMD_READ_CACHE)
+		chip->parameters.supports_read_cache;
+
 	memorg->pagesize = le32_to_cpu(p->byte_per_page);
 	mtd->writesize = memorg->pagesize;
 
diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index f15ef90aec8c..bf79bf2b5866 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -303,6 +303,9 @@  int nand_onfi_detect(struct nand_chip *chip)
 			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
 	}
 
+	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_READ_CACHE)
+		chip->parameters.supports_read_cache;
+
 	onfi = kzalloc(sizeof(*onfi), GFP_KERNEL);
 	if (!onfi) {
 		ret = -ENOMEM;
diff --git a/include/linux/mtd/jedec.h b/include/linux/mtd/jedec.h
index 0b6b59f7cfbd..56047a4e54c9 100644
--- a/include/linux/mtd/jedec.h
+++ b/include/linux/mtd/jedec.h
@@ -21,6 +21,9 @@  struct jedec_ecc_info {
 /* JEDEC features */
 #define JEDEC_FEATURE_16_BIT_BUS	(1 << 0)
 
+/* JEDEC Optional Commands */
+#define JEDEC_OPT_CMD_READ_CACHE	BIT(1)
+
 struct nand_jedec_params {
 	/* rev info and features block */
 	/* 'J' 'E' 'S' 'D'  */
diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
index a7376f9beddf..55ab2e4d62f9 100644
--- a/include/linux/mtd/onfi.h
+++ b/include/linux/mtd/onfi.h
@@ -55,6 +55,7 @@ 
 #define ONFI_SUBFEATURE_PARAM_LEN	4
 
 /* ONFI optional commands SET/GET FEATURES supported? */
+#define ONFI_OPT_CMD_READ_CACHE		BIT(1)
 #define ONFI_OPT_CMD_SET_GET_FEATURES	BIT(2)
 
 struct nand_onfi_params {
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 90a141ba2a5a..766856fcaba8 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -233,6 +233,7 @@  struct nand_parameters {
 	/* Generic parameters */
 	const char *model;
 	bool supports_set_get_features;
+	bool supports_read_cache;
 	DECLARE_BITMAP(set_feature_list, ONFI_FEATURE_NUMBER);
 	DECLARE_BITMAP(get_feature_list, ONFI_FEATURE_NUMBER);