On Mon, 27 Jun 2022 23:43:25 -0400 Jesse Taube <mr.bossman...@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.przyw...@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.218e2...@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...@yifangu.com/ > >> > >> Signed-off-by: Andre Przywara <andre.przyw...@arm.com> > >> Reported-by: George Hilliard <thirtythreefo...@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; > >> } > > >
