| Message ID | 475b63c52b17187089d3f2ecf8ff4b8c45a7e58b.1518704854.git.jan.kundrat@cesnet.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 10, 2018 at 01:25:32PM +0100, Jan Kundrát wrote: > Add a SW-controlled timer for inserting delays between individual words > as transmitted over the SPI bus. The DT property name is loosely > modelled after a similar, but HW-based feature in spi-davinci.c (commit > 365a7bb32e09) -- hence the DT property name. I think this is a sensible and reasonable thing to want to do however I think we should move this from a DT property to being something in the transfer structure which drivers then implement. That way if a device has an inter-word delay requirement it can go in the individual device driver so it always gets applied. Does that make sense to you?
On čtvrtek 15. února 2018 16:19:07 CET, Mark Brown wrote: > I think this is a sensible and reasonable thing to want to do however I > think we should move this from a DT property to being something in the > transfer structure which drivers then implement. That way if a device > has an inter-word delay requirement it can go in the individual device > driver so it always gets applied. Does that make sense to you? Yep, that indeed makes sense (Damn, my easy attempt didn't make it :).) How should the interface look like? Is spi_controller->mode_bits a good place for controllers to advertise this feature (SPI_WORD_DELAY, perhaps?) as supported, with spi-orion being the only implementation now? If we allow each spi_transfer to override this value, is there a common place in the SPI core which somehow validates a spi_transfer against each controller's capabilities? I see a code like that (bad_bits, ugly_bits) in spi_setup which looks like something to be called per-device, not per-transfer. My most important use case is, however, the userspace-facing spidev, and its ioctl complicates stuff a bit. The relevant struct has a 16bit padding now (used to be 32bit prior to https://patchwork.kernel.org/patch/3715391/). Is it OK to use this padding for this feature? Or should I perhaps eat just 8bits and limit the delays to an arbitrary value of 255us? Or should I not care about that now and let somebody else come up with another "bigger" ioctl when they need that space? With kind regards, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 15, 2018 at 04:45:55PM +0100, Jan Kundrát wrote: > How should the interface look like? Is spi_controller->mode_bits a good > place for controllers to advertise this feature (SPI_WORD_DELAY, perhaps?) > as supported, with spi-orion being the only implementation now? If we allow Yup. > each spi_transfer to override this value, is there a common place in the SPI > core which somehow validates a spi_transfer against each controller's > capabilities? I see a code like that (bad_bits, ugly_bits) in spi_setup > which looks like something to be called per-device, not per-transfer. __spi_validate(). > My most important use case is, however, the userspace-facing spidev, and its > ioctl complicates stuff a bit. The relevant struct has a 16bit padding now > (used to be 32bit prior to https://patchwork.kernel.org/patch/3715391/). Is > it OK to use this padding for this feature? Or should I perhaps eat just > 8bits and limit the delays to an arbitrary value of 255us? Or should I not > care about that now and let somebody else come up with another "bigger" > ioctl when they need that space? Ugh. Using the whole padding is probably OK.
T24gVGh1LCAyMDE4LTAyLTE1IGF0IDE1OjQ5ICswMDAwLCBNYXJrIEJyb3duIHdyb3RlOg0KPiBP biBUaHUsIEZlYiAxNSwgMjAxOCBhdCAwNDo0NTo1NVBNICswMTAwLCBKYW4gS3VuZHLDoXQgd3Jv dGU6DQo+IA0KPiA+IE15IG1vc3QgaW1wb3J0YW50IHVzZSBjYXNlIGlzLCBob3dldmVyLCB0aGUg dXNlcnNwYWNlLWZhY2luZyBzcGlkZXYsIGFuZCBpdHMNCj4gPiBpb2N0bCBjb21wbGljYXRlcyBz dHVmZiBhIGJpdC4gVGhlIHJlbGV2YW50IHN0cnVjdCBoYXMgYSAxNmJpdCBwYWRkaW5nIG5vdw0K PiA+ICh1c2VkIHRvIGJlIDMyYml0IHByaW9yIHRvIGh0dHBzOi8vcGF0Y2h3b3JrLmtlcm5lbC5v cmcvcGF0Y2gvMzcxNTM5MS8pLiBJcw0KPiA+IGl0IE9LIHRvIHVzZSB0aGlzIHBhZGRpbmcgZm9y IHRoaXMgZmVhdHVyZT8gT3Igc2hvdWxkIEkgcGVyaGFwcyBlYXQganVzdA0KPiA+IDhiaXRzIGFu ZCBsaW1pdCB0aGUgZGVsYXlzIHRvIGFuIGFyYml0cmFyeSB2YWx1ZSBvZiAyNTV1cz8gT3Igc2hv dWxkIEkgbm90DQo+ID4gY2FyZSBhYm91dCB0aGF0IG5vdyBhbmQgbGV0IHNvbWVib2R5IGVsc2Ug Y29tZSB1cCB3aXRoIGFub3RoZXIgImJpZ2dlciINCj4gPiBpb2N0bCB3aGVuIHRoZXkgbmVlZCB0 aGF0IHNwYWNlPw0KPiANCj4gVWdoLiAgVXNpbmcgdGhlIHdob2xlIHBhZGRpbmcgaXMgcHJvYmFi bHkgT0suDQoNClRoZXJlIGNvdWxkIGJlIGEgZmxhZyB0aGF0IHN3aXRjaGVzIGRlbGF5X3VzIGZy b20gYmVpbmcgYSBwb3N0IHhmZXINCmRlbGF5IHRvIGJlaW5nIGEgcG9zdCB3b3JkIGRlbGF5LiBU aGVuIG5vIG5lZWQgZmllbGQgbmVlZHMgdG8gYmUgYWRkZWQuDQogSWYgc29tZW9uZSB3YW50cyBi b3RoIGEgcG9zdCB3b3JkIGFuZCBwb3N0IHhmZXIgZGVsYXkgaW4gdGhlIHNhbWUNCnhmZXIsIHRo ZW4gdGhleSdsbCBqdXN0IG5lZWQgdG8gc3BsaXQgaXQsIHdoaWNoIHNob3VsZCBiZSBwb3NzaWJs ZQ0Kc2luY2UgaXQgbXVzdCBiZSBsb25nZXIgdGhhbiBvbmUgd29yZCB0byBuZWVkIGEgZGVsYXkg YWZ0ZXIgZWFjaCB3b3JkLg0KDQpOb3QgdGhhdCBncmVhdCBlaXRoZXIu -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 15, 2018 at 05:18:08PM +0000, Trent Piepho wrote: > On Thu, 2018-02-15 at 15:49 +0000, Mark Brown wrote: > > Ugh. Using the whole padding is probably OK. > There could be a flag that switches delay_us from being a post xfer > delay to being a post word delay. Then no need field needs to be added. > If someone wants both a post word and post xfer delay in the same > xfer, then they'll just need to split it, which should be possible > since it must be longer than one word to need a delay after each word. > Not that great either. Yeah, that'd work too but isn't a model of elegance either. I wonder if just making a new ioctl() might not be better... not ideal either but probably cleaner.
diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt index 8434a65fc12a..ed35cba1bc33 100644 --- a/Documentation/devicetree/bindings/spi/spi-orion.txt +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt @@ -29,6 +29,10 @@ Optional properties: used, the name must be "core", and "axi" (the latter is only for Armada 7K/8K). +Optional properties of child nodes (SPI slave devices): +- linux,spi-wdelay : If present and non-zero, specifies a delay in + microseconds between words transferred over the SPI bus. + Example: spi@10600 { @@ -77,3 +81,19 @@ are used in the default indirect (PIO) mode): For further information on the MBus bindings, please see the MBus DT documentation: Documentation/devicetree/bindings/bus/mvebu-mbus.txt + +Example of a per-child inter-word delay: + + spi0: spi@10600 { + /* ... */ + + some_slave_device@2 { + reg = <2>; + compatible = "something"; + /* ... */ + + /* Wait 3 microseconds between all words within all + SPI transactions */ + linux,spi-wdelay = /bits/ 16 <3>; + }; + }; diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index 1eccc2287079..1a9475857808 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -92,6 +92,7 @@ struct orion_direct_acc { struct orion_child_options { struct orion_direct_acc direct_access; + u16 word_delay; }; struct orion_spi { @@ -469,6 +470,8 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) if (orion_spi_write_read_8bit(spi, &tx, &rx) < 0) goto out; count--; + if (orion_spi->child[cs].word_delay) + udelay(orion_spi->child[cs].word_delay); } while (count); } else if (word_len == 16) { const u16 *tx = xfer->tx_buf; @@ -477,6 +480,8 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer) do { if (orion_spi_write_read_16bit(spi, &tx, &rx) < 0) goto out; + if (orion_spi->child[cs].word_delay) + udelay(orion_spi->child[cs].word_delay); count -= 2; } while (count); } @@ -681,7 +686,7 @@ static int orion_spi_probe(struct platform_device *pdev) goto out_rel_axi_clk; } - /* Scan all SPI devices of this controller for direct mapped devices */ + /* Scan all SPI devices of this controller for direct mapped devices and word delay */ for_each_available_child_of_node(pdev->dev.of_node, np) { u32 cs; @@ -694,6 +699,12 @@ static int orion_spi_probe(struct platform_device *pdev) continue; } + spi->child[cs].word_delay = 0; + if (!of_property_read_u16(np, "linux,spi-wdelay", + &spi->child[cs].word_delay)) + dev_info(&pdev->dev, "%pOF: %dus delay between words\n", + np, spi->child[cs].word_delay); + /* * Check if an address is configured for this SPI device. If * not, the MBus mapping via the 'ranges' property in the 'soc' @@ -705,6 +716,12 @@ static int orion_spi_probe(struct platform_device *pdev) if (status) continue; + if (spi->child[cs].word_delay) { + dev_warn(&pdev->dev, + "%pOF linux,spi-wdelay takes preference over a direct-mode", np); + continue; + } + /* * Only map one page for direct access. This is enough for the * simple TX transfer which only writes to the first word.
Add a SW-controlled timer for inserting delays between individual words as transmitted over the SPI bus. The DT property name is loosely modelled after a similar, but HW-based feature in spi-davinci.c (commit 365a7bb32e09) -- hence the DT property name. My HW sucks. One of the less-serious troubles is that it requires a 3us delay between all SPI words. It also requires "big" transfers, easily around 40kB, which is 20k of 16-bit words. It's also a specialised, proprietary thing with no need for an in-kernel driver, so I'm using spidev to access it. There's a limit in spidev's SPI_IOC_MESSAGE and ioctl architecture in general which means that I can stash at most 512 individual spi_ioc_transfer into one ioctl. When I needed to transfer small pockets, that was enough -- I could simply build a series of hundreds of spi_ioc_trnasfers, two bytes each, with a proper delay_usecs to persuade the SPI core to implement these delays for me. However, this won't work if I need to send more than 1kB of data that way. It seems that there's nothing generic in Linux to implement this feature. The TI's spi-davinci.c can do something liek this in HW. People at various forums apparently want to do something similar on Zynq and on RPis, perhaps in SW. I wasn't able to find any ready-made patches, though. My SoC (88F68xx, Marvell Armada A38x) apparently cannot do this natively, so this patch simply adds a call to udelay which does the trick for me. Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz> --- Documentation/devicetree/bindings/spi/spi-orion.txt | 20 ++++++++++++++++++++ drivers/spi/spi-orion.c | 19 ++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-)