Re: [PATCH v11 16/19] mtd: spinand: negotiate optimal controller operating point before dirmap creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mtd: spinand: spinand_create_dirmap() overwrites per-op max_freq limits
- [High] mtd: spinand: missing configure_chip() call in spinand_try_ranked_variant()
- [High] mtd: spinand: spinand_op_find_best_variant() ranks and overclocks low-freq ops
- [Medium] mtd: spinand: broken cont_read_cache matching in spinand_op_shape_matches()
- [Medium] mtd: spinand: missing cont_read_cache shape check on initial tuning success
--

commit 8733b92e8d67a304d901c8ece339d2fe4c54f7ad
Author: Santhosh Kumar K <s-k6@ti.com>

mtd: spinand: negotiate optimal controller operating point before dirmap creation

This commit introduces spinand_optimize_controller() to perform PHY
calibration and ranked variant fallback before creating direct mappings in
spinand_probe(). It also re-pairs continuous-read templates by protocol shape
when falling back to another read variant and embeds the calibrated max_freq
into the dirmap operation templates.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 9dc43c8c4162d..645528b6c2d77 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1300,6 +1300,7 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>  	info.length = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
>  	info.primary_op_tmpl = *spinand->op_templates->update_cache;
>  	info.primary_op_tmpl.data.ecc = enable_ecc;
> +	info.primary_op_tmpl.max_freq = spinand->max_write_op.max_freq;
>  	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
>  					  spinand->spimem, &info);
>  	if (IS_ERR(desc))
> @@ -1310,9 +1311,11 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>  	/* Read descriptor */
>  	info.primary_op_tmpl = *spinand->op_templates->read_cache;
>  	info.primary_op_tmpl.data.ecc = enable_ecc;
> +	info.primary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
>  	if (secondary_op) {
>  		info.secondary_op_tmpl = *spinand->op_templates->cont_read_cache;
>  		info.secondary_op_tmpl.data.ecc = enable_ecc;
> +		info.secondary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
>  	}

[Severity: High]
Does unconditionally overwriting info.primary_op_tmpl.max_freq and
info.secondary_op_tmpl.max_freq in spinand_create_dirmap() discard the
per-variant datasheet frequency limits defined by flash drivers?

When mem->spi->post_config_max_speed_hz is 0, or when
spi_mem_execute_tuning() is unsupported (-EOPNOTSUPP) or fails in
spinand_optimize_controller(), spinand->max_read_op.max_freq and
spinand->max_write_op.max_freq remain 0.

In spinand_create_dirmap(), info.primary_op_tmpl and info.secondary_op_tmpl
are first initialized from *spinand->op_templates->update_cache,
*spinand->op_templates->read_cache, and
*spinand->op_templates->cont_read_cache, which carry per-variant frequency
caps in drivers such as drivers/mtd/nand/spi/winbond.c (e.g., 54 MHz,
80 MHz, 86 MHz, 104 MHz, 105 MHz, 120 MHz, 124 MHz, 133 MHz, and 162 MHz).

Overwriting max_freq with 0 causes spi_mem_adjust_op_freq() during
dirmap creation:

drivers/spi/spi-mem.c:spi_mem_adjust_op_freq() {
    ...
	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
		op->max_freq = mem->spi->max_speed_hz;
    ...
}

to raise the operation frequency all the way up to mem->spi->max_speed_hz
(for instance, clocking an 80 MHz 1S-4D-4D or 120 MHz 8D-8D-8D op at
166 MHz). Even when tuning succeeds, stamping spinand->max_read_op.max_freq
onto the dirmap templates ignores any lower per-variant max_freq ceiling on
read_cache or cont_read_cache.

