linux-kernel - Re: [PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Nov 2021 09:32:04 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Herve Codina <herve.codina@...tlin.com>
Cc:     Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

Hi Herve,

herve.codina@...tlin.com wrote on Fri, 19 Nov 2021 08:39:09 +0100:

> Under certain circumstances, the timing settings calculated by
> the FSMC NAND controller driver were inaccurate.
> These settings lead to incorrect data reads or fallback to

led to

oneOf:
  - fallbacks to timing mode 0
  - fallbacks to mode 0
but "mode X timing" does not look good :) (same below, it applies all
over this commit log)

> mode 0 timings depending on NAND chip used.

depending on the NAND chip used

> 
> The timing computation did not take into account the following
> constraint given in SPEAr3xx reference manual:
>   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

See below for the style request (this really is nitpicking, I'm sorry)

> 
> Enhance the timings calculation by taking into account this
> additional constraint.
> 
> The change has no impact on slow timing mode such as mode 0.

This change?
slow timing modes

> Indeed, on mode 0 timing, computed values are the same with and
> without the patch.
> 
> The NAND that previously used the mode 0 fallback timing is
> working in mode 3 timing.

I believe we should be more generic:
"NANDs which previously stayed in mode 0 because of ... can now work
at higher speeds and NANDs which were not working at all because of the
corrupted data returned interact at high speeds without troubles.

Overall improvement on a <flash name/type>:"

> The read/writes accesses performances (flash_speed tool result)
> are the following:
>                         mode0       mode3
> eraseblock write speed  3220 KiB/s  4511 KiB/s
> eraseblock read speed   4491 KiB/s  7529 KiB/s
> 
> The NAND where data reads were previously incorrect are correct
> and this NAND is also working at mode 3 timing.
> 
> Signed-off-by: Herve Codina <herve.codina@...tlin.com>
> ---
> Changes v1 to v2:
> - Commit log reword
> - Added performance details in commit log
> - Used #define for TOUTDEL and TINDEL and
>   Fixed coding style
> - Used max3()
> 
>  drivers/mtd/nand/raw/fsmc_nand.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 0a6c9ef0ea8b..56b9da252346 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -94,6 +94,14 @@
>  
>  #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
>  
> +/*
> + * According to SPEAr300 Reference Manual (RM0082)
> + *  TOUDEL = 7ns (Output delay from the flip-flops to the board)
> + *  TINDEL = 5ns (Input delay from the board to the flipflop)
> + */
> +#define TOUTDEL	7000
> +#define TINDEL	5000
> +
>  struct fsmc_nand_timings {
>  	u8 tclr;
>  	u8 tar;
> @@ -278,7 +286,7 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  {
>  	unsigned long hclk = clk_get_rate(host->clk);
>  	unsigned long hclkn = NSEC_PER_SEC / hclk;
> -	u32 thiz, thold, twait, tset;
> +	u32 thiz, thold, twait, tset, twait_min;
>  
>  	if (sdrt->tRC_min < 30000)
>  		return -EOPNOTSUPP;
> @@ -310,13 +318,6 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->thold > FSMC_THOLD_MASK)
>  		tims->thold = FSMC_THOLD_MASK;
>  
> -	twait = max(sdrt->tRP_min, sdrt->tWP_min);
> -	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> -	if (tims->twait == 0)
> -		tims->twait = 1;
> -	else if (tims->twait > FSMC_TWAIT_MASK)
> -		tims->twait = FSMC_TWAIT_MASK;
> -
>  	tset = max(sdrt->tCS_min - sdrt->tWP_min,
>  		   sdrt->tCEA_max - sdrt->tREA_max);
>  	tims->tset = DIV_ROUND_UP(tset / 1000, hclkn) - 1;
> @@ -325,6 +326,21 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->tset > FSMC_TSET_MASK)
>  		tims->tset = FSMC_TSET_MASK;
>  
> +	/*
> +	 * According to SPEAr300 Reference Manual (RM0082) which gives more
> +	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
> +	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

It is certainly best if in plain text we follow the same spacing
convention than in the code: (tset * TCLK). Same applies to your commit
log.

> +	 */
> +	twait_min = sdrt->tCEA_max - ((tims->tset + 1) * hclkn * 1000)
> +		    + TOUTDEL + TINDEL;
> +	twait = max3(sdrt->tRP_min, sdrt->tWP_min, twait_min);
> +
> +	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> +	if (tims->twait == 0)
> +		tims->twait = 1;
> +	else if (tims->twait > FSMC_TWAIT_MASK)
> +		tims->twait = FSMC_TWAIT_MASK;
> +
>  	return 0;
>  }
>  

Otherwise looks good.

Thanks,
Miquèl