| Message ID | 20200211060425.1619471-8-seanga2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Add Sipeed Maix support | expand |
Hi Sean > The dw spi devices on the Kendryte K210 must be operated in a specific > fasion which cannot be achived through multiple writes to via dw_spi_xfer > (as it is currently written). This patch adds an implementation of exec_op, > which gives correct behaviour when reading/writing spi flash. > > I would like to be able to modify the existing dw_spi_xfer function such > that it works properly (e.g. with the mmc_spi driver). However, the only > example code I have to work off is Kendryte's sdk (which is written in the > exec_op style), and I do not have access to the datasheet (if anyone does, > I would love to have a look!). > > Signed-off-by: Sean Anderson <seanga2 at gmail.com> > --- > > Changes in v4: > - New > > drivers/spi/designware_spi.c | 123 +++++++++++++++++++++++++++++++++-- > 1 file changed, 119 insertions(+), 4 deletions(-) > Patch 6 and 7 shall be applied via spi tree. You mix them together, the patchwork will become complicated. It will be better for me to wait for a period of time to get the approval(Reviewed-by or Acked-by) of SPI MAINTAINER. Thanks Rick > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c > index 04cc873754..277eb19a0b 100644 > --- a/drivers/spi/designware_spi.c > +++ b/drivers/spi/designware_spi.c > @@ -17,6 +17,7 @@ > #include <errno.h> > #include <malloc.h> > #include <spi.h> > +#include <spi-mem.h> > #include <fdtdec.h> > #include <reset.h> > #include <linux/compat.h> > @@ -107,8 +108,8 @@ struct dw_spi_priv { > int len; > > u32 fifo_len; /* depth of the FIFO buffer */ > - void *tx; > - void *tx_end; > + const void *tx; > + const void *tx_end; > void *rx; > void *rx_end; > > @@ -344,7 +345,7 @@ static void dw_writer(struct dw_spi_priv *priv) > txw = *(u16 *)(priv->tx); > } > dw_write(priv, DW_SPI_DR, txw); > - debug("%s: tx=0x%02x\n", __func__, txw); > + log_io("tx=0x%02x\n", txw); > priv->tx += priv->bits_per_word >> 3; > } > } > @@ -356,7 +357,7 @@ static void dw_reader(struct dw_spi_priv *priv) > > while (max--) { > rxw = dw_read(priv, DW_SPI_DR); > - debug("%s: rx=0x%02x\n", __func__, rxw); > + log_io("rx=0x%02x\n", rxw); > > /* Care about rx if the transfer's original "rx" is not null */ > if (priv->rx_end - priv->len) { > @@ -483,6 +484,115 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, > return ret; > } > > +static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) > +{ > + bool read = op->data.dir == SPI_MEM_DATA_IN; > + int pos, i, ret = 0; > + struct udevice *bus = slave->dev->parent; > + struct dw_spi_platdata *plat = dev_get_platdata(bus); > + struct dw_spi_priv *priv = dev_get_priv(bus); > + u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes; > + u8 op_buf[op_len]; > + u32 cr0; > + > + if (read) > + priv->tmode = SPI_TMOD_EPROMREAD; > + else > + priv->tmode = SPI_TMOD_TO; > + > + debug("%s: buf=%p len=%u [bytes]\n", > + __func__, op->data.buf.in, op->data.nbytes); > + > + cr0 = GEN_CTRL0(priv, plat); > + debug("%s: cr0=%08x\n", __func__, cr0); > + > + spi_enable_chip(priv, 0); > + dw_write(priv, DW_SPI_CTRL0, cr0); > + if (read) > + dw_write(priv, DW_SPI_CTRL1, op->data.nbytes - 1); > + spi_enable_chip(priv, 1); > + > + /* From spi_mem_exec_op */ > + pos = 0; > + op_buf[pos++] = op->cmd.opcode; > + if (op->addr.nbytes) { > + for (i = 0; i < op->addr.nbytes; i++) > + op_buf[pos + i] = op->addr.val >> > + (8 * (op->addr.nbytes - i - 1)); > + > + pos += op->addr.nbytes; > + } > + if (op->dummy.nbytes) > + memset(op_buf + pos, 0xff, op->dummy.nbytes); > + > + priv->tx = &op_buf; > + priv->tx_end = priv->tx + op_len; > + while (priv->tx != priv->tx_end) > + dw_writer(priv); > + > + /* > + * XXX: The following are tight loops! Enabling debug messages may cause > + * them to fail because we are not reading/writing the fifo fast enough. > + * > + * We heuristically break out of the loop when we stop getting data. > + * This is to stop us from hanging if the device doesn't send any data > + * (either at all, or after sending a response). For example, one flash > + * chip I tested did not send anything back after the first 64K of data. > + */ > + if (read) { > + /* If we have gotten any data back yet */ > + bool got_data = false; > + /* How many times we have looped without reading anything */ > + int loops_since_read = 0; > + struct spi_mem_op *mut_op = (struct spi_mem_op *)op; > + > + priv->rx = op->data.buf.in; > + priv->rx_end = priv->rx + op->data.nbytes; > + > + dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev)); > + while (priv->rx != priv->rx_end) { > + void *last_rx = priv->rx; > + > + dw_reader(priv); > + if (priv->rx == last_rx) { > + loops_since_read++; > + /* Thresholds are arbitrary */ > + if (loops_since_read > 256) > + break; > + else if (got_data && loops_since_read > 32) > + break; > + } else { > + got_data = true; > + loops_since_read = 0; > + } > + } > + > + /* Update with the actual amount of data read */ > + mut_op->data.nbytes -= priv->rx_end - priv->rx; > + } else { > + u32 val; > + > + priv->tx = op->data.buf.out; > + priv->tx_end = priv->tx + op->data.nbytes; > + > + /* Fill up the write fifo before starting the transfer */ > + dw_writer(priv); > + dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev)); > + while (priv->tx != priv->tx_end) > + dw_writer(priv); > + > + if (readl_poll_timeout(priv->regs + DW_SPI_SR, val, > + (val & SR_TF_EMPT) && !(val & SR_BUSY), > + RX_TIMEOUT * 1000)) { > + ret = -ETIMEDOUT; > + } > + } > + > + dw_write(priv, DW_SPI_SER, 0); > + debug("%s: %u bytes xfered\n", __func__, op->data.nbytes); > + return ret; > +} > + > static int dw_spi_set_speed(struct udevice *bus, uint speed) > { > struct dw_spi_platdata *plat = dev_get_platdata(bus); > @@ -546,8 +656,13 @@ static int dw_spi_remove(struct udevice *bus) > return 0; > } > > +static const struct spi_controller_mem_ops dw_spi_mem_ops = { > + .exec_op = dw_spi_exec_op, > +}; > + > static const struct dm_spi_ops dw_spi_ops = { > .xfer = dw_spi_xfer, > + .mem_ops = &dw_spi_mem_ops, > .set_speed = dw_spi_set_speed, > .set_mode = dw_spi_set_mode, > /* > -- > 2.25.0 >
On 2/18/20 3:34 AM, Rick Chen wrote: > Hi Sean > >> The dw spi devices on the Kendryte K210 must be operated in a specific >> fasion which cannot be achived through multiple writes to via dw_spi_xfer >> (as it is currently written). This patch adds an implementation of exec_op, >> which gives correct behaviour when reading/writing spi flash. >> >> I would like to be able to modify the existing dw_spi_xfer function such >> that it works properly (e.g. with the mmc_spi driver). However, the only >> example code I have to work off is Kendryte's sdk (which is written in the >> exec_op style), and I do not have access to the datasheet (if anyone does, >> I would love to have a look!). >> >> Signed-off-by: Sean Anderson <seanga2 at gmail.com> >> --- >> >> Changes in v4: >> - New >> >> drivers/spi/designware_spi.c | 123 +++++++++++++++++++++++++++++++++-- >> 1 file changed, 119 insertions(+), 4 deletions(-) >> > > Patch 6 and 7 shall be applied via spi tree. > You mix them together, the patchwork will become complicated. > It will be better for me to wait for a period of time to get the > approval(Reviewed-by or Acked-by) of SPI MAINTAINER. > > Thanks > Rick So is this just waiting for approval, or is there a change I should make on my end? --Sean
Hi Sean > >> The dw spi devices on the Kendryte K210 must be operated in a specific > >> fasion which cannot be achived through multiple writes to via dw_spi_xfer > >> (as it is currently written). This patch adds an implementation of exec_op, > >> which gives correct behaviour when reading/writing spi flash. > >> > >> I would like to be able to modify the existing dw_spi_xfer function such > >> that it works properly (e.g. with the mmc_spi driver). However, the only > >> example code I have to work off is Kendryte's sdk (which is written in the > >> exec_op style), and I do not have access to the datasheet (if anyone does, > >> I would love to have a look!). > >> > >> Signed-off-by: Sean Anderson <seanga2 at gmail.com> > >> --- > >> > >> Changes in v4: > >> - New > >> > >> drivers/spi/designware_spi.c | 123 +++++++++++++++++++++++++++++++++-- > >> 1 file changed, 119 insertions(+), 4 deletions(-) > >> > > > > Patch 6 and 7 shall be applied via spi tree. > > You mix them together, the patchwork will become complicated. > > It will be better for me to wait for a period of time to get the > > approval(Reviewed-by or Acked-by) of SPI MAINTAINER. > > > > Thanks > > Rick > > So is this just waiting for approval, or is there a change I should make > on my end? If they are not highly dependent, you shall separate and send them individually. But we can wait for few days and see, if it still no response. You can consider separate them. Thanks Rick > > --Sean
On 2/18/20 8:39 PM, Rick Chen wrote: >>> >>> Patch 6 and 7 shall be applied via spi tree. >>> You mix them together, the patchwork will become complicated. >>> It will be better for me to wait for a period of time to get the >>> approval(Reviewed-by or Acked-by) of SPI MAINTAINER. >>> >>> Thanks >>> Rick >> >> So is this just waiting for approval, or is there a change I should make >> on my end? > > If they are not highly dependent, you shall separate and send them individually. > But we can wait for few days and see, if it still no response. > You can consider separate them. These patches add support for the on-board spi flash. In the past I have separated out some unrelated patches, and have been told to keep them in a series where they are used. --Sean
On Wed, Feb 19, 2020 at 11:51 AM Sean Anderson <seanga2 at gmail.com> wrote: > > On 2/18/20 8:39 PM, Rick Chen wrote: > >>> > >>> Patch 6 and 7 shall be applied via spi tree. > >>> You mix them together, the patchwork will become complicated. > >>> It will be better for me to wait for a period of time to get the > >>> approval(Reviewed-by or Acked-by) of SPI MAINTAINER. > >>> > >>> Thanks > >>> Rick > >> > >> So is this just waiting for approval, or is there a change I should make > >> on my end? > > > > If they are not highly dependent, you shall separate and send them individually. > > But we can wait for few days and see, if it still no response. > > You can consider separate them. > > These patches add support for the on-board spi flash. In the past I have > separated out some unrelated patches, and have been told to keep them in > a series where they are used. Yes, keeping all related patch in one series help maintainers in some way. I would prefer all patches in this series go via the riscv tree. However as Rick pointed out, we do need the SPI maintainer's review comments. So Jagan, are you able to give some review? Thanks! Regards, Bin
diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c index 04cc873754..277eb19a0b 100644 --- a/drivers/spi/designware_spi.c +++ b/drivers/spi/designware_spi.c @@ -17,6 +17,7 @@ #include <errno.h> #include <malloc.h> #include <spi.h> +#include <spi-mem.h> #include <fdtdec.h> #include <reset.h> #include <linux/compat.h> @@ -107,8 +108,8 @@ struct dw_spi_priv { int len; u32 fifo_len; /* depth of the FIFO buffer */ - void *tx; - void *tx_end; + const void *tx; + const void *tx_end; void *rx; void *rx_end; @@ -344,7 +345,7 @@ static void dw_writer(struct dw_spi_priv *priv) txw = *(u16 *)(priv->tx); } dw_write(priv, DW_SPI_DR, txw); - debug("%s: tx=0x%02x\n", __func__, txw); + log_io("tx=0x%02x\n", txw); priv->tx += priv->bits_per_word >> 3; } } @@ -356,7 +357,7 @@ static void dw_reader(struct dw_spi_priv *priv) while (max--) { rxw = dw_read(priv, DW_SPI_DR); - debug("%s: rx=0x%02x\n", __func__, rxw); + log_io("rx=0x%02x\n", rxw); /* Care about rx if the transfer's original "rx" is not null */ if (priv->rx_end - priv->len) { @@ -483,6 +484,115 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, return ret; } +static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) +{ + bool read = op->data.dir == SPI_MEM_DATA_IN; + int pos, i, ret = 0; + struct udevice *bus = slave->dev->parent; + struct dw_spi_platdata *plat = dev_get_platdata(bus); + struct dw_spi_priv *priv = dev_get_priv(bus); + u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes; + u8 op_buf[op_len]; + u32 cr0; + + if (read) + priv->tmode = SPI_TMOD_EPROMREAD; + else + priv->tmode = SPI_TMOD_TO; + + debug("%s: buf=%p len=%u [bytes]\n", + __func__, op->data.buf.in, op->data.nbytes); + + cr0 = GEN_CTRL0(priv, plat); + debug("%s: cr0=%08x\n", __func__, cr0); + + spi_enable_chip(priv, 0); + dw_write(priv, DW_SPI_CTRL0, cr0); + if (read) + dw_write(priv, DW_SPI_CTRL1, op->data.nbytes - 1); + spi_enable_chip(priv, 1); + + /* From spi_mem_exec_op */ + pos = 0; + op_buf[pos++] = op->cmd.opcode; + if (op->addr.nbytes) { + for (i = 0; i < op->addr.nbytes; i++) + op_buf[pos + i] = op->addr.val >> + (8 * (op->addr.nbytes - i - 1)); + + pos += op->addr.nbytes; + } + if (op->dummy.nbytes) + memset(op_buf + pos, 0xff, op->dummy.nbytes); + + priv->tx = &op_buf; + priv->tx_end = priv->tx + op_len; + while (priv->tx != priv->tx_end) + dw_writer(priv); + + /* + * XXX: The following are tight loops! Enabling debug messages may cause + * them to fail because we are not reading/writing the fifo fast enough. + * + * We heuristically break out of the loop when we stop getting data. + * This is to stop us from hanging if the device doesn't send any data + * (either at all, or after sending a response). For example, one flash + * chip I tested did not send anything back after the first 64K of data. + */ + if (read) { + /* If we have gotten any data back yet */ + bool got_data = false; + /* How many times we have looped without reading anything */ + int loops_since_read = 0; + struct spi_mem_op *mut_op = (struct spi_mem_op *)op; + + priv->rx = op->data.buf.in; + priv->rx_end = priv->rx + op->data.nbytes; + + dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev)); + while (priv->rx != priv->rx_end) { + void *last_rx = priv->rx; + + dw_reader(priv); + if (priv->rx == last_rx) { + loops_since_read++; + /* Thresholds are arbitrary */ + if (loops_since_read > 256) + break; + else if (got_data && loops_since_read > 32) + break; + } else { + got_data = true; + loops_since_read = 0; + } + } + + /* Update with the actual amount of data read */ + mut_op->data.nbytes -= priv->rx_end - priv->rx; + } else { + u32 val; + + priv->tx = op->data.buf.out; + priv->tx_end = priv->tx + op->data.nbytes; + + /* Fill up the write fifo before starting the transfer */ + dw_writer(priv); + dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev)); + while (priv->tx != priv->tx_end) + dw_writer(priv); + + if (readl_poll_timeout(priv->regs + DW_SPI_SR, val, + (val & SR_TF_EMPT) && !(val & SR_BUSY), + RX_TIMEOUT * 1000)) { + ret = -ETIMEDOUT; + } + } + + dw_write(priv, DW_SPI_SER, 0); + debug("%s: %u bytes xfered\n", __func__, op->data.nbytes); + return ret; +} + static int dw_spi_set_speed(struct udevice *bus, uint speed) { struct dw_spi_platdata *plat = dev_get_platdata(bus); @@ -546,8 +656,13 @@ static int dw_spi_remove(struct udevice *bus) return 0; } +static const struct spi_controller_mem_ops dw_spi_mem_ops = { + .exec_op = dw_spi_exec_op, +}; + static const struct dm_spi_ops dw_spi_ops = { .xfer = dw_spi_xfer, + .mem_ops = &dw_spi_mem_ops, .set_speed = dw_spi_set_speed, .set_mode = dw_spi_set_mode, /*
The dw spi devices on the Kendryte K210 must be operated in a specific fasion which cannot be achived through multiple writes to via dw_spi_xfer (as it is currently written). This patch adds an implementation of exec_op, which gives correct behaviour when reading/writing spi flash. I would like to be able to modify the existing dw_spi_xfer function such that it works properly (e.g. with the mmc_spi driver). However, the only example code I have to work off is Kendryte's sdk (which is written in the exec_op style), and I do not have access to the datasheet (if anyone does, I would love to have a look!). Signed-off-by: Sean Anderson <seanga2 at gmail.com> --- Changes in v4: - New drivers/spi/designware_spi.c | 123 +++++++++++++++++++++++++++++++++-- 1 file changed, 119 insertions(+), 4 deletions(-)