| Message ID | 20220609090939.25828-1-qianfanguijin@163.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v1] drivers: spi: sunxi: Fix spi speed settting | expand |
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. 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;
在 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 ... > > 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;
在 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; >
在 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. But I don't have a SUN4I_A10 board, could you please test and check this patch? 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");
On Sat, 2 Jul 2022 11:09:37 +0800 qianfan <qianfanguijin@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@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: 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@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@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; >
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");
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;