* [PATCH 0/2] spi: support inter-word delays @ 2019-01-25 11:44 Jonas Bonn 2019-01-25 11:44 ` [PATCH 1/2] spi: support inter-word delay requirement for devices Jonas Bonn 2019-01-25 11:44 ` [PATCH 2/2] spi-atmel: support inter-word delay Jonas Bonn 0 siblings, 2 replies; 15+ messages in thread From: Jonas Bonn @ 2019-01-25 11:44 UTC (permalink / raw) To: linux-kernel, linux-spi; +Cc: Jonas Bonn This short series adds support for SPI inter-word delays and configures the spi-atmel driver to honour the setting. Some SPI slaves are so slow that they are unable to keep up even at the SPI controller's lowest available clock frequency. I have such a configuration where an AVR-based SPI slave is unable to feed the SPI bus fast enough even the SPI master runs at the lowest possible clock speed. Jonas Bonn (2): spi: support inter-word delay requirement for devices spi-atmel: support inter-word delay Documentation/devicetree/bindings/spi/spi-bus.txt | 1 + drivers/spi/spi-atmel.c | 11 ++++++----- drivers/spi/spi.c | 4 ++++ include/linux/spi/spi.h | 1 + 4 files changed, 12 insertions(+), 5 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 11:44 [PATCH 0/2] spi: support inter-word delays Jonas Bonn @ 2019-01-25 11:44 ` Jonas Bonn 2019-01-25 11:53 ` Baolin Wang 2019-01-25 11:44 ` [PATCH 2/2] spi-atmel: support inter-word delay Jonas Bonn 1 sibling, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-25 11:44 UTC (permalink / raw) To: linux-kernel, linux-spi Cc: Jonas Bonn, Mark Brown, Rob Herring, Mark Rutland, devicetree Some devices are slow and cannot keep up with the SPI bus and therefore require a short delay between words of the SPI transfer. The example of this that I'm looking at is a SAMA5D2 with a minimum SPI clock of 400kHz talking to an AVR-based SPI slave. The AVR cannot put bytes on the bus fast enough to keep up with the SoC's SPI controller even at the lowest bus speed. This patch introduces the ability to specify a required inter-word delay for SPI devices. It is up to the controller driver to configure itself accordingly in order to introduce the requested delay. Signed-off-by: Jonas Bonn <jonas@norrbonn.se> CC: Mark Brown <broonie@kernel.org> CC: Rob Herring <robh+dt@kernel.org> CC: Mark Rutland <mark.rutland@arm.com> CC: linux-spi@vger.kernel.org CC: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/spi/spi-bus.txt | 1 + drivers/spi/spi.c | 4 ++++ include/linux/spi/spi.h | 1 + 3 files changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt index 1f6e86f787ef..a5f20060676d 100644 --- a/Documentation/devicetree/bindings/spi/spi-bus.txt +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt @@ -77,6 +77,7 @@ All slave nodes can contain the following optional properties: Defaults to 1 if not present. - spi-rx-delay-us - Microsecond delay after a read transfer. - spi-tx-delay-us - Microsecond delay after a write transfer. +- spi-word-delay-us - Microsecond delay between individual words of a transfer Some SPI controllers and devices support Dual and Quad SPI transfer mode. It allows data in the SPI system to be transferred using 2 wires (DUAL) or 4 diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 9a7def7c3237..cd4d4065eca2 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1692,6 +1692,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, } spi->max_speed_hz = value; + if (!of_property_read_u32(nc, "spi-word-delay-us", &value)) { + spi->word_delay = value; + } + return 0; } diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 314d922ca607..e5200dd9d750 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -164,6 +164,7 @@ struct spi_device { char modalias[SPI_NAME_SIZE]; const char *driver_override; int cs_gpio; /* chip select gpio */ + uint16_t word_delay; /* inter-word delay (us) */ /* the statistics */ struct spi_statistics statistics; -- 2.19.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 11:44 ` [PATCH 1/2] spi: support inter-word delay requirement for devices Jonas Bonn @ 2019-01-25 11:53 ` Baolin Wang 2019-01-25 12:06 ` Jonas Bonn 0 siblings, 1 reply; 15+ messages in thread From: Baolin Wang @ 2019-01-25 11:53 UTC (permalink / raw) To: Jonas Bonn; +Cc: LKML, linux-spi, Mark Brown, Rob Herring, Mark Rutland, DTML Hi, On Fri, 25 Jan 2019 at 19:44, Jonas Bonn <jonas@norrbonn.se> wrote: > > Some devices are slow and cannot keep up with the SPI bus and therefore > require a short delay between words of the SPI transfer. > > The example of this that I'm looking at is a SAMA5D2 with a minimum SPI > clock of 400kHz talking to an AVR-based SPI slave. The AVR cannot put > bytes on the bus fast enough to keep up with the SoC's SPI controller > even at the lowest bus speed. > > This patch introduces the ability to specify a required inter-word > delay for SPI devices. It is up to the controller driver to configure > itself accordingly in order to introduce the requested delay. Can we configure it at runtime by the device rather than at DT time by the controller? If yes, we already have a patch for this, please check: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eeaceb8b7d1fb64b6030249ca0dd1d902ef3069e > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > CC: Mark Brown <broonie@kernel.org> > CC: Rob Herring <robh+dt@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > CC: linux-spi@vger.kernel.org > CC: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/spi/spi-bus.txt | 1 + > drivers/spi/spi.c | 4 ++++ > include/linux/spi/spi.h | 1 + > 3 files changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt > index 1f6e86f787ef..a5f20060676d 100644 > --- a/Documentation/devicetree/bindings/spi/spi-bus.txt > +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt > @@ -77,6 +77,7 @@ All slave nodes can contain the following optional properties: > Defaults to 1 if not present. > - spi-rx-delay-us - Microsecond delay after a read transfer. > - spi-tx-delay-us - Microsecond delay after a write transfer. > +- spi-word-delay-us - Microsecond delay between individual words of a transfer > > Some SPI controllers and devices support Dual and Quad SPI transfer mode. > It allows data in the SPI system to be transferred using 2 wires (DUAL) or 4 > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 9a7def7c3237..cd4d4065eca2 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1692,6 +1692,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > } > spi->max_speed_hz = value; > > + if (!of_property_read_u32(nc, "spi-word-delay-us", &value)) { > + spi->word_delay = value; > + } > + > return 0; > } > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 314d922ca607..e5200dd9d750 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -164,6 +164,7 @@ struct spi_device { > char modalias[SPI_NAME_SIZE]; > const char *driver_override; > int cs_gpio; /* chip select gpio */ > + uint16_t word_delay; /* inter-word delay (us) */ > > /* the statistics */ > struct spi_statistics statistics; > -- > 2.19.1 > -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 11:53 ` Baolin Wang @ 2019-01-25 12:06 ` Jonas Bonn 2019-01-25 17:47 ` Mark Brown 0 siblings, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-25 12:06 UTC (permalink / raw) To: Baolin Wang; +Cc: LKML, linux-spi, Mark Brown, Rob Herring, Mark Rutland, DTML Hi, On 25/01/2019 12:53, Baolin Wang wrote: > Hi, > On Fri, 25 Jan 2019 at 19:44, Jonas Bonn <jonas@norrbonn.se> wrote: >> >> Some devices are slow and cannot keep up with the SPI bus and therefore >> require a short delay between words of the SPI transfer. >> >> The example of this that I'm looking at is a SAMA5D2 with a minimum SPI >> clock of 400kHz talking to an AVR-based SPI slave. The AVR cannot put >> bytes on the bus fast enough to keep up with the SoC's SPI controller >> even at the lowest bus speed. >> >> This patch introduces the ability to specify a required inter-word >> delay for SPI devices. It is up to the controller driver to configure >> itself accordingly in order to introduce the requested delay. > > Can we configure it at runtime by the device rather than at DT time by > the controller? If yes, we already have a patch for this, please > check: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eeaceb8b7d1fb64b6030249ca0dd1d902ef3069e > It's a characteristic of the SPI slave, in the same sense as CPOL/CPHA are, and therefore it makes sense to specify it in the device tree. Having this as device property rather than a transfer property allows this to be configured one time in setup() rather than having to fiddle with the configuration register for every transfer. The two approaches are complementary. /Jonas >> >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >> CC: Mark Brown <broonie@kernel.org> >> CC: Rob Herring <robh+dt@kernel.org> >> CC: Mark Rutland <mark.rutland@arm.com> >> CC: linux-spi@vger.kernel.org >> CC: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/spi/spi-bus.txt | 1 + >> drivers/spi/spi.c | 4 ++++ >> include/linux/spi/spi.h | 1 + >> 3 files changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt >> index 1f6e86f787ef..a5f20060676d 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt >> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt >> @@ -77,6 +77,7 @@ All slave nodes can contain the following optional properties: >> Defaults to 1 if not present. >> - spi-rx-delay-us - Microsecond delay after a read transfer. >> - spi-tx-delay-us - Microsecond delay after a write transfer. >> +- spi-word-delay-us - Microsecond delay between individual words of a transfer >> >> Some SPI controllers and devices support Dual and Quad SPI transfer mode. >> It allows data in the SPI system to be transferred using 2 wires (DUAL) or 4 >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c >> index 9a7def7c3237..cd4d4065eca2 100644 >> --- a/drivers/spi/spi.c >> +++ b/drivers/spi/spi.c >> @@ -1692,6 +1692,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, >> } >> spi->max_speed_hz = value; >> >> + if (!of_property_read_u32(nc, "spi-word-delay-us", &value)) { >> + spi->word_delay = value; >> + } >> + >> return 0; >> } >> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h >> index 314d922ca607..e5200dd9d750 100644 >> --- a/include/linux/spi/spi.h >> +++ b/include/linux/spi/spi.h >> @@ -164,6 +164,7 @@ struct spi_device { >> char modalias[SPI_NAME_SIZE]; >> const char *driver_override; >> int cs_gpio; /* chip select gpio */ >> + uint16_t word_delay; /* inter-word delay (us) */ >> >> /* the statistics */ >> struct spi_statistics statistics; >> -- >> 2.19.1 >> > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 12:06 ` Jonas Bonn @ 2019-01-25 17:47 ` Mark Brown 2019-01-25 17:50 ` Mark Brown 0 siblings, 1 reply; 15+ messages in thread From: Mark Brown @ 2019-01-25 17:47 UTC (permalink / raw) To: Jonas Bonn; +Cc: Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML [-- Attachment #1: Type: text/plain, Size: 1204 bytes --] On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: > On 25/01/2019 12:53, Baolin Wang wrote: > > On Fri, 25 Jan 2019 at 19:44, Jonas Bonn <jonas@norrbonn.se> wrote: > > Can we configure it at runtime by the device rather than at DT time by > > the controller? If yes, we already have a patch for this, please > > check: > It's a characteristic of the SPI slave, in the same sense as CPOL/CPHA are, > and therefore it makes sense to specify it in the device tree. No, that doesn't follow at all. There's two bits here - where the configuration gets done and the mechanism by which it gets done. If something in DT is completely orthogonal to which device it is a property of. > Having this as device property rather than a transfer property allows this > to be configured one time in setup() rather than having to fiddle with the > configuration register for every transfer. That doesn't mean that the coniguration should be done in DT though, and given that this presumably is a property of the device there seems to be no reason why we'd have it in DT - if every instance of the device is going to need to set the property we should just figure it out from the compatble string instead. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 17:47 ` Mark Brown @ 2019-01-25 17:50 ` Mark Brown 2019-01-26 7:52 ` Jonas Bonn 0 siblings, 1 reply; 15+ messages in thread From: Mark Brown @ 2019-01-25 17:50 UTC (permalink / raw) To: Jonas Bonn; +Cc: Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML [-- Attachment #1: Type: text/plain, Size: 819 bytes --] On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote: > On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: > > Having this as device property rather than a transfer property allows this > > to be configured one time in setup() rather than having to fiddle with the > > configuration register for every transfer. > That doesn't mean that the coniguration should be done in DT though, and > given that this presumably is a property of the device there seems to be > no reason why we'd have it in DT - if every instance of the device is > going to need to set the property we should just figure it out from the > compatble string instead. To be clear here: the suggestion is to add a parameter the slave device can set in spi_device which sets the default word_delay similarly to how max_speed_hz works. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-25 17:50 ` Mark Brown @ 2019-01-26 7:52 ` Jonas Bonn 2019-01-26 10:25 ` Geert Uytterhoeven 0 siblings, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-26 7:52 UTC (permalink / raw) To: Mark Brown; +Cc: Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML On 25/01/2019 18:50, Mark Brown wrote: > On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote: >> On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: > >>> Having this as device property rather than a transfer property allows this >>> to be configured one time in setup() rather than having to fiddle with the >>> configuration register for every transfer. > >> That doesn't mean that the coniguration should be done in DT though, and >> given that this presumably is a property of the device there seems to be >> no reason why we'd have it in DT - if every instance of the device is >> going to need to set the property we should just figure it out from the >> compatble string instead. > > To be clear here: the suggestion is to add a parameter the slave device > can set in spi_device which sets the default word_delay similarly to how > max_speed_hz works. > I'm confused... isn't that exactly what this patch does? It adds a field word_delay to spi_device in the same manner as max_speed_hz. I also added the ability to set it via DT, which I can break out into a separate patch if that's an issue. Or is the problem that it's set via DT, at all? Documentation/devicetree/bindings/spi-bus.txt documents 10 other slave-node properties related to transfer characteristics; word_delay is just another such characteristic. But again, I'm having trouble parsing your response Is the patch wrong, should be broken up, or you just misunderstood it? Thanks, /Jonas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-26 7:52 ` Jonas Bonn @ 2019-01-26 10:25 ` Geert Uytterhoeven 2019-01-26 15:40 ` Jonas Bonn 0 siblings, 1 reply; 15+ messages in thread From: Geert Uytterhoeven @ 2019-01-26 10:25 UTC (permalink / raw) To: Jonas Bonn Cc: Mark Brown, Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML Hi Jonas, On Sat, Jan 26, 2019 at 8:53 AM Jonas Bonn <jonas@norrbonn.se> wrote: > On 25/01/2019 18:50, Mark Brown wrote: > > On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote: > >> On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: > >>> Having this as device property rather than a transfer property allows this > >>> to be configured one time in setup() rather than having to fiddle with the > >>> configuration register for every transfer. > > > >> That doesn't mean that the coniguration should be done in DT though, and > >> given that this presumably is a property of the device there seems to be > >> no reason why we'd have it in DT - if every instance of the device is > >> going to need to set the property we should just figure it out from the > >> compatble string instead. > > > > To be clear here: the suggestion is to add a parameter the slave device > > can set in spi_device which sets the default word_delay similarly to how > > max_speed_hz works. > > I'm confused... isn't that exactly what this patch does? It adds a > field word_delay to spi_device in the same manner as max_speed_hz. > > I also added the ability to set it via DT, which I can break out into a > separate patch if that's an issue. Or is the problem that it's set via > DT, at all? Documentation/devicetree/bindings/spi-bus.txt documents 10 > other slave-node properties related to transfer characteristics; > word_delay is just another such characteristic. > > But again, I'm having trouble parsing your response Is the patch wrong, > should be broken up, or you just misunderstood it? IIUIC, Mark means that it may be a non-configurable property of the slave device, and thus should be handled (fixed setting) in the SPI slave driver. Compare this to CPHA/CPOL, which are properties of the SPI slave device, but which may be configurable. E.g. many SPI FLASHes support multiple configurations. See e.g. commit 9c5becce21af35e5 ("ARM: shmobile: koelsch: Fix QSPI mode of SPI-Flash into mode3"). Again, max_speed_hz is something different: while both the SPI master and slave may support high speeds, board wiring (capacitance/inductance) may need to force a slower speed than supported by the devices, so it makes sense to make that configurable from DT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-26 10:25 ` Geert Uytterhoeven @ 2019-01-26 15:40 ` Jonas Bonn 2019-01-28 7:41 ` Geert Uytterhoeven 0 siblings, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-26 15:40 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Mark Brown, Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML Hi Geert, On 26/01/2019 11:25, Geert Uytterhoeven wrote: > Hi Jonas, > > On Sat, Jan 26, 2019 at 8:53 AM Jonas Bonn <jonas@norrbonn.se> wrote: >> On 25/01/2019 18:50, Mark Brown wrote: >>> On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote: >>>> On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: >>>>> Having this as device property rather than a transfer property allows this >>>>> to be configured one time in setup() rather than having to fiddle with the >>>>> configuration register for every transfer. >>> >>>> That doesn't mean that the coniguration should be done in DT though, and >>>> given that this presumably is a property of the device there seems to be >>>> no reason why we'd have it in DT - if every instance of the device is >>>> going to need to set the property we should just figure it out from the >>>> compatble string instead. >>> >>> To be clear here: the suggestion is to add a parameter the slave device >>> can set in spi_device which sets the default word_delay similarly to how >>> max_speed_hz works. >> >> I'm confused... isn't that exactly what this patch does? It adds a >> field word_delay to spi_device in the same manner as max_speed_hz. >> >> I also added the ability to set it via DT, which I can break out into a >> separate patch if that's an issue. Or is the problem that it's set via >> DT, at all? Documentation/devicetree/bindings/spi-bus.txt documents 10 >> other slave-node properties related to transfer characteristics; >> word_delay is just another such characteristic. >> >> But again, I'm having trouble parsing your response Is the patch wrong, >> should be broken up, or you just misunderstood it? > > IIUIC, Mark means that it may be a non-configurable property of the slave > device, and thus should be handled (fixed setting) in the SPI slave driver. OK, so given that the "compatible" property identifies the hardware and there is no other _hardware_ configuration detail that affects the required inter-word delay, then setting the property in the device tree is not allowed as the driver has enough info to set it properly. Fair enough! > > Compare this to CPHA/CPOL, which are properties of the SPI slave device, > but which may be configurable. E.g. many SPI FLASHes support multiple > configurations. See e.g. commit 9c5becce21af35e5 ("ARM: shmobile: koelsch: > Fix QSPI mode of SPI-Flash into mode3"). There is nothing about the hardware referenced in that patch that enforces either mode. Why does the driver get to defer to DT here? Looking at Documentation/devicetree/bindings/spi/spi-bus.txt: spi-lsb-first: used only by MAXIM DS-1302 which is always LSB first; driver should set this spi-3wire: again, only set by MAXIM DS-1302 which always needs this setting; driver could set this spi-tx-delay-us: spi-rx-delay-us: only parsed by RMI4 driver... no DTS files in kernel set these. I hardly see how these are "hardware" settings rather than message settings, but the driver can set these in any case. Anyway, the point is, looking at the current documentation, it's pretty difficult to understand whether devicetree is restricted to describing hardware or is also for configuring drivers... > > Again, max_speed_hz is something different: while both the SPI master and > slave may support high speeds, board wiring (capacitance/inductance) may > need to force a slower speed than supported by the devices, so it makes > sense to make that configurable from DT. Yeah, that one make sense. But if the DT is only overriding the maximum device frequency that the driver should otherwise be setting, why is spi-max-frequency a _required_ property? Thanks for taking the time to provide the additional explanation. I had to ruminate on it for a bit, but I think it's starting to sink in now! /Jonas > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-26 15:40 ` Jonas Bonn @ 2019-01-28 7:41 ` Geert Uytterhoeven 2019-01-28 11:47 ` Mark Brown 0 siblings, 1 reply; 15+ messages in thread From: Geert Uytterhoeven @ 2019-01-28 7:41 UTC (permalink / raw) To: Jonas Bonn Cc: Mark Brown, Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML Hi Jonas, On Sat, Jan 26, 2019 at 4:40 PM Jonas Bonn <jonas@norrbonn.se> wrote: > On 26/01/2019 11:25, Geert Uytterhoeven wrote: > > On Sat, Jan 26, 2019 at 8:53 AM Jonas Bonn <jonas@norrbonn.se> wrote: > >> On 25/01/2019 18:50, Mark Brown wrote: > >>> On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote: > >>>> On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote: > >>>>> Having this as device property rather than a transfer property allows this > >>>>> to be configured one time in setup() rather than having to fiddle with the > >>>>> configuration register for every transfer. > >>> > >>>> That doesn't mean that the coniguration should be done in DT though, and > >>>> given that this presumably is a property of the device there seems to be > >>>> no reason why we'd have it in DT - if every instance of the device is > >>>> going to need to set the property we should just figure it out from the > >>>> compatble string instead. > >>> > >>> To be clear here: the suggestion is to add a parameter the slave device > >>> can set in spi_device which sets the default word_delay similarly to how > >>> max_speed_hz works. > >> > >> I'm confused... isn't that exactly what this patch does? It adds a > >> field word_delay to spi_device in the same manner as max_speed_hz. > >> > >> I also added the ability to set it via DT, which I can break out into a > >> separate patch if that's an issue. Or is the problem that it's set via > >> DT, at all? Documentation/devicetree/bindings/spi-bus.txt documents 10 > >> other slave-node properties related to transfer characteristics; > >> word_delay is just another such characteristic. > >> > >> But again, I'm having trouble parsing your response Is the patch wrong, > >> should be broken up, or you just misunderstood it? > > > > IIUIC, Mark means that it may be a non-configurable property of the slave > > device, and thus should be handled (fixed setting) in the SPI slave driver. > > OK, so given that the "compatible" property identifies the hardware and > there is no other _hardware_ configuration detail that affects the > required inter-word delay, then setting the property in the device tree > is not allowed as the driver has enough info to set it properly. Fair > enough! > > > > > Compare this to CPHA/CPOL, which are properties of the SPI slave device, > > but which may be configurable. E.g. many SPI FLASHes support multiple > > configurations. See e.g. commit 9c5becce21af35e5 ("ARM: shmobile: koelsch: > > Fix QSPI mode of SPI-Flash into mode3"). > > There is nothing about the hardware referenced in that patch that > enforces either mode. Why does the driver get to defer to DT here? The default is non-cpol and non-pha. The device supports both (perhaps even all four modes, didn't check). > Looking at Documentation/devicetree/bindings/spi/spi-bus.txt: > > spi-lsb-first: used only by MAXIM DS-1302 which is always LSB first; > driver should set this For DS1302, this is probable true. For e.g. a generic shift register (e.g. hc595), the driver cannot know. > spi-3wire: again, only set by MAXIM DS-1302 which always needs this > setting; driver could set this For DS1302, this is probable true. Some devices may support both 4-wire or 3-wire mode? > spi-tx-delay-us: > spi-rx-delay-us: only parsed by RMI4 driver... no DTS files in kernel > set these. I hardly see how these are "hardware" settings rather than > message settings, but the driver can set these in any case. > > Anyway, the point is, looking at the current documentation, it's pretty > difficult to understand whether devicetree is restricted to describing > hardware or is also for configuring drivers... True. > > Again, max_speed_hz is something different: while both the SPI master and > > slave may support high speeds, board wiring (capacitance/inductance) may > > need to force a slower speed than supported by the devices, so it makes > > sense to make that configurable from DT. > > Yeah, that one make sense. But if the DT is only overriding the maximum > device frequency that the driver should otherwise be setting, why is > spi-max-frequency a _required_ property? That's a good question. Legacy? > Thanks for taking the time to provide the additional explanation. I had > to ruminate on it for a bit, but I think it's starting to sink in now! You're welcome! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-28 7:41 ` Geert Uytterhoeven @ 2019-01-28 11:47 ` Mark Brown 2019-01-28 11:51 ` Jonas Bonn 0 siblings, 1 reply; 15+ messages in thread From: Mark Brown @ 2019-01-28 11:47 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Jonas Bonn, Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML [-- Attachment #1: Type: text/plain, Size: 612 bytes --] On Mon, Jan 28, 2019 at 08:41:05AM +0100, Geert Uytterhoeven wrote: > On Sat, Jan 26, 2019 at 4:40 PM Jonas Bonn <jonas@norrbonn.se> wrote: > > spi-3wire: again, only set by MAXIM DS-1302 which always needs this > > setting; driver could set this > For DS1302, this is probable true. > Some devices may support both 4-wire or 3-wire mode? IIRC yes. > > Yeah, that one make sense. But if the DT is only overriding the maximum > > device frequency that the driver should otherwise be setting, why is > > spi-max-frequency a _required_ property? > That's a good question. Legacy? It's a documentation error. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-28 11:47 ` Mark Brown @ 2019-01-28 11:51 ` Jonas Bonn 2019-01-28 11:54 ` Geert Uytterhoeven 0 siblings, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-28 11:51 UTC (permalink / raw) To: Mark Brown, Geert Uytterhoeven Cc: Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML On 28/01/2019 12:47, Mark Brown wrote: > On Mon, Jan 28, 2019 at 08:41:05AM +0100, Geert Uytterhoeven wrote: >> On Sat, Jan 26, 2019 at 4:40 PM Jonas Bonn <jonas@norrbonn.se> wrote: > >>> spi-3wire: again, only set by MAXIM DS-1302 which always needs this >>> setting; driver could set this > >> For DS1302, this is probable true. >> Some devices may support both 4-wire or 3-wire mode? > > IIRC yes. Just for the record, the datasheet says it's exclusively 3-wire. But it's not important... /Jonas > >>> Yeah, that one make sense. But if the DT is only overriding the maximum >>> device frequency that the driver should otherwise be setting, why is >>> spi-max-frequency a _required_ property? > >> That's a good question. Legacy? > > It's a documentation error. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] spi: support inter-word delay requirement for devices 2019-01-28 11:51 ` Jonas Bonn @ 2019-01-28 11:54 ` Geert Uytterhoeven 0 siblings, 0 replies; 15+ messages in thread From: Geert Uytterhoeven @ 2019-01-28 11:54 UTC (permalink / raw) To: Jonas Bonn Cc: Mark Brown, Baolin Wang, LKML, linux-spi, Rob Herring, Mark Rutland, DTML Hi Jonas, On Mon, Jan 28, 2019 at 12:51 PM Jonas Bonn <jonas@norrbonn.se> wrote: > On 28/01/2019 12:47, Mark Brown wrote: > > On Mon, Jan 28, 2019 at 08:41:05AM +0100, Geert Uytterhoeven wrote: > >> On Sat, Jan 26, 2019 at 4:40 PM Jonas Bonn <jonas@norrbonn.se> wrote: > > > >>> spi-3wire: again, only set by MAXIM DS-1302 which always needs this > >>> setting; driver could set this > > > >> For DS1302, this is probable true. > >> Some devices may support both 4-wire or 3-wire mode? > > > > IIRC yes. > > Just for the record, the datasheet says it's exclusively 3-wire. But > it's not important... With "some devices" I meant "some other non-DS1302 devices supporting 3-wire mode". Sorry for being unclear. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] spi-atmel: support inter-word delay 2019-01-25 11:44 [PATCH 0/2] spi: support inter-word delays Jonas Bonn 2019-01-25 11:44 ` [PATCH 1/2] spi: support inter-word delay requirement for devices Jonas Bonn @ 2019-01-25 11:44 ` Jonas Bonn 2019-01-25 11:47 ` Jonas Bonn 1 sibling, 1 reply; 15+ messages in thread From: Jonas Bonn @ 2019-01-25 11:44 UTC (permalink / raw) To: linux-kernel, linux-spi Cc: Jonas Bonn, Nicolas Ferre, Mark Brown, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel If the SPI slave requires an inter-word delay, configure the DLYBCT register accordingly. Tested on a SAMA5D2 board (derived from SAMA5D2-Xplained reference board). Signed-off-by: Jonas Bonn <jonas@norrbonn.se> CC: Nicolas Ferre <nicolas.ferre@microchip.com> CC: Mark Brown <broonie@kernel.org> CC: Alexandre Belloni <alexandre.belloni@bootlin.com> CC: Ludovic Desroches <ludovic.desroches@microchip.com> CC: linux-spi@vger.kernel.org CC: linux-arm-kernel@lists.infradead.org --- drivers/spi/spi-atmel.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c index 74fddcd3282b..88ff3fef56e9 100644 --- a/drivers/spi/spi-atmel.c +++ b/drivers/spi/spi-atmel.c @@ -1209,13 +1209,14 @@ static int atmel_spi_setup(struct spi_device *spi) csr |= SPI_BIT(CSAAT); /* DLYBS is mostly irrelevant since we manage chipselect using GPIOs. - * - * DLYBCT would add delays between words, slowing down transfers. - * It could potentially be useful to cope with DMA bottlenecks, but - * in those cases it's probably best to just use a lower bitrate. */ csr |= SPI_BF(DLYBS, 0); - csr |= SPI_BF(DLYBCT, 0); + + /* DLYBCT adds delays between words. This is useful for slow devices + * that need a bit of time to setup the next transfer. + */ + csr |= SPI_BF(DLYBCT, + clamp_t(u8, (as->spi_clk/1000000*spi->word_delay)>>5, 1, 255)); /* chipselect must have been muxed as GPIO (e.g. in board setup) */ npcs_pin = (unsigned long)spi->controller_data; -- 2.19.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] spi-atmel: support inter-word delay 2019-01-25 11:44 ` [PATCH 2/2] spi-atmel: support inter-word delay Jonas Bonn @ 2019-01-25 11:47 ` Jonas Bonn 0 siblings, 0 replies; 15+ messages in thread From: Jonas Bonn @ 2019-01-25 11:47 UTC (permalink / raw) To: linux-kernel, linux-spi Cc: Nicolas Ferre, Mark Brown, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel On 25/01/2019 12:44, Jonas Bonn wrote: > If the SPI slave requires an inter-word delay, configure the DLYBCT > register accordingly. > > Tested on a SAMA5D2 board (derived from SAMA5D2-Xplained reference > board). > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > CC: Nicolas Ferre <nicolas.ferre@microchip.com> > CC: Mark Brown <broonie@kernel.org> > CC: Alexandre Belloni <alexandre.belloni@bootlin.com> > CC: Ludovic Desroches <ludovic.desroches@microchip.com> > CC: linux-spi@vger.kernel.org > CC: linux-arm-kernel@lists.infradead.org > --- > drivers/spi/spi-atmel.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c > index 74fddcd3282b..88ff3fef56e9 100644 > --- a/drivers/spi/spi-atmel.c > +++ b/drivers/spi/spi-atmel.c > @@ -1209,13 +1209,14 @@ static int atmel_spi_setup(struct spi_device *spi) > csr |= SPI_BIT(CSAAT); > > /* DLYBS is mostly irrelevant since we manage chipselect using GPIOs. > - * > - * DLYBCT would add delays between words, slowing down transfers. > - * It could potentially be useful to cope with DMA bottlenecks, but > - * in those cases it's probably best to just use a lower bitrate. > */ > csr |= SPI_BF(DLYBS, 0); > - csr |= SPI_BF(DLYBCT, 0); > + > + /* DLYBCT adds delays between words. This is useful for slow devices > + * that need a bit of time to setup the next transfer. > + */ > + csr |= SPI_BF(DLYBCT, > + clamp_t(u8, (as->spi_clk/1000000*spi->word_delay)>>5, 1, 255)); Ah... this is stupid. The clamping here _always_ introduces a delay, which is not what we want... will resubmit this. /Jonas > > /* chipselect must have been muxed as GPIO (e.g. in board setup) */ > npcs_pin = (unsigned long)spi->controller_data; > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-28 11:54 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-25 11:44 [PATCH 0/2] spi: support inter-word delays Jonas Bonn 2019-01-25 11:44 ` [PATCH 1/2] spi: support inter-word delay requirement for devices Jonas Bonn 2019-01-25 11:53 ` Baolin Wang 2019-01-25 12:06 ` Jonas Bonn 2019-01-25 17:47 ` Mark Brown 2019-01-25 17:50 ` Mark Brown 2019-01-26 7:52 ` Jonas Bonn 2019-01-26 10:25 ` Geert Uytterhoeven 2019-01-26 15:40 ` Jonas Bonn 2019-01-28 7:41 ` Geert Uytterhoeven 2019-01-28 11:47 ` Mark Brown 2019-01-28 11:51 ` Jonas Bonn 2019-01-28 11:54 ` Geert Uytterhoeven 2019-01-25 11:44 ` [PATCH 2/2] spi-atmel: support inter-word delay Jonas Bonn 2019-01-25 11:47 ` Jonas Bonn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).