Skip to contentSkip to conversation

spi: orion.c: Add direct access mode

PATCH Superseded Stefan Roese 3 participants 9 messages Apr 7, 2016 – Apr 16, 2016 SPI

v3 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

This patch adds support for the direct access mode to the Orion SPI driver which is used on the Marvell Armada based SoCs. In this direct mode, all data written to (or read from) a specifically mapped MBus window (linked to 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.

Both, direct-read and -write mode are supported. But only the write mode has been tested. 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.

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.

Cc Thomas Petazzoni <[email protected]>
Cc Gregory CLEMENT <[email protected]>
Cc Andrew Lunn <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Mark Brown <[email protected]>

---
v3:
- Used static MBus windows again, as suggested by Arnd
- No new DT bindings needed for this. The MBus windows need to be configured in the 'reg' property of the SPI controller. This is done in a separate patch for the Armada platforms.
- The direct mapping is configured and enable by adding the specific MBus entry to the 'ranges' property of the 'soc' node in the board dts file.

v2:
- Use one MBus window for each SPI controller instead of one for for each SPI device. This MBus window is re-assigned, once the CS changes.
- Assert the CS over the entire message
- Don't restrict the direct access mode to only direct write mode
- Add check for max size before using memcpy()
- Remove spidev from DT bindings documentation

 drivers/spi/spi-orion.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
drivers/spi/spi-orion.c +86 -0
index a87cfd4..69e710e 100644
@@ -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)
@@ -78,11 +82,18 @@ struct orion_spi_dev {
 	bool			is_errata_50mhz_ac;
 };
 
+struct orion_direct_acc {
+	void __iomem		*vaddr;
+	u32			size;
+};
+
 struct orion_spi {
 	struct spi_master	*master;
 	void __iomem		*base;
 	struct clk              *clk;
 	const struct orion_spi_dev *devdata;
+
+	struct orion_direct_acc	direct_access[ORION_NUM_CHIPSELECTS];
 };
 
 static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg)
@@ -372,10 +383,44 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 {
 	unsigned int count;
 	int word_len;
+	struct orion_spi *orion_spi;
+	int cs = spi->chip_select;
 
 	word_len = spi->bits_per_word;
 	count = xfer->len;
 
+	orion_spi = spi_master_get_devdata(spi->master);
+
+	/* Use SPI direct access mode if base address is available */
+	if (orion_spi->direct_access[cs].vaddr) {
+		struct orion_direct_acc *da = &orion_spi->direct_access[cs];
+
+		if (count > da->size) {
+			dev_err(&spi->dev,
+				"max transfer size exceeded (%d > %d)\n",
+				count, da->size);
+			return 0;
+		}
+
+		if (xfer->tx_buf) {
+			/*
+			 * Send the tx-data to the SPI device via the direct
+			 * mapped address window
+			 */
+			memcpy(da->vaddr, xfer->tx_buf, count);
+		}
+
+		if (xfer->rx_buf) {
+			/*
+			 * Read the rx-data from the SPI device via the direct
+			 * mapped address window
+			 */
+			memcpy(xfer->rx_buf, da->vaddr, count);
+		}
+
+		return count;
+	}
+
 	if (word_len == 8) {
 		const u8 *tx = xfer->tx_buf;
 		u8 *rx = xfer->rx_buf;
@@ -425,6 +470,10 @@ static int orion_spi_reset(struct orion_spi *orion_spi)
 {
 	/* Verify that the CS is deasserted */
 	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG, 0x1);
+
+	/* Don't deassert CS between the direct mapped SPI transfers */
+	writel(0, spi_reg(orion_spi, SPI_DIRECT_WRITE_CONFIG_REG));
+
 	return 0;
 }
 
@@ -504,6 +553,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 	struct resource *r;
 	unsigned long tclk_hz;
 	int status = 0;
+	struct device_node *np;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*spi));
 	if (master == NULL) {
@@ -576,6 +626,42 @@ 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) {
+		u32 cs;
+
+		/* Get chip-select number from the "reg" property */
+		status = of_property_read_u32(np, "reg", &cs);
+		if (status) {
+			dev_err(&pdev->dev,
+				"%s has no valid 'reg' property (%d)\n",
+				np->full_name, status);
+			status = 0;
+			continue;
+		}
+
+		/*
+		 * Check if an address is configured for this SPI device. If
+		 * not, the MBus mapping via the 'ranges' property in the 'soc'
+		 * node is not configured and this device should not use the
+		 * direct mode. In this case, just continue with the next
+		 * device.
+		 */
+		status = of_address_to_resource(pdev->dev.of_node, cs + 1, r);
+		if (status)
+			continue;
+
+		spi->direct_access[cs].vaddr = devm_ioremap_resource(&pdev->dev,
+								     r);
+		if (IS_ERR(spi->direct_access[cs].vaddr)) {
+			status = PTR_ERR(spi->direct_access[cs].vaddr);
+			goto out_rel_clk;
+		}
+
+		spi->direct_access[cs].size = resource_size(r);
+		dev_info(&pdev->dev, "CS%d configured for direct access\n", cs);
+	}
+
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
Signature · 1 line
2.8.1
maintainer
#1 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 12 lines · Stefan Roese
drivers/spi/spi-orion.c +4 -0
@@ -372,10 +383,44 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 {
 	unsigned int count;
 	int word_len;
+	struct orion_spi *orion_spi;
+	int cs = spi->chip_select;
 
 	word_len = spi->bits_per_word;
 	count = xfer->len;
 
+	orion_spi = spi_master_get_devdata(spi->master);
+

drivers/spi/spi-orion.c orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) · line 403

