| Message ID | 1460030811-13787-1-git-send-email-sr@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 07, 2016 at 02:06:51PM +0200, Stefan Roese wrote: > @@ -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; > + } 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. > + 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); > + } 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. > + 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.
On 11.04.2016 12:57, Mark Brown wrote: > On Thu, Apr 07, 2016 at 02:06:51PM +0200, Stefan Roese wrote: > >> @@ -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; >> + } > > 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. > 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. >> + 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); >> + } > > 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. > 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. >> + 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. 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> > 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@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>; > }; > }; > > 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
On Tue, Apr 12, 2016 at 01:43:27PM +0200, Stefan Roese wrote: > On 11.04.2016 12:57, Mark Brown wrote: > > On Thu, Apr 07, 2016 at 02:06:51PM +0200, Stefan Roese wrote: > >> + 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. Logging is not useful error handling, software doesn't see it and will continue on potentially corrupting data. > > 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. That seems a lot safer since if recieve doesn't work that's likely to upset things like flashes. > >> + 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.
On 12.04.2016 21:27, Mark Brown wrote: > On Tue, Apr 12, 2016 at 01:43:27PM +0200, Stefan Roese wrote: >> On 11.04.2016 12:57, Mark Brown wrote: >>> On Thu, Apr 07, 2016 at 02:06:51PM +0200, Stefan Roese wrote: <snip> >>>> + 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. 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
On Wed, Apr 13, 2016 at 07:30:15AM +0200, Stefan Roese wrote: > On 12.04.2016 21:27, Mark Brown wrote: > >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).
On 13.04.2016 07:59, Mark Brown wrote: > On Wed, Apr 13, 2016 at 07:30:15AM +0200, Stefan Roese wrote: >> On 12.04.2016 21:27, Mark Brown wrote: > >>> 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. 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@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@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. > 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
On Wed, Apr 13, 2016 at 01:40:00PM +0200, Stefan Roese wrote: > On 13.04.2016 07:59, Mark Brown wrote: > > 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? 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.
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
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index a87cfd4..69e710e 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) @@ -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);
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. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Mark Brown <broonie@kernel.org> --- 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(+)