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

[Santhosh Kumar K <s-k6@ti.com>]

Hello Miquel,

On 13/08/26 14:05, Miquel Raynal wrote:
> Hi Santhosh,
> 
> I have only one comment on the spinand bits.
> 
>> +/*
>> + * spinand_try_ranked_variant() - Try controller optimization on variants in
>> + *				  performance order.
>> + * @spinand:    SPI NAND device
>> + * @mem:        SPI memory device
>> + * @iface:      bus interface to iterate (ODTR or SSDR)
>> + * @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. 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_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;
>> +	const struct spi_mem_op *best;
>> +	int ret;
>> +
>> +	if (!variants)
>> +		return false;
>> +
>> +	while ((best = spinand_op_find_best_variant(spinand, variants, iface,
>> +						    *tried_mask))) {
>> +		*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) {
>> +			if (iface == ODTR)
>> +				spinand->odtr_op_templates.read_cache = best;
>> +			else
>> +				spinand->ssdr_op_templates.read_cache = best;
>> +			spinand->cont_read_possible = false;
> 
> Why do you disable continuous reads? I know it is not the same as the
> read template, but it only differs by a few dummy cycles, so everything
> should work as expected. I believe without complexifying much the logic
> we should be able to support it.

Yeah, right! No need to disable continuous read as the only difference
is the dummy cycle count, my bad!

I've updated the logic to re-pair cont_read_cache with the selected
read_cache variant based on the protocol shape, while ignoring dummy
cycles. This keeps continuous read enabled without any unnecessary
complexity. I'll include this in v8.

Thanks,
Santhosh.

> 
> Thanks,
> Miquèl