Re: [PATCH v1] drivers: spi: sunxi: Fix spi speed settting

[Andre Przywara <andre.przywara@arm.com>]

On Thu,  9 Jun 2022 17:09:39 +0800
qianfanguijin@163.com wrote:

Hi Qianfan,

> From: qianfan Zhao <qianfanguijin@163.com>
> 
> dm_spi_claim_bus run spi_set_speed_mode first and then ops->claim_bus,
> but spi clock is enabled when sun4i_spi_claim_bus, that will make
> sun4i_spi_set_speed doesn't work.

Thanks for bringing this up, and sorry for the delay (please CC: the
U-Boot sunxi maintainers!).
So this is very similar to the patch as I sent earlier:
https://lore.kernel.org/u-boot/20220503212040.27884-3-andre.przywara@arm.com/

Can you please check whether this works for you as well, then reply to
that patch?
I put my version of the patch plus more fixes and F1C100s support to:
https://source.denx.de/u-boot/custodians/u-boot-sunxi/-/commits/next/

Also I am curious under what circumstances and on what board you saw the
issue? In my case it was on the F1C100s, which has a higher base clock
(200 MHz instead of 24 MHz), so everything gets badly overclocked.

Thanks!
Andre

> Fix it.
> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  drivers/spi/spi-sunxi.c | 78 ++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index b6cd7ddafa..1043cde976 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -224,6 +224,7 @@ err_ahb:
>  static int sun4i_spi_claim_bus(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
> +	u32 div, reg;
>  	int ret;
>  
>  	ret = sun4i_spi_set_clock(dev->parent, true);
> @@ -233,12 +234,38 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>  	setbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE |
>  		     SUN4I_CTL_MASTER | SPI_BIT(priv, SPI_GCR_TP));
>  
> +	/* Setup clock divider */
> +	div = SUN4I_SPI_MAX_RATE / (2 * priv->freq);
> +	reg = readl(SPI_REG(priv, SPI_CCR));
> +
> +	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> +		if (div > 0)
> +			div--;
> +
> +		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
> +		reg |= SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
> +	} else {
> +		div = __ilog2(SUN4I_SPI_MAX_RATE) - __ilog2(priv->freq);
> +		reg &= ~((SUN4I_CLK_CTL_CDR1_MASK << 8) | SUN4I_CLK_CTL_DRS);
> +		reg |= SUN4I_CLK_CTL_CDR1(div);
> +	}
> +
> +	writel(reg, SPI_REG(priv, SPI_CCR));
> +
>  	if (priv->variant->has_soft_reset)
>  		setbits_le32(SPI_REG(priv, SPI_GCR),
>  			     SPI_BIT(priv, SPI_GCR_SRST));
>  
> -	setbits_le32(SPI_REG(priv, SPI_TCR), SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
> -		     SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW));
> +	/* Setup the transfer control register */
> +	reg = SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
> +	      SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW);
> +
> +	if (priv->mode & SPI_CPOL)
> +		reg |= SPI_BIT(priv, SPI_TCR_CPOL);
> +	if (priv->mode & SPI_CPHA)
> +		reg |= SPI_BIT(priv, SPI_TCR_CPHA);
> +
> +	writel(reg, SPI_REG(priv, SPI_TCR));
>  
>  	return 0;
>  }
> @@ -329,67 +356,22 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>  {
>  	struct sun4i_spi_plat *plat = dev_get_plat(dev);
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev);
> -	unsigned int div;
> -	u32 reg;
>  
>  	if (speed > plat->max_hz)
>  		speed = plat->max_hz;
>  
>  	if (speed < SUN4I_SPI_MIN_RATE)
>  		speed = SUN4I_SPI_MIN_RATE;
> -	/*
> -	 * Setup clock divider.
> -	 *
> -	 * 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 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
> -	 * DRS bit.
> -	 *
> -	 * First try CDR2, and if we can't reach the expected
> -	 * frequency, fall back to CDR1.
> -	 */
> -
> -	div = SUN4I_SPI_MAX_RATE / (2 * speed);
> -	reg = readl(SPI_REG(priv, SPI_CCR));
> -
> -	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> -		if (div > 0)
> -			div--;
> -
> -		reg &= ~(SUN4I_CLK_CTL_CDR2_MASK | SUN4I_CLK_CTL_DRS);
> -		reg |= SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
> -	} else {
> -		div = __ilog2(SUN4I_SPI_MAX_RATE) - __ilog2(speed);
> -		reg &= ~((SUN4I_CLK_CTL_CDR1_MASK << 8) | SUN4I_CLK_CTL_DRS);
> -		reg |= SUN4I_CLK_CTL_CDR1(div);
> -	}
>  
>  	priv->freq = speed;
> -	writel(reg, SPI_REG(priv, SPI_CCR));
> -
>  	return 0;
>  }
>  
>  static int sun4i_spi_set_mode(struct udevice *dev, uint mode)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev);
> -	u32 reg;
> -
> -	reg = readl(SPI_REG(priv, SPI_TCR));
> -	reg &= ~(SPI_BIT(priv, SPI_TCR_CPOL) | SPI_BIT(priv, SPI_TCR_CPHA));
> -
> -	if (mode & SPI_CPOL)
> -		reg |= SPI_BIT(priv, SPI_TCR_CPOL);
> -
> -	if (mode & SPI_CPHA)
> -		reg |= SPI_BIT(priv, SPI_TCR_CPHA);
>  
>  	priv->mode = mode;
> -	writel(reg, SPI_REG(priv, SPI_TCR));
> -
>  	return 0;
>  }
>  
> @@ -441,7 +423,7 @@ static int sun4i_spi_of_to_plat(struct udevice *bus)
>  	plat->variant = (struct sun4i_spi_variant *)dev_get_driver_data(bus);
>  	plat->max_hz = fdtdec_get_int(gd->fdt_blob, node,
>  				      "spi-max-frequency",
> -				      SUN4I_SPI_DEFAULT_RATE);
> +				      SUN4I_SPI_MAX_RATE);
>  
>  	if (plat->max_hz > SUN4I_SPI_MAX_RATE)
>  		plat->max_hz = SUN4I_SPI_MAX_RATE;