| Message ID | 1447948422-4915-2-git-send-email-mweseloh42@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi Marcus, On Fri, Nov 20, 2015 at 2:53 AM, Marcus Weseloh <mweseloh42@gmail.com> wrote: > Adds support and documentation for a new slave device property > "sun4i,spi-wdelay" that allows to set the SPI Wait Clock Register per > device / transfer. The SPI hardware will wait the specified amount of > SPI clock periods (plus a constant 3 clock periods) before transmitting > the next word. Should you possibly hide the 3 clock periods from the user? I.e. they set whatever they want for the wdelay, we set it to the closest number we can that's greater or equal to what they ask for. Thanks,
Hi Julian, 2015-11-19 23:59 GMT+01:00 Julian Calaby <julian.calaby@gmail.com>: > Should you possibly hide the 3 clock periods from the user? > > I.e. they set whatever they want for the wdelay, we set it to the > closest number we can that's greater or equal to what they ask for. That's a good idea and much better than having to remember to subtract 3 cycles from the desired wait time! But it would mean that this magic number becomes part of the driver code. I have found no official documentation that mentions those additional cycles. While I have checked many different transmission speeds using both CDR1 and CDR2 divider configurations, there is still the possibility that the behaviour changes with weird SPI module configurations... And I've only tested it on A20 hardware. So it would be great if somebody else with access to A10 hardware and an oscilloscope could check if we have a consistent 3 cycle overhead. Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcus, On Fri, Nov 20, 2015 at 7:45 PM, Marcus Weseloh <mweseloh42@gmail.com> wrote: > Hi Julian, > > 2015-11-19 23:59 GMT+01:00 Julian Calaby <julian.calaby@gmail.com>: >> Should you possibly hide the 3 clock periods from the user? >> >> I.e. they set whatever they want for the wdelay, we set it to the >> closest number we can that's greater or equal to what they ask for. > > That's a good idea and much better than having to remember to subtract > 3 cycles from the desired wait time! > > But it would mean that this magic number becomes part of the driver > code. I have found no official documentation that mentions those > additional cycles. While I have checked many different transmission > speeds using both CDR1 and CDR2 divider configurations, there is still > the possibility that the behaviour changes with weird SPI module > configurations... And I've only tested it on A20 hardware. So it would > be great if somebody else with access to A10 hardware and an > oscilloscope could check if we have a consistent 3 cycle overhead. Having magic numbers is kind-of a drivers' job. (and the wdelay should arguably be a core-spi thing, not a sunxi thing, but that's a separate discussion) If it is different for other SoCs, then you might have to move that constant somewhere else and introduce new compatible strings for them. Thanks,
Hi Julien, 2015-11-20 11:12 GMT+01:00 Julian Calaby <julian.calaby@gmail.com>: > Having magic numbers is kind-of a drivers' job. Yes, of course. What I meant was that I didn't feel comfortable to include this magic number in driver code because I'm not 100% sure if it is correct across all SPI configurations and SoCs that this driver supports (A10 / A20). > (and the wdelay should > arguably be a core-spi thing, not a sunxi thing, but that's a separate > discussion) I've been thinking about that, but it seemed to big a change to attempt with my limited kernel hacking experience. Mark, do you think we should rather talk about adding support for options like this delay to spi-core? Or would it be OK to add it to the sun4i driver and possibly refactor later? Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 19, 2015 at 04:53:42PM +0100, Marcus Weseloh wrote: > Adds support and documentation for a new slave device property > "sun4i,spi-wdelay" that allows to set the SPI Wait Clock Register per > device / transfer. The SPI hardware will wait the specified amount of > SPI clock periods (plus a constant 3 clock periods) before transmitting > the next word. > > The constant additional 3 clock periods are not documented by the vendor > and have been determined by analyzing the generated waveforms across > many different transmission speeds. > > Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com> > --- > Documentation/devicetree/bindings/spi/spi-sun4i.txt | 11 +++++++++++ > drivers/spi/spi-sun4i.c | 7 +++++++ > 2 files changed, 18 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-sun4i.txt b/Documentation/devicetree/bindings/spi/spi-sun4i.txt > index de827f5..9c4d723 100644 > --- a/Documentation/devicetree/bindings/spi/spi-sun4i.txt > +++ b/Documentation/devicetree/bindings/spi/spi-sun4i.txt > @@ -10,6 +10,10 @@ Required properties: > - "mod": the parent module clock > - clock-names: Must contain the clock names described just above > > +Optional properties for slave devices: > +- sun4i,spi-wdelay : delay between transmission of words, specified in number > + of SPI clock periods (actual delay is wdelay + 3 clock periods) Seems like a common property to me. For a common one, it should be the actual delay and the driver needs to subtract the 3 clock periods here. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 20, 2015 at 02:56:34PM +0100, Marcus Weseloh wrote: > Hi Julien, > > 2015-11-20 11:12 GMT+01:00 Julian Calaby <julian.calaby@gmail.com>: > > Having magic numbers is kind-of a drivers' job. I guess all my comments were already raised... > > Yes, of course. What I meant was that I didn't feel comfortable to > include this magic number in driver code because I'm not 100% sure if > it is correct across all SPI configurations and SoCs that this driver > supports (A10 / A20). You could not subtract off 3 that way you meet the minimum time no matter what. If other platforms are not setting this property, then I would expect the behavior is unchanged. If they do want to set it, it is their job to validate that it is correct. If it differs, the compatible string can help with that. > > (and the wdelay should > > arguably be a core-spi thing, not a sunxi thing, but that's a separate > > discussion) > > I've been thinking about that, but it seemed to big a change to > attempt with my limited kernel hacking experience. It is not any bigger. You just need to document it in the core binding. It would still be read by the drivers using it. > Mark, do you think we should rather talk about adding support for > options like this delay to spi-core? Or would it be OK to add it to > the sun4i driver and possibly refactor later? > > Cheers, > > Marcus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-11-20 17:12 GMT+01:00 Rob Herring <robh@kernel.org>: > On Fri, Nov 20, 2015 at 02:56:34PM +0100, Marcus Weseloh wrote: >> > (and the wdelay should >> > arguably be a core-spi thing, not a sunxi thing, but that's a separate >> > discussion) >> >> I've been thinking about that, but it seemed to big a change to >> attempt with my limited kernel hacking experience. > > It is not any bigger. You just need to document it in the core binding. > It would still be read by the drivers using it. Julien, Rob: thanks for your comments! Ok, I will make the following changes: - remove "sun4i,spi-wdelay" from the sun4i binding and add the property to the spi-bus.txt binding instead - remove the comment about the additional 3 cycles from the documentation - modfy the spi-sun4i driver to take care of the minimum 3 cycle period Does that sound right? And maybe I could also use a more descriptive name for the property, maybe "spi-word-wait-cycles"? Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Nov 20, 2015 at 05:45:48PM +0100, Marcus Weseloh wrote: > 2015-11-20 17:12 GMT+01:00 Rob Herring <robh@kernel.org>: > > On Fri, Nov 20, 2015 at 02:56:34PM +0100, Marcus Weseloh wrote: > >> > (and the wdelay should > >> > arguably be a core-spi thing, not a sunxi thing, but that's a separate > >> > discussion) > >> > >> I've been thinking about that, but it seemed to big a change to > >> attempt with my limited kernel hacking experience. > > > > It is not any bigger. You just need to document it in the core binding. > > It would still be read by the drivers using it. > > Julien, Rob: thanks for your comments! Ok, I will make the following changes: > > - remove "sun4i,spi-wdelay" from the sun4i binding and add the > property to the spi-bus.txt binding instead > - remove the comment about the additional 3 cycles from the documentation > - modfy the spi-sun4i driver to take care of the minimum 3 cycle period > > Does that sound right? > > And maybe I could also use a more descriptive name for the property, > maybe "spi-word-wait-cycles"? I don't think it should be in a clock-rate dependant unit. Using micro or nano-seconds would be more appropriate I guess. Maxime
2015-11-22 20:45 GMT+01:00 Maxime Ripard <maxime.ripard@free-electrons.com>: >> Julien, Rob: thanks for your comments! Ok, I will make the following changes: >> >> - remove "sun4i,spi-wdelay" from the sun4i binding and add the >> property to the spi-bus.txt binding instead >> - remove the comment about the additional 3 cycles from the documentation >> - modfy the spi-sun4i driver to take care of the minimum 3 cycle period >> >> Does that sound right? >> >> And maybe I could also use a more descriptive name for the property, >> maybe "spi-word-wait-cycles"? > > I don't think it should be in a clock-rate dependant unit. Using micro > or nano-seconds would be more appropriate I guess. Thanks Maxime, using a time based value instead of cycles sounds like a much better approach. However... I'm starting to think that the proposed inter-word wait time DT property is just an ugly workaround. Let me explain my use-case: I'm developing a driver for a sensor that requires a minimum wait time between words. The wait time depends on the mode the sensor is set to: 37.5us in slow mode, 12.5us in fast mode. I initially used spidev to test the sensor from userspace. And for that use case, the "spi-wdelay" property that I proposed works well. But now I am writing the proper protocol driver and suddenly the explicit wait time setting seems just wrong. Ideally, the protocol driver would just expose a DT property that allows to choose between "slow" and "fast" mode. I think that the correct approach would be to extend the SPI controller API to allow protocol drivers to set an inter-word delay. That would keep the magic numbers inside my protocol driver and out of the devicetree. And an additional ioctl call could set that inter-word delay from spidev, allowing userspace to set this value as well if needed. Mark: would you be open to such a change to the SPI controller API? I could use the already available spi_transfer.delay_usecs for this, but I would require that I wrap each word in a single transfer, which adds significant processing overhead to the communication with the sensor. Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/spi-sun4i.txt b/Documentation/devicetree/bindings/spi/spi-sun4i.txt index de827f5..9c4d723 100644 --- a/Documentation/devicetree/bindings/spi/spi-sun4i.txt +++ b/Documentation/devicetree/bindings/spi/spi-sun4i.txt @@ -10,6 +10,10 @@ Required properties: - "mod": the parent module clock - clock-names: Must contain the clock names described just above +Optional properties for slave devices: +- sun4i,spi-wdelay : delay between transmission of words, specified in number + of SPI clock periods (actual delay is wdelay + 3 clock periods) + Example: spi1: spi@01c06000 { @@ -21,4 +25,11 @@ spi1: spi@01c06000 { status = "disabled"; #address-cells = <1>; #size-cells = <0>; + + spi1_0 { + compatible = "example,dummy"; + reg = <0>; + spi-max-frequency = <1000000>; /* 1Mhz = 1us clock period */ + sun4i,spi-wdelay = <2>; /* delay 5us (2 + 3 clock periods) */ + }; }; diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c index f60a6d6..a8e39f1 100644 --- a/drivers/spi/spi-sun4i.c +++ b/drivers/spi/spi-sun4i.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/of.h> #include <linux/spi/spi.h> @@ -173,6 +174,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master, unsigned int tx_len = 0; int ret = 0; u32 reg; + u32 wdelay = 0; /* We don't support transfer larger than the FIFO */ if (tfr->len > SUN4I_FIFO_DEPTH) @@ -261,6 +263,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master, sun4i_spi_write(sspi, SUN4I_CLK_CTL_REG, reg); + /* Set optional inter-word wait cycles */ + of_property_read_u32(spi->dev.of_node, "sun4i,spi-wdelay", + &wdelay); + sun4i_spi_write(sspi, SUN4I_WAIT_REG, (u16)wdelay); + /* Setup the transfer now... */ if (sspi->tx_buf) tx_len = tfr->len;
Adds support and documentation for a new slave device property "sun4i,spi-wdelay" that allows to set the SPI Wait Clock Register per device / transfer. The SPI hardware will wait the specified amount of SPI clock periods (plus a constant 3 clock periods) before transmitting the next word. The constant additional 3 clock periods are not documented by the vendor and have been determined by analyzing the generated waveforms across many different transmission speeds. Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com> --- Documentation/devicetree/bindings/spi/spi-sun4i.txt | 11 +++++++++++ drivers/spi/spi-sun4i.c | 7 +++++++ 2 files changed, 18 insertions(+)