| Message ID | 20231102082303.354944-2-mchitale@ventanamicro.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | drivers: xilinx_spi: Fixes for MMC_SPI | expand |
On 11/2/23 09:23, Mayuresh Chitale wrote: > Add the xfer callback which is used by the MMC_SPI driver and generally by > the dm_spi_xfer callback. Also probe the fifo_depth during init as is > done in the linux spi-xilinx driver and fix a compiler warning while at it. They are independent things that's why please separate them by that topics. Also update subject to describe issue there. > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > --- > drivers/spi/xilinx_spi.c | 70 ++++++++++++++++++++++++++++++---------- > 1 file changed, 53 insertions(+), 17 deletions(-) > > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c > index b58a3f632a..0d4ff25600 100644 > --- a/drivers/spi/xilinx_spi.c > +++ b/drivers/spi/xilinx_spi.c > @@ -67,7 +67,7 @@ > /* SPI Slave Select Register (spissr), [1] p13, [2] p13 */ > #define SPISSR_MASK(cs) (1 << (cs)) > #define SPISSR_ACT(cs) ~SPISSR_MASK(cs) > -#define SPISSR_OFF ~0UL > +#define SPISSR_OFF (~0U) > > /* SPI Software Reset Register (ssr) */ > #define SPISSR_RESET_VALUE 0x0a > @@ -109,6 +109,27 @@ struct xilinx_spi_priv { > u8 startup; > }; > > +/* Detect fifo depth. Ported from linux sha: 4c9a761402d78 */ don't add linux sha1 here. Add it to commit message or I normally just provide a link to that patch. https://lore.kernel.org/r/1422029330-10971-5-git-send-email-ricardo.ribalda@gmail.com > +static int xilinx_spi_find_buffer_size(struct xilinx_spi_regs *regs) > +{ > + u8 sr; > + int n_words = 0; > + > + /* > + * Before the buffer_size detection we reset the core > + * to make sure we start with a clean state. > + */ > + writel(SPISSR_RESET_VALUE, ®s->srr); > + > + /* Fill the Tx FIFO with as many words as possible */ > + do { > + writel(0, ®s->spidtr); > + sr = readl(®s->spisr); > + n_words++; > + } while (!(sr & SPISR_TX_FULL)); > + > + return n_words; > +} newline here. > static int xilinx_spi_probe(struct udevice *bus) > { > struct xilinx_spi_priv *priv = dev_get_priv(bus); > @@ -116,6 +137,8 @@ static int xilinx_spi_probe(struct udevice *bus) > > regs = priv->regs = dev_read_addr_ptr(bus); > priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0); > + if (!priv->fifo_depth) > + priv->fifo_depth = xilinx_spi_find_buffer_size(regs); > > writel(SPISSR_RESET_VALUE, ®s->srr); > > @@ -197,7 +220,7 @@ static u32 xilinx_spi_fill_txfifo(struct udevice *bus, const u8 *txp, > return i; > } > > -static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, u32 rxbytes) > +static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, int rxbytes) Unrelated. Or do we have a case where we ask for reading negative number of bytes? > { > struct xilinx_spi_priv *priv = dev_get_priv(bus); > struct xilinx_spi_regs *regs = priv->regs; > @@ -217,9 +240,9 @@ static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, u32 rxbytes) > return i; > } > > -static int start_transfer(struct spi_slave *spi, const void *dout, void *din, u32 len) > +static int start_transfer(struct udevice *dev, const void *dout, void *din, u32 len) All these spi -> dev changes should be in separate patch. I expect they are preparation for xfer but still better to do it separately. > { > - struct udevice *bus = spi->dev->parent; > + struct udevice *bus = dev->parent; > struct xilinx_spi_priv *priv = dev_get_priv(bus); > struct xilinx_spi_regs *regs = priv->regs; > u32 count, txbytes, rxbytes; > @@ -259,10 +282,9 @@ static int start_transfer(struct spi_slave *spi, const void *dout, void *din, u3 > return 0; > } > > -static void xilinx_spi_startup_block(struct spi_slave *spi) > +static void xilinx_spi_startup_block(struct udevice *dev) > { > - struct dm_spi_slave_plat *slave_plat = > - dev_get_parent_plat(spi->dev); > + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev); > unsigned char txp; > unsigned char rxp[8]; > > @@ -270,13 +292,25 @@ static void xilinx_spi_startup_block(struct spi_slave *spi) > * Perform a dummy read as a work around for > * the startup block issue. > */ > - spi_cs_activate(spi->dev, slave_plat->cs); > + spi_cs_activate(dev, slave_plat->cs); > txp = 0x9f; > - start_transfer(spi, (void *)&txp, NULL, 1); > + start_transfer(dev, (void *)&txp, NULL, 1); > > - start_transfer(spi, NULL, (void *)rxp, 6); > + start_transfer(dev, NULL, (void *)rxp, 6); > > - spi_cs_deactivate(spi->dev); > + spi_cs_deactivate(dev); > +} > + > +static int xilinx_spi_xfer(struct udevice *dev, unsigned int bitlen, > + const void *dout, void *din, unsigned long flags) > +{ > + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev); > + int ret; > + > + spi_cs_activate(dev, slave_plat->cs); > + ret = start_transfer(dev, dout, din, (bitlen / 8)); no need for () around > + spi_cs_deactivate(dev); > + return ret; > } > > static int xilinx_spi_mem_exec_op(struct spi_slave *spi, > @@ -294,14 +328,15 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, > * as QSPI provides command. So first command fails. > */ > if (!startup) { > - xilinx_spi_startup_block(spi); > + xilinx_spi_startup_block(spi->dev); > startup++; > } > > spi_cs_activate(spi->dev, slave_plat->cs); > > if (op->cmd.opcode) { > - ret = start_transfer(spi, (void *)&op->cmd.opcode, NULL, 1); > + ret = start_transfer(spi->dev, (void *)&op->cmd.opcode, > + NULL, 1); > if (ret) > goto done; > } > @@ -313,7 +348,7 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, > addr_buf[i] = op->addr.val >> > (8 * (op->addr.nbytes - i - 1)); > > - ret = start_transfer(spi, (void *)addr_buf, NULL, > + ret = start_transfer(spi->dev, (void *)addr_buf, NULL, > op->addr.nbytes); > if (ret) > goto done; > @@ -322,16 +357,16 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, > dummy_len = (op->dummy.nbytes * op->data.buswidth) / > op->dummy.buswidth; > > - ret = start_transfer(spi, NULL, NULL, dummy_len); > + ret = start_transfer(spi->dev, NULL, NULL, dummy_len); > if (ret) > goto done; > } > if (op->data.nbytes) { > if (op->data.dir == SPI_MEM_DATA_IN) { > - ret = start_transfer(spi, NULL, > + ret = start_transfer(spi->dev, NULL, > op->data.buf.in, op->data.nbytes); > } else { > - ret = start_transfer(spi, op->data.buf.out, > + ret = start_transfer(spi->dev, op->data.buf.out, > NULL, op->data.nbytes); > } > if (ret) > @@ -427,6 +462,7 @@ static const struct spi_controller_mem_ops xilinx_spi_mem_ops = { > static const struct dm_spi_ops xilinx_spi_ops = { > .claim_bus = xilinx_spi_claim_bus, > .release_bus = xilinx_spi_release_bus, > + .xfer = xilinx_spi_xfer, > .set_speed = xilinx_spi_set_speed, > .set_mode = xilinx_spi_set_mode, > .mem_ops = &xilinx_spi_mem_ops, M
Hi Michal, On Thu, 2023-11-02 at 09:37 +0100, Michal Simek wrote: > On 11/2/23 09:23, Mayuresh Chitale wrote: > > Add the xfer callback which is used by the MMC_SPI driver and > > generally by > > the dm_spi_xfer callback. Also probe the fifo_depth during init as > > is > > done in the linux spi-xilinx driver and fix a compiler warning > > while at it. > > They are independent things that's why please separate them by that > topics. > > Also update subject to describe issue there. Ok. > > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > > --- > > drivers/spi/xilinx_spi.c | 70 ++++++++++++++++++++++++++++++----- > > ----- > > 1 file changed, 53 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c > > index b58a3f632a..0d4ff25600 100644 > > --- a/drivers/spi/xilinx_spi.c > > +++ b/drivers/spi/xilinx_spi.c > > @@ -67,7 +67,7 @@ > > /* SPI Slave Select Register (spissr), [1] p13, [2] p13 */ > > #define SPISSR_MASK(cs) (1 << (cs)) > > #define SPISSR_ACT(cs) ~SPISSR_MASK(cs) > > -#define SPISSR_OFF ~0UL > > +#define SPISSR_OFF (~0U) > > > > /* SPI Software Reset Register (ssr) */ > > #define SPISSR_RESET_VALUE 0x0a > > @@ -109,6 +109,27 @@ struct xilinx_spi_priv { > > u8 startup; > > }; > > > > +/* Detect fifo depth. Ported from linux sha: 4c9a761402d78 */ > > don't add linux sha1 here. > Add it to commit message or I normally just provide a link to that > patch. > https://lore.kernel.org/r/1422029330-10971-5-git-send-email-ricardo.ribalda@gmail.com Ok. > > > > > +static int xilinx_spi_find_buffer_size(struct xilinx_spi_regs > > *regs) > > +{ > > + u8 sr; > > + int n_words = 0; > > + > > + /* > > + * Before the buffer_size detection we reset the core > > + * to make sure we start with a clean state. > > + */ > > + writel(SPISSR_RESET_VALUE, ®s->srr); > > + > > + /* Fill the Tx FIFO with as many words as possible */ > > + do { > > + writel(0, ®s->spidtr); > > + sr = readl(®s->spisr); > > + n_words++; > > + } while (!(sr & SPISR_TX_FULL)); > > + > > + return n_words; > > +} > > newline here. Ok. > > > static int xilinx_spi_probe(struct udevice *bus) > > { > > struct xilinx_spi_priv *priv = dev_get_priv(bus); > > @@ -116,6 +137,8 @@ static int xilinx_spi_probe(struct udevice > > *bus) > > > > regs = priv->regs = dev_read_addr_ptr(bus); > > priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0); > > + if (!priv->fifo_depth) > > + priv->fifo_depth = xilinx_spi_find_buffer_size(regs); > > > > writel(SPISSR_RESET_VALUE, ®s->srr); > > > > @@ -197,7 +220,7 @@ static u32 xilinx_spi_fill_txfifo(struct > > udevice *bus, const u8 *txp, > > return i; > > } > > > > -static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, > > u32 rxbytes) > > +static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, > > int rxbytes) > > Unrelated. Or do we have a case where we ask for reading negative > number of bytes? I will revert the change. > > > { > > struct xilinx_spi_priv *priv = dev_get_priv(bus); > > struct xilinx_spi_regs *regs = priv->regs; > > @@ -217,9 +240,9 @@ static u32 xilinx_spi_read_rxfifo(struct > > udevice *bus, u8 *rxp, u32 rxbytes) > > return i; > > } > > > > -static int start_transfer(struct spi_slave *spi, const void *dout, > > void *din, u32 len) > > +static int start_transfer(struct udevice *dev, const void *dout, > > void *din, u32 len) > > All these spi -> dev changes should be in separate patch. I expect > they are > preparation for xfer but still better to do it separately. Ok. > > > { > > - struct udevice *bus = spi->dev->parent; > > + struct udevice *bus = dev->parent; > > struct xilinx_spi_priv *priv = dev_get_priv(bus); > > struct xilinx_spi_regs *regs = priv->regs; > > u32 count, txbytes, rxbytes; > > @@ -259,10 +282,9 @@ static int start_transfer(struct spi_slave > > *spi, const void *dout, void *din, u3 > > return 0; > > } > > > > -static void xilinx_spi_startup_block(struct spi_slave *spi) > > +static void xilinx_spi_startup_block(struct udevice *dev) > > { > > - struct dm_spi_slave_plat *slave_plat = > > - dev_get_parent_plat(spi->dev); > > + struct dm_spi_slave_plat *slave_plat = > > dev_get_parent_plat(dev); > > unsigned char txp; > > unsigned char rxp[8]; > > > > @@ -270,13 +292,25 @@ static void xilinx_spi_startup_block(struct > > spi_slave *spi) > > * Perform a dummy read as a work around for > > * the startup block issue. > > */ > > - spi_cs_activate(spi->dev, slave_plat->cs); > > + spi_cs_activate(dev, slave_plat->cs); > > txp = 0x9f; > > - start_transfer(spi, (void *)&txp, NULL, 1); > > + start_transfer(dev, (void *)&txp, NULL, 1); > > > > - start_transfer(spi, NULL, (void *)rxp, 6); > > + start_transfer(dev, NULL, (void *)rxp, 6); > > > > - spi_cs_deactivate(spi->dev); > > + spi_cs_deactivate(dev); > > +} > > + > > +static int xilinx_spi_xfer(struct udevice *dev, unsigned int > > bitlen, > > + const void *dout, void *din, unsigned long > > flags) > > +{ > > + struct dm_spi_slave_plat *slave_plat = > > dev_get_parent_plat(dev); > > + int ret; > > + > > + spi_cs_activate(dev, slave_plat->cs); > > + ret = start_transfer(dev, dout, din, (bitlen / 8)); > > no need for () around Ok. > > > + spi_cs_deactivate(dev); > > + return ret; > > } > > > > static int xilinx_spi_mem_exec_op(struct spi_slave *spi, > > @@ -294,14 +328,15 @@ static int xilinx_spi_mem_exec_op(struct > > spi_slave *spi, > > * as QSPI provides command. So first command fails. > > */ > > if (!startup) { > > - xilinx_spi_startup_block(spi); > > + xilinx_spi_startup_block(spi->dev); > > startup++; > > } > > > > spi_cs_activate(spi->dev, slave_plat->cs); > > > > if (op->cmd.opcode) { > > - ret = start_transfer(spi, (void *)&op->cmd.opcode, > > NULL, 1); > > + ret = start_transfer(spi->dev, (void *)&op->cmd.opcode, > > + NULL, 1); > > if (ret) > > goto done; > > } > > @@ -313,7 +348,7 @@ static int xilinx_spi_mem_exec_op(struct > > spi_slave *spi, > > addr_buf[i] = op->addr.val >> > > (8 * (op->addr.nbytes - i - 1)); > > > > - ret = start_transfer(spi, (void *)addr_buf, NULL, > > + ret = start_transfer(spi->dev, (void *)addr_buf, NULL, > > op->addr.nbytes); > > if (ret) > > goto done; > > @@ -322,16 +357,16 @@ static int xilinx_spi_mem_exec_op(struct > > spi_slave *spi, > > dummy_len = (op->dummy.nbytes * op->data.buswidth) / > > op->dummy.buswidth; > > > > - ret = start_transfer(spi, NULL, NULL, dummy_len); > > + ret = start_transfer(spi->dev, NULL, NULL, dummy_len); > > if (ret) > > goto done; > > } > > if (op->data.nbytes) { > > if (op->data.dir == SPI_MEM_DATA_IN) { > > - ret = start_transfer(spi, NULL, > > + ret = start_transfer(spi->dev, NULL, > > op->data.buf.in, op- > > >data.nbytes); > > } else { > > - ret = start_transfer(spi, op->data.buf.out, > > + ret = start_transfer(spi->dev, op- > > >data.buf.out, > > NULL, op->data.nbytes); > > } > > if (ret) > > @@ -427,6 +462,7 @@ static const struct spi_controller_mem_ops > > xilinx_spi_mem_ops = { > > static const struct dm_spi_ops xilinx_spi_ops = { > > .claim_bus = xilinx_spi_claim_bus, > > .release_bus = xilinx_spi_release_bus, > > + .xfer = xilinx_spi_xfer, > > .set_speed = xilinx_spi_set_speed, > > .set_mode = xilinx_spi_set_mode, > > .mem_ops = &xilinx_spi_mem_ops, > > M
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c index b58a3f632a..0d4ff25600 100644 --- a/drivers/spi/xilinx_spi.c +++ b/drivers/spi/xilinx_spi.c @@ -67,7 +67,7 @@ /* SPI Slave Select Register (spissr), [1] p13, [2] p13 */ #define SPISSR_MASK(cs) (1 << (cs)) #define SPISSR_ACT(cs) ~SPISSR_MASK(cs) -#define SPISSR_OFF ~0UL +#define SPISSR_OFF (~0U) /* SPI Software Reset Register (ssr) */ #define SPISSR_RESET_VALUE 0x0a @@ -109,6 +109,27 @@ struct xilinx_spi_priv { u8 startup; }; +/* Detect fifo depth. Ported from linux sha: 4c9a761402d78 */ +static int xilinx_spi_find_buffer_size(struct xilinx_spi_regs *regs) +{ + u8 sr; + int n_words = 0; + + /* + * Before the buffer_size detection we reset the core + * to make sure we start with a clean state. + */ + writel(SPISSR_RESET_VALUE, ®s->srr); + + /* Fill the Tx FIFO with as many words as possible */ + do { + writel(0, ®s->spidtr); + sr = readl(®s->spisr); + n_words++; + } while (!(sr & SPISR_TX_FULL)); + + return n_words; +} static int xilinx_spi_probe(struct udevice *bus) { struct xilinx_spi_priv *priv = dev_get_priv(bus); @@ -116,6 +137,8 @@ static int xilinx_spi_probe(struct udevice *bus) regs = priv->regs = dev_read_addr_ptr(bus); priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0); + if (!priv->fifo_depth) + priv->fifo_depth = xilinx_spi_find_buffer_size(regs); writel(SPISSR_RESET_VALUE, ®s->srr); @@ -197,7 +220,7 @@ static u32 xilinx_spi_fill_txfifo(struct udevice *bus, const u8 *txp, return i; } -static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, u32 rxbytes) +static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, int rxbytes) { struct xilinx_spi_priv *priv = dev_get_priv(bus); struct xilinx_spi_regs *regs = priv->regs; @@ -217,9 +240,9 @@ static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp, u32 rxbytes) return i; } -static int start_transfer(struct spi_slave *spi, const void *dout, void *din, u32 len) +static int start_transfer(struct udevice *dev, const void *dout, void *din, u32 len) { - struct udevice *bus = spi->dev->parent; + struct udevice *bus = dev->parent; struct xilinx_spi_priv *priv = dev_get_priv(bus); struct xilinx_spi_regs *regs = priv->regs; u32 count, txbytes, rxbytes; @@ -259,10 +282,9 @@ static int start_transfer(struct spi_slave *spi, const void *dout, void *din, u3 return 0; } -static void xilinx_spi_startup_block(struct spi_slave *spi) +static void xilinx_spi_startup_block(struct udevice *dev) { - struct dm_spi_slave_plat *slave_plat = - dev_get_parent_plat(spi->dev); + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev); unsigned char txp; unsigned char rxp[8]; @@ -270,13 +292,25 @@ static void xilinx_spi_startup_block(struct spi_slave *spi) * Perform a dummy read as a work around for * the startup block issue. */ - spi_cs_activate(spi->dev, slave_plat->cs); + spi_cs_activate(dev, slave_plat->cs); txp = 0x9f; - start_transfer(spi, (void *)&txp, NULL, 1); + start_transfer(dev, (void *)&txp, NULL, 1); - start_transfer(spi, NULL, (void *)rxp, 6); + start_transfer(dev, NULL, (void *)rxp, 6); - spi_cs_deactivate(spi->dev); + spi_cs_deactivate(dev); +} + +static int xilinx_spi_xfer(struct udevice *dev, unsigned int bitlen, + const void *dout, void *din, unsigned long flags) +{ + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev); + int ret; + + spi_cs_activate(dev, slave_plat->cs); + ret = start_transfer(dev, dout, din, (bitlen / 8)); + spi_cs_deactivate(dev); + return ret; } static int xilinx_spi_mem_exec_op(struct spi_slave *spi, @@ -294,14 +328,15 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, * as QSPI provides command. So first command fails. */ if (!startup) { - xilinx_spi_startup_block(spi); + xilinx_spi_startup_block(spi->dev); startup++; } spi_cs_activate(spi->dev, slave_plat->cs); if (op->cmd.opcode) { - ret = start_transfer(spi, (void *)&op->cmd.opcode, NULL, 1); + ret = start_transfer(spi->dev, (void *)&op->cmd.opcode, + NULL, 1); if (ret) goto done; } @@ -313,7 +348,7 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, addr_buf[i] = op->addr.val >> (8 * (op->addr.nbytes - i - 1)); - ret = start_transfer(spi, (void *)addr_buf, NULL, + ret = start_transfer(spi->dev, (void *)addr_buf, NULL, op->addr.nbytes); if (ret) goto done; @@ -322,16 +357,16 @@ static int xilinx_spi_mem_exec_op(struct spi_slave *spi, dummy_len = (op->dummy.nbytes * op->data.buswidth) / op->dummy.buswidth; - ret = start_transfer(spi, NULL, NULL, dummy_len); + ret = start_transfer(spi->dev, NULL, NULL, dummy_len); if (ret) goto done; } if (op->data.nbytes) { if (op->data.dir == SPI_MEM_DATA_IN) { - ret = start_transfer(spi, NULL, + ret = start_transfer(spi->dev, NULL, op->data.buf.in, op->data.nbytes); } else { - ret = start_transfer(spi, op->data.buf.out, + ret = start_transfer(spi->dev, op->data.buf.out, NULL, op->data.nbytes); } if (ret) @@ -427,6 +462,7 @@ static const struct spi_controller_mem_ops xilinx_spi_mem_ops = { static const struct dm_spi_ops xilinx_spi_ops = { .claim_bus = xilinx_spi_claim_bus, .release_bus = xilinx_spi_release_bus, + .xfer = xilinx_spi_xfer, .set_speed = xilinx_spi_set_speed, .set_mode = xilinx_spi_set_mode, .mem_ops = &xilinx_spi_mem_ops,
Add the xfer callback which is used by the MMC_SPI driver and generally by the dm_spi_xfer callback. Also probe the fifo_depth during init as is done in the linux spi-xilinx driver and fix a compiler warning while at it. Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> --- drivers/spi/xilinx_spi.c | 70 ++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 17 deletions(-)