Re: [PATCH v4 14/16] mtd: spinand: negotiate optimal PHY operating point before dirmap creation

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 18/06/2026 at 13:07:23 +0530, Santhosh Kumar K <s-k6@ti.com> wrote:

> Dirmap descriptors encode the op template including the operating
> frequency at creation time, so PHY tuning must complete before dirmaps
> are created to ensure the validated frequency is embedded in the
> descriptors from the start.

Good idea.

> Move dirmap creation from spinand_init() to spinand_probe(), after a
> new spinand_configure_phy()

Fine with the idea, but in the core I would like to avoid TI specific
naming. We can call this helper spinand_optimize_controller(); for
instance.

Please, do not use "PHY" in the SPI NAND core, because I find it too TI
specific and we need generic enough naming so that we can include other
controller's optimizations in this logic, if ever needed.

> call that negotiates the best available PHY
> operating point. spinand_configure_phy() tries the pre-selected variant
> first. If the controller signals that PHY tuning is not applicable for
> that op, spinand_try_phy_ranked() iterates remaining variants in
> performance order — DTR variants first, then SDR variants after
> switching the bus interface if needed. On full failure the device falls
> back to the best available non-PHY mode.

Makes sense.

> Add spinand_reset_max_ops() to copy op templates with max_freq zeroed

spinand_reset_max_freq_ops()?