Quoted 10 lines
drivers/spi/spi-orion.c +10 -0
+	/* Use SPI direct access mode if base address is available */
+	if (orion_spi->direct_access[cs].vaddr) {
+		struct orion_direct_acc *da = &orion_spi->direct_access[cs];
+
+		if (count > da->size) {
+			dev_err(&spi->dev,
+				"max transfer size exceeded (%d > %d)\n",
+				count, da->size);
+			return 0;
+		}

This seems obviously broken, we're neither returning an error nor falling back to PIO here but instead silently ignoring the transfer. If we can't fall back then I'd expect the driver to be setting max_transfer_size but it isn't, it's probably a good idea from a performance point of view to set it even if we can fall back.

Quoted 5 lines
drivers/spi/spi-orion.c +5 -0
+		if (xfer->tx_buf) {
+			/*
+			 * Send the tx-data to the SPI device via the direct
+			 * mapped address window
+			 */

drivers/spi/spi-orion.c orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) · line 419

Quoted 10 lines
drivers/spi/spi-orion.c +10 -0
+			memcpy(da->vaddr, xfer->tx_buf, count);
+		}
+
+		if (xfer->rx_buf) {
+			/*
+			 * Read the rx-data from the SPI device via the direct
+			 * mapped address window
+			 */
+			memcpy(xfer->rx_buf, da->vaddr, count);
+		}

Do bidirectional transfers work properly with this method (how much incoming data can we queue up, is there memory backing the whole region?), and are we guaranteed that the transfer finished by the time we return? I'm also not seeing anything here that handles word size so I suspect this will cause data corruption with anything other than 8 bit words and the driver does support 16 bit with PIO. If only 8 bit words can be supported with direct access I'd expect to see the bits_per_word_mask updated.

drivers/spi/spi-orion.c static int orion_spi_probe(struct platform_device *pdev) · line 663

Quoted 10 lines
drivers/spi/spi-orion.c +10 -0
+               spi->direct_access[cs].vaddr = devm_ioremap_resource(&pdev->dev,
+                                                                    r);
+               if (IS_ERR(spi->direct_access[cs].vaddr)) {
+                       status = PTR_ERR(spi->direct_access[cs].vaddr);
+                       goto out_rel_clk;
+               }
+
+               spi->direct_access[cs].size = resource_size(r);
+               dev_info(&pdev->dev, "CS%d configured for direct access\n", cs);
+       }

