Re: [PATCH RFC 5/7] spi: Add RX sampling point adjustment

[Frieder Schrempf]

On 09.03.26 16:25, Miquel Raynal wrote:
> Hello,
> 
> + Santhosh
> 
> On 03/03/2026 at 17:29:26 +01, Frieder Schrempf <frieder@xxxxxxx> wrote:
> 
>> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>>
>> Some SPI devices such as SPI NAND chips specify a clock-to-RX-sampling
>> delay constraint, which means that for the data read from the device
>> at a certain clock rate, we need to make sure that the point at
>> which the data is sampled is correct.
>>
>> The default is to assume that the data can be sampled one half clock
>> cycle after the triggering clock edge. For high clock rates, this
>> can be too early.
>>
>> To check this we introduce a new core function spi_set_rx_sampling_point()
>> and a handler set_rx_sampling_point() in the SPI controller.
>>
>> Controllers implementing set_rx_sampling_point() can calculate the
>> sampling point delay using the helper spi_calc_rx_sampling_point()
>> and store the value to set the appropriate registers during transfer.
>>
>> In case the controller capabilities are not sufficient to compensate
>> the RX delay, spi_set_rx_sampling_point() returns a reduced clock
>> rate value that is safe to use.
>>
>> This commit does not introduce generic logic for controllers that
>> don't implement set_rx_sampling_point() in order to reduce the clock
>> rate if the RX sampling delay requirement can not be met.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>> ---
>>  drivers/spi/spi.c       | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/spi/spi.h |  8 ++++++
>>  2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 61f7bde8c7fbb..b039007ed430f 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -3988,6 +3988,77 @@ static int spi_set_cs_timing(struct spi_device *spi)
>>  	return status;
>>  }
>>  
>> +/**
>> + * spi_calc_rx_sampling_point - calculate RX sampling delay cycles
>> + * @spi: the device that requires specific a RX sampling delay
>> + * @freq: pointer to the clock frequency setpoint for the calculation. This gets
>> + *        altered to a reduced value if required.
>> + * @max_delay_cycles: the upper limit of supported delay cycles
>> + * @delay_cycles_per_clock_cycle: the ratio between delay cycles and
>> + *				  master clock cycles
>> + *
>> + * This function takes in the rx_sampling_delay_ns value from the SPI device
>> + * and the given clock frequency setpoint and calculates the required sampling
>> + * delay cycles to meet the device's spec. It uses the given controller
>> + * constraints and if those are exceeded, it adjusts the clock frequency
>> + * setpoint to a lower value that is safe to be used.
>> + *
>> + * Return: calculated number of delay cycles
>> + */
>> +unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
>> +					u16 max_delay_cycles,
>> +					u16 delay_cycles_per_clock_cycle)
>> +{
>> +	unsigned long long temp;
>> +	u16 delay_cycles;
>> +
>> +	/* if sampling delay is zero, we assume the default sampling point can be used */
>> +	if (!spi->rx_sampling_delay_ns)
>> +		return 0;
>> +
>> +	temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
>> +	do_div(temp, 1000000000UL);
>> +	delay_cycles = temp;
>> +
>> +	if (delay_cycles > max_delay_cycles) {
>> +		/*
>> +		 * Reduce the clock to the point where the sampling delay requirement
>> +		 * can be met.
>> +		 */
>> +		delay_cycles = max_delay_cycles;
>> +		temp = (1000000000UL * delay_cycles);
>> +		do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
>> +		*freq = temp;
> 
> This is silently modifying the spi_device structure, I don't like this much.

Right, I agree. I think we will likely need some struct type to hold the
frequency and sampling delay values anyway. So maybe this function could
then return a value of such type.

> 
>> +	}
>> +
>> +	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);
>> +
>> +	return delay_cycles;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_calc_rx_sampling_point);
>> +
>> +/**
>> + * spi_set_rx_sampling_point - set the RX sampling delay in the controller driver
>> + * @spi: the device that requires specific a RX sampling delay
>> + * @freq: the clock frequency setpoint for the RX sampling delay calculation
>> + *
>> + * This function calls the set_rx_sampling_point() handle in the controller
>> + * driver it is available. This makes sure that the controller uses the proper
>> + * RX sampling point adjustment. This function should be called whenever
>> + * the devices rx_sampling_delay_ns or the currently used clock frequency
>> + * changes.
>> + *
>> + * Return: adjusted clock frequency
>> + */
>> +unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
>> +{
>> +	if (spi->controller->set_rx_sampling_point)
>> +		return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
>> +
>> +	return freq;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_set_rx_sampling_point);
>> +
>>  /**
>>   * spi_setup - setup SPI mode and clock rate
>>   * @spi: the device whose settings are being modified
>> @@ -4090,6 +4161,8 @@ int spi_setup(struct spi_device *spi)
>>  		}
>>  	}
>>  
>> +	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);
> 
> I believe we need a clearer yet stronger logic around the setting of
> max_speed_hz.
> 
> The "maximum speed" parameter is reaching its limit. This is clearly
> what needs to be discussed with Santhosh. The SPI tuning series is
> playing with this value as well.

I agree. I'm still missing parts of the bigger picture here, so I'm
currently not really sure how this might look like. But I'm open to any
discussions and suggestions.