[PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Having setup time 0 violates tAR, tCLR of some chips, for instance
TOSHIBA TC58NVG2S3ETAI0 cannot be detected successfully (first ID byte
being read duplicated, i.e. 98 98 dc 90 15 76 14 03 instead of
98 dc 90 15 76 ...).

Atmel Application Notes postulated 1 cycle NRD_SETUP without explanation
[1], but it looks more appropriate to just calculate setup time properly.

[1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
Cc: stable@vger.kernel.org
Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
v2:
- Cc'ed stable
- reformatted atmel_smc_cs_conf_set_setup() call
- rebased onto mtd/fixes

 drivers/mtd/nand/raw/atmel/nand-controller.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
index dedcca87defc7..ad0eff385e123 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -1377,14 +1377,24 @@ static int atmel_smc_nand_prepare_smcconf(struct atmel_nand *nand,
 	if (ret)
 		return ret;
 
+	/*
+	 * Read setup timing depends on the operation done on the NAND:
+	 *
+	 * NRD_SETUP = max(tAR, tCLR)
+	 */
+	timeps = max(conf->timings.sdr.tAR_min, conf->timings.sdr.tCLR_min);
+	ncycles = DIV_ROUND_UP(timeps, mckperiodps);
+	totalcycles += ncycles;
+	ret = atmel_smc_cs_conf_set_setup(smcconf, ATMEL_SMC_NRD_SHIFT, ncycles);
+	if (ret)
+		return ret;
+
 	/*
 	 * The read cycle timing is directly matching tRC, but is also
 	 * dependent on the setup and hold timings we calculated earlier,
 	 * which gives:
 	 *
-	 * NRD_CYCLE = max(tRC, NRD_PULSE + NRD_HOLD)
-	 *
-	 * NRD_SETUP is always 0.
+	 * NRD_CYCLE = max(tRC, NRD_SETUP + NRD_PULSE + NRD_HOLD)
 	 */
 	ncycles = DIV_ROUND_UP(conf->timings.sdr.tRC_min, mckperiodps);
 	ncycles = max(totalcycles, ncycles);
-- 
2.50.1

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Alexander Dahl]

Hei hei,

Am Thu, Aug 21, 2025 at 02:00:57PM +0200 schrieb A. Sverdlin:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Having setup time 0 violates tAR, tCLR of some chips, for instance
> TOSHIBA TC58NVG2S3ETAI0 cannot be detected successfully (first ID byte
> being read duplicated, i.e. 98 98 dc 90 15 76 14 03 instead of
> 98 dc 90 15 76 ...).
> 
> Atmel Application Notes postulated 1 cycle NRD_SETUP without explanation
> [1], but it looks more appropriate to just calculate setup time properly.
> 
> [1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
> Cc: stable@vger.kernel.org
> Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Tested-by: Alexander Dahl <ada@thorsis.com>

Threw this on top of 6.12.39-rt11 and tested on two custom platforms
both with a Spansion S34ML02G1 SLC 2GBit flash chip, but with
different SoCs (sama5d2, sam9x60).  We had difficulties with the
timing of those NAND flash chips in the past and I wanted to make sure
this patch does not break our setup.  Seems fine in a quick test,
reading and writing and reading back is successful.

Kernel log output is unobtrusive, here for the first device:

  [  +0.001100] AT91: Detected SoC family: sam9x60
  [  +0.000024] AT91: Detected SoC: sam9x60 64MiB DDR2 SiP, revision 2
  …
  [  +0.011069] atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
  [  +0.000836] nand: device found, Manufacturer ID: 0x01, Chip ID: 0xda
  [  +0.000031] nand: AMD/Spansion S34ML02G1
  [  +0.000011] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
  [  +0.051629] Bad block table found at page 131008, version 0xFF
  [  +0.005350] Bad block table found at page 130944, version 0xFF
  [  +0.000479] 5 fixed-partitions partitions found on MTD device atmel_nand
  [  +0.000047] Creating 5 MTD partitions on "atmel_nand":
  [  +0.000025] 0x000000000000-0x000000040000 : "at91bootstrap"
  [  +0.001104] 0x000000040000-0x000000140000 : "u-boot"
  [  +0.001063] 0x000000140000-0x000000180000 : "U-Boot Env"
  [  +0.026887] 0x000000180000-0x0000001c0000 : "U-Boot Env Redundant"
  [  +0.001022] 0x000000200000-0x00000ff00000 : "UBI"
  …
  [  +0.043872] ubi0: attaching mtd4
  [  +0.932080] ubi0: scanning is finished
  [  +0.032686] ubi0: attached mtd4 (name "UBI", size 253 MiB)
  [  +0.000048] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 126976 bytes
  [  +0.000022] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 2048
  [  +0.000018] ubi0: VID header offset: 2048 (aligned 2048), data offset: 4096
  [  +0.000018] ubi0: good PEBs: 2024, bad PEBs: 0, corrupted PEBs: 0
  [  +0.000017] ubi0: user volume: 4, internal volumes: 1, max. volumes count: 128
  [  +0.000017] ubi0: max/mean erase counter: 7/3, WL threshold: 4096, image sequence number: 1905565080
  [  +0.000023] ubi0: available PEBs: 2, total reserved PEBs: 2022, PEBs reserved for bad PEB handling: 40
  [  +0.000043] ubi0: background thread "ubi_bgt0d" started, PID 47

And for the second:

  [  +0.001519] AT91: Detected SoC family: sama5d2
  [  +0.000030] AT91: Detected SoC: sama5d27c 128MiB SiP, revision 2
  …
  [  +0.002585] atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
  [  +0.036590] nand: device found, Manufacturer ID: 0x01, Chip ID: 0xda
  [  +0.000044] nand: AMD/Spansion S34ML02G1
  [  +0.000017] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
  [  +0.001571] Bad block table found at page 131008, version 0xFF
  [  +0.001249] Bad block table found at page 130944, version 0xFF
  [  +0.000572] 6 cmdlinepart partitions found on MTD device atmel_nand
  [  +0.000037] Creating 6 MTD partitions on "atmel_nand":
  [  +0.000026] 0x000000000000-0x000000040000 : "bootstrap"
  [  +0.001496] 0x000000040000-0x000000100000 : "uboot"
  [  +0.031417] 0x000000100000-0x000000140000 : "env1"
  [  +0.013718] 0x000000140000-0x000000180000 : "env2"
  [  +0.001276] 0x000000180000-0x000000200000 : "reserved"
  [  +0.001209] 0x000000200000-0x000010000000 : "UBI"
  …
  [  +0.055885] ubi0: attaching mtd5
  [  +1.468075] ubi0: scanning is finished
  [  +0.047323] ubi0: attached mtd5 (name "UBI", size 254 MiB)
  [  +0.000065] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 126976 bytes
  [  +0.000022] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 2048
  [  +0.000017] ubi0: VID header offset: 2048 (aligned 2048), data offset: 4096
  [  +0.000017] ubi0: good PEBs: 2028, bad PEBs: 4, corrupted PEBs: 0
  [  +0.000016] ubi0: user volume: 4, internal volumes: 1, max. volumes count: 128
  [  +0.000018] ubi0: max/mean erase counter: 6/3, WL threshold: 4096, image sequence number: 1534026892
  [  +0.000022] ubi0: available PEBs: 0, total reserved PEBs: 2028, PEBs reserved for bad PEB handling: 36
  [  +0.000050] ubi0: background thread "ubi_bgt0d" started, PID 49

Thanks and greets
Alex

> ---
> v2:
> - Cc'ed stable
> - reformatted atmel_smc_cs_conf_set_setup() call
> - rebased onto mtd/fixes
> 
>  drivers/mtd/nand/raw/atmel/nand-controller.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
> index dedcca87defc7..ad0eff385e123 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -1377,14 +1377,24 @@ static int atmel_smc_nand_prepare_smcconf(struct atmel_nand *nand,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Read setup timing depends on the operation done on the NAND:
> +	 *
> +	 * NRD_SETUP = max(tAR, tCLR)
> +	 */
> +	timeps = max(conf->timings.sdr.tAR_min, conf->timings.sdr.tCLR_min);
> +	ncycles = DIV_ROUND_UP(timeps, mckperiodps);
> +	totalcycles += ncycles;
> +	ret = atmel_smc_cs_conf_set_setup(smcconf, ATMEL_SMC_NRD_SHIFT, ncycles);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * The read cycle timing is directly matching tRC, but is also
>  	 * dependent on the setup and hold timings we calculated earlier,
>  	 * which gives:
>  	 *
> -	 * NRD_CYCLE = max(tRC, NRD_PULSE + NRD_HOLD)
> -	 *
> -	 * NRD_SETUP is always 0.
> +	 * NRD_CYCLE = max(tRC, NRD_SETUP + NRD_PULSE + NRD_HOLD)
>  	 */
>  	ncycles = DIV_ROUND_UP(conf->timings.sdr.tRC_min, mckperiodps);
>  	ncycles = max(totalcycles, ncycles);
> -- 
> 2.50.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Sverdlin, Alexander]

Hi Alexander,

On Mon, 2025-08-25 at 13:02 +0200, Alexander Dahl wrote:
> > Having setup time 0 violates tAR, tCLR of some chips, for instance
> > TOSHIBA TC58NVG2S3ETAI0 cannot be detected successfully (first ID byte
> > being read duplicated, i.e. 98 98 dc 90 15 76 14 03 instead of
> > 98 dc 90 15 76 ...).
> > 
> > Atmel Application Notes postulated 1 cycle NRD_SETUP without explanation
> > [1], but it looks more appropriate to just calculate setup time properly.
> > 
> > [1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
> > Cc: stable@vger.kernel.org
> > Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Tested-by: Alexander Dahl <ada@thorsis.com>
> 
> Threw this on top of 6.12.39-rt11 and tested on two custom platforms
> both with a Spansion S34ML02G1 SLC 2GBit flash chip, but with
> different SoCs (sama5d2, sam9x60).  We had difficulties with the
> timing of those NAND flash chips in the past and I wanted to make sure
> this patch does not break our setup.  Seems fine in a quick test,
> reading and writing and reading back is successful.

thank you for your feedback!

Do you see an opportunity to drop the downstream timing quirks with my patch?
I actually have another patch related to timings, but it's based on code-review
only and the theoretical issue never manifested itself in practice on our side...
(it's about missing ndelay at the end of atmel_smc_nand_exec_instr())

Regards,
-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Alexander Dahl]

Hello Alexander,

Am Mon, Aug 25, 2025 at 12:21:24PM +0000 schrieb Sverdlin, Alexander:
> Hi Alexander,
> 
> On Mon, 2025-08-25 at 13:02 +0200, Alexander Dahl wrote:
> > > Having setup time 0 violates tAR, tCLR of some chips, for instance
> > > TOSHIBA TC58NVG2S3ETAI0 cannot be detected successfully (first ID byte
> > > being read duplicated, i.e. 98 98 dc 90 15 76 14 03 instead of
> > > 98 dc 90 15 76 ...).
> > > 
> > > Atmel Application Notes postulated 1 cycle NRD_SETUP without explanation
> > > [1], but it looks more appropriate to just calculate setup time properly.
> > > 
> > > [1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
> > > Cc: stable@vger.kernel.org
> > > Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > Tested-by: Alexander Dahl <ada@thorsis.com>
> > 
> > Threw this on top of 6.12.39-rt11 and tested on two custom platforms
> > both with a Spansion S34ML02G1 SLC 2GBit flash chip, but with
> > different SoCs (sama5d2, sam9x60).  We had difficulties with the
> > timing of those NAND flash chips in the past and I wanted to make sure
> > this patch does not break our setup.  Seems fine in a quick test,
> > reading and writing and reading back is successful.
> 
> thank you for your feedback!
> 
> Do you see an opportunity to drop the downstream timing quirks with my patch?

Which downstream do you refer to?

> I actually have another patch related to timings, but it's based on code-review
> only and the theoretical issue never manifested itself in practice on our side...

I also have another one, for which fixes got merged in at91bootstrap
and U-Boot already, but I never found the time to upstream it on
Linux.  It's related to pulse timing.

> (it's about missing ndelay at the end of atmel_smc_nand_exec_instr())

That does not ring a bell here, sorry.

Greets
Alex

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Sverdlin, Alexander]

Hi Alexander,

On Mon, 2025-08-25 at 15:22 +0200, Alexander Dahl wrote:
> > > Threw this on top of 6.12.39-rt11 and tested on two custom platforms
> > > both with a Spansion S34ML02G1 SLC 2GBit flash chip, but with
> > > different SoCs (sama5d2, sam9x60).  We had difficulties with the
                                          ^^^^^^^^^^^^^^^^^^^
[*]

> > > timing of those NAND flash chips in the past and I wanted to make sure
> > > this patch does not break our setup.  Seems fine in a quick test,
> > > reading and writing and reading back is successful.
> > 
> > thank you for your feedback!
> > 
> > Do you see an opportunity to drop the downstream timing quirks with my patch?
> 
> Which downstream do you refer to?

That's how I understood the phrase above, that some adjustments are still required
on your side additionally to standard timings. Sorry for the confusion, if it turns
out to be a misunderstanding on my side!

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Alexander Dahl]

Hello Alexander,

Am Mon, Aug 25, 2025 at 01:41:05PM +0000 schrieb Sverdlin, Alexander:
> Hi Alexander,
> 
> On Mon, 2025-08-25 at 15:22 +0200, Alexander Dahl wrote:
> > > > Threw this on top of 6.12.39-rt11 and tested on two custom platforms
> > > > both with a Spansion S34ML02G1 SLC 2GBit flash chip, but with
> > > > different SoCs (sama5d2, sam9x60).  We had difficulties with the
>                                           ^^^^^^^^^^^^^^^^^^^
> [*]
> 
> > > > timing of those NAND flash chips in the past and I wanted to make sure
> > > > this patch does not break our setup.  Seems fine in a quick test,
> > > > reading and writing and reading back is successful.
> > > 
> > > thank you for your feedback!
> > > 
> > > Do you see an opportunity to drop the downstream timing quirks with my patch?
> > 
> > Which downstream do you refer to?
> 
> That's how I understood the phrase above, that some adjustments are still required
> on your side additionally to standard timings. Sorry for the confusion, if it turns
> out to be a misunderstanding on my side!

I don't think your patch is sufficient to solve our problems with
sam9x60 and S34ML02G1.  Different parts of the timings are affected.
You changed NRD_SETUP while I changed NRD_PULSE.  I just wanted to
make sure both patches do not interfere.

For reference, these are the changes in at91bootstrap and u-boot I
referred to:

https://github.com/linux4sam/at91bootstrap/issues/174
https://github.com/linux4sam/at91bootstrap/commit/e2dfd8141d00613a37acee66ef5724f70f34a538

https://source.denx.de/u-boot/u-boot/-/commit/344e2f2cd4a407f847b301804f37d036e8a0a10c

My problem might be fixed with commit f552a7c7e0a1 ("mtd: rawnand:
atmel: set pmecc data setup time"), but I did not have time to test
that assumption.

Sorry for the confusion.

Greets
Alex

Re: [PATCH v2] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing

[Miquel Raynal]

On Thu, 21 Aug 2025 14:00:57 +0200, A. Sverdlin wrote:
> Having setup time 0 violates tAR, tCLR of some chips, for instance
> TOSHIBA TC58NVG2S3ETAI0 cannot be detected successfully (first ID byte
> being read duplicated, i.e. 98 98 dc 90 15 76 14 03 instead of
> 98 dc 90 15 76 ...).
> 
> Atmel Application Notes postulated 1 cycle NRD_SETUP without explanation
> [1], but it looks more appropriate to just calculate setup time properly.
> 
> [...]

Applied to mtd/fixes, thanks!

[1/1] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing
      commit: fd779eac2d659668be4d3dbdac0710afd5d6db12

Patche(s) should be available on mtd/linux.git and will be
part of the next PR (provided that no robot complains by then).

Kind regards,
Miquèl