SV: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

SV: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Rickard X Andersson]

Hi Miquel,

> 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. 

> > 
> > +static int th58nvg2s3hbai4_init_data_interface(struct nand_chip
>
> I renamed the hook so the helper should be
> th58..._choose_data_interface()
>
> > *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.

I have tested this on my new patchset and it works.

Best regards,
Rickard
________________________________________
Från: Miquel Raynal <miquel.raynal@bootlin.com>
Skickat: den 19 maj 2020 14:08
Till: Rickard X Andersson
Kopia: linux-mtd@lists.infradead.org
Ämne: Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

Hi Rickard,

Rickard Andersson <rickaran@axis.com> wrote on Thu, 14 May 2020
11:13:42 +0200:

> From: Rickard x Andersson <rickaran@axis.com>
>
> 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 <rickaran@axis.com>
> ---
>  drivers/mtd/nand/raw/nand_ids.c     |  3 ++
>  drivers/mtd/nand/raw/nand_toshiba.c | 61 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
> index e0dbc2e316c7..8b676e8b481b 100644
> --- a/drivers/mtd/nand/raw/nand_ids.c
> +++ b/drivers/mtd/nand/raw/nand_ids.c
> @@ -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),
> diff --git a/drivers/mtd/nand/raw/nand_toshiba.c b/drivers/mtd/nand/raw/nand_toshiba.c
> index b6efaf5195bb..380499cfa491 100644
> --- a/drivers/mtd/nand/raw/nand_toshiba.c
> +++ b/drivers/mtd/nand/raw/nand_toshiba.c
> @@ -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,

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.

> +     .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)
>  {
> @@ -194,6 +240,13 @@ static void toshiba_nand_decode_id(struct
> nand_chip *chip) }
>  }
>
> +static int th58nvg2s3hbai4_init_data_interface(struct nand_chip

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

> *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.

> +
> +     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))
> @@ -217,6 +276,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);
>
>       return 0;
>  }




Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Miquel Raynal]

Hi Rickard,

Rickard X Andersson <Rickard.Andersson@axis.com> wrote on Wed, 20 May
2020 13:32:14 +0000:

> Hi Miquel,
> 
> > 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. 

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 :)

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

SV: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Rickard X Andersson]

> > 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 :)

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).

Thanks,
Rickard
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Miquel Raynal]

Hi Rickard,

Rickard X Andersson <Rickard.Andersson@axis.com> wrote on Wed, 20 May
2020 14:16:57 +0000:

> > > 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 :)  
> 
> 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).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

SV: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Rickard X Andersson]

> > 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).

Thanks! Now I understand, will fix this on monday.

BR
Rickard
________________________________________
Från: Miquel Raynal <miquel.raynal@bootlin.com>
Skickat: den 20 maj 2020 16:29
Till: Rickard X Andersson
Kopia: linux-mtd@lists.infradead.org; Boris Brezillon
Ämne: Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

Hi Rickard,

Rickard X Andersson <Rickard.Andersson@axis.com> wrote on Wed, 20 May
2020 14:16:57 +0000:

> > > 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 :)
>
> 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).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Boris Brezillon]

On Wed, 20 May 2020 14:42:31 +0000
Rickard X Andersson <Rickard.Andersson@axis.com> wrote:

> > > 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?

> > 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.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
2020 16:50:22 +0200:

> On Wed, 20 May 2020 14:42:31 +0000
> Rickard X Andersson <Rickard.Andersson@axis.com> wrote:
> 
> > > > 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?
> 
> > > 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.

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().

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Boris Brezillon]

On Wed, 20 May 2020 17:12:46 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
> 2020 16:50:22 +0200:
> 
> > On Wed, 20 May 2020 14:42:31 +0000
> > Rickard X Andersson <Rickard.Andersson@axis.com> wrote:
> >   
> > > > > 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?
> >   
> > > > 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.  
> 
> 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).

> 
> 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();

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
2020 17:26:52 +0200:

> On Wed, 20 May 2020 17:12:46 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
> > 2020 16:50:22 +0200:
> >   
> > > On Wed, 20 May 2020 14:42:31 +0000
> > > Rickard X Andersson <Rickard.Andersson@axis.com> wrote:
> > >     
> > > > > > 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?
> > >     
> > > > > 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.    
> > 
> > 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).

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.

> 
> > 
> > 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();

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.


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Boris Brezillon]

