Re: [PATCH v1] drivers: spi: sunxi: Fix spi speed settting

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 2 Jul 2022 16:20:37 +0800
qianfan <qianfanguijin@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@163.com wrote:
> >>>
> >>> Hi Qianfan,
> >>>  
> >>>> From: qianfan Zhao <qianfanguijin@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@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@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@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@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");