On Fri, 12 Jul 2024 19:14:57 +0200
Michael Walle <mwa...@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 <mwa...@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--;
