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

[qianfan <qianfanguijin@163.com>]

在 2022/6/28 8:34, Andre Przywara 写道:
> 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.
I tested based on those two commits:

spi: sunxi: refactor SPI speed/mode programming
spi: sunxi: improve SPI clock calculation

And there are a couple of questions:

1. sun4i_spi_of_to_plat try reading "spi-max-frequency" from the spi bus 
node:

static int sun4i_spi_of_to_plat(struct udevice *bus)
{
     struct sun4i_spi_plat *plat = dev_get_plat(bus);
     int node = dev_of_offset(bus);

     plat->base = dev_read_addr(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);

     if (plat->max_hz > SUN4I_SPI_MAX_RATE)
         plat->max_hz = SUN4I_SPI_MAX_RATE;

     return 0;
}

Seems this is not a correct way. "spi-max-frequency" should reading from 
spi device,
not spi bus. On my dts, no "spi-max-frequency" prop on spi bus node, 
this will make
plat->max_hz has default SUN4I_SPI_DEFAULT_RATE(1M) value.

&spi2 {
     pinctrl-names = "default";
     pinctrl-0 = <&spi2_cs0_pb_pin &spi2_pb_pins>;
     status = "okay";

     lcd@0 {
         compatible = "sitronix,st75161";
         spi-max-frequency = <12000000>;
         reg = <0>;
         spi-cpol;
         spi-cpha;

So on my patch, I had changed the default plat->max_hz to 
SUN4I_SPI_MAX_RATE.

2. When I changed the default plat->max_hz to SUN4I_SPI_MAX_RATE:

2.1: sun4i_spi_set_speed_mode doesn't consider when div = 1(freq = 
SUNXI_INPUT_CLOCK),
the spi running in 12M even if the spi-max-frequency is setted to 24M.

2.2: on my R40 based board, spi can't work when the spi clock <= 6M.
I had check the CCR register, the value is correct, from logic analyzer
only the first byte is sent. Next is the serial console logs:

spi clock = 6M:
CCR: 00001001
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
...

spi clock = 4M:
CCR: 00001002
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
ERROR: sun4i_spi: Timeout transferring data
...

>
> 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;