On Sat, 2 Jul 2022 16:20:37 +0800 qianfan <[email protected]> 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 > >>> [email protected] wrote: > >>> > >>> Hi Qianfan, > >>> > >>>> From: qianfan Zhao <[email protected]> > >>>> > >>>> 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/[email protected]/ > >>> > >>> > >>> > >>> 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 <[email protected]> > >>>> --- > >>>> 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/[email protected]/ 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 <[email protected]> > 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 <[email protected]> > --- > 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");
