[PATCH 2/7] spi: sunxi: refactor SPI speed/mode programming
Jesse Taube
mr.bossman075 at gmail.com
Tue Jun 28 05:43:25 CEST 2022
On 6/27/22 20:31, Andre Przywara wrote:
> On Tue, 3 May 2022 22:20:35 +0100
> Andre Przywara <andre.przywara at arm.com> wrote:
>
> Hi,
>
>> As George rightfully pointed out [1], the spi-sunxi driver programs the
>> speed and mode settings only when the respective functions are called,
>> but this gets lost over a call to release_bus(). That asserts the
>> reset line, thus forces each SPI register back to its default value.
>> Adding to that, trying to program SPI_CCR and SPI_TCR might be pointless
>> in the first place, when the reset line is still asserted (before
>> claim_bus()), so those setting won't apply most of the time. In reality
>> I see two nested claim_bus() calls for the first use, so settings between
>> the two would work (for instance for the initial "sf probe"). However
>> later on the speed setting is not programmed into the hardware anymore.
>
> So this issue was addressed with patches by both George (earlier, in a
> different way) and Qianfan (later, in a very similar way).
>
> Can someone (Jagan?) please have a look at this change from the U-Boot
> SPI perspective? And also the other changes in this series?
> I pushed them to the sunxi/next branch:
> https://source.denx.de/u-boot/custodians/u-boot-sunxi/-/commits/next/
>
> So can people please test this and report whether this now works as
> expected?
I'm very confused I have forgotten much about this patch set.
I'm going to test it, but why has it only been merged now?
Thanks,
Jesse
> Thanks,
> Andre
>
>> So far we get away with that default frequency, because that is a rather
>> tame 24 MHz, which most SPI flash chips can handle just fine.
>>
>> Move the actual register programming into a separate function, and use
>> .set_speed and .set_mode just to set the variables in our priv structure.
>> Then we only call this new function in claim_bus(), when we are sure
>> that register accesses actually work and are preserved.
>>
>> [1] https://lore.kernel.org/u-boot/20210725231636.879913-17-me@yifangu.com/
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> Reported-by: George Hilliard <thirtythreeforty at gmail.com>
>> ---
>> drivers/spi/spi-sunxi.c | 95 ++++++++++++++++++++++-------------------
>> 1 file changed, 52 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
>> index b6cd7ddafad..d6b2dd09514 100644
>> --- a/drivers/spi/spi-sunxi.c
>> +++ b/drivers/spi/spi-sunxi.c
>> @@ -221,6 +221,56 @@ err_ahb:
>> return ret;
>> }
>>
>> +static void sun4i_spi_set_speed_mode(struct udevice *dev)
>> +{
>> + struct sun4i_spi_priv *priv = dev_get_priv(dev);
>> + unsigned int div;
>> + u32 reg;
>> +
>> + /*
>> + * 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 * 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));
>> +
>> + reg = readl(SPI_REG(priv, SPI_TCR));
>> + reg &= ~(SPI_BIT(priv, SPI_TCR_CPOL) | SPI_BIT(priv, SPI_TCR_CPHA));
>> +
>> + 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));
>> +}
>> +
>> static int sun4i_spi_claim_bus(struct udevice *dev)
>> {
>> struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>> @@ -240,6 +290,8 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>> setbits_le32(SPI_REG(priv, SPI_TCR), SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
>> SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW));
>>
>> + sun4i_spi_set_speed_mode(dev->parent);
>> +
>> return 0;
>> }
>>
>> @@ -329,46 +381,14 @@ 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;
>> }
>> @@ -376,19 +396,8 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>> 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;
>> }
>
More information about the U-Boot
mailing list