[PATCH 0/3] spi: Support SPI I/O protocol lines

[PATCH 0/3] spi: Support SPI I/O protocol lines

[Jagan Teki]

Some of the SPI controllers have a special set of format
registers that defines how the transfer initiated to the
FIFO by means of I/O protocol lines.
  
Each mode of transfer from slave would be required to configure
the I/O protocol lines so-that the master would identify how
many number I/O protocol lines were used and alter the protocol
bits on the controller.

To address this issue (on these kinds of SPI controllers) this
series is trying to send the I/O protocol lines being used
on particular transfers.

patch 1: Transfer the opcode alone

patch 2: Add SPI I/O protocol lines via spi->proto 

patch 3: Use spi->proto on SiFive SPI controller

Any inputs?
Jagan.

Jagan Teki (3):
  spi: spi-mem: Xfer opcode alone for non spi-mem
  spi: Support SPI I/O protocol lines
  spi: sifive: Fix format register proto field

 drivers/spi/spi-mem.c    | 68 +++++++++++++++++++++++++---------------
 drivers/spi/spi-sifive.c | 11 +++++--
 drivers/spi/spi-uclass.c |  7 +++++
 include/spi.h            |  9 ++++++
 4 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH 1/3] spi: spi-mem: Xfer opcode alone for non spi-mem

[Jagan Teki]

Some of the SPI controllers have a special set of format
registers that defines how the transfer initiated to the
FIFO by means of I/O protocol lines.

Each mode of transfer from slave would be required to configure
the I/O protocol lines so-that the master would identify how
many number I/O protocol lines were used and alter the protocol
bits on the controller.

If a particular transfer combined opcode and address together,
then it would be difficult for the master to identify how many
I/O protocol lines are being used for opcode and address separately.

For example a transfer of SNOR_1_4_4 is not possible to identify
the master that how many I/O protocol lines by spi-nor if that
particular transfer initites the single xfer for both opcode and
address.

To address this issue (on these kind of SPI controllers) this
patch is trying to send the opcode alone as both opcode and
address send together in current code.

On the performance note there is no significant issue on the
transfer rate for adding this opcode xfer separately.

Sample test on 32MiB flash used in SiFive platform with
PP and RF opcodes.

With existing code:
=> sf update 0x90000000 0x0 0x2000000
device 0 whole chip
33554432 bytes written, 0 bytes skipped in 543.795s, speed 63191 B/s

With opcode alone xfer:
=> sf update 0x90000000 0x0 0x2000000
device 0 whole chip
33554432 bytes written, 0 bytes skipped in 541.739s, speed 63429 B/s

Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/spi-mem.c | 63 ++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index e900c997bd..7f4039e856 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -336,15 +336,18 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	if (msg.actual_length != totalxferlen)
 		return -EIO;
 #else
+	u8 opcode = op->cmd.opcode;
 
-	if (op->data.nbytes) {
-		if (op->data.dir == SPI_MEM_DATA_IN)
-			rx_buf = op->data.buf.in;
-		else
-			tx_buf = op->data.buf.out;
-	}
+	flag = SPI_XFER_BEGIN;
+	if (!op->addr.nbytes && !op->dummy.nbytes && !op->data.nbytes)
+		flag |= SPI_XFER_END;
 
-	op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
+	/* send the opcode */
+	ret = spi_xfer(slave, 8, (void *)&opcode, NULL, flag);
+	if (ret < 0) {
+		dev_err(slave->dev, "failed to xfer opcode\n");
+		return ret;
+	}
 
 	/*
 	 * Avoid using malloc() here so that we can use this code in SPL where
@@ -355,41 +358,51 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	 * data being sent, only the op-code and address. In fact, it should be
 	 * possible to just use a small fixed value here instead of op_len.
 	 */
+	op_len = op->addr.nbytes + op->dummy.nbytes;
 	u8 op_buf[op_len];
 
-	op_buf[pos++] = op->cmd.opcode;
-
+	/* send the addr + dummy */
 	if (op->addr.nbytes) {
+		/* fill address */
 		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);
+		/* fill dummy */
+		if (op->dummy.nbytes)
+			memset(op_buf + pos, 0xff, op->dummy.nbytes);
 