> @@ -1559,22 +1562,51 @@ static int spinand_init_odtr_instruction_set(struct spinand_device *spinand)
>  	return 0;
>  }
>  
> +/**
> + * spinand_op_shape_matches() - Check whether two ops share the same protocol
> + *				 shape.
> + * @a: first op
> + * @b: second op
> + *
> + * Compares cmd/addr/data buswidth, addr.nbytes, and DTR-ness, ignoring
> + * dummy cycle count and address value. A continuous-read op and its
> + * corresponding read_cache op are expected to differ only in those ignored
> + * fields: continuous read typically needs fewer dummy cycles, and always
> + * targets an internally tracked address rather than the caller's.
> + */
> +static bool spinand_op_shape_matches(const struct spi_mem_op *a,
> +				     const struct spi_mem_op *b)
> +{
> +	return a->cmd.buswidth == b->cmd.buswidth &&
> +	       a->cmd.dtr == b->cmd.dtr &&
> +	       a->addr.nbytes == b->addr.nbytes &&
> +	       a->addr.buswidth == b->addr.buswidth &&
> +	       a->addr.dtr == b->addr.dtr &&
> +	       a->data.buswidth == b->data.buswidth &&
> +	       a->data.dtr == b->data.dtr;
> +}

[Severity: Medium]
As reported by Sashiko on v4 [1], can spinand_op_shape_matches() ever match
an SSDR continuous-read variant in spinand_find_cont_read_variant(), or pair
the right dummy-cycle sub-variant in ODTR mode?

In drivers/mtd/nand/spi/winbond.c (the only driver defining cont_read_cache
variant tables), all SSDR read_cache variants use a 2-byte address
(addr.nbytes == 2, dummy.nbytes == ndummy), whereas all SSDR continuous-read
variants convert one address byte into dummy cycles:

#define WINBOND_CONT_READ_FROM_CACHE_1S_8S_8S_OP(ndummy, buf, len, freq) \
	SPI_MEM_OP(SPI_MEM_OP_CMD(0xcb, 1),				\
		   SPI_MEM_OP_ADDR(1, 0, 8),				\
		   SPI_MEM_OP_DUMMY(ndummy - 1, 8),			\
		   SPI_MEM_OP_DATA_IN(len, buf, 8),			\
		   SPI_MEM_OP_MAX_FREQ(freq))

Because spinand_op_shape_matches() requires a->addr.nbytes == b->addr.nbytes,
spinand_find_cont_read_variant(spinand, best, SSDR) rejects every SSDR entry
and always returns NULL, which causes spinand_try_ranked_variant() to set
spinand->cont_read_possible = false on every SSDR fallback.

Conversely, for ODTR variants where WINBOND_CONT_READ_FROM_CACHE_8D_8D_8D_OP
also has addr.nbytes == 2, spinand_op_shape_matches() ignores dummy cycle
counts and spinand_find_cont_read_variant() passes skip_mask == 0 to
spinand_op_find_best_variant(). If read_cache falls back to a different
dummy-cycle sub-variant, won't spinand_find_cont_read_variant() still pick
the first cont_read_cache sub-variant with a mismatched dummy cycle count,
even though both ops share a single hardware VCR dummy clock register?

[ ... ]

