Re: [PATCH] mtd: rawnand: sunxi: fix H6/H616 controller timings

Hello James,

On 14/07/2026 at 19:31:38 -06, James Hilliard <[email protected]> wrote:

> The NAND timing calculation was written for the original A10 NDFC. It
> assumes command and address setup and hold intervals T1-T4, T7 and T11
> are one controller clock and uses the A10 timing-register encodings.
>
> The H6/H616 NDFC instead defines those intervals as two internal clock
> cycles and uses different encodings for tWB, tADL, tWHR and tRHW, as
> documented in the H616 User Manual.
>
> Describe the timing characteristics in the controller capability data
> so the clock solver can select a rate permitted by the NAND SDR timings
> and program valid delay fields. Keep the legacy A10 behavior unchanged.
>
> Fixes: 88fd4e4deae8 ("mtd: rawnand: sunxi: Add support for H616 nand controller")

Cc: stable missing here

> Signed-off-by: James Hilliard <[email protected]>

...

> @@ -1667,16 +1681,35 @@ static int sunxi_nfc_hw_ecc_write_oob(struct nand_chip *nand, int page)
>  	return nand_prog_page_end_op(nand);
>  }
>  
> -static const s32 tWB_lut[] = {6, 12, 16, 20};
> -static const s32 tRHW_lut[] = {4, 8, 12, 20};
> +static const struct sunxi_nfc_timings sun4i_a10_nfc_timings = {
> +	.setup_cycles = 1,
> +	.tWB = { 6, 12, 16, 20 },
> +	.tADL = { 7, 15, 23, 31 },
> +	.tWHR = { 7, 15, 23, 31 },
> +	.tRHW = { 4, 8, 12, 20 },
> +};

This patch looks overall correct but must be split. For instance, the
fact that you drop the LUT in favour of you own timing array shall be
done in a preparation patch, without adding new timings, nor adding the
new controller timings.

Then in a second time you could add tADL and tWHR support, etc.

> +
> +static const struct sunxi_nfc_timings sun50i_h6_nfc_timings = {
> +	.setup_cycles = 2,
> +	.tWB = { 28, 44, 60, 76 },
> +	.tADL = { 0, 12, 28, 44 },
> +	.tWHR = { 0, 12, 28, 44 },
> +	.tRHW = { 8, 24, 40, 56 },
> +};
> +
> +static void sunxi_nand_update_min_period(u32 *min_period, u32 duration,
> +					 unsigned int cycles)
> +{
> +	*min_period = max(*min_period, DIV_ROUND_UP(duration, cycles));
> +}
>  
> -static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration,
> -		u32 clk_period)
> +static int sunxi_nand_lookup_timing(const u8 *lut, u32 duration,
> +				    u32 clk_period)

This change is fine but likely also unrelated and should (maybe) be moved in its
own commit.

>  {
>  	u32 clk_cycles = DIV_ROUND_UP(duration, clk_period);
>  	int i;
>  
> -	for (i = 0; i < lut_size; i++) {
> +	for (i = 0; i < SUNXI_NFC_TIMING_STEPS; i++) {
>  		if (clk_cycles <= lut[i])
>  			return i;
>  	}

...

> @@ -1703,77 +1734,65 @@ static int sunxi_nfc_setup_interface(struct nand_chip *nand, int csline,
>  		return -ENOTSUPP;
>  
>  	/* T1 <=> tCLS */
> -	if (timings->tCLS_min > min_clk_period)
> -		min_clk_period = timings->tCLS_min;
> +	sunxi_nand_update_min_period(&min_clk_period, timings->tCLS_min,
> +				     nfc_timings->setup_cycles);

I am not sure I get the added value of this helper?

>  
>  	/* T2 <=> tCLH */
> -	if (timings->tCLH_min > min_clk_period)
> -		min_clk_period = timings->tCLH_min;
> +	sunxi_nand_update_min_period(&min_clk_period, timings->tCLH_min,
> +				     nfc_timings->setup_cycles);
>  

...

>  
>  static const struct sunxi_nfc_caps sunxi_nfc_h616_caps = {
> @@ -2641,6 +2664,7 @@ static const struct sunxi_nfc_caps sunxi_nfc_h616_caps = {
>  	.nuser_data_tab = ARRAY_SIZE(sunxi_user_data_len_h6),
>  	.max_ecc_steps = 32,
>  	.sram_size = 8192,
> +	.timings = &sun50i_h6_nfc_timings,

And this should be the last change in your series.

BTW would it be relevant to align the various names ? (h6 timings in the
h616 structure).

Thanks,
Miquèl