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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: "A. Sverdlin" <alexander.sverdlin@siemens.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>,
	 linux-mtd@lists.infradead.org,
	Richard Weinberger <richard@nod.at>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 Nicolas Ferre <nicolas.ferre@microchip.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	 Claudiu Beznea <claudiu.beznea@tuxon.dev>,
	 linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mtd: nand: raw: atmel: Respect tAR, tCLR in read setup timing
Date: Tue, 01 Jul 2025 16:39:07 +0200	[thread overview]
Message-ID: <87ikkcou0k.fsf@bootlin.com> (raw)
In-Reply-To: <20250701133333.3871085-1-alexander.sverdlin@siemens.com> (A. Sverdlin's message of "Tue, 1 Jul 2025 15:33:28 +0200")

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