> @@ -2073,6 +2144,199 @@ static void spinand_cleanup(struct spinand_device *spinand)
>  	kfree(spinand->scratchbuf);
>  }
>  
> +/**
> + * spinand_try_ranked_variant() - Try controller optimization on variants in
> + *				  performance order.
[ ... ]
> +static bool spinand_try_ranked_variant(struct spinand_device *spinand,
> +				       struct spi_mem *mem,
> +				       enum spinand_bus_interface iface,
> +				       u32 *tried_mask)
> +{
> +	const struct spinand_op_variants *variants = spinand->all_read_variants;
> +	struct spinand_mem_ops *templates = iface == ODTR ?
> +		&spinand->odtr_op_templates : &spinand->ssdr_op_templates;
> +	const struct spi_mem_op *best;
> +	int ret;
> +
> +	if (!variants)
> +		return false;
> +
> +	while ((best = spinand_op_find_best_variant(spinand, variants, iface,
> +						    *tried_mask, NULL))) {
> +		*tried_mask |= BIT(best - variants->ops);
> +		spinand->max_read_op = *best;
> +		spinand->max_read_op.max_freq = 0;
> +		spinand->max_write_op.max_freq = 0;
> +		ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +					     &spinand->max_write_op);
> +		if (ret && ret != -EOPNOTSUPP)
> +			dev_dbg(&mem->spi->dev, "%s optimization failed: %d\n",
> +				iface == ODTR ? "ODTR" : "SSDR", ret);
> +		if (!ret && spinand->max_read_op.max_freq) {
> +			templates->read_cache = best;
> +			if (spinand->all_cont_read_variants) {
> +				templates->cont_read_cache =
> +					spinand_find_cont_read_variant(spinand, best, iface);
> +				/* Only clear; never re-enable a denied capability. */
> +				if (!templates->cont_read_cache)
> +					spinand->cont_read_possible = false;
> +			}
> +			return true;
> +		}
> +	}
> +	return false;
> +}

[Severity: High]
Doesn't spinand_try_ranked_variant() need to update templates->read_cache and
call spinand->configure_chip(spinand, iface) for each candidate variant
before running spi_mem_execute_tuning() (and restore it if all candidates
fail)?

On ODTR-capable Winbond flashes (W35N01JW, W35N02JW, and W35N04JW),
spinand->configure_chip (w35n0xjw_vcr_cfg() in drivers/mtd/nand/spi/winbond.c)
inspects spinand->odtr_op_templates.read_cache or
spinand->ssdr_op_templates.read_cache to program the flash chip's Volatile
Configuration Registers with that variant's exact dummy clock count
(W35N01JW_VCR_DUMMY_CLOCK_REG) and I/O mode (W35N01JW_VCR_IO_MODE_REG:
OCTAL_DDR, OCTAL_SDR, or SINGLE_SDR):

drivers/mtd/nand/spi/winbond.c:w35n0xjw_vcr_cfg() {
    ...
	switch (iface) {
	case SSDR:
		ref_op = spinand->ssdr_op_templates.read_cache;
		break;
	case ODTR:
		ref_op = spinand->odtr_op_templates.read_cache;
		break;
	default:
		return -EOPNOTSUPP;
	}

	dummy_cycles = ((ref_op->dummy.nbytes * 8) / ref_op->dummy.buswidth) /
		(ref_op->dummy.dtr ? 2 : 1);
    ...
}

In spinand_optimize_controller(), configure_chip() is only called once before
Pass 1 (during spinand_init_flash()) and once before Pass 2 using the initial
read_cache template. When spinand_try_ranked_variant() iterates across
variants in read_cache_octal_variants that have different dummy cycle counts
(e.g., 24 vs 16 dummy bytes in ODTR, or 20, 16, 12, and 8 dummy cycles in
SSDR) or a different VCR I/O mode (1S-8S-8S OCTAL_SDR vs 1S-1S-1S
SINGLE_SDR), the flash hardware remains programmed for the initial template's
dummy cycles and I/O mode both during spi_mem_execute_tuning() and after
templates->read_cache = best is committed.

> +
> +/**
> + * spinand_reset_max_freq_ops() - Copy op templates and zero max_freq on both.
> + * @spinand:    SPI NAND device
> + * @templates:  op template set to copy from
> + *
> + * Called before execute_tuning so max_freq starts at zero; execute_tuning sets
> + * it to the validated clock rate only on success. A non-zero max_freq means
> + * controller-optimized; zero means the base rate applies.
> + */
> +static void spinand_reset_max_freq_ops(struct spinand_device *spinand,
> +				       struct spinand_mem_ops *templates)
> +{
> +	spinand->max_read_op = *templates->read_cache;
> +	spinand->max_read_op.max_freq = 0;
> +	spinand->max_write_op = *templates->write_cache;
> +	spinand->max_write_op.max_freq = 0;
> +}

