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

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

Posted by A. Sverdlin 3 months, 1 week ago

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.

Without the fix we've measured -2ns tAR delay (REn asserted before ALE
deassert!); with the fix -- 60ns (subject to module clock).

[1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/mtd/nand/raw/atmel/nand-controller.c | 17 ++++++++++++++---
 1 file changed, 14 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..844df72f45063 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -1377,14 +1377,25 @@ 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.0

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

Posted by Balamanikandan.Gunasundar@microchip.com 2 months, 2 weeks ago

Hi,

This change looks good to me. But it didn't apply in any of my trees. Am 
i missing some thing?

However I got this patch modified and tested in few of our boards.

Thanks,
Bala.

On 01/07/25 7:03 pm, A. Sverdlin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 
> Without the fix we've measured -2ns tAR delay (REn asserted before ALE
> deassert!); with the fix -- 60ns (subject to module clock).
> 
> [1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
> Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>   drivers/mtd/nand/raw/atmel/nand-controller.c | 17 ++++++++++++++---
>   1 file changed, 14 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..844df72f45063 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -1377,14 +1377,25 @@ 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.0
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

Posted by Miquel Raynal 2 months, 2 weeks ago

Hi Bala,

On 22/07/2025 at 09:51:43 GMT, <Balamanikandan.Gunasundar@microchip.com> wrote:

> Hi,
>
> This change looks good to me. But it didn't apply in any of my trees. Am 
> i missing some thing?
>
> However I got this patch modified and tested in few of our boards.

Stable is missing, and I took the opportunity to report a nit, hence
this patch was marked done on my side, waiting for v2.

Thanks,
Miquèl

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

Posted by Miquel Raynal 3 months, 1 week ago

Hello Alexander,

On 01/07/2025 at 15:33:28 +02, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> 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.
>
> Without the fix we've measured -2ns tAR delay (REn asserted before ALE
> deassert!); with the fix -- 60ns (subject to module clock).
>
> [1] Link: https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ApplicationNotes/ApplicationNotes/doc6255.pdf
> Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Please add Cc: stable

> ---
>  drivers/mtd/nand/raw/atmel/nand-controller.c | 17 ++++++++++++++---
>  1 file changed, 14 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..844df72f45063 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -1377,14 +1377,25 @@ 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);

Nit as you'll need to resend anyway :), this can be on the previous line.

Otherwise lgtm.

Thanks,
Miquèl