[PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read
Jean Pihet
jean.pihet at newoldbits.com
Tue Dec 1 20:06:37 CET 2020
Pratyush,
On Mon, Nov 30, 2020 at 2:09 PM Pratyush Yadav <p.yadav at ti.com> wrote:
>
> Hi Jean,
>
> Quick disclaimer: I am not familiar with the IP or the driver so I just
> gave this patch a brief read and did not dig into it a lot. This is in
> no way a comprehensive review.
>
> On 29/11/20 11:39AM, Jean Pihet wrote:
> > TI QSPI: the TI QSPI IP has a limitation of 64MB of MMIO so use
> > the MMIO mode to read below 64MB and fallback to the SW generated
> > sequences to read above 64MB.
> > The TI QSPI IP has another limitation of 4096 words per frame,
> > so split the accesses into smaller ones.
>
> Please split this change...
Ok
>
> > Also fix the TI QSPI operating frequency in order to correctly
> > generate the required SPI CLK frequency.
>
> ... and this one into a separate patch. If they cause any bugs or
> regressions it would be easier to bisect and revert.
>
> >
> > Signed-off-by: Jean Pihet <jean.pihet at newoldbits.com>
> > ---
> > drivers/spi/Kconfig | 9 ++++++
> > drivers/spi/ti_qspi.c | 68 ++++++++++++++++++++++++++++++++++++-------
> > 2 files changed, 66 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index f7a9852565..5d24029ff0 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -395,10 +395,19 @@ config TEGRA210_QSPI
> > config TI_QSPI
> > bool "TI QSPI driver"
> > imply TI_EDMA3
> > + imply TI_QSPI_MQX_FRAME_SIZE
> Typo? ^
Oh yes, this is a leftover.
>
> > help
> > Enable the TI Quad-SPI (QSPI) driver for DRA7xx and AM43xx evms.
> > This driver support spi flash single, quad and memory reads.
> >
> > +config TI_QSPI_MAX_FRAME_SIZE
> > + int "TI QSPI max frame size"
> > + default 4096
> > + help
> > + TI Quad-SPI (QSPI) IP block has a maximum number of 4096 words in a
> > + frame, in non-memory mapped mode. A frame includes the command,
> > + arguments and dummy bytes.
>
> Can the frame size change between IP versions/implementations? If it can
> not then I don't see a need for defining a config here. Just use 4096
> directly in the driver.
I am not sure about that. The TRM has a fixed frame size, are there
other sizes in other chip implementation.
>
> > +
> > config UNIPHIER_SPI
> > bool "Socionext UniPhier SPI driver"
> > depends on ARCH_UNIPHIER
> > diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
> > index 5fdbb49442..57192b9c0a 100644
> > --- a/drivers/spi/ti_qspi.c
> > +++ b/drivers/spi/ti_qspi.c
> > @@ -29,7 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >
> > /* ti qpsi register bit masks */
> > #define QSPI_TIMEOUT 2000000
> > -#define QSPI_FCLK 192000000
> > +#define QSPI_FCLK 48000000
>
> This is fishy. Different platforms can have different FCLK values.
> Looking at ti_qspi_ids[], it looks like am4372 uses 192 MHz and dra7xxx
> uses 76.8 MHz. Changing QSPI_FCLK like this might break am4372.
>
> Do what dra7xx does and add an entry for your platform in the ids table
> and set .data to the FCLK value for your platform.
Ok!
>
> > #define QSPI_DRA7XX_FCLK 76800000
> > #define QSPI_WLEN_MAX_BITS 128
> > #define QSPI_WLEN_MAX_BYTES (QSPI_WLEN_MAX_BITS >> 3)
> > @@ -41,10 +41,11 @@ DECLARE_GLOBAL_DATA_PTR;
> > #define QSPI_EN_CS(n) (n << 28)
> > #define QSPI_WLEN(n) ((n-1) << 19)
> > #define QSPI_3_PIN BIT(18)
> > -#define QSPI_RD_SNGL BIT(16)
> > +#define QSPI_RD_SNGL (1 << 16)
> > +#define QSPI_RD_DUAL (3 << 16)
> > +#define QSPI_RD_QUAD (7 << 16)
> > #define QSPI_WR_SNGL (2 << 16)
> > #define QSPI_INVAL (4 << 16)
> > -#define QSPI_RD_QUAD (7 << 16)
> > /* device control */
> > #define QSPI_CKPHA(n) (1 << (2 + n*8))
> > #define QSPI_CSPOL(n) (1 << (1 + n*8))
> > @@ -155,6 +156,7 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > const void *dout, void *din, unsigned long flags)
> > {
> > struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev);
> > + struct spi_slave *spi_slave = dev_get_parent_priv(dev);
> > struct ti_qspi_priv *priv;
> > struct udevice *bus;
> > uint words = bitlen >> 3; /* fixed 8-bit word length */
> > @@ -182,7 +184,6 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >
> > /* Setup command reg */
> > priv->cmd = 0;
> > - priv->cmd |= QSPI_WLEN(8);
> > priv->cmd |= QSPI_EN_CS(cs);
> > if (priv->mode & SPI_3WIRE)
> > priv->cmd |= QSPI_3_PIN;
> > @@ -206,15 +207,16 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > writel(data, &priv->base->data1);
> > data = cpu_to_be32(*txbuf++);
> > writel(data, &priv->base->data);
> > - cmd &= ~QSPI_WLEN_MASK;
> > cmd |= QSPI_WLEN(QSPI_WLEN_MAX_BITS);
> > xfer_len = QSPI_WLEN_MAX_BYTES;
> > } else {
> > writeb(*txp, &priv->base->data);
> > + cmd |= QSPI_WLEN(8);
> > xfer_len = 1;
> > }
> > debug("tx cmd %08x dc %08x\n",
> > cmd | QSPI_WR_SNGL, priv->dc);
> > + // Only 1-bit write is supported
>
> Do not use C++ style comments. Same for the other such comments below.
>
> > writel(cmd | QSPI_WR_SNGL, &priv->base->cmd);
> > status = readl(&priv->base->status);
> > timeout = QSPI_TIMEOUT;
> > @@ -229,9 +231,17 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > debug("tx done, status %08x\n", status);
> > }
> > if (rxp) {
> > + u32 rx;
> > + /* Check for dual and quad reads */
> > + u32 readcmd_cmd = QSPI_RD_SNGL | QSPI_WLEN(8);
> > + if (spi_slave->flags & SPI_XFER_DUAL_READ)
> > + readcmd_cmd = QSPI_RD_DUAL | QSPI_WLEN(16);
> > + if (spi_slave->flags & SPI_XFER_QUAD_READ)
> > + readcmd_cmd = QSPI_RD_QUAD | QSPI_WLEN(32);
> > +
> > debug("rx cmd %08x dc %08x\n",
> > - ((u32)(priv->cmd | QSPI_RD_SNGL)), priv->dc);
> > - writel(priv->cmd | QSPI_RD_SNGL, &priv->base->cmd);
> > + ((u32)(priv->cmd | readcmd_cmd)), priv->dc);
> > + writel(priv->cmd | readcmd_cmd, &priv->base->cmd);
> > status = readl(&priv->base->status);
> > timeout = QSPI_TIMEOUT;
> > while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
> > @@ -241,10 +251,28 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > }
> > status = readl(&priv->base->status);
> > }
> > - *rxp++ = readl(&priv->base->data);
> > - xfer_len = 1;
> > - debug("rx done, status %08x, read %02x\n",
> > - status, *(rxp-1));
> > + rx = readl(&priv->base->data);
> > + if (spi_slave->flags & SPI_XFER_QUAD_READ) {
> > + if (words >= 1)
> > + *rxp++ = rx >> 24;
> > + if (words >= 2)
> > + *rxp++ = rx >> 16;
> > + if (words >= 3)
> > + *rxp++ = rx >> 8;
> > + if (words >= 4)
> > + *rxp++ = rx;
> > + xfer_len = (words >= 4) ? 4 : words;
> > + } else if (spi_slave->flags & SPI_XFER_DUAL_READ) {
> > + if (words >= 1)
> > + *rxp++ = rx >> 8;
> > + if (words >= 2)
> > + *rxp++ = rx;
> > + xfer_len = (words >= 2) ? 2 : words;
> > + } else {
> > + *rxp++ = rx;
> > + xfer_len = 1;
> > + }
> > + debug("rx done, status %08x, rx=%08x\n", status, rx);
> > }
> > words -= xfer_len;
> > }
> > @@ -353,6 +381,23 @@ static int ti_qspi_exec_mem_op(struct spi_slave *slave,
> > return ret;
> > }
> >
> > +static int ti_qspi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
> > +{
> > +#ifdef CONFIG_TI_QSPI_MAX_FRAME_SIZE
>
> Try to avoid conditional compilation. Using `if (CONFIG_IS_ENABLED(TI_QSPI_MQX_FRAME_SIZE))`
> should work just as fine I think.
Ok. If the frame size is fixed to 4096, there is no need for the conditional.
>
> > + // Adjust the data length to fit in the QSPI max frame length
> > + if (op->data.dir == SPI_MEM_DATA_IN) {
> > + unsigned int data_len = CONFIG_TI_QSPI_MAX_FRAME_SIZE -
> > + sizeof(op->cmd.opcode) - op->addr.nbytes -
> > + op->dummy.nbytes;
> > + // Align the data on cache line
> > + data_len = data_len & ~(ARCH_DMA_MINALIGN - 1);
> > + op->data.nbytes = min(op->data.nbytes, data_len);
> > + }
> > +#endif
> > +
> > + return 0;
> > +}
> > +
> > static int ti_qspi_claim_bus(struct udevice *dev)
> > {
> > struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
> > @@ -482,6 +527,7 @@ static int ti_qspi_ofdata_to_platdata(struct udevice *bus)
> >
> > static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> > .exec_op = ti_qspi_exec_mem_op,
> > + .adjust_op_size = ti_qspi_adjust_size,
> > };
> >
> > static const struct dm_spi_ops ti_qspi_ops = {
> > --
> > 2.26.2
> >
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India
Thanks!
BR,
Jean
More information about the U-Boot
mailing list