mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

I just a branch with a new helper to fill this field, it is called onfi_find_equivalent_sdr_mode().

It is only compile tested so please verify it works.

Quoted 2 lines

+static int th58nvg2s3hbai4_init_data_interface(struct nand_chip

I renamed the hook so the helper should be
th58..._choose_data_interface()

Quoted 2 lines

*chip) +{
+     chip->data_interface = th58nvg2s3hbai4_timings;

this data interface should be tested against the controller's capabilities and return an error otherwise.

Please check that, in case of error (simulate it) it fallbacks to mode 0 and does not fail silently.

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 | 61 +++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

drivers/mtd/nand/raw/nand_ids.c +3 -0

index e0dbc2e316c7..8b676e8b481b 100644
@@ -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 +0 -0

index b6efaf5195bb..380499cfa491 100644

drivers/mtd/nand/raw/nand_toshiba.c +43 -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,
+             .tRR_min = 20000,
+             .tRST_max = 500000000,
+             .tWB_max = 100000,
+             .tWC_min = 25000,
+             .tWH_min = 10000,
+             .tWHR_min = 60000,
+             .tWP_min = 12000,
+             .tWW_min = 100000,
+     }
+};
+
+
 static int toshiba_nand_benand_read_eccstatus_op(struct nand_chip
 *chip, u8 *buf)
 {

drivers/mtd/nand/raw/nand_toshiba.c +10 -0

+
+     return 0;
+}
+
 static int tc58teg5dclta00_init(struct nand_chip *chip)
 {
      struct mtd_info *mtd = nand_to_mtd(chip);

@@ -205,6 +258,12 @@ static int tc58teg5dclta00_init(struct nand_chip
 *chip) return 0;
 }
 
+static int th58nvg2s3hbai4_init(struct nand_chip *chip)
+{
+     chip->ops.init_data_interface =
 th58nvg2s3hbai4_init_data_interface;
+     return 0;
+}
+
 static int toshiba_nand_init(struct nand_chip *chip)
 {

      if (nand_is_slc(chip))

Hi Miquel,

Quoted 4 lines

I just a branch with a new helper to fill this field, it is called onfi_find_equivalent_sdr_mode().

It is only compile tested so please verify it works.

I have created a new patchset that I will send shortly. This patchset does however not use onfi_find_equivalent_sdr_mode(..), that could be a future improvment. My patchset falls back to mode 0 if the specialized timings does not work for the controller.

Quoted 1 line

I have created a new patchset that I will send shortly. This patchset does however not use onfi_find_equivalent_sdr_mode(..), that could be a future improvment. My patchset falls back to mode 0 if the specialized timings does not work for the controller.

Thanks for updating!

Actually I wrote it because of a previous discussion with Boris who told me that this mode field would be badly understood and he actually got it right as in your previous submission this field was set to 0 while, IIRC, you told me it was close to mode 3. This is important to controllers that cannot tweak the parameters but just pick an ONFI mode. So the timings they choose must fit the slowest mins and fastest maxs of your new set of timings. Hence the use of the helper which seems needed. It is actually pretty straightforward so I don't understand your choice of not making use of it?

As this is the primary contribution of this type, I would like to get it right so that other contributors can refer to it :)

Quoted 14 lines

Quoted 1 line

I have created a new patchset that I will send shortly. This patchset does however not use onfi_find_equivalent_sdr_mode(..), that could be a future improvment. My patchset falls back to mode 0 if the specialized timings does not work for the controller.

Thanks for updating!

Actually I wrote it because of a previous discussion with Boris who told me that this mode field would be badly understood and he actually got it right as in your previous submission this field was set to 0 while, IIRC, you told me it was close to mode 3. This is important to controllers that cannot tweak the parameters but just pick an ONFI mode. So the timings they choose must fit the slowest mins and fastest maxs of your new set of timings. Hence the use of the helper which seems needed. It is actually pretty straightforward so I don't understand your choice of not making use of it?

Quoted 2 lines

