From: Frieder Schrempf <frieder.schrempf@kontron.de>

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@kontron.de>
---
 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;
+	}
+
+	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);
+
 	status = spi_set_cs_timing(spi);
 	if (status) {
 		mutex_unlock(&spi->controller->io_mutex);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4f8a0c28e1d46..f5be4f54d1424 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -685,6 +685,9 @@ struct spi_controller {
 	 */
 	int (*set_cs_timing)(struct spi_device *spi);
 
+	/* set RX sampling point */
+	unsigned int (*set_rx_sampling_point)(struct spi_device *spi, unsigned int freq);
+
 	/*
 	 * Bidirectional bulk transfers
 	 *
@@ -1337,6 +1340,11 @@ extern int spi_setup(struct spi_device *spi);
 extern int spi_async(struct spi_device *spi, struct spi_message *message);
 extern int spi_target_abort(struct spi_device *spi);
 
+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 int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq);
+
 static inline size_t
 spi_max_message_size(struct spi_device *spi)
 {

-- 
2.53.0

Hello,

+ Santhosh

On 03/03/2026 at 17:29:26 +01, Frieder Schrempf <frieder@fris.de> wrote:

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> 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@kontron.de>
> ---
>  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.

> +	}
> +
> +	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.

Cheers,
Miquèl

On 09.03.26 16:25, Miquel Raynal wrote:
> Hello,
> 
> + Santhosh
> 
> On 03/03/2026 at 17:29:26 +01, Frieder Schrempf <frieder@fris.de> wrote:
> 
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> 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@kontron.de>
>> ---
>>  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.

From: Frank Li (AI-BOT) <frank.li@nxp.com>

> +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;

Potential overflow: multiplying three u32/u16 values before assigning to
u64. Cast operands to u64 before multiplication to avoid intermediate
overflow.

> +	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;

Same overflow risk in denominator multiplication. Cast to u64 first.

> +	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);

Line exceeds 80 chars. Break into two lines.

> +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;
> +}

Bug: function receives 'freq' parameter but ignores it and passes
'spi->max_speed_hz' to the handler instead. Should pass 'freq' or remove
the parameter.

> +	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);

This call happens after spi_set_cs_timing() check but before the mutex is
unlocked. Verify this is the intended placement and that no locking issues
exist with controller callbacks.

AI bot review and may be useless.