[PATCH RFC] spi: orion.c: Add direct write mode

[PATCH RFC] spi: orion.c: Add direct write mode

[Stefan Roese]

This patch adds support for the direct write mode to the Orion SPI
driver which is used on the Marvell Armada based SoCs. In this direct
mode, all data written to a specifically mapped MBus window (linked
to only one SPI chip-select on one of the SPI controllers) will be
transferred directly to the SPI bus. Without the need to control the
SPI registers in between. This can improve the SPI transfer rate in
such cases.

Currently only the direct write mode is supported. This mode especially
benefits from the SPI direct mode, as the data bytes are written
head-to-head to the SPI bus, without any additional addresses, that
are also written in the direct read mode.

One use-case for this direct write mode is, programming a FPGA bitstream
image into the FPGA connected to the SPI bus at maximum speed.

This mode is described in chapter "22.5.2 Direct Write to SPI" in the
Marvell Armada XP Functional Spec Datasheet.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Nadav Haklai <nadavh@marvell.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
Cc: Mark Brown <broonie@kernel.org>
---

 .../devicetree/bindings/spi/spi-orion.txt          | 26 ++++++++++
 drivers/spi/spi-orion.c                            | 59 ++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
index 98bc698..b22ebc5 100644
--- a/Documentation/devicetree/bindings/spi/spi-orion.txt
+++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
@@ -12,6 +12,8 @@ Required properties:
 - cell-index : Which of multiple SPI controllers is this.
 Optional properties:
 - interrupts : Is currently not used.
+- direct-addr : The phandle to the node containing the base address
+                of the direct-mapped MBus window for this SPI device.
 
 Example:
        spi@10600 {
@@ -23,3 +25,27 @@ Example:
 	       interrupts = <23>;
 	       status = "disabled";
        };
+
+Example with direct-write mode:
+	soc {
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000	/* internal regs */
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000	/* BootROM       */
+			  MBUS_ID(0x01, 0x5e) 0 0 0xf2000000 0x100000>;	/* SPI0 CS1      */
+
+		spi0_cs1: spi@015e {
+			compatible = "marvell,spi-direct-mode";
+			reg = <MBUS_ID(0x01, 0x5e) 0 0x100000>;
+		};
+
+	...
+
+	spi@10600 {
+		compatible = "marvell,orion-spi";
+		status = "okay";
+
+		spidev@1 {
+			compatible = "spidev";
+			direct-addr = <&spi0_cs1>;
+			reg = <1>;
+		};
+	};
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index a87cfd4..c028679 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/clk.h>
 #include <linux/sizes.h>
@@ -43,6 +44,9 @@
 #define ORION_SPI_INT_CAUSE_REG		0x10
 #define ORION_SPI_TIMING_PARAMS_REG	0x18
 
+/* Register for the "Direct Mode" */
+#define SPI_DIRECT_WRITE_CONFIG_REG	0x20
+
 #define ORION_SPI_TMISO_SAMPLE_MASK	(0x3 << 6)
 #define ORION_SPI_TMISO_SAMPLE_1	(1 << 6)
 #define ORION_SPI_TMISO_SAMPLE_2	(2 << 6)
@@ -83,6 +87,7 @@ struct orion_spi {
 	void __iomem		*base;
 	struct clk              *clk;
 	const struct orion_spi_dev *devdata;
+	void __iomem		*slave_direct_addr[8];
 };
 
 static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg)
