[PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read
Pratyush Yadav
p.yadav at ti.com
Mon Nov 30 14:09:00 CET 2020
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...
> 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? ^
> 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.
> +
> 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.
> #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.
> + // 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
More information about the U-Boot
mailing list