Hi Martin,
+ Bean and Domenico
martin@geanix.com wrote on Mon, 25 Sep 2023 13:01:06 +0200:
Hi Rouven,
On Fri, 2023-09-22 at 16:17 +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 supports 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 now 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") Cc: stable@vger.kernel.org Signed-off-by: Rouven Czerwinski r.czerwinski@pengutronix.de
Thanks for this. It works as expected for my Toshiba chip, obviously because it doesn't use ONFI or JEDEC.
Unfortunately, my Micron chip does use ONFI, and it sets the cached- read-supported bit. It then fails when reading afterwords:
kernel: ONFI_OPT_CMD_READ_CACHE # debug added by me kernel: nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xdc kernel: nand: Micron MT29F4G08ABAFAWP kernel: nand: 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256 kernel: nand: continued read supported # debug added by me kernel: Bad block table found at page 131008, version 0x01 kernel: Bad block table found at page 130944, version 0x01 kernel: 2 fixed-partitions partitions found on MTD device gpmi-nand kernel: Creating 2 MTD partitions on "gpmi-nand": kernel: 0x000000000000-0x000000800000 : "boot" kernel: 0x000000800000-0x000020000000 : "ubi" kernel: gpmi-nand 1806000.nand-controller: driver registered.
...
kernel: ubi0: default fastmap pool size: 100 kernel: ubi0: default fastmap WL pool size: 50 kernel: ubi0: attaching mtd1 kernel: ubi0: scanning is finished kernel: ubi0: attached mtd1 (name "ubi", size 504 MiB) kernel: ubi0: PEB size: 262144 bytes (256 KiB), LEB size: 253952 bytes kernel: ubi0: min./max. I/O unit sizes: 4096/4096, sub-page size 4096 kernel: ubi0: VID header offset: 4096 (aligned 4096), data offset: 8192 kernel: ubi0: good PEBs: 2012, bad PEBs: 4, corrupted PEBs: 0 kernel: ubi0: user volume: 9, internal volumes: 1, max. volumes count: 128 kernel: ubi0: max/mean erase counter: 4/2, WL threshold: 4096, image sequence number: 1431497221 kernel: ubi0: available PEBs: 12, total reserved PEBs: 2000, PEBs reserved for bad PEB handling: 36 kernel: block ubiblock0_4: created from ubi0:4(rootfs.a) kernel: ubi0: background thread "ubi_bgt0d" started, PID 36 kernel: block ubiblock0_6: created from ubi0:6(appfs.a) kernel: block ubiblock0_7: created from ubi0:7(appfs.b)
...
kernel: SQUASHFS error: Unable to read directory block [4b6d15c:ed1] kernel: SQUASHFS error: Unable to read directory block [4b6f15e:125] kernel: SQUASHFS error: Unable to read directory block [4b6d15c:1dae] kernel: SQUASHFS error: Unable to read directory block [4b6d15c:ed1] (d-sysctl)[55]: systemd-sysctl.service: Failed to set up credentials: Protocol error kernel: SQUASHFS error: Unable to read directory block [4b73162:14f0] kernel: SQUASHFS error: Unable to read directory block [4b6f15e:838] systemd[1]: Starting Create Static Device Nodes in /dev... kernel: SQUASHFS error: Unable to read directory block [4b6d15c:ed1] kernel: SQUASHFS error: Unable to read directory block [4b6d15c:ed1] kernel: SQUASHFS error: Unable to read directory block [4b6f15e:838] kernel: SQUASHFS error: Unable to read directory block [4b6d15c:1dae] kernel: SQUASHFS error: Unable to read directory block [4b6f15e:125]
I've briefly tried adding some error info the the squashfs error messages, but it looks like it's getting bad data. I.e. one failure a sanity check of `dir_count`:
if (dir_count > SQUASHFS_DIR_COUNT) goto data_error;
It fails with `dir_count` being 1952803684 ...
So is this a case of wrong/bad timings?
Miquel: I can tell from the code, that the READCACHESEQ operations are followed by NAND_OP_WAIT_RDY(tR_max, tRR_min). From the Micron datasheet[0], it should be NAND_OP_WAIT_RDY(tRCBSY_max, tRR_min), where tRCBSY is defined to be between 3 and 25 µs.
Not sure if this is related though.
I believe tR_max is bigger (200us), but please check that this is actually the case, you can also try enlarging this delay. But if it was a timeout issue we would probably see errors from the controller driver?
Bean, Domenico, is there any AN or specific document which I missed asking not to use this feature on specific chips? Are they supposed to work?
[0] (login required) https://www.micron.com/products/nand-flash/slc-nand/part-catalog/mt29f4g08ab...
// Martin
v2:
- change title as suggested by Miquel
- adjust controller sentence from implement to support
- fix missing true assignement for flashes
- add CC stable instead of empty line
- add documentation comment for new supports_read_cache bool inside
nand parameter struct
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 | 2 ++ 6 files changed, 15 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..b3cc8f360529 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 = true;
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..861975e44b55 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 = true;
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..c29ace15a053 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -225,6 +225,7 @@ struct gpio_desc; * struct nand_parameters - NAND generic parameters from the parameter page * @model: Model name * @supports_set_get_features: The NAND chip supports setting/getting features
- @supports_read_cache: The NAND chip supports read cache
operations * @set_feature_list: Bitmap of features that can be set * @get_feature_list: Bitmap of features that can be get * @onfi: ONFI specific parameters @@ -233,6 +234,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);
base-commit: 42dc814987c1feb6410904e58cfd4c36c4146150
Thanks, Miquèl