[PATCH v1] drivers: spi: sunxi: Fix spi speed settting
Andre Przywara
andre.przywara at arm.com
Mon Jul 18 12:21:28 CEST 2022
On Sat, 2 Jul 2022 16:20:37 +0800
qianfan <qianfanguijin at 163.com> wrote:
Hi Qianfan,
again our mailserver dropped this email, so sorry for the delay!
> 在 2022/7/2 15:50, qianfan 写道:
> >
> >
> > 在 2022/7/2 11:09, qianfan 写道:
> >>
> >>
> >> 在 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:
> >>
> >> 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;
> >>
> >> 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
> >> ...
> > Add udelay(1) before sun4i_spi_drain_fifo in sun4i_spi_xfer can fix it.
> > But I don't know why.
> >>
> >>>
> >>> 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;
> >>
> >
> Hi everyone:
>
> I had fixed "Timeout transferring data" issue and tested on sun8i r40
> platform.
Yes, Icenowy figured that and posted a very similar patch:
https://patchwork.ozlabs.org/project/uboot/patch/20220628064924.390103-1-uwu@icenowy.me/
I will take her patch, before my series, to make sure we don't introduce
non-working commits.
> But I don't have a SUN4I_A10 board, could you please test and check this
> patch?
I didn't test on an A10, but on H6 and A64, where there is the exact same
issue.
It it still very odd why this happens, exactly: the old code seems to
genuinely wait to 1 second, so plenty of time to send anything out.
And if I read the FSR register after the XCH poll returned, I see it being
fine, so the previous check should have matched as well.
Also I can confirm your other observation: introducing some odd delay
*after* the check seems to fix it.
So I would very much like to find the real reason for this, but we should
fix the existing real-world problems first.
If anyone could investigate this further, I would be very grateful.
Thanks,
Andre
> From 514e9396509593515b7fa848cbc4b8eccf948547 Mon Sep 17 00:00:00 2001
> From: qianfan Zhao <qianfanguijin at 163.com>
> Date: Sat, 2 Jul 2022 16:07:18 +0800
> Subject: [PATCH] spi: sunxi: Fix transfer timeout when running at a low
> frequency
>
> sun4i_spi_xfer will report error messages when running at a low
> frequency such as 6MHz, at least on SUN8I R40 platform:
> ERROR: sun4i_spi: Timeout transferring data
>
> Fix the waiting condition.
>
> Signed-off-by: qianfan Zhao <qianfanguijin at 163.com>
> ---
> drivers/spi/spi-sunxi.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index d123adc68a..55b2de8339 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -400,7 +400,7 @@ static int sun4i_spi_xfer(struct udevice *dev,
> unsigned int bitlen,
> struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
>
> u32 len = bitlen / 8;
> - u32 rx_fifocnt;
> + u32 tcr;
> u8 nbytes;
> int ret;
>
> @@ -438,12 +438,10 @@ static int sun4i_spi_xfer(struct udevice *dev,
> unsigned int bitlen,
> setbits_le32(SPI_REG(priv, SPI_TCR),
> SPI_BIT(priv, SPI_TCR_XCH));
>
> - /* Wait till RX FIFO to be empty */
> - ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
> - rx_fifocnt,
> - (((rx_fifocnt &
> - SPI_BIT(priv, SPI_FSR_RF_CNT_MASK)) >>
> - SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
> + /* Wait untill transfer done */
> + ret = readl_poll_timeout(SPI_REG(priv, SPI_TCR),
> + tcr,
> + (!(tcr & SPI_BIT(priv, SPI_TCR_XCH))),
> SUN4I_SPI_TIMEOUT_US);
> if (ret < 0) {
> printf("ERROR: sun4i_spi: Timeout transferring data\n");
More information about the U-Boot
mailing list