-	/* 1st transfer: opcode + address + dummy cycles */
-	flag = SPI_XFER_BEGIN;
-	/* Make sure to set END bit if no tx or rx data messages follow */
-	if (!tx_buf && !rx_buf)
-		flag |= SPI_XFER_END;
+		/* make sure to set end flag, if no data bytes */
+		if (!op->data.nbytes)
+			flag |= SPI_XFER_END;
 
-	ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
-	if (ret)
-		return ret;
+		ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
+		if (ret < 0) {
+			dev_err(slave->dev, "failed to xfer addr + dummy\n");
+			return ret;
+		}
+	}
 
-	/* 2nd transfer: rx or tx data path */
-	if (tx_buf || rx_buf) {
-		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
-			       rx_buf, SPI_XFER_END);
-		if (ret)
+	/* send/received the data */
+	if (op->data.nbytes) {
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			rx_buf = op->data.buf.in;
+		else
+			tx_buf = op->data.buf.out;
+
+		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, rx_buf,
+			       SPI_XFER_END);
+		if (ret) {
+			dev_err(slave->dev, "failed to xfer data\n");
 			return ret;
+		}
 	}
 
 	spi_release_bus(slave);
 
+	debug("%02x ", op->cmd.opcode);
 	for (i = 0; i < pos; i++)
 		debug("%02x ", op_buf[i]);
 	debug("| [%dB %s] ",
-- 
2.17.1

[PATCH 2/3] spi: Support SPI I/O protocol lines

[Jagan Teki]

Some of the SPI controllers have a special set of format
registers that defines how the transfer is initiated to the
FIFO by means of I/O protocol lines.

Each mode of transfer from slave would be required to configure
the I/O protocol lines so-that the master would identify how
many number I/O protocol lines were used and alter the protocol
bits on the controller.

So, add the I/O protocol lines support via proto.

Slave would fill the number I/O protocol lines in proto then
the master would alter the protocol bits on SPI controller
based on the proto number.

Slave would fill the number I/O protocol lines in the proto then
the master would alter the protocol bits on the SPI controller
based on the proto number.

This would happen for each transfer alone instead combined
transfers since each transfer has its own set of I/O protocol
lines.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/spi-mem.c    | 5 +++++
 drivers/spi/spi-uclass.c | 7 +++++++
 include/spi.h            | 9 +++++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 7f4039e856..4f655b23de 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -337,6 +337,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		return -EIO;
 #else
 	u8 opcode = op->cmd.opcode;
+	slave->proto = op->cmd.buswidth;
 
 	flag = SPI_XFER_BEGIN;
 	if (!op->addr.nbytes && !op->dummy.nbytes && !op->data.nbytes)
@@ -378,6 +379,8 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		if (!op->data.nbytes)
 			flag |= SPI_XFER_END;
 
+		slave->proto = op->addr.buswidth;
+
 		ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
 		if (ret < 0) {
 			dev_err(slave->dev, "failed to xfer addr + dummy\n");
@@ -392,6 +395,8 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		else
 			tx_buf = op->data.buf.out;
 
+		slave->proto = op->data.buswidth;
+
 		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, rx_buf,
 			       SPI_XFER_END);
 		if (ret) {
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 4a02d95a34..d602701566 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -86,12 +86,19 @@ int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
 {
 	struct udevice *bus = dev->parent;
 	struct dm_spi_ops *ops = spi_get_ops(bus);
+	struct dm_spi_slave_platdata *plat = dev_get_parent_platdata(dev);
+	struct spi_slave *slave = dev_get_parent_priv(dev);
 
 	if (bus->uclass->uc_drv->id != UCLASS_SPI)
 		return -EOPNOTSUPP;
 	if (!ops->xfer)
 		return -ENOSYS;
 
+	if (!slave->proto)
+		plat->proto = SPI_PROTO_SINGLE;
+	else
+		plat->proto = slave->proto;
+
 	return ops->xfer(dev, bitlen, dout, din, flags);
 }
 
diff --git a/include/spi.h b/include/spi.h
index 2b4929fc79..e1a1ef5ee8 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -57,11 +57,14 @@ struct dm_spi_bus {
  * @cs:		Chip select number (0..n-1)
  * @max_hz:	Maximum bus speed that this slave can tolerate
  * @mode:	SPI mode to use for this device (see SPI mode flags)
+ * @proto:	Number of IO protocol lines used for writing or reading.
+ *		If 0 then the default SPI_PROTO_SINGLE is used.
  */
 struct dm_spi_slave_platdata {
 	unsigned int cs;
 	uint max_hz;
 	uint mode;
+	uint proto;
 };
 
 #endif /* CONFIG_DM_SPI */
@@ -116,6 +119,8 @@ enum spi_polarity {
  * @max_hz:		Maximum speed for this slave
  * @speed:		Current bus speed. This is 0 until the bus is first
  *			claimed.
+ * @proto:		Number of IO protocol lines used for writing or reading.
+ *			If 0 then the default SPI_PROTO_SINGLE is used.
  * @bus:		ID of the bus that the slave is attached to. For
  *			driver model this is the sequence number of the SPI
  *			bus (bus->seq) so does not need to be stored
@@ -134,6 +139,10 @@ struct spi_slave {
 	struct udevice *dev;	/* struct spi_slave is dev->parentdata */
 	uint max_hz;
 	uint speed;
+	uint proto;
+#define SPI_PROTO_QUAD		4 /* 4 lines I/O protocol transfer */
+#define SPI_PROTO_DUAL		2 /* 2 lines I/O protocol transfer */
+#define SPI_PROTO_SINGLE	1 /* 1 line I/O protocol transfer */
 #else
 	unsigned int bus;
 	unsigned int cs;
-- 
2.17.1

[PATCH 3/3] spi: sifive: Fix format register proto field

[Jagan Teki]

SiFive SPI controller has a proto bit field in frame format
register which would be used to configure the SPI I/O protocol
lines used on specific transfer.?

Right now the driver is configuring this proto using slave->mode
which is used for data transfer and opcode, address vary depending
on the particular transfer at runtime.

Now the SPI framework supports per transfer I/O protocol lines,
so use spi->proto instead of slave-mode.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/spi-sifive.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c
index 8f5efb51a3..336b683476 100644
--- a/drivers/spi/spi-sifive.c
+++ b/drivers/spi/spi-sifive.c
@@ -146,12 +146,17 @@ static void sifive_spi_prep_transfer(struct sifive_spi *spi,
 
 	/* Number of wires ? */
 	cr &= ~SIFIVE_SPI_FMT_PROTO_MASK;
-	if ((slave_plat->mode & SPI_TX_QUAD) || (slave_plat->mode & SPI_RX_QUAD))
+	switch (slave_plat->proto) {
+	case SPI_PROTO_QUAD:
 		cr |= SIFIVE_SPI_FMT_PROTO_QUAD;
-	else if ((slave_plat->mode & SPI_TX_DUAL) || (slave_plat->mode & SPI_RX_DUAL))
+		break;
+	case SPI_PROTO_DUAL:
 		cr |= SIFIVE_SPI_FMT_PROTO_DUAL;
-	else
+		break;
+	default:
 		cr |= SIFIVE_SPI_FMT_PROTO_SINGLE;
+		break;
+	}
 
 	/* SPI direction in/out ? */
 	cr &= ~SIFIVE_SPI_FMT_DIR;
-- 
2.17.1

[PATCH 3/3] spi: sifive: Fix format register proto field

[Bin Meng]

On Mon, Apr 20, 2020 at 8:09 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> SiFive SPI controller has a proto bit field in frame format
> register which would be used to configure the SPI I/O protocol
> lines used on specific transfer.
>
> Right now the driver is configuring this proto using slave->mode
> which is used for data transfer and opcode, address vary depending
> on the particular transfer at runtime.
>
> Now the SPI framework supports per transfer I/O protocol lines,
> so use spi->proto instead of slave-mode.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/spi/spi-sifive.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>

This patch does not apply on top of u-boot/master.

Please rebase and resend.

Regards,
Bin

[PATCH 0/3] spi: Support SPI I/O protocol lines

[Vignesh Raghavendra]

Hi Jagan,

On 20/04/20 5:39 pm, Jagan Teki wrote:
> Some of the SPI controllers have a special set of format
> registers that defines how the transfer initiated to the
> FIFO by means of I/O protocol lines.
>   
> Each mode of transfer from slave would be required to configure
> the I/O protocol lines so-that the master would identify how
> many number I/O protocol lines were used and alter the protocol
> bits on the controller.
> 
> To address this issue (on these kinds of SPI controllers) this
> series is trying to send the I/O protocol lines being used
> on particular transfers.
> 
> patch 1: Transfer the opcode alone

Has this been tested on more than one SPI controller? Is this safe to do?

> 
> patch 2: Add SPI I/O protocol lines via spi->proto 
> 
> patch 3: Use spi->proto on SiFive SPI controller
> 
> Any inputs?

Why cannot SiFive SPI controller implement spi_mem_ops? Is there a non
flash SPI device that supports quad mode?

> Jagan.
> 
> Jagan Teki (3):
>   spi: spi-mem: Xfer opcode alone for non spi-mem
>   spi: Support SPI I/O protocol lines
>   spi: sifive: Fix format register proto field
> 
>  drivers/spi/spi-mem.c    | 68 +++++++++++++++++++++++++---------------
>  drivers/spi/spi-sifive.c | 11 +++++--
>  drivers/spi/spi-uclass.c |  7 +++++
>  include/spi.h            |  9 ++++++
>  4 files changed, 67 insertions(+), 28 deletions(-)
> 

[PATCH 0/3] spi: Support SPI I/O protocol lines

[Jagan Teki]

On Tue, Apr 21, 2020 at 12:49 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi Jagan,
>
> On 20/04/20 5:39 pm, Jagan Teki wrote:
> > Some of the SPI controllers have a special set of format
> > registers that defines how the transfer initiated to the
> > FIFO by means of I/O protocol lines.
> >
> > Each mode of transfer from slave would be required to configure
> > the I/O protocol lines so-that the master would identify how
> > many number I/O protocol lines were used and alter the protocol
> > bits on the controller.
> >
> > To address this issue (on these kinds of SPI controllers) this
> > series is trying to send the I/O protocol lines being used
> > on particular transfers.
> >
> > patch 1: Transfer the opcode alone
>
> Has this been tested on more than one SPI controller? Is this safe to do?

Yes. Well it is the proper way for handling all types of use cases
(like Linux tx/rx_nbits does).

>
> >
> > patch 2: Add SPI I/O protocol lines via spi->proto
> >
> > patch 3: Use spi->proto on SiFive SPI controller
> >
> > Any inputs?
>
> Why cannot SiFive SPI controller implement spi_mem_ops? Is there a non
> flash SPI device that supports quad mode?

Adding this generic code in spi-mem looks like adding generic code to
work for the sake of this controller. Eventually two code bases do the
same job with some buswidth extension. Lets hold for some time for
testing other boards.

Jagan.

[PATCH 3/3] spi: sifive: Fix format register proto field

[Sagar Kadam]

Hi Bin, Jagan,

Thanks Jagan for posting the patches to enable QUAD SPI-NOR on HiFive Unleashed
along with other sequels.

> -----Original Message-----
> From: Bin Meng <bmeng.cn@gmail.com>
> Sent: Tuesday, April 21, 2020 4:44 AM
> To: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Vignesh R <vigneshr@ti.com>; U-Boot Mailing List <u-
> boot at lists.denx.de>; Suneel Garapati <suneelglinux@gmail.com>; Sagar
> Kadam <sagar.kadam@sifive.com>; Bhargav Shah
> <bhargavshah1988@gmail.com>; Simon Glass <sjg@chromium.org>; Tom
> Rini <trini@konsulko.com>; linux-amarula <linux-
> amarula at amarulasolutions.com>
> Subject: Re: [PATCH 3/3] spi: sifive: Fix format register proto field
> 
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
> 
> On Mon, Apr 20, 2020 at 8:09 PM Jagan Teki
> <jagan@amarulasolutions.com> wrote:
> >
> > SiFive SPI controller has a proto bit field in frame format register
> > which would be used to configure the SPI I/O protocol lines used on
> > specific transfer.
> >
> > Right now the driver is configuring this proto using slave->mode which
> > is used for data transfer and opcode, address vary depending on the
> > particular transfer at runtime.
> >
> > Now the SPI framework supports per transfer I/O protocol lines, so use
> > spi->proto instead of slave-mode.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/spi/spi-sifive.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> 

> This patch does not apply on top of u-boot/master.
> 
> Please rebase and resend.

I guess Bin, you will also have to add following two patch series [1] and [2] before this set.
I tested this and other series with following dependency chain over 
u-boot/master(e4837da7828293ea49abc579f939c0f5c4b127c3)

1> 1-2-mtd-spi-nor-Enable-QE-bit-for-ISSI-flash.patch
2> spi-sifive-Tidy-up-dm_spi_slave_platdata-variable.patch
3> spi: Support SPI I/O protocol lines
4> riscv: sifive/fu540: Enable SPI-NOR support

I could verify flash erase/read/write operations along with mmc spi.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20200420100607.23009-1-jagan at amarulasolutions.com/
[2] https://patchwork.amarulasolutions.com/patch/1083/


Thanks & BR,
Sagar Kadam

> 
> Regards,
> Bin

[PATCH 3/3] spi: sifive: Fix format register proto field

[Jagan Teki]

Hi Sagar and Bin,

On Tue, Apr 21, 2020 at 9:17 PM Sagar Kadam <sagar.kadam@sifive.com> wrote:
>
> Hi Bin, Jagan,
>
> Thanks Jagan for posting the patches to enable QUAD SPI-NOR on HiFive Unleashed
> along with other sequels.
>
> > -----Original Message-----
> > From: Bin Meng <bmeng.cn@gmail.com>
> > Sent: Tuesday, April 21, 2020 4:44 AM
> > To: Jagan Teki <jagan@amarulasolutions.com>
> > Cc: Vignesh R <vigneshr@ti.com>; U-Boot Mailing List <u-
> > boot at lists.denx.de>; Suneel Garapati <suneelglinux@gmail.com>; Sagar
> > Kadam <sagar.kadam@sifive.com>; Bhargav Shah
> > <bhargavshah1988@gmail.com>; Simon Glass <sjg@chromium.org>; Tom
> > Rini <trini@konsulko.com>; linux-amarula <linux-
> > amarula at amarulasolutions.com>
> > Subject: Re: [PATCH 3/3] spi: sifive: Fix format register proto field
> >
> > [External Email] Do not click links or attachments unless you recognize the
> > sender and know the content is safe
> >
> > On Mon, Apr 20, 2020 at 8:09 PM Jagan Teki
> > <jagan@amarulasolutions.com> wrote:
> > >
> > > SiFive SPI controller has a proto bit field in frame format register
> > > which would be used to configure the SPI I/O protocol lines used on
> > > specific transfer.
> > >
> > > Right now the driver is configuring this proto using slave->mode which
> > > is used for data transfer and opcode, address vary depending on the
> > > particular transfer at runtime.
> > >
> > > Now the SPI framework supports per transfer I/O protocol lines, so use
> > > spi->proto instead of slave-mode.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  drivers/spi/spi-sifive.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> >
>
> > This patch does not apply on top of u-boot/master.
> >
> > Please rebase and resend.
>
> I guess Bin, you will also have to add following two patch series [1] and [2] before this set.
> I tested this and other series with following dependency chain over
> u-boot/master(e4837da7828293ea49abc579f939c0f5c4b127c3)
>
> 1> 1-2-mtd-spi-nor-Enable-QE-bit-for-ISSI-flash.patch
> 2> spi-sifive-Tidy-up-dm_spi_slave_platdata-variable.patch
> 3> spi: Support SPI I/O protocol lines
> 4> riscv: sifive/fu540: Enable SPI-NOR support
>
> I could verify flash erase/read/write operations along with mmc spi.

Send the v4 w/o any dependencies, but on top of u-boot-spi/master
tree. I/O protocol changes are now handled in spi-sifive via spi-mem
exec_op for now since the actual generic code patch[3] will take some
time to be in ML as it affects all platforms.

Please have a test at earliest.

[3] https://patchwork.ozlabs.org/project/uboot/patch/20200420120921.12840-2-jagan at amarulasolutions.com/

Jagan.