I seem to be missing where we configure the hardware to connect the
windows to the chip selects. We just seem to map the windows but never
otherwise interact with the hardware.
-------------- 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/20160411/a428120e/attachment.sig>

#2 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 12 lines · Mark Brown
Quoted 10 lines · Stefan Roese
drivers/spi/spi-orion.c +2 -0
@@ -372,10 +383,44 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
  {
  	unsigned int count;
  	int word_len;
+	struct orion_spi *orion_spi;
+	int cs = spi->chip_select;
 
  	word_len = spi->bits_per_word;
  	count = xfer->len;
 
Quoted 15 lines

drivers/spi/spi-orion.c orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) · line 403

Quoted 12 lines
drivers/spi/spi-orion.c +12 -0
+	orion_spi = spi_master_get_devdata(spi->master);
+
+	/* Use SPI direct access mode if base address is available */
+	if (orion_spi->direct_access[cs].vaddr) {
+		struct orion_direct_acc *da = &orion_spi->direct_access[cs];
+
+		if (count > da->size) {
+			dev_err(&spi->dev,
+				"max transfer size exceeded (%d > %d)\n",
+				count, da->size);
+			return 0;
+		}

This seems obviously broken, we're neither returning an error nor falling back to PIO here but instead silently ignoring the transfer.

Its not silently ignored (dev_err above) but I agree, we should be able to fall back to PIO mode in this condition. I'll change this in v4.

Quoted 4 lines

If
we can't fall back then I'd expect the driver to be setting max_transfer_size but it isn't, it's probably a good idea from a performance point of view to set it even if we can fall back.

Okay, I'll add max_transfer_size() to v4.

Quoted 12 lines
Quoted 12 lines
drivers/spi/spi-orion.c +12 -0
+		if (xfer->tx_buf) {
+			/*
+			 * Send the tx-data to the SPI device via the direct
+			 * mapped address window
+			 */
+			memcpy(da->vaddr, xfer->tx_buf, count);
+		}
+
+		if (xfer->rx_buf) {
+			/*
+			 * Read the rx-data from the SPI device via the direct
+			 * mapped address window
Quoted 8 lines

drivers/spi/spi-orion.c orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) · line 419

Quoted 3 lines
drivers/spi/spi-orion.c +3 -0
+			 */
+			memcpy(xfer->rx_buf, da->vaddr, count);
+		}

Do bidirectional transfers work properly with this method (how much incoming data can we queue up, is there memory backing the whole region?), and are we guaranteed that the transfer finished by the time we return?

Frankly, I have no idea if bidirectional transfers work properly. I have no means to test it. As stated in the commit text, this direct mode is only tested for TX as this is the mode that I'm using for the FPGA bitstream programming. So its perhaps best to remove the RX path completely from the direct mode for now. It can be added once someone has a board / platform that can make use of this direct RX mode.

Quoted 5 lines

I'm also not seeing anything here that handles word size so
I suspect this will cause data corruption with anything other than 8 bit words and the driver does support 16 bit with PIO. If only 8 bit words can be supported with direct access I'd expect to see the bits_per_word_mask updated.

'bits_per_word_mask' is controller specific. And one controller may have PIO SPI devices connected additionally to direct mode SPI devices. So restricting this controller to 8 bit in general doesn't seem to be a good idea to me. I can add a check for 16bit to the direct mode and fall back to PIO mode though.

Quoted 11 lines

drivers/spi/spi-orion.c static int orion_spi_probe(struct platform_device *pdev) · line 663

Quoted 10 lines
drivers/spi/spi-orion.c +10 -0
+               spi->direct_access[cs].vaddr = devm_ioremap_resource(&pdev->dev,
+                                                                    r);
+               if (IS_ERR(spi->direct_access[cs].vaddr)) {
+                       status = PTR_ERR(spi->direct_access[cs].vaddr);
+                       goto out_rel_clk;
+               }
+
+               spi->direct_access[cs].size = resource_size(r);
+               dev_info(&pdev->dev, "CS%d configured for direct access\n", cs);
+       }
Quoted 3 lines

