diff mbox series

[3/5] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate

Message ID 20260902-spi-sun4i-fixes-v1-3-19985ef75673@pengutronix.de (mailing list archive)
State New
Headers
Series spi: spi-sun4i: assorted fixes |

Commit Message

Jonas Rebmann Sept. 2, 2026, 3:35 p.m. UTC 
From: Marc Kleine-Budde <mkl@pengutronix.de>

A SPI transfer defines the _maximum_ speed of the SPI transfer. However
the driver doesn't take into account that the clock divider is always
rounded down (due to integer arithmetic). This results in a too high
clock rate for the SPI transfer.

E.g.: with an mclk_rate of 24 MHz and an SPI transfer speed of 10 MHz,
the original code calculates a reg of "0", which results in an effective
divider of "2" and a 12 MHz clock for the SPI transfer.

Use DIV_ROUND_UP() instead of a plain integer division to fix the
problem.

While there simplify the divider calculation for the CDR1 case, use
order_base_2() instead of two ilog2() calculations.

Fixes: b5f6517948cc ("spi: sunxi: Add Allwinner A10 SPI controller driver")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/spi/spi-sun4i.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 3:47 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Off-by-one in the CDR1 clock divider calculation omits a required '- 1', resulting in an SPI clock that runs at less than half the requested speed, which deterministically causes false transfer timeouts for large payloads.
--

commit e08997f492b7dd1b9df03ec9d36b3a190b6b3325
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate

This commit modifies sun4i_spi_transfer_one() to use DIV_ROUND_UP() for 
the clock rate calculation to prevent the SPI transfer clock from 
exceeding the requested maximum speed. It also simplifies the CDR1 
divider calculation by using order_base_2().

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 3649bcabcc9a7..ae009d5984505 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
[ ... ]
> @@ -296,14 +296,12 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  	 * First try CDR2, and if we can't reach the expected
>  	 * frequency, fall back to CDR1.
>  	 */
> -	div = mclk_rate / (2 * tfr->speed_hz);
> -	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> -		if (div > 0)
> -			div--;
> -
> -		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
> +	div_cdr1 = DIV_ROUND_UP(mclk_rate, tfr->speed_hz);
> +	div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
> +	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> +		reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
>  	} else {
> -		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
> +		div = min(SUN4I_CLK_CTL_CDR1_MASK, order_base_2(div_cdr1));
>  		reg = SUN4I_CLK_CTL_CDR1(div);

[Severity: High]
Does this calculation in sun4i_spi_transfer_one() set the divider exponent
one higher than required?

The CDR1 formula is MOD_CLK / (2 ^ (div + 1)). By using order_base_2(div_cdr1)
without subtracting 1, the actual divider becomes 2 * 2^ceil(...), dropping
the clock speed below speed_hz / 2.

Could this break the tx_time calculation for large payloads and result in
spurious -ETIMEDOUT failures when a low speed_hz falls back to CDR1? 
Should this subtract 1 from the order_base_2() result?

>  	}
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 3649bcabcc9a..ae009d598450 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -212,7 +212,7 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 				  struct spi_transfer *tfr)
 {
 	struct sun4i_spi *sspi = spi_controller_get_devdata(host);
-	unsigned int mclk_rate, div;
+	unsigned int mclk_rate, div, div_cdr1, div_cdr2;
 	unsigned long time_left;
 	unsigned int start, end, tx_time;
 	unsigned int tx_len = 0;
@@ -296,14 +296,12 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 	 * First try CDR2, and if we can't reach the expected
 	 * frequency, fall back to CDR1.
 	 */
-	div = mclk_rate / (2 * tfr->speed_hz);
-	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
-		if (div > 0)
-			div--;
-
-		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
+	div_cdr1 = DIV_ROUND_UP(mclk_rate, tfr->speed_hz);
+	div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
+	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
+		reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
+		div = min(SUN4I_CLK_CTL_CDR1_MASK, order_base_2(div_cdr1));
 		reg = SUN4I_CLK_CTL_CDR1(div);
 	}