> before each execute_tuning call, enforcing the invariant that a non-zero
> max_freq only results from a successful tuning.
>
> PHY failure is non-fatal; the device operates at the conservative base
> rate.
>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> ---
>  drivers/mtd/nand/spi/core.c | 214 ++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spinand.h |  11 ++
>  2 files changed, 216 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index b678d0534297..5dcfaabaf2cc 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1280,10 +1280,16 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>  	/* The plane number is passed in MSB just above the column address */
>  	info.offset = plane << fls(nand->memorg.pagesize);
>  
> +	/*
> +	 * Propagate the validated PHY frequency into the dirmap op templates
> +	 * at construction time.
> +	 */
> +
>  	/* Write descriptor */
>  	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))
> @@ -1294,9 +1300,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;
>  	}
>  	desc = spinand_create_rdesc(spinand, &info);
>  	if (IS_ERR(desc))
> @@ -1744,6 +1752,17 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  				spinand->cont_read_possible = false;
>  		}
>  
> +		/*
> +		 * Save the full read variant list (ODTR and SSDR ops) for PHY
> +		 * tuning iteration. Only saved when all ODTR templates are
> +		 * valid so spinand_configure_phy() knows ranked fallback is
> +		 * available.
> +		 */
> +		if (spinand->odtr_op_templates.read_cache &&
> +		    spinand->odtr_op_templates.write_cache &&
> +		    spinand->odtr_op_templates.update_cache)
> +			spinand->phy_read_variants = info->op_variants.read_cache;
> +
>  		return 0;
>  	}
>  
> @@ -1922,7 +1941,6 @@ static int spinand_mtd_suspend(struct mtd_info *mtd)
>  
>  static int spinand_init(struct spinand_device *spinand)
>  {
> -	struct device *dev = &spinand->spimem->spi->dev;
>  	struct mtd_info *mtd = spinand_to_mtd(spinand);
>  	struct nand_device *nand = mtd_to_nanddev(mtd);
>  	int ret;
> @@ -2014,14 +2032,6 @@ static int spinand_init(struct spinand_device *spinand)
>  	mtd->ecc_step_size = nanddev_get_ecc_conf(nand)->step_size;
>  	mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3, 4);
>  
> -	ret = spinand_create_dirmaps(spinand);
> -	if (ret) {
> -		dev_err(dev,
> -			"Failed to create direct mappings for read/write operations (err = %d)\n",
> -			ret);
> -		goto err_cleanup_ecc_engine;
> -	}
> -
>  	return 0;
>  
>  err_cleanup_ecc_engine:
> @@ -2050,6 +2060,178 @@ static void spinand_cleanup(struct spinand_device *spinand)
>  	kfree(spinand->scratchbuf);
>  }
>  
> +/*
> + * spinand_try_phy_ranked() - Try PHY tuning on variants in performance order.
> + * @spinand:    SPI NAND device
> + * @mem:        SPI memory device
> + * @odtr:       true to iterate ODTR variants, false for SSDR variants
> + * @tried_mask: bitmask of already-tried variant indices; updated on each try
> + *
> + * Iterates the full read variant list in descending performance order,
> + * skipping variants in @tried_mask, and calls execute_tuning on each until one
> + * succeeds. The fastest PHY-tunable variant is chosen regardless of which was
> + * pre-selected at init; ranked iteration finds the best available variant
> + * without re-trying already-attempted ones.
> + *
> + * On success, sets spinand->max_read_op and updates the matching
> + * odtr_op_templates.read_cache or ssdr_op_templates.read_cache.
> + */
> +static bool spinand_try_phy_ranked(struct spinand_device *spinand,
> +				   struct spi_mem *mem, bool odtr,
> +				   u32 *tried_mask)
> +{
> +	const struct spinand_op_variants *variants = spinand->phy_read_variants;
> +	const struct spi_mem_op *best;
> +	int ret;
> +
> +	if (!variants)
> +		return false;
> +
> +	while ((best = spinand_op_find_best(spinand, variants, odtr,
> +					    *tried_mask))) {

Ok, now I get the mask.

Why do you care about odtr flag at all? We should just take the fastest
modes in descending order, I don't really understand why mentioning odtr
is needed? Maybe your goal is to make a faster loop, taking the fastest
mode in each case and compare? This should work in most cases, given the
fact that we tend to order the variants by speed, but they are not
always in perfect order. I already faced cases where faster modes
appeared to be down the list because of the game of dummy cycles. But
that is probably not impacting PHY mode since we are using the fastest
speed (with only the mode with the maximum number of dummy cycles)
usable.

So in the end, if you sort ODTR vs. SSDR for optimization reasons, ok.

> +		*tried_mask |= BIT(best - variants->ops);
> +		spinand->max_read_op = *best;
> +		spinand->max_read_op.max_freq = 0;
> +		ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +					     &spinand->max_write_op);
> +		if (ret && ret != -EOPNOTSUPP)
> +			dev_warn(&mem->spi->dev, "%s PHY tuning failed: %d\n",
> +				 odtr ? "ODTR" : "SSDR", ret);
> +		if (!ret && spinand->max_read_op.max_freq) {
> +			if (odtr)
> +				spinand->odtr_op_templates.read_cache = best;
> +			else
> +				spinand->ssdr_op_templates.read_cache = best;
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +/*
> + * spinand_reset_max_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
> + * PHY-validated; zero means the base rate applies.
> + */
> +static void spinand_reset_max_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;
> +}
> +
> +/*
> + * spinand_configure_phy() - Negotiate the optimal PHY operating point.
> + * @spinand:    SPI NAND device
> + * @mem:        SPI memory device
> + *
> + * Tries the pre-selected variant first.  If the controller signals that
> + * PHY tuning is not applicable for that specific op, iterates all remaining
> + * variants in performance order.  For devices that support both DTR and SDR
> + * interfaces, DTR variants are tried first; if all fail the device is
> + * switched to SDR mode and SDR variants are tried.  On full failure the
> + * device falls back to the best available non-PHY mode.  Devices that
> + * support only SDR skip the DTR ranked pass entirely.
> + *
> + * PHY failure is never fatal.
> + *
> + * Note: tried_mask is u32, supporting up to 32 variants total across both
> + * ODTR and SSDR. Flash devices with more than 32 read variants are not
> + * supported.
> + */
> +static void spinand_configure_phy(struct spinand_device *spinand,
> +				  struct spi_mem *mem)
> +{
> +	u32 tried_mask;
> +	int ret;
> +
> +	spinand_reset_max_ops(spinand, spinand->op_templates);
> +
> +	ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +				     &spinand->max_write_op);
> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_warn(&mem->spi->dev, "Failed to execute PHY tuning: %d\n",
> +			 ret);

I guess we don't want to warn in this case. You can keep it as a dbg log
if you want.

> +	/*
> +	 * 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;
> +
> +	if (!mem->spi->post_config_max_speed_hz || spinand->bus_iface == SSDR ||
> +	    !spinand->phy_read_variants)
> +		return;

Why not moving this earlier? (except the SSDR check) this way we return
even earlier if the tuning cannot happen.

I am not sure I understand your use of phy_read_variants here. Can it
really be NULL?

> +	if (WARN_ON(spinand->phy_read_variants->nops > 32))
> +		return;
> +
> +	/* Mark the pre-selected ODTR variant as already tried */
> +	tried_mask = BIT(spinand->odtr_op_templates.read_cache -
> +			 spinand->phy_read_variants->ops);
> +
> +	dev_dbg(&mem->spi->dev,
> +		"PHY tuning skipped for current op; searching for best PHY variant\n");
> +
> +	/* Pass 1: try all remaining ODTR variants in performance order */
> +	if (spinand_try_phy_ranked(spinand, mem, true, &tried_mask))
> +		return;
> +
> +	/*
> +	 * Pass 2: switch to SSDR and try all SSDR variants in performance
> +	 * order.
> +	 *
> +	 * Only enter if we actually have SSDR support and a reconfigure
> +	 * callback.

It is not possible to enable ODTR without the configure callback.

> The hardware is still in ODTR mode here so no
> +	 * configure_chip call is needed to undo; just set up the ODTR non-PHY
> +	 * fallback and return.
> +	 */
> +	if (!spinand->ssdr_op_templates.read_cache ||
> +	    !spinand->ssdr_op_templates.write_cache ||

I don't think this is ever possible, is it?

> +	    !spinand->configure_chip)
> +		goto use_odtr_non_phy;

I don't think we will ever use this path.

> +
> +	if (spinand->configure_chip(spinand, SSDR))
> +		goto use_odtr_non_phy;

We should handle chips with no ODTR support. I'm not sure the logic is
robust enough for them. Have you tried by commenting out the ODTR
variants of your chip?

> +
> +	spinand->op_templates = &spinand->ssdr_op_templates;
> +	spinand->bus_iface = SSDR;
> +	spinand->max_write_op = *spinand->ssdr_op_templates.write_cache;
> +	spinand->max_write_op.max_freq = 0;
> +
> +	/*
> +	 * Only ODTR variants were candidates in Pass 1; SSDR bit positions
> +	 * are clear
> +	 */
> +	if (spinand_try_phy_ranked(spinand, mem, false, &tried_mask))
> +		return;
> +
> +	/*
> +	 * All PHY attempts exhausted.  Revert to ODTR for non-PHY DTR
> +	 * operation.  If revert fails, stay in SSDR — a mode mismatch
> +	 * (ODTR op templates on SSDR-mode device) would corrupt data.
> +	 */
> +	if (spinand->configure_chip(spinand, ODTR)) {
> +		dev_warn(&mem->spi->dev,
> +			 "Failed to revert to ODTR, staying in SSDR non-PHY\n");
> +		spinand_reset_max_ops(spinand, &spinand->ssdr_op_templates);
> +		return;
> +	}
> +
> +use_odtr_non_phy:
> +	spinand->op_templates = &spinand->odtr_op_templates;
> +	spinand->bus_iface = ODTR;
> +	spinand_reset_max_ops(spinand, &spinand->odtr_op_templates);
> +}