I seem to be missing where we configure the hardware to connect the windows to the chip selects. We just seem to map the windows but never otherwise interact with the hardware.

The connection between the CS and the window is done in the DT. All is now implemented as Arnd has suggested:

On 29.03.2016 14:39, Arnd Bergmann wrote:
<snip>

Quoted 24 lines

What we really have though is additional registers that belong to the SPI controller and that are not part of internal-regs, so we need to move it up one level out of the internal-regs node in order to add more registers:

        soc {
                #address-cells = <2>;
                #size-cells = <1>;
               spi0: spi at 10600 {
			compatible = "marvell,armada-370-spi";
                       reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>, /* control */
			       <MBUS_ID(0x01, 0x1e) 0 0x100000>; /* CS0 */
			       <MBUS_ID(0x01, 0x5e) 0 0x100000>; /* CS1 */
			       <MBUS_ID(0x01, 0x9e) 0 0x100000>; /* CS2 */
			       <MBUS_ID(0x01, 0xee) 0 0x100000>; /* CS3 */
			       <MBUS_ID(0x01, 0x1f) 0 0x100000>; /* CS4 */
			       <MBUS_ID(0x01, 0x5f) 0 0x100000>; /* CS5 */
			       <MBUS_ID(0x01, 0x9f) 0 0x100000>; /* CS6 */
			       <MBUS_ID(0x01, 0xef) 0 0x100000>; /* CS7 */
                       #address-cells = <1>;
                       #size-cells = <0>;
		};
	};
Quoted 8 lines

This lists all the addresses as seen from the MBus, but doesn't put them into the CPU address space, which is then done by adding an entry in the 'ranges' property of the soc node:

	soc {
		ranges = <MBUS_ID(0x01, 0x1e) 0  0xe0000000  0x20000>; /* SPI 0 CS0 128kb */
		...
	};

I've Cc'ed you on the Armada dts patches as well. The only board currently enabling this direct mode via the 'ranges' property in the 'soc' node is an Armada XP based board that is still out-of-tree. I'll send the patches for it at some later time.

Thanks,
Stefan

maintainer
#3 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 13 lines · Stefan Roese
Quoted 11 lines · Mark Brown

drivers/spi/spi-orion.c orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) · line 403

Quoted 6 lines · Stefan Roese
drivers/spi/spi-orion.c +6 -0
+		if (count > da->size) {
+			dev_err(&spi->dev,
+				"max transfer size exceeded (%d > %d)\n",
+				count, da->size);
+			return 0;
+		}

This seems obviously broken, we're neither returning an error nor falling back to PIO here but instead silently ignoring the transfer.

Quoted 3 lines

Its not silently ignored (dev_err above) but I agree, we should be able to fall back to PIO mode in this condition. I'll change this in v4.

Logging is not useful error handling, software doesn't see it and will continue on potentially corrupting data.

Quoted 5 lines
Quoted 4 lines

Do bidirectional transfers work properly with this method (how much incoming data can we queue up, is there memory backing the whole region?), and are we guaranteed that the transfer finished by the time we return?

Quoted 7 lines

Frankly, I have no idea if bidirectional transfers work properly. I have no means to test it. As stated in the commit text, this direct mode is only tested for TX as this is the mode that I'm using for the FPGA bitstream programming. So its perhaps best to remove the RX path completely from the direct mode for now. It can be added once someone has a board / platform that can make use of this direct RX mode.

That seems a lot safer since if recieve doesn't work that's likely to upset things like flashes.

Quoted 10 lines
Quoted 7 lines

drivers/spi/spi-orion.c static int orion_spi_probe(struct platform_device *pdev) · line 656