[Severity: High]
As reported by Miquel Raynal on v3 [2], won't evaluating variants at the
pre-config base clock in spinand_op_find_best_variant() and then zeroing
max_read_op.max_freq before spi_mem_execute_tuning() select low-dummy-cycle,
low-frequency variants and overclock them at post_config_max_speed_hz?

When spi-max-post-config-frequency-hz is set in DT, mem->spi->max_speed_hz
holds the lower pre-configuration base clock (e.g., 25 MHz) while
mem->spi->post_config_max_speed_hz holds the high-speed target (e.g., 120 MHz
or 166 MHz).

In spinand_op_find_best_variant(), spi_mem_adjust_op_freq() clamps every
candidate in variants->ops[i] (such as Winbond's read_cache_octal_variants:
8D-8D-8D with 24 dummy bytes @ 120 MHz vs 16 dummy bytes @ 86 MHz, or
1S-8S-8S with 20 dummy bytes @ unlimited, 16 @ 162 MHz, 12 @ 124 MHz, and
8 @ 86 MHz) down to the 25 MHz base rate in mem->spi->max_speed_hz.

At the same 25 MHz frequency, same-buswidth ops differ only in dummy cycle
count, so spi_mem_calc_op_duration() ranks the variant with the fewest dummy
cycles and lowest frequency limit (e.g., 86 MHz) as the fastest variant, and
ranks the 20-dummy-cycle 166 MHz variant last.

Then spinand_reset_max_freq_ops() and spinand_try_ranked_variant() zero
spinand->max_read_op.max_freq = 0, discarding the 86 MHz limit and allowing
spi_mem_execute_tuning() to set max_read_op.max_freq to
post_config_max_speed_hz (120 MHz or 166 MHz), violating the flash datasheet's
minimum dummy cycles for that clock rate.

> +
> +/**
> + * spinand_optimize_controller() - Negotiate the optimal controller operating
> + *				   point for the SPI NAND device.
[ ... ]
> +static void spinand_optimize_controller(struct spinand_device *spinand,
> +					struct spi_mem *mem)
> +{
> +	u32 tried_mask;
> +	int ret;
> +
> +	/* Skip entirely when no post-config target is configured. */
> +	if (!mem->spi->post_config_max_speed_hz)
> +		return;
> +
> +	spinand_reset_max_freq_ops(spinand, spinand->op_templates);
> +
> +	ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +				     &spinand->max_write_op);
> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_dbg(&mem->spi->dev, "Controller optimization failed: %d\n",
> +			ret);
> +
> +	/*
> +	 * Any non-zero return or a set max_freq means we are done (error,
> +	 * unsupported, or success). Fallback only for the op-specific "skip"
> +	 * signal: ret == 0 with max_freq still 0.
> +	 */
> +	if (ret || spinand->max_read_op.max_freq)
> +		return;

[Severity: Medium]
As reported by Sashiko on v3 [3], if the initial spi_mem_execute_tuning()
call succeeds here, doesn't spinand_optimize_controller() return without
verifying that spinand->op_templates->cont_read_cache matches the protocol
shape of spinand->op_templates->read_cache?

In spinand_match_and_init(), cont_read_cache is selected independently from
info->op_variants.cont_read_cache without checking addr.nbytes equality
against read_cache. When the initial tuning succeeds, spinand_create_dirmap()
stamps spinand->max_read_op.max_freq onto info.secondary_op_tmpl even when
cont_read_cache has a different addr.nbytes (e.g., 1 address byte instead of
2 address bytes on Winbond SSDR variants) or a different protocol shape that
was never validated by spinand_op_shape_matches().

[1] https://lore.kernel.org/r/20260618080209.5701E1F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/87mrxjg9zi.fsf@bootlin.com
[3] https://lore.kernel.org/r/20260527190440.9DB221F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105105832.2134313-1-s-k6@ti.com?part=16

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/