As this is the primary contribution of this type, I would like to get it right so that other contributors can refer to it :)

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.
4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Quoted 14 lines

Quoted 1 line

I have created a new patchset that I will send shortly. This patchset does however not use onfi_find_equivalent_sdr_mode(..), that could be a future improvment. My patchset falls back to mode 0 if the specialized timings does not work for the controller.

Thanks for updating!

Actually I wrote it because of a previous discussion with Boris who told me that this mode field would be badly understood and he actually got it right as in your previous submission this field was set to 0 while, IIRC, you told me it was close to mode 3. This is important to controllers that cannot tweak the parameters but just pick an ONFI mode. So the timings they choose must fit the slowest mins and fastest maxs of your new set of timings. Hence the use of the helper which seems needed. It is actually pretty straightforward so I don't understand your choice of not making use of it?

Quoted 2 lines

As this is the primary contribution of this type, I would like to get it right so that other contributors can refer to it :)

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Quoted 8 lines

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.

Quoted 6 lines

4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Quoted 8 lines · On Wed, 20 May 2020 14:42:31 +0000 Rickard X Andersson…

Quoted 8 lines

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.

Why do we have to set the default_timing_mode field? Can't we just set timings.mode directly?

Quoted 6 lines

Quoted 6 lines

4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Again, I don't see why setting chip->default_timing_mode is needed here, and I'm not sure trying remaining ONFI modes is useful, I guess we can just fall back on mode 0 in that case.

Hi Boris,

Boris Brezillon <[email protected]> wrote on Wed, 20 May 2020 16:50:22 +0200:

Quoted 22 lines

Quoted 8 lines · On Wed, 20 May 2020 14:42:31 +0000 Rickard X Andersson…

Quoted 8 lines

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.

Why do we have to set the default_timing_mode field? Can't we just set timings.mode directly?

Quoted 6 lines

Quoted 6 lines

4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Quoted 3 lines

Again, I don't see why setting chip->default_timing_mode is needed here, and I'm not sure trying remaining ONFI modes is useful, I guess we can just fall back on mode 0 in that case.

It is needed because of the logic in nand_reset() which does not apply the data interface after a reset if this field is null.

Otherwise I also wondered if falling back to regular ONFI mode was useful. If this is not, we can just return after the call to chip->ops.choose_data_interface().

Quoted 27 lines · On Wed, 20 May 2020 17:12:46 +0200 Miquel Raynal <[email protected]>…

Hi Boris,

Boris Brezillon <[email protected]> wrote on Wed, 20 May 2020 16:50:22 +0200:

Quoted 22 lines

Quoted 8 lines · On Wed, 20 May 2020 14:42:31 +0000 Rickard X Andersson…

Quoted 8 lines

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.

Why do we have to set the default_timing_mode field? Can't we just set timings.mode directly?

Quoted 6 lines

Quoted 6 lines

4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Quoted 6 lines

Quoted 3 lines

Again, I don't see why setting chip->default_timing_mode is needed here, and I'm not sure trying remaining ONFI modes is useful, I guess we can just fall back on mode 0 in that case.

It is needed because of the logic in nand_reset() which does not apply the data interface after a reset if this field is null.

We should probably replace that check by a memcmp():

	if (!memcmp(&chip->data_interface, saved_data_intf,
		    sizeof(saved_data_intf)))
		return 0;

And maybe we should allocate this struct instead of copying things around (have a "default/reset timings" object that's shared by all drivers and matches timing mode 0, and a "best timing" object that's allocated at init time).

Quoted 5 lines

Otherwise I also wondered if falling back to regular ONFI mode was useful. If this is not, we can just return after the call to chip->ops.choose_data_interface().

Or maybe we could expose this logic as a helper:

static int
nand_choose_best_sdr_timings(struct nand_chip *,
struct nand_sdr_timings *best_timings)
{
/*
* 1/ pick the closest mode and assign best_timings->mode
* using onfi_find_equivalent_sdr_mode()
* 2/ call controller->setup_data_interface(check_only, best_timings);
* 3/ pick timings of best_timings->mode - 1 if it fails and go back to
* #2, return 0 otherwise.
*/
}

