[PATCH 2/2] spi: sunxi: fix clock divider calculation for max frequency setting

[Andre Przywara]

On Fri, 12 Jul 2024 19:14:57 +0200
Michael Walle <mwalle at kernel.org> wrote:

Hi,

> If the maximum frequency is requested, we still fall into the CDR2
> handling. But there the minimal divider is 2. For the sun6i and sun8i we
> can do better with the CDR1 setting where the minimal divider is 1:
>   SPI_CLK = MOD_CLK / 2 ^ cdr with cdr = 0
> 
> Thus, handle the div = 1 case specially.

Yes, this is correct: we lose half the performance without this patch,
in the (common) full clock speed case.
However ....

> 
> While at it, correct the comment above the calculation.
> 
> Signed-off-by: Michael Walle <mwalle at kernel.org>
> ---
>  drivers/spi/spi-sunxi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index bfb402902b8..3048ab0ecf7 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -249,6 +249,8 @@ static void sun4i_spi_set_speed_mode(struct udevice *dev)
>  	 * We have two choices there. Either we can use the clock
>  	 * divide rate 1, which is calculated thanks to this formula:
>  	 * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
> +	 * Or for sun6i/sun8i variants:
> +	 * SPI_CLK = MOD_CLK / (2 ^ cdr)
>  	 * Or we can use CDR2, which is calculated with the formula:
>  	 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
>  	 * Whether we use the former or the latter is set through the
> @@ -256,12 +258,15 @@ static void sun4i_spi_set_speed_mode(struct udevice *dev)
>  	 *
>  	 * First try CDR2, and if we can't reach the expected
>  	 * frequency, fall back to CDR1.
> +	 * There is one exception if the requested clock is the input
> +	 * clock. In that case we always use CDR1 because we'll get a
> +	 * 1:1: ration for sun6i/sun8i variants.
>  	 */
>  
>  	div = DIV_ROUND_UP(SUNXI_INPUT_CLOCK, priv->freq);
>  	reg = readl(SPI_REG(priv, SPI_CCR));
>  
> -	if ((div / 2) <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> +	if (div != 1 && ((div / 2) <= (SUN4I_CLK_CTL_CDR2_MASK + 1))) {
>  		div /= 2;

This is still not fully correct, is it? If I ask for 10 MHz, the
algorithm should select 8 MHz (24/3) or actually 6 MHz (24/4), but it
chooses 12 MHz (24/2), which is too much.
So I think this division here should be either:
	div = (div + 1) / 2;
or:
	div = DIV_ROUND_UP(div, 2);

Can someone confirm this?

Cheers,
Andre

>  		if (div > 0)
>  			div--;