[PATCH] spi: dts: sun4i: Add support for inter-word wait cycles

[PATCH] spi: dts: sun4i: Add support for inter-word wait cycles

[Marcus Weseloh]

Hi all,

the Allwinner A10/A20 SPI module supports an option to configure a number of
clock periods to wait between each word ("SPI Wait Clock Register" in the A20
manual). This is a very useful option if talking to devices which specify a
minimum amount of inter-word wait time.

I initially tried to find a way to let SPI protocol drivers specify this
option, but I couldn't find a mechanism to pass additional options to the spi
master. So I took the spi-davinci driver as an example (it implements a very
similiar functionality) and added a new devicetree property.

While testing this patch I noticed that the SPI module always adds a constant
3 clock cycles to the number set in the Wait Clock Register. That number stays
constant across many different baud rates, so I documented it in the
devicetree binding file.

One thing I am unsure of is the device example in the binding
documentation.  I used "example,dummy" as compatible... is this acceptable or
should I use a device/compatible that actually exists somewhere?

Oh... and should I split binding documentation and code changing patch?

Best regards,

  Marcus


Marcus Weseloh (1):
  spi: dts: sun4i: Add support for inter-word wait cycles using the SPI
    Wait Clock Register

 Documentation/devicetree/bindings/spi/spi-sun4i.txt | 11 +++++++++++
 drivers/spi/spi-sun4i.c                             |  7 +++++++
 2 files changed, 18 insertions(+)

-- 
1.9.1

[PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Marcus Weseloh]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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)
+
 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;
-- 
1.9.1

Re: [linux-sunxi] [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Julian Calaby]

Hi Marcus,

On Fri, Nov 20, 2015 at 2:53 AM, Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Marcus Weseloh]

Hi Julian,

2015-11-19 23:59 GMT+01:00 Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 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

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Julian Calaby]

Hi Marcus,

On Fri, Nov 20, 2015 at 7:45 PM, Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Julian,
>
> 2015-11-19 23:59 GMT+01:00 Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> 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,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Marcus Weseloh]

Hi Julien,

2015-11-20 11:12 GMT+01:00 Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 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

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Rob Herring]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 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

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Marcus Weseloh]

2015-11-20 17:12 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Marcus Weseloh]

2015-11-22 20:45 GMT+01:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>> 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

Re: [PATCH] spi: dts: sun4i: Add support for inter-word wait cycles using the SPI Wait Clock Register

[Rob Herring]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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