This way the driver doesn't have to duplicate the logic, it only has to fill the best_timings struct accordingly, and the core can simply do:

	if (chip->ops.choose_data_interface)
		return chip->ops.choose_data_interface();

Hi Boris,

Boris Brezillon <[email protected]> wrote on Wed, 20 May 2020 17:26:52 +0200:

Quoted 39 lines

Quoted 27 lines · On Wed, 20 May 2020 17:12:46 +0200 Miquel Raynal <[email protected]>…

Hi Boris,

Boris Brezillon <[email protected]> wrote on Wed, 20 May 2020 16:50:22 +0200:

Quoted 22 lines

Quoted 8 lines · On Wed, 20 May 2020 14:42:31 +0000 Rickard X Andersson…

Quoted 8 lines

Quoted 2 lines

If I understand you correctly you want me to use onfi_find_equivalent_sdr_mode in order to find the corresponding onfi mode. Then you want me to use onfi_fill_data_interface and loop towards mode 0 checking which mode the controller accepts? I just thought it was a "messy" to duplicate this code in all vendor drivers. Or do you mean that I should just use onfi_find_equivalent_sdr_mode to set ."timings.mode" and let nand_base to do the looping in case error is returned from th58nvg2s3hbai4_choose_data_interface (i.e specialized timings not accepted by the controller).

Sorry for the misunderstanding. What I think you should try is:
1/ call onfi_find_equivalent_sdr_mode() to set the timings.mode field. 2/ call nand_controller_supports_data_interface()
3/ if the controller supports the timings, set chip->default_timing_mode accordingly and return 0.

Why do we have to set the default_timing_mode field? Can't we just set timings.mode directly?

Quoted 6 lines

Quoted 6 lines

4/ if the controller does not support the timings, you may want to propose other standard timings to test by setting chip->default_timing_mode anyway but returning an error which means "best interface has not been found yet" so the rest of the choose_data_interface() helper will try the remaining ONFI modes automatically (fallbacks to 0 anyway).

Quoted 6 lines

Quoted 3 lines

Again, I don't see why setting chip->default_timing_mode is needed here, and I'm not sure trying remaining ONFI modes is useful, I guess we can just fall back on mode 0 in that case.

It is needed because of the logic in nand_reset() which does not apply the data interface after a reset if this field is null.

We should probably replace that check by a memcmp():

Quoted 8 lines

	if (!memcmp(&chip->data_interface, saved_data_intf,
		    sizeof(saved_data_intf)))
		return 0;

And maybe we should allocate this struct instead of copying things around (have a "default/reset timings" object that's shared by all drivers and matches timing mode 0, and a "best timing" object that's allocated at init time).

Indeed, checking if a pointer has been set is much less expensive than a memcmp I suppose. I'll try to come up with something.

Quoted 21 lines

Quoted 5 lines

Otherwise I also wondered if falling back to regular ONFI mode was useful. If this is not, we can just return after the call to chip->ops.choose_data_interface().

Or maybe we could expose this logic as a helper:

static int
nand_choose_best_sdr_timings(struct nand_chip *,
struct nand_sdr_timings *best_timings)
{
/*
* 1/ pick the closest mode and assign best_timings->mode
* using onfi_find_equivalent_sdr_mode()
* 2/ call controller->setup_data_interface(check_only, best_timings);
* 3/ pick timings of best_timings->mode - 1 if it fails and go back to
* #2, return 0 otherwise.
*/
}

Quoted 6 lines

This way the driver doesn't have to duplicate the logic, it only has to fill the best_timings struct accordingly, and the core can simply do:

	if (chip->ops.choose_data_interface)
		return chip->ops.choose_data_interface();  

Well, that's very close to what I just proposed, no?

The
difference being that I was reusing the existing code (and already adapted it to DDR modes BTW) because "picking timings of best_timings->mode - 1 if it fails and go back to #2" is precisely what nand_choose_data_interface() does.

Let me propose something with the above changes.