Quoted 3 lines
drivers/spi/spi-orion.c +3 -0
+               spi->direct_access[cs].vaddr = devm_ioremap_resource(&pdev->dev,
+                                                                    r);
+               if (IS_ERR(spi->direct_access[cs].vaddr)) {

I seem to be missing where we configure the hardware to connect the windows to the chip selects. We just seem to map the windows but never otherwise interact with the hardware.

The connection between the CS and the window is done in the DT. All is now implemented as Arnd has suggested:

Writing something in the DT won't magically configure anything in the
hardware which is (as I said) the bit I'm missing.
-------------- 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/20160412/2896a43e/attachment.sig>

#4 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 3 lines · Mark Brown
Quoted 2 lines · Stefan Roese
Quoted 1 line · Mark Brown

On Thu, Apr 07, 2016 at 02:06:51PM +0200, Stefan Roese wrote:

<snip>

Quoted 8 lines
Quoted 7 lines
Quoted 7 lines

drivers/spi/spi-orion.c static int orion_spi_probe(struct platform_device *pdev) · line 656

Quoted 3 lines
drivers/spi/spi-orion.c +3 -0
+               spi->direct_access[cs].vaddr = devm_ioremap_resource(&pdev->dev,
+                                                                    r);
+               if (IS_ERR(spi->direct_access[cs].vaddr)) {

I seem to be missing where we configure the hardware to connect the windows to the chip selects. We just seem to map the windows but never otherwise interact with the hardware.

Quoted 5 lines
Quoted 2 lines

The connection between the CS and the window is done in the DT. All is now implemented as Arnd has suggested:

Writing something in the DT won't magically configure anything in the hardware which is (as I said) the bit I'm missing.

The MBus driver (drivers/bus/mvebu-mbus.c) takes care of the MBus window mapping. This is done, if the board dts files has one (or multiple) entries for the SPI device(s) added to the 'ranges' property of the 'soc' node.

This is the common method to map such windows. So no new code and no new bindings are needed for this to work.

Thanks,
Stefan

maintainer
#5 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 9 lines · Stefan Roese
Quoted 2 lines · Mark Brown

Writing something in the DT won't magically configure anything in the hardware which is (as I said) the bit I'm missing.

The MBus driver (drivers/bus/mvebu-mbus.c) takes care of the MBus window mapping. This is done, if the board dts files has one (or multiple) entries for the SPI device(s) added to the 'ranges' property of the 'soc' node.

I'm still unclear how mapping the windows in results in connecting the
windows to the SPI block. It also seems like we need to document what
the meaning of the reg properties of the device is, it's all very well
saying that there's a generic MBus binding but we still need to say how
the device will interpret the MBus windows that it has (assuming they
are configured to specific hardware functions transparently).
-------------- 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/20160413/999ec302/attachment.sig>

#6 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 6 lines · Mark Brown
Quoted 4 lines · Stefan Roese
Quoted 2 lines · Mark Brown

Writing something in the DT won't magically configure anything in the hardware which is (as I said) the bit I'm missing.

Quoted 7 lines
Quoted 4 lines

The MBus driver (drivers/bus/mvebu-mbus.c) takes care of the MBus window mapping. This is done, if the board dts files has one (or multiple) entries for the SPI device(s) added to the 'ranges' property of the 'soc' node.

I'm still unclear how mapping the windows in results in connecting the windows to the SPI block.

This is a hardware (SoC) specific mapping. For example the Armada XP lists in "Table 6: CPU Interface Mbus Decoding Units IDs and Attributes" of its Functional Specification Manual, that the SPI controller has the "Target Unit ID" 0x1. And that "Attributes[7:0]" need to get configured to these values for the SPI controller SPI chip-selects:

0x5E = SPI0 CS1 request
0x9E = SPI0 CS2 request
0xDE = SPI0 CS3 request
0x1F = SPI0 CS4 request
0x5F = SPI0 CS5 request
0x9F = SPI0 CS6 request
0xDF = SPI0 CS7 request
0x1A = SPI1 CS0 request
0x5A = SPI1 CS1 request
0x9A = SPI1 CS2 request
0xDA = SPI1 CS3 request
0x1B = SPI1 CS4 request
0x5B = SPI1 CS5 request
0x9B = SPI1 CS6 request
0xDB = SPI1 CS7 request

Because of this, I've added these values to the Armada XP-370 dtsi file:

spi0: spi at 10600 {
- reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>;
+ reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28 /* control */

+			       MBUS_ID(0x01, 0x1e) 0 0x100000	/* CS0 */
+			       MBUS_ID(0x01, 0x5e) 0 0x100000	/* CS1 */
+			       MBUS_ID(0x01, 0x9e) 0 0x100000	/* CS2 */
+			       MBUS_ID(0x01, 0xde) 0 0x100000	/* CS3 */
+			       MBUS_ID(0x01, 0x1f) 0 0x100000	/* CS4 */
+			       MBUS_ID(0x01, 0x5f) 0 0x100000	/* CS5 */
+			       MBUS_ID(0x01, 0x9f) 0 0x100000	/* CS6 */
+			       MBUS_ID(0x01, 0xdf) 0 0x100000>;	/* CS7 */

...

spi1: spi at 10680 {
- reg = <MBUS_ID(0xf0, 0x01) 0x10680 0x28>;
+ reg = <MBUS_ID(0xf0, 0x01) 0x10680 0x28 /* control */

+			       MBUS_ID(0x01, 0x1a) 0 0x100000	/* CS0 */
+			       MBUS_ID(0x01, 0x5a) 0 0x100000	/* CS1 */
+			       MBUS_ID(0x01, 0x9a) 0 0x100000	/* CS2 */
+			       MBUS_ID(0x01, 0xda) 0 0x100000	/* CS3 */
+			       MBUS_ID(0x01, 0x1b) 0 0x100000	/* CS4 */
+			       MBUS_ID(0x01, 0x5b) 0 0x100000	/* CS5 */
+			       MBUS_ID(0x01, 0x9b) 0 0x100000	/* CS6 */
+			       MBUS_ID(0x01, 0xdb) 0 0x100000>;	/* CS7 */

The first value of the MBUS_ID macro representing the 'target' from the manual and the 2nd one the 'attribute'. These 'target' and 'attribute' values may vary between different Marvell SoCs.

Please take a look at Documentation/devicetree/bindings/bus/mvebu-mbus.txt as this describes the mbus driver and its bindings and usage quite good.

Quoted 5 lines

It also seems like we need to document what
the meaning of the reg properties of the device is, it's all very well saying that there's a generic MBus binding but we still need to say how the device will interpret the MBus windows that it has (assuming they are configured to specific hardware functions transparently).

Should I add something to the bindings documentation of the orion-spi driver? With an example of how this direct mode can is enabled on a specific board for e.g. SPI0-DEV1? Or what do you think is missing in the mbus bindings documentation I mentioned above?

Thanks,
Stefan

maintainer
#7 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

Quoted 8 lines · Stefan Roese
Quoted 5 lines · Mark Brown

It also seems like we need to document what
the meaning of the reg properties of the device is, it's all very well saying that there's a generic MBus binding but we still need to say how the device will interpret the MBus windows that it has (assuming they are configured to specific hardware functions transparently).

Quoted 5 lines

Should I add something to the bindings documentation of the orion-spi driver? With an example of how this direct mode can is enabled on a specific board for e.g. SPI0-DEV1? Or what do you think is missing in the mbus bindings documentation I mentioned above?

I think the biggest thing that's missing is a reference from the Orion
binding to the MBus binding that the reader can follow (probably added
to the reg property documentation). I did look there and couldn't see
anything about how this would 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/20160413/9731a27a/attachment.sig>

#8 lore raw

PATCH v3 1/1 spi: orion.c: Add direct access mode

In reply to Stefan Roese
Quoted 8 lines · On Thursday 07 April 2016 14:06:51 Stefan Roese wrote:

v3:
- Used static MBus windows again, as suggested by Arnd
- No new DT bindings needed for this. The MBus windows need to be configured in the 'reg' property of the SPI controller. This is done in a separate patch for the Armada platforms.
- The direct mapping is configured and enable by adding the specific MBus entry to the 'ranges' property of the 'soc' node in the board dts file.

The new approach looks good to me, thanks!

As Mark pointed out, you still need to document the fact that the SPI controller can now have additional entries in its 'reg' property, and it would be nice to reference the mbus binding from the spi controller binding as a help for people that want to enable the direct mode.

Arnd