>  static int spinand_probe(struct spi_mem *mem)
>  {
>  	struct spinand_device *spinand;
> @@ -2072,6 +2254,20 @@ static int spinand_probe(struct spi_mem *mem)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Negotiate the best PHY operating point before creating dirmaps so
> +	 * the validated frequency is available at dirmap construction time.
> +	 */
> +	spinand_configure_phy(spinand, mem);
> +
> +	ret = spinand_create_dirmaps(spinand);
> +	if (ret) {
> +		dev_err(&mem->spi->dev,
> +			"Failed to create direct mappings for read/write operations (err = %d)\n",
> +			ret);
> +		goto err_spinand_cleanup;
> +	}
> +
>  	ret = mtd_device_register(mtd, NULL, 0);
>  	if (ret)
>  		goto err_spinand_cleanup;
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index ec6efcfeef83..50a8319cf11e 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -791,8 +791,19 @@ struct spinand_device {
>  	struct spinand_mem_ops *op_templates;
>  	enum spinand_bus_interface bus_iface;
>  
> +	/*
> +	 * Full read variant list (ODTR and SSDR ops together), saved when ODTR
> +	 * templates are valid. Used by spinand_configure_phy() to iterate all
> +	 * candidates when the pre-selected variant cannot be PHY-tuned.
> +	 */
> +	const struct spinand_op_variants *phy_read_variants;

Why do you prefix this pointer name with "phy"? The variants pointed are
basically all read variants, no more no less. Even though you want to
use them for PHY tuning purposes, I don't think it is wise to name the
member here.

> +
>  	struct spinand_dirmap *dirmaps;
>  
> +	/* Persistent op templates updated by execute_tuning with validated speed. */
> +	struct spi_mem_op max_read_op;
> +	struct spi_mem_op max_write_op;
> +
>  	int (*select_target)(struct spinand_device *spinand,
>  			     unsigned int target);
>  	unsigned int cur_target;

Thanks,
Miquèl