On Wed, 20 May 2020 22:43:54 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
> 2020 17:26:52 +0200:
> 
> > On Wed, 20 May 2020 17:12:46 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 20 May
> > > 2020 16:50:22 +0200:
> > >     
> > > > On Wed, 20 May 2020 14:42:31 +0000
> > > > Rickard X Andersson <Rickard.Andersson@axis.com> wrote:
> > > >       
> > > > > > > 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?
> > > >       
> > > > > > 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.      
> > > 
> > > 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).  
> 
> 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.
> 
> >   
> > > 
> > > 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();  
> 
> Well, that's very close to what I just proposed, no?

Yes, the difference being that you now have the 2 paths clearly
separated, and drivers can easily deviate if they need to. It's just a
helper (that can be re-used in the fallback path BTW), so it's pretty
easy to not call it, or call it as part of something more complex.

> 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.

Yes, I know, and I'm just suggesting to make that a public helper so
vendor drivers can re-use it, if they want.

> 
> Let me propose something with the above changes.
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: rawnand: Use the ->init_data_interface() hook

[Rickard Andersson]

From: Rickard x Andersson <rickaran@axis.com>

Check if the NAND vendor provided hook works and check that
the NAND controller can handle the timings.

Signed-off-by: Rickard x Andersson <rickaran@axis.com>
---
 drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 8744f0033f78..974050a04f23 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -955,6 +955,30 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
 	return ret;
 }
 
+static int nand_try_init_data_interface(struct nand_chip *chip)
+{
+	int ret = chip->ops.init_data_interface(chip);
+
+	if (!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 */
+		chip->ops.init_data_interface = NULL;
+		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
@@ -980,8 +1004,11 @@ static int nand_choose_data_interface(struct nand_chip *chip)
 	 * ->init_data_interface() is expected to update the entire chip's
 	 * nand_data_interface structure.
 	 */
-	if (nand_has_init_data_interface(chip))
-		return chip->ops.init_data_interface(chip);
+	if (nand_has_init_data_interface(chip)) {
+		ret = nand_try_init_data_interface(chip);
+		if (!ret)
+			return 0;
+	}
 
 	/*
 	 * First try to identify the best timings from ONFI parameters and
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Rickard Andersson]

From: Rickard x Andersson <rickaran@axis.com>

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 <rickaran@axis.com>
---
 drivers/mtd/nand/raw/nand_ids.c     |  3 ++
 drivers/mtd/nand/raw/nand_toshiba.c | 61 +++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
index e0dbc2e316c7..8b676e8b481b 100644
--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -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),
diff --git a/drivers/mtd/nand/raw/nand_toshiba.c b/drivers/mtd/nand/raw/nand_toshiba.c
index b6efaf5195bb..380499cfa491 100644
--- a/drivers/mtd/nand/raw/nand_toshiba.c
+++ b/drivers/mtd/nand/raw/nand_toshiba.c
@@ -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,
+		.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)
 {
@@ -194,6 +240,13 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
 	}
 }
 
+static int th58nvg2s3hbai4_init_data_interface(struct nand_chip *chip)
+{
+	chip->data_interface = th58nvg2s3hbai4_timings;
+
+	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))
@@ -217,6 +276,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);
 
 	return 0;
 }
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: Add timings for Kioxia TH58NVG2S3HBAI4

[Miquel Raynal]

Hi Rickard,

Rickard Andersson <rickaran@axis.com> wrote on Thu, 14 May 2020
11:13:42 +0200:

> From: Rickard x Andersson <rickaran@axis.com>
> 
> 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 <rickaran@axis.com>
> ---
>  drivers/mtd/nand/raw/nand_ids.c     |  3 ++
>  drivers/mtd/nand/raw/nand_toshiba.c | 61 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
> index e0dbc2e316c7..8b676e8b481b 100644
> --- a/drivers/mtd/nand/raw/nand_ids.c
> +++ b/drivers/mtd/nand/raw/nand_ids.c
> @@ -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),
> diff --git a/drivers/mtd/nand/raw/nand_toshiba.c b/drivers/mtd/nand/raw/nand_toshiba.c
> index b6efaf5195bb..380499cfa491 100644
> --- a/drivers/mtd/nand/raw/nand_toshiba.c
> +++ b/drivers/mtd/nand/raw/nand_toshiba.c
> @@ -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,

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.

> +	.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)
>  {
> @@ -194,6 +240,13 @@ static void toshiba_nand_decode_id(struct
> nand_chip *chip) }
>  }
>  
> +static int th58nvg2s3hbai4_init_data_interface(struct nand_chip

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

> *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.

> +
> +	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))
> @@ -217,6 +276,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);
>  
>  	return 0;
>  }




Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/