@@ -372,10 +377,29 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 {
 	unsigned int count;
 	int word_len;
+	struct orion_spi *orion_spi;
+	void __iomem *direct_addr;
 
 	word_len = spi->bits_per_word;
 	count = xfer->len;
 
+	/* Use SPI direct write mode if such an address is provided via DT */
+	orion_spi = spi_master_get_devdata(spi->master);
+	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
+	if (direct_addr && xfer->tx_buf) {
+		/* Deassert CS between the SPI transfers */
+		writel(0x00010000, spi_reg(orion_spi,
+					   SPI_DIRECT_WRITE_CONFIG_REG));
+
+		/*
+		 * Send the tx-data to the SPI device via the direct mapped
+		 * address window
+		 */
+		memcpy(direct_addr, xfer->tx_buf, count);
+
+		return count;
+	}
+
 	if (word_len == 8) {
 		const u8 *tx = xfer->tx_buf;
 		u8 *rx = xfer->rx_buf;
@@ -501,6 +525,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 	const struct orion_spi_dev *devdata;
 	struct spi_master *master;
 	struct orion_spi *spi;
+	struct device_node *np;
 	struct resource *r;
 	unsigned long tclk_hz;
 	int status = 0;
@@ -576,6 +601,40 @@ static int orion_spi_probe(struct platform_device *pdev)
 		goto out_rel_clk;
 	}
 
+	/*
+	 * Scan all SPI devices of this controller for direct mapped devices
+	 */
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		struct device_node *direct_addr_np;
+
+		/*
+		 * Get "direct-addr" device node with the mapping info
+		 */
+		direct_addr_np = of_parse_phandle(np, "direct-addr", 0);
+		if (direct_addr_np) {
+			struct resource res;
+			u32 cs;
+			int rc;
+
+			/* Get chip-select number from the "reg" property */
+			rc = of_property_read_u32(np, "reg", &cs);
+			if (rc) {
+				dev_err(&pdev->dev,
+					"%s has no valid 'reg' property (%d)\n",
+					direct_addr_np->full_name, rc);
+				continue;
+			}
+
+			/*
+			 * Store the address to use it later for the direct
+			 * access
+			 */
+			rc = of_address_to_resource(direct_addr_np, 0, &res);
+			spi->slave_direct_addr[cs] =
+				devm_ioremap_resource(&pdev->dev, &res);
+		}
+	}
+
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
-- 
2.6.5

[PATCH RFC] spi: orion.c: Add direct write mode

[Stefan Roese]

This patch adds support for the direct write mode to the Orion SPI
driver which is used on the Marvell Armada based SoCs. In this direct
mode, all data written to a specifically mapped MBus window (linked
to only one SPI chip-select on one of the SPI controllers) will be
transferred directly to the SPI bus. Without the need to control the
SPI registers in between. This can improve the SPI transfer rate in
such cases.

Currently only the direct write mode is supported. This mode especially
benefits from the SPI direct mode, as the data bytes are written
head-to-head to the SPI bus, without any additional addresses, that
are also written in the direct read mode.

One use-case for this direct write mode is, programming a FPGA bitstream
image into the FPGA connected to the SPI bus at maximum speed.

This mode is described in chapter "22.5.2 Direct Write to SPI" in the
Marvell Armada XP Functional Spec Datasheet.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Nadav Haklai <nadavh@marvell.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
Cc: Mark Brown <broonie@kernel.org>
---

 .../devicetree/bindings/spi/spi-orion.txt          | 26 ++++++++++
 drivers/spi/spi-orion.c                            | 59 ++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
index 98bc698..b22ebc5 100644
--- a/Documentation/devicetree/bindings/spi/spi-orion.txt
+++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
@@ -12,6 +12,8 @@ Required properties:
 - cell-index : Which of multiple SPI controllers is this.
 Optional properties:
 - interrupts : Is currently not used.
+- direct-addr : The phandle to the node containing the base address
+                of the direct-mapped MBus window for this SPI device.
 
 Example:
        spi at 10600 {
@@ -23,3 +25,27 @@ Example:
 	       interrupts = <23>;
 	       status = "disabled";
        };
+
+Example with direct-write mode:
+	soc {
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000	/* internal regs */
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000	/* BootROM       */
+			  MBUS_ID(0x01, 0x5e) 0 0 0xf2000000 0x100000>;	/* SPI0 CS1      */
+
+		spi0_cs1: spi at 015e {
+			compatible = "marvell,spi-direct-mode";
+			reg = <MBUS_ID(0x01, 0x5e) 0 0x100000>;
+		};
+
+	...
+
+	spi at 10600 {
+		compatible = "marvell,orion-spi";
+		status = "okay";
+
+		spidev at 1 {
+			compatible = "spidev";
+			direct-addr = <&spi0_cs1>;
+			reg = <1>;
+		};
+	};
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index a87cfd4..c028679 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/clk.h>
 #include <linux/sizes.h>
@@ -43,6 +44,9 @@
 #define ORION_SPI_INT_CAUSE_REG		0x10
 #define ORION_SPI_TIMING_PARAMS_REG	0x18
 
+/* Register for the "Direct Mode" */
+#define SPI_DIRECT_WRITE_CONFIG_REG	0x20
+
 #define ORION_SPI_TMISO_SAMPLE_MASK	(0x3 << 6)
 #define ORION_SPI_TMISO_SAMPLE_1	(1 << 6)
 #define ORION_SPI_TMISO_SAMPLE_2	(2 << 6)
@@ -83,6 +87,7 @@ struct orion_spi {
 	void __iomem		*base;
 	struct clk              *clk;
 	const struct orion_spi_dev *devdata;
+	void __iomem		*slave_direct_addr[8];
 };
 
 static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg)
@@ -372,10 +377,29 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 {
 	unsigned int count;
 	int word_len;
+	struct orion_spi *orion_spi;
+	void __iomem *direct_addr;
 
 	word_len = spi->bits_per_word;
 	count = xfer->len;
 
+	/* Use SPI direct write mode if such an address is provided via DT */
+	orion_spi = spi_master_get_devdata(spi->master);
+	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
+	if (direct_addr && xfer->tx_buf) {
+		/* Deassert CS between the SPI transfers */
+		writel(0x00010000, spi_reg(orion_spi,
+					   SPI_DIRECT_WRITE_CONFIG_REG));
+
+		/*
+		 * Send the tx-data to the SPI device via the direct mapped
+		 * address window
+		 */
+		memcpy(direct_addr, xfer->tx_buf, count);
+
+		return count;
+	}
+
 	if (word_len == 8) {
 		const u8 *tx = xfer->tx_buf;
 		u8 *rx = xfer->rx_buf;
@@ -501,6 +525,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 	const struct orion_spi_dev *devdata;
 	struct spi_master *master;
 	struct orion_spi *spi;
+	struct device_node *np;
 	struct resource *r;
 	unsigned long tclk_hz;
 	int status = 0;
@@ -576,6 +601,40 @@ static int orion_spi_probe(struct platform_device *pdev)
 		goto out_rel_clk;
 	}
 
+	/*
+	 * Scan all SPI devices of this controller for direct mapped devices
+	 */
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		struct device_node *direct_addr_np;
+
+		/*
+		 * Get "direct-addr" device node with the mapping info
+		 */
+		direct_addr_np = of_parse_phandle(np, "direct-addr", 0);
+		if (direct_addr_np) {
+			struct resource res;
+			u32 cs;
+			int rc;
+
+			/* Get chip-select number from the "reg" property */
+			rc = of_property_read_u32(np, "reg", &cs);
+			if (rc) {
+				dev_err(&pdev->dev,
+					"%s has no valid 'reg' property (%d)\n",
+					direct_addr_np->full_name, rc);
+				continue;
+			}
+
+			/*
+			 * Store the address to use it later for the direct
+			 * access
+			 */
+			rc = of_address_to_resource(direct_addr_np, 0, &res);
+			spi->slave_direct_addr[cs] =
+				devm_ioremap_resource(&pdev->dev, &res);
+		}
+	}
+
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
-- 
2.6.5

Re: [PATCH RFC] spi: orion.c: Add direct write mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

On Tue, Jan 12, 2016 at 10:02:19AM +0100, Stefan Roese wrote:

> +- direct-addr : The phandle to the node containing the base address
> +                of the direct-mapped MBus window for this SPI device.

Why are we not just making the MBus window a normal resource on the SPI
controller and why do we need to use this only for a single device?  If
we can program which device is used then we can just reconfigure
whenever we change devices and use this optimisation for everything.

> +		spidev@1 {
> +			compatible = "spidev";
> +			direct-addr = <&spi0_cs1>;
> +			reg = <1>;
> +		};

This is broken, spidev should never appear in a DT (and the driver will
complain loudly if it does).  Describe the actual hardware.

> +	/* Use SPI direct write mode if such an address is provided via DT */
> +	orion_spi = spi_master_get_devdata(spi->master);
> +	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
> +	if (direct_addr && xfer->tx_buf) {
> +		/* Deassert CS between the SPI transfers */
> +		writel(0x00010000, spi_reg(orion_spi,
> +					   SPI_DIRECT_WRITE_CONFIG_REG));

This is badly broken, we should be asserting /CS over the entire message
unless the individual transfer says otherwise.  I'm surprised this
works.

> +		/*
> +		 * Send the tx-data to the SPI device via the direct mapped
> +		 * address window
> +		 */
> +		memcpy(direct_addr, xfer->tx_buf, count);

What if we are transferring more data than the window mapped in
direct_addr and what if the transfer is bidirectional?  It looks like we
can only do this for transmit only transfers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH RFC] spi: orion.c: Add direct write mode

[Mark Brown]

On Tue, Jan 12, 2016 at 10:02:19AM +0100, Stefan Roese wrote:

> +- direct-addr : The phandle to the node containing the base address
> +                of the direct-mapped MBus window for this SPI device.

Why are we not just making the MBus window a normal resource on the SPI
controller and why do we need to use this only for a single device?  If
we can program which device is used then we can just reconfigure
whenever we change devices and use this optimisation for everything.

> +		spidev at 1 {
> +			compatible = "spidev";
> +			direct-addr = <&spi0_cs1>;
> +			reg = <1>;
> +		};

This is broken, spidev should never appear in a DT (and the driver will
complain loudly if it does).  Describe the actual hardware.

> +	/* Use SPI direct write mode if such an address is provided via DT */
> +	orion_spi = spi_master_get_devdata(spi->master);
> +	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
> +	if (direct_addr && xfer->tx_buf) {
> +		/* Deassert CS between the SPI transfers */
> +		writel(0x00010000, spi_reg(orion_spi,
> +					   SPI_DIRECT_WRITE_CONFIG_REG));

This is badly broken, we should be asserting /CS over the entire message
unless the individual transfer says otherwise.  I'm surprised this
works.

> +		/*
> +		 * Send the tx-data to the SPI device via the direct mapped
> +		 * address window
> +		 */
> +		memcpy(direct_addr, xfer->tx_buf, count);

What if we are transferring more data than the window mapped in
direct_addr and what if the transfer is bidirectional?  It looks like we
can only do this for transmit only transfers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160112/c6201ed0/attachment.sig>

Re: [PATCH RFC] spi: orion.c: Add direct write mode

[Stefan Roese]

On 12.01.2016 11:13, Mark Brown wrote:
> On Tue, Jan 12, 2016 at 10:02:19AM +0100, Stefan Roese wrote:
>
>> +- direct-addr : The phandle to the node containing the base address
>> +                of the direct-mapped MBus window for this SPI device.
>
> Why are we not just making the MBus window a normal resource on the SPI
> controller and why do we need to use this only for a single device?  If
> we can program which device is used then we can just reconfigure
> whenever we change devices and use this optimisation for everything.

Interesting idea. It should be possible to add a dynamic MBus
window allocation / configuration, similar to what pci-mvebu.c
does. I'll give it a try to implement this dynamic MBus
configuration in the next version.

>> +		spidev@1 {
>> +			compatible = "spidev";
>> +			direct-addr = <&spi0_cs1>;
>> +			reg = <1>;
>> +		};
>
> This is broken, spidev should never appear in a DT (and the driver will
> complain loudly if it does).  Describe the actual hardware.

Yes. This was meant just as an example - a quite bad one, I agree.
I'll change this.

>> +	/* Use SPI direct write mode if such an address is provided via DT */
>> +	orion_spi = spi_master_get_devdata(spi->master);
>> +	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
>> +	if (direct_addr && xfer->tx_buf) {
>> +		/* Deassert CS between the SPI transfers */
>> +		writel(0x00010000, spi_reg(orion_spi,
>> +					   SPI_DIRECT_WRITE_CONFIG_REG));
>
> This is badly broken, we should be asserting /CS over the entire message
> unless the individual transfer says otherwise.  I'm surprised this
> works.

It works, at least on the FPGA that I'm programming the bitstream into
right now. The reason for deasserting the CS after each SPI transfer
was, to work around potential problems with concurrent accesses to
other SPI devices connected to the same SPI controller. Like SPI NOR
flash devices using the "normal" indirect mode. Deasserting the CS
seemed like the "safe" way to me here.

>> +		/*
>> +		 * Send the tx-data to the SPI device via the direct mapped
>> +		 * address window
>> +		 */
>> +		memcpy(direct_addr, xfer->tx_buf, count);
>
> What if we are transferring more data than the window mapped in
> direct_addr

Right, a check is missing here.

> and what if the transfer is bidirectional?  It looks like we
> can only do this for transmit only transfers.

This patch only adds support for the direct write mode. As the main
purpose is to speed up the SPI TX throughput for FPGA bitstream
programming. It could be extended to support also the direct read
mode. But this would need more configuration options, like the
number of address-bytes to transfer in each read access. Not sure
how this should interact with the SPI flash driver from the MTD
subsystem.

I would prefer to not adding this direct read mode just yet. It
can be added later, if really needed.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC] spi: orion.c: Add direct write mode

[Stefan Roese]

On 12.01.2016 11:13, Mark Brown wrote:
> On Tue, Jan 12, 2016 at 10:02:19AM +0100, Stefan Roese wrote:
>
>> +- direct-addr : The phandle to the node containing the base address
>> +                of the direct-mapped MBus window for this SPI device.
>
> Why are we not just making the MBus window a normal resource on the SPI
> controller and why do we need to use this only for a single device?  If
> we can program which device is used then we can just reconfigure
> whenever we change devices and use this optimisation for everything.

Interesting idea. It should be possible to add a dynamic MBus
window allocation / configuration, similar to what pci-mvebu.c
does. I'll give it a try to implement this dynamic MBus
configuration in the next version.

>> +		spidev at 1 {
>> +			compatible = "spidev";
>> +			direct-addr = <&spi0_cs1>;
>> +			reg = <1>;
>> +		};
>
> This is broken, spidev should never appear in a DT (and the driver will
> complain loudly if it does).  Describe the actual hardware.

Yes. This was meant just as an example - a quite bad one, I agree.
I'll change this.

>> +	/* Use SPI direct write mode if such an address is provided via DT */
>> +	orion_spi = spi_master_get_devdata(spi->master);
>> +	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
>> +	if (direct_addr && xfer->tx_buf) {
>> +		/* Deassert CS between the SPI transfers */
>> +		writel(0x00010000, spi_reg(orion_spi,
>> +					   SPI_DIRECT_WRITE_CONFIG_REG));
>
> This is badly broken, we should be asserting /CS over the entire message
> unless the individual transfer says otherwise.  I'm surprised this
> works.

It works, at least on the FPGA that I'm programming the bitstream into
right now. The reason for deasserting the CS after each SPI transfer
was, to work around potential problems with concurrent accesses to
other SPI devices connected to the same SPI controller. Like SPI NOR
flash devices using the "normal" indirect mode. Deasserting the CS
seemed like the "safe" way to me here.

>> +		/*
>> +		 * Send the tx-data to the SPI device via the direct mapped
>> +		 * address window
>> +		 */
>> +		memcpy(direct_addr, xfer->tx_buf, count);
>
> What if we are transferring more data than the window mapped in
> direct_addr

Right, a check is missing here.

> and what if the transfer is bidirectional?  It looks like we
> can only do this for transmit only transfers.

This patch only adds support for the direct write mode. As the main
purpose is to speed up the SPI TX throughput for FPGA bitstream
programming. It could be extended to support also the direct read
mode. But this would need more configuration options, like the
number of address-bytes to transfer in each read access. Not sure
how this should interact with the SPI flash driver from the MTD
subsystem.

I would prefer to not adding this direct read mode just yet. It
can be added later, if really needed.

Thanks,
Stefan

Re: [PATCH RFC] spi: orion.c: Add direct write mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

On Thu, Jan 14, 2016 at 08:01:56AM +0100, Stefan Roese wrote:
> On 12.01.2016 11:13, Mark Brown wrote:

> >>+	/* Use SPI direct write mode if such an address is provided via DT */
> >>+	orion_spi = spi_master_get_devdata(spi->master);
> >>+	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
> >>+	if (direct_addr && xfer->tx_buf) {
> >>+		/* Deassert CS between the SPI transfers */
> >>+		writel(0x00010000, spi_reg(orion_spi,
> >>+					   SPI_DIRECT_WRITE_CONFIG_REG));

> >This is badly broken, we should be asserting /CS over the entire message
> >unless the individual transfer says otherwise.  I'm surprised this
> >works.

> It works, at least on the FPGA that I'm programming the bitstream into
> right now. The reason for deasserting the CS after each SPI transfer
> was, to work around potential problems with concurrent accesses to
> other SPI devices connected to the same SPI controller. Like SPI NOR
> flash devices using the "normal" indirect mode. Deasserting the CS
> seemed like the "safe" way to me here.

No, this is not remotely safe - it is going to break any device where
the driver uses more than one transfer per message.  Instead of one
operation on the bus with /CS held over the entire operation the device
will see multiple operations.

> >and what if the transfer is bidirectional?  It looks like we
> >can only do this for transmit only transfers.

> This patch only adds support for the direct write mode. As the main
> purpose is to speed up the SPI TX throughput for FPGA bitstream
> programming. It could be extended to support also the direct read
> mode. But this would need more configuration options, like the
> number of address-bytes to transfer in each read access. Not sure
> how this should interact with the SPI flash driver from the MTD
> subsystem.

> I would prefer to not adding this direct read mode just yet. It
> can be added later, if really needed.

I can't see anything which will prevent trying to use direct write in
combination with a read which I'd not expect to work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH RFC] spi: orion.c: Add direct write mode

[Mark Brown]

On Thu, Jan 14, 2016 at 08:01:56AM +0100, Stefan Roese wrote:
> On 12.01.2016 11:13, Mark Brown wrote:

> >>+	/* Use SPI direct write mode if such an address is provided via DT */
> >>+	orion_spi = spi_master_get_devdata(spi->master);
> >>+	direct_addr = orion_spi->slave_direct_addr[spi->chip_select];
> >>+	if (direct_addr && xfer->tx_buf) {
> >>+		/* Deassert CS between the SPI transfers */
> >>+		writel(0x00010000, spi_reg(orion_spi,
> >>+					   SPI_DIRECT_WRITE_CONFIG_REG));

> >This is badly broken, we should be asserting /CS over the entire message
> >unless the individual transfer says otherwise.  I'm surprised this
> >works.

> It works, at least on the FPGA that I'm programming the bitstream into
> right now. The reason for deasserting the CS after each SPI transfer
> was, to work around potential problems with concurrent accesses to
> other SPI devices connected to the same SPI controller. Like SPI NOR
> flash devices using the "normal" indirect mode. Deasserting the CS
> seemed like the "safe" way to me here.

No, this is not remotely safe - it is going to break any device where
the driver uses more than one transfer per message.  Instead of one
operation on the bus with /CS held over the entire operation the device
will see multiple operations.

> >and what if the transfer is bidirectional?  It looks like we
> >can only do this for transmit only transfers.

> This patch only adds support for the direct write mode. As the main
> purpose is to speed up the SPI TX throughput for FPGA bitstream
> programming. It could be extended to support also the direct read
> mode. But this would need more configuration options, like the
> number of address-bytes to transfer in each read access. Not sure
> how this should interact with the SPI flash driver from the MTD
> subsystem.

> I would prefer to not adding this direct read mode just yet. It
> can be added later, if really needed.

I can't see anything which will prevent trying to use direct write in
combination with a read which I'd not expect to work.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160114/d0fc1ebc/attachment.sig>