[PATCH v2 1/2] spi: sunxi: fix CDR2 calculation

[PATCH v2 1/2] spi: sunxi: fix CDR2 calculation

[Michael Walle]

The CDR2 divider calculation always yield a frequency greater than the
requested one. Use DIV_ROUND_UP() to keep the frequency equal or below
the requested one. This way, we can also drop the "if div > 0" check
because we know for a fact that div cannot be zero.

FWIW, this aligns the CDR2 calculation with the linux driver.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/spi/spi-sunxi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
index bfb402902b8..f110a8b7658 100644
--- a/drivers/spi/spi-sunxi.c
+++ b/drivers/spi/spi-sunxi.c
@@ -233,7 +233,7 @@ err_ahb:
 static void sun4i_spi_set_speed_mode(struct udevice *dev)
 {
 	struct sun4i_spi_priv *priv = dev_get_priv(dev);
-	unsigned int div;
+	unsigned int div, div_cdr2;
 	u32 reg;
 
 	/*
@@ -259,15 +259,12 @@ static void sun4i_spi_set_speed_mode(struct udevice *dev)
 	 */
 
 	div = DIV_ROUND_UP(SUNXI_INPUT_CLOCK, priv->freq);
+	div_cdr2 = DIV_ROUND_UP(div, 2);
 	reg = readl(SPI_REG(priv, SPI_CCR));
 
-	if ((div / 2) <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
-		div /= 2;
-		if (div > 0)
-			div--;
-
+	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
 		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
-		reg |= SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
+		reg |= SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
 	} else {
 		div = fls(div - 1);
 		/* The F1C100s encodes the divider as 2^(n+1) */
-- 
2.39.2

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

[Michael Walle]

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.

While at it, correct the comment above the calculation.

Signed-off-by: Michael Walle <mwalle@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 f110a8b7658..81f1298adea 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,13 +258,16 @@ 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);
 	div_cdr2 = DIV_ROUND_UP(div, 2);
 	reg = readl(SPI_REG(priv, SPI_CCR));
 
-	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
+	if (div != 1 && (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1))) {
 		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
 		reg |= SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
 	} else {
-- 
2.39.2

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

[Andre Przywara]

On Thu, 18 Jul 2024 22:42:53 +0200
Michael Walle <mwalle@kernel.org> wrote:

> 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.
> 
> While at it, correct the comment above the calculation.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Applied to sunxi/master.

Cheers,
Andre

> ---
>  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 f110a8b7658..81f1298adea 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,13 +258,16 @@ 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);
>  	div_cdr2 = DIV_ROUND_UP(div, 2);
>  	reg = readl(SPI_REG(priv, SPI_CCR));
>  
> -	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> +	if (div != 1 && (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1))) {
>  		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
>  		reg |= SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
>  	} else {

Re: [PATCH v2 1/2] spi: sunxi: fix CDR2 calculation

[Andre Przywara]

On Thu, 18 Jul 2024 22:42:52 +0200
Michael Walle <mwalle@kernel.org> wrote:

Hi,

> The CDR2 divider calculation always yield a frequency greater than the
> requested one. Use DIV_ROUND_UP() to keep the frequency equal or below
> the requested one. This way, we can also drop the "if div > 0" check
> because we know for a fact that div cannot be zero.
> 
> FWIW, this aligns the CDR2 calculation with the linux driver.

Thanks, that looks alright now. I tested some corner cases, and it seems
to do the right thing (TM) now.

> Suggested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Michael Walle <mwalle@kernel.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Applied to sunxi/master.

Cheers,
Andre.

> ---
>  drivers/spi/spi-sunxi.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index bfb402902b8..f110a8b7658 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -233,7 +233,7 @@ err_ahb:
>  static void sun4i_spi_set_speed_mode(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev);
> -	unsigned int div;
> +	unsigned int div, div_cdr2;
>  	u32 reg;
>  
>  	/*
> @@ -259,15 +259,12 @@ static void sun4i_spi_set_speed_mode(struct udevice *dev)
>  	 */
>  
>  	div = DIV_ROUND_UP(SUNXI_INPUT_CLOCK, priv->freq);
> +	div_cdr2 = DIV_ROUND_UP(div, 2);
>  	reg = readl(SPI_REG(priv, SPI_CCR));
>  
> -	if ((div / 2) <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> -		div /= 2;
> -		if (div > 0)
> -			div--;
> -
> +	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>  		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
> -		reg |= SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
> +		reg |= SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
>  	} else {
>  		div = fls(div - 1);
>  		/* The F1C100s encodes the divider as 2^(n+1) */