mtd: rawnand: Add a helper for testing data interface
Subsystems (2)
PATCH v2 1/2 mtd: rawnand: Add a helper for testing data interface
From: Rickard x Andersson <[email protected]>
This helper tests the current data interface timings. If the controller does not accept the timings then the timings are erased and onfi mode 0 timings are chosen at a later stage.
--- drivers/mtd/nand/raw/internals.h | 1 + drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 10 deletions(-)
drivers/mtd/nand/raw/internals.h +1 -0
@@ -100,6 +100,7 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf,void nand_decode_ext_id (struct nand_chip *chip);void panic_nand_wait (struct nand_chip *chip,unsigned long timeo);void sanitize_string (uint8_t *s,size_t len); +int nand_test_data_interface (struct nand_chip *chip);static inline bool nand_has_exec_op (struct nand_chip *chip) {
drivers/mtd/nand/raw/nand_base.c +28 -10
@@ -956,6 +956,32 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) }/** +* nand_test_data_interface - Check if controller can handle the current +* timings. Clear timings if not usable. +* +* @chip: The NAND chip +*/ +int nand_test_data_interface (struct nand_chip *chip) +{ +int ret; +/* +* Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the +* controller supports the requested timings. +*/ + ret = chip->controller->ops->setup_data_interface (chip, +NAND_DATA_IFACE_CHECK_ONLY , + &chip->data_interface); + +if (ret) { +/* The provided data interface timings did not work */ +memset (&chip->data_interface,0 , +sizeof (struct nand_data_interface )); + } + +return ret; +} + +/** * nand_choose_data_interface - find the best data interface and timings * @chip: The NAND chip *
@@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip)if (chip->parameters.onfi) { modes = chip->parameters.onfi->async_timing_mode; }else { -if (!chip->default_timing_mode) -return 0 ; - modes =GENMASK (chip->default_timing_mode,0 ); }
@@ -1005,13 +1028,8 @@ static int nand_choose_data_interface(struct nand_chip *chip)if (ret)continue ; -/* -* Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the -* controller supports the requested timings. -*/ - ret = chip->controller->ops->setup_data_interface (chip, -NAND_DATA_IFACE_CHECK_ONLY , - &chip->data_interface); +/* Check if the controller supports the requested timings. */ + ret =nand_test_data_interface (chip);if (!ret) { chip->default_timing_mode = mode;break ;
Signature · 6 lines
2.11.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
PATCH v2 2/2 mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4
From: Rickard x Andersson <[email protected]>
The Kioxia/Toshiba TH58NVG2S3HBAI4 NAND memory is not a ONFI compliant memory. The timings of the memory is quite close to ONFI mode 4 but is breaking that spec.
Erase block read speed is increased from 6910 KiB/s to 13490 KiB/s. Erase block write speed is increased from 3350 KiB/s to 4410 KiB/s.
Tested on IMX6SX which has a NAND controller supporting EDO mode.
--- drivers/mtd/nand/raw/nand_ids.c | 3 ++ drivers/mtd/nand/raw/nand_toshiba.c | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)
drivers/mtd/nand/raw/nand_ids.c +3 -0
@@ -52,6 +52,9 @@ struct nand_flash_dev nand_flash_ids[] = { { .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },SZ_8K ,SZ_8K ,SZ_2M ,NAND_NEED_SCRAMBLING ,6 ,640 , NAND_ECC_INFO(40, SZ_1K), 4 }, + {"TH58NVG2S3HBAI4 4G 3.3V 8-bit", + { .id = {0x98, 0xdc, 0x91, 0x15, 0x76} }, + SZ_2K, SZ_512, SZ_128K, 0, 5, 128, NAND_ECC_INFO(8, SZ_512) },LEGACY_ID_NAND ("NAND 4MiB 5V 8-bit" ,0x6B ,4 ,SZ_8K ,SP_OPTIONS ), LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),
drivers/mtd/nand/raw/nand_toshiba.c +66 -0
90 lines not on this page · raw message
Signature · 6 lines
2.11.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Re: [PATCH v2 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4
Quoted 75 lines · On Wed, 20 May 2020 15:38:54 +0200 Rickard Andersson <[email protected]> wrote:
From: Rickard x Andersson <[email protected]>
The Kioxia/Toshiba TH58NVG2S3HBAI4 NAND memory is not a ONFI compliant memory. The timings of the memory is quite close to ONFI mode 4 but is breaking that spec.
Erase block read speed is increased from 6910 KiB/s to 13490 KiB/s. Erase block write speed is increased from 3350 KiB/s to 4410 KiB/s.
Tested on IMX6SX which has a NAND controller supporting EDO mode.
Signed-off-by Rickard x Andersson <[email protected]>--- drivers/mtd/nand/raw/nand_ids.c | 3 ++ drivers/mtd/nand/raw/nand_toshiba.c | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)drivers/mtd/nand/raw/nand_ids.c +3
-0@@ -52,6 +52,9 @@ struct nand_flash_dev nand_flash_ids[] = { { .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },SZ_8K ,SZ_8K ,SZ_2M ,NAND_NEED_SCRAMBLING ,6 ,640 , NAND_ECC_INFO(40, SZ_1K), 4 }, + {"TH58NVG2S3HBAI4 4G 3.3V 8-bit", + { .id = {0x98, 0xdc, 0x91, 0x15, 0x76} }, + SZ_2K, SZ_512, SZ_128K, 0, 5, 128, NAND_ECC_INFO(8, SZ_512) },LEGACY_ID_NAND ("NAND 4MiB 5V 8-bit" ,0x6B ,4 ,SZ_8K ,SP_OPTIONS ), LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),drivers/mtd/nand/raw/nand_toshiba.c +34
-0@@ -26,6 +26,52 @@/* Max ECC Steps for BENAND */ #define TOSHIBA_NAND_MAX_ECC_STEPS 8 +static const struct nand_data_interface th58nvg2s3hbai4_timings = { + .type =NAND_SDR_IFACE , + .timings.mode =0 , + .timings.sdr = { + .tPROG_max =700000000 , + .tBERS_max =5000000000 , + .tCCS_min =500000 , + .tR_max =200000000 , + .tADL_min =400000 , + .tALH_min =5000 , + .tALS_min =12000 , + .tAR_min =10000 , + .tCEA_max =25000 , + .tCEH_min =20000 , + .tCH_min =5000 , + .tCHZ_max =20000 , + .tCLH_min =5000 , + .tCLR_min =10000 , + .tCLS_min =12000 , + .tCOH_min =0 , + .tCS_min =20000 , + .tDH_min =5000 , + .tDS_min =12000 , + .tFEAT_max =1000000 , + .tIR_min =0 , + .tITC_max =1000000 , + .tRC_min =25000 , + .tREA_max =20000 , + .tREH_min =10000 , + .tRHOH_min =25000 , + .tRHW_min =30000 , + .tRHZ_max =60000 , + .tRLOH_min =5000 , + .tRP_min =12000 ,
drivers/mtd/nand/raw/nand_toshiba.c line 72
Quoted 10 lines
drivers/mtd/nand/raw/nand_toshiba.c +10
-0+ .tRR_min = 20000, + .tRST_max = 500000000, + .tWB_max = 100000, + .tWC_min = 25000, + .tWH_min = 10000, + .tWHR_min = 60000, + .tWP_min = 12000, + .tWW_min = 100000, + } +};
If we don't want the RO section to grow considerably we should try to come with a solution to express things in term of deviation from an existing ONFI timing mode. I'd expect most parameters to match.
Quoted 6 lines
drivers/mtd/nand/raw/nand_toshiba.c +2
-0+ +static int toshiba_nand_benand_read_eccstatus_op (struct nand_chip *chip,u8 *buf) {
drivers/mtd/nand/raw/nand_toshiba.c static void toshiba_nand_decode_id(struct nand_chip *chip) · line 247
Quoted 8 lines
drivers/mtd/nand/raw/nand_toshiba.c +5
-0@@ -194,6 +240,18 @@ static void toshiba_nand_decode_id(struct nand_chip *chip) } } +static int th58nvg2s3hbai4_choose_data_interface (struct nand_chip *chip) +{ +int ret; + + chip->data_interface = th58nvg2s3hbai4_timings;
Something like:
/* Start from the closest timing mode. */ onfi_fill_data_interface(chip, SDR, modeX); /* Patch only what you need to patch. */ chip->data_interface.timings.tXX = YY; ...
Quoted 24 lines
drivers/mtd/nand/raw/nand_toshiba.c +13
-0+ +/* Check if the controller can handle the timings */ + ret =nand_test_data_interface (chip); + +return ret; +} +static int tc58teg5dclta00_init (struct nand_chip *chip) {struct mtd_info *mtd =nand_to_mtd (chip);@@ -205,6 +263,12 @@ static int tc58teg5dclta00_init(struct nand_chip *chip)return 0 ; } +static int th58nvg2s3hbai4_init (struct nand_chip *chip) +{ + chip->ops.choose_data_interface = th58nvg2s3hbai4_choose_data_interface; +return 0 ; +} +static int toshiba_nand_init (struct nand_chip *chip) {if (nand_is_slc (chip))
drivers/mtd/nand/raw/nand_toshiba.c static int toshiba_nand_init(struct nand_chip *chip) · line 285
Quoted 5 lines
drivers/mtd/nand/raw/nand_toshiba.c +2
-0@@ -217,6 +281,8 @@ static int toshiba_nand_init(struct nand_chip *chip)if (!strcmp ("TC58TEG5DCLTA00" , chip->parameters.model))tc58teg5dclta00_init (chip); +if (!strncmp ("TH58NVG2S3HBAI4" , chip->parameters.model,15 )) +th58nvg2s3hbai4_init (chip);
I think we should add a table with all the chips needing a specific initialization.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Miquel Raynal
maintainerHi Rickard,
Rickard Andersson <[email protected]> wrote on Wed, 20 May 2020 15:38:53 +0200:
Quoted 4 lines
From: Rickard x Andersson <[email protected]>
This helper tests the current data interface timings. If the controller does not accept the timings then the timings
s/accept/support/
Quoted 2 lines
are erased and onfi mode 0 timings are chosen at a later stage.
See below, I don't think this is needed.
Quoted 24 lines
Signed-off-by Rickard x Andersson <[email protected]>--- drivers/mtd/nand/raw/internals.h | 1 + drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 10 deletions(-)drivers/mtd/nand/raw/internals.h +1
-0@@ -100,6 +100,7 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf,void nand_decode_ext_id (struct nand_chip *chip);void panic_nand_wait (struct nand_chip *chip,unsigned long timeo);void sanitize_string (uint8_t *s,size_t len); +int nand_test_data_interface (struct nand_chip *chip);static inline bool nand_has_exec_op (struct nand_chip *chip) {drivers/mtd/nand/raw/nand_base.c +0
-0
drivers/mtd/nand/raw/nand_base.c static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) · line 959
Quoted 4 lines
drivers/mtd/nand/raw/nand_base.c +1
-0@@ -956,6 +956,32 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) }/** +* nand_test_data_interface - Check if controller can handle the current
I'm fine adding an helper for that. It could also be used for DDR timing support.
What about renaming it "nand_controller_supports_data_interface()"?
drivers/mtd/nand/raw/nand_base.c static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) · line 966
Quoted 7 lines
drivers/mtd/nand/raw/nand_base.c +7
-0+* timings. Clear timings if not usable. +* +* @chip: The NAND chip +*/ +int nand_test_data_interface (struct nand_chip *chip) +{ +int ret;
Missing extra line
drivers/mtd/nand/raw/nand_base.c static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) · line 973
Quoted 7 lines
drivers/mtd/nand/raw/nand_base.c +7
-0+/* +* Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the +* controller supports the requested timings. +*/ + ret = chip->controller->ops->setup_data_interface (chip, +NAND_DATA_IFACE_CHECK_ONLY , + &chip->data_interface);
Could you align these lines to the opened parenthesis?
drivers/mtd/nand/raw/nand_base.c static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) · line 978
Quoted 5 lines
drivers/mtd/nand/raw/nand_base.c +5
-0+ +if (ret) { +/* The provided data interface timings did not work */ +memset (&chip->data_interface,0 , +sizeof (struct nand_data_interface ));
I'm not sure this is needed. I think it will only be necessary in your case, so I think it's better to move it ouf of this function.
drivers/mtd/nand/raw/nand_base.c static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) · line 982
Quoted 4 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+ } + +return ret; +}
Maybe it is best to move this function in internals.h directly.
drivers/mtd/nand/raw/nand_base.c static int nand_choose_data_interface(struct nand_chip *chip) · line 999
Quoted 12 lines
drivers/mtd/nand/raw/nand_base.c +2
-3+ +/** * nand_choose_data_interface - find the best data interface and timings * @chip: The NAND chip * @@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip)if (chip->parameters.onfi) { modes = chip->parameters.onfi->async_timing_mode; }else { -if (!chip->default_timing_mode) -return 0 ; -
This should not be removed
Quoted 9 lines
drivers/mtd/nand/raw/nand_base.c +0
-2modes =GENMASK (chip->default_timing_mode,0 ); }@@ -1005,13 +1028,8 @@ static int nand_choose_data_interface(struct nand_chip *chip)if (ret)continue ; -/* -* Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
drivers/mtd/nand/raw/nand_base.c static int nand_choose_data_interface(struct nand_chip *chip) · line 1035
Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +2
-5-* controller supports the requested timings. -*/ - ret = chip->controller->ops->setup_data_interface (chip, -NAND_DATA_IFACE_CHECK_ONLY , - &chip->data_interface); +/* Check if the controller supports the requested timings. */ + ret =nand_test_data_interface (chip);if (!ret) { chip->default_timing_mode = mode;break ;
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Miquel,
Comments on two of your comments. (I am fine with all the other comments.)
Quoted 9 lines
Quoted 7 lines
+ /*
+ * PassNAND_DATA_IFACE_CHECK_ONLYto only check if the
+ * controller supports the requested timings.
+ */
+ ret = chip->controller->ops->setup_data_interface(chip,
+ NAND_DATA_IFACE_CHECK_ONLY,
+ &chip->data_interface);Could you align these lines to the opened parenthesis?
Then the lines will have 80+ characters.
Quoted 9 lines
drivers/mtd/nand/raw/nand_base.c static int nand_choose_data_interface(struct nand_chip *chip) · line 999
Quoted 7 lines
drivers/mtd/nand/raw/nand_base.c +0
-3@@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip) if (chip->parameters.onfi) { modes = chip->parameters.onfi->async_timing_mode; } else { - if (!chip->default_timing_mode) - return 0; -This should not be removed
Then onfi_fill_data_interface would not be called for default_timing_mode 0. (In case we have called chip->ops.choose_data_interface and got an error.).
BR,
Rickard
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Quoted 12 lines · On Wed, 20 May 2020 14:52:52 +0000 Rickard X Andersson…
Hi Miquel,
Comments on two of your comments. (I am fine with all the other comments.)
Quoted 8 lines
Quoted 7 lines
+ /*
+ * PassNAND_DATA_IFACE_CHECK_ONLYto only check if the
+ * controller supports the requested timings.
+ */
+ ret = chip->controller->ops->setup_data_interface(chip,
+ NAND_DATA_IFACE_CHECK_ONLY,
+ &chip->data_interface);
Quoted 3 lines
Quoted 1 line
Could you align these lines to the opened parenthesis?
Then the lines will have 80+ characters.
Just add a local ops var.
const struct nand_controller_ops *ops = chip->controller->ops; ret = ops->setup_data_interface(chip, NAND_DATA_IFACE_CHECK_ONLY, &chip->data_interface);
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/