[PATCH v1] drivers: spi: sunxi: Fix spi speed settting
Andre Przywara
andre.przywara at arm.com
Mon Jul 18 12:20:59 CEST 2022
On Sat, 2 Jul 2022 11:09:37 +0800
qianfan <qianfanguijin at 163.com> wrote:
Hi Qianfan,
I am sorry for the late reply, our mailserver apparently always filters
your email, so I just found your answer by browsing through patchwork.
Anyway, many thanks for answering, and your testing and observations are all
very good! (and sadly true ;-)
Answers inline below:
> 在 2022/6/28 8:34, Andre Przywara 写道:
> > On Thu, 9 Jun 2022 17:09:39 +0800
> > qianfanguijin at 163.com wrote:
> >
> > Hi Qianfan,
> >
> >> From: qianfan Zhao <qianfanguijin at 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:
Yes, indeed, that's broken. I found this myself the other day, when trying
to debug the other issue.
I asked about this here:
https://lore.kernel.org/u-boot/20220711165215.218e21ae@donnerap.cambridge.arm.com/
> 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 at 0 {
> compatible = "sitronix,st75161";
> spi-max-frequency = <12000000>;
> reg = <0>;
> spi-cpol;
> spi-cpha;
> fdtdec_get_int
> So on my patch, I had changed the default plat->max_hz to
> SUN4I_SPI_MAX_RATE.
Indeed, I did something very similar: remove that fdtdec_get_int() call
above and use the the max clock rate.
I will send a patch to that effect later, unless you beat me to it.
> 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
Yes, I have seen this as well: With the now 1 MHz clock, for some odd
reasons there is only one byte sent out. I don't have fancy measurement
tools, but this is what the FIFO register told me.
As you figured yourself (in the other mail), checking the XCH bit fixes
this problem, but it is unclear why, since the trigger sequence is the
same. I replied there.
Cheers,
Andre
> >
> > Thanks!
> > Andre
> >
> >> Fix it.
> >>
> >> Signed-off-by: qianfan Zhao <qianfanguijin at 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;
>
More information about the U-Boot
mailing list