diff mbox series

[v4,07/17] spi: dw: Add mem_ops

Message ID 20200211060425.1619471-8-seanga2@gmail.com
State New
Headers show
Series riscv: Add Sipeed Maix support | expand

Commit Message

Sean Anderson Feb. 11, 2020, 6:04 a.m. UTC
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(-)

Comments

Rick Chen Feb. 18, 2020, 8:34 a.m. UTC | #1
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
>
Sean Anderson Feb. 18, 2020, 4:32 p.m. UTC | #2
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
Rick Chen Feb. 19, 2020, 1:39 a.m. UTC | #3
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
Sean Anderson Feb. 19, 2020, 3:51 a.m. UTC | #4
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
Bin Meng Feb. 19, 2020, 3:31 p.m. UTC | #5
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 mbox series

Patch

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,
 	/*