[PATCH 2/7] spi: sunxi: refactor SPI speed/mode programming
Andre Przywara
andre.przywara at arm.com
Mon Jul 18 12:17:04 CEST 2022
On Mon, 27 Jun 2022 23:43:25 -0400
Jesse Taube <mr.bossman075 at gmail.com> wrote:
Hi Jesse,
> 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?
Because no one reviewed it, or even replied to it - hence my email. And
apparently it was even broken.
The code quality in U-Boot is not really great, as the problems fixed with
this series and the other problems emerged lately ([1]) demonstrate. So I
am very reluctant to take any patches without a meaningful review, or at
least without confirmation of their functionality - regardless of the
author. For instance I just found a bug in my patch 4/7 ...
And I didn't really merge them yet, I just pushed them to the sunxi/next
branch, to expose them more widely and encourage testing - which
apparently worked out as planned ;-)
Cheers,
Andre
[1]
https://lore.kernel.org/u-boot/20220711165215.218e21ae@donnerap.cambridge.arm.com/
> >> 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