Re: [PATCHv2] wlcore: spi: add wl18xx support

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Arnd Bergmann]

On Sunday 10 April 2016 07:37:23 Reizer, Eyal wrote:
> Add support for using with both wl12xx and wl18xx.
> 
> - all wilink family needs special init command for entering wspi mode.
>   extra clock cycles should be sent after the spi init command while the
>   cs pin is high.
> - switch to controling the cs pin from the spi driver for achieveing the
>   above.
> - the selected cs gpio is read from the spi device-tree node using the
>   cs-gpios field and setup as a gpio.
> - See the example below for specifying the cs gpio using the cs-gpios entry
> &spi0   {
>         ...
>         cs-gpios = <&gpio0 5 0>;
>         ...
>         wlcore: wlcore@0 {
>                 compatible = "ti,wl1835";
>         ...
>         ...
>         };
> };
> 
> Signed-off-by: Eyal Reizer <eyalr@ti.com>

I don't think this can work in general: not all SPI hosts uses GPIOs
for controlling CS, so the logic can't work, and it's also a layering
violation for the driver to look at the parent.

I would suggest fixing this using a new API function from the SPI
core, if we don't already have a generic way to do it.

	Arnd

RE: [PATCHv2] wlcore: spi: add wl18xx support

[Reizer, Eyal]

> >
> > - all wilink family needs special init command for entering wspi mode.
> >   extra clock cycles should be sent after the spi init command while the
> >   cs pin is high.
> > - switch to controling the cs pin from the spi driver for achieveing the
> >   above.
> > - the selected cs gpio is read from the spi device-tree node using the
> >   cs-gpios field and setup as a gpio.
> > - See the example below for specifying the cs gpio using the cs-gpios entry
> > &spi0   {
> >         ...
> >         cs-gpios = <&gpio0 5 0>;
> >         ...
> >         wlcore: wlcore@0 {
> >                 compatible = "ti,wl1835";
> >         ...
> >         ...
> >         };
> > };
> >
> > Signed-off-by: Eyal Reizer <eyalr-l0cyMroinI0@public.gmane.org>
> 
> I don't think this can work in general: not all SPI hosts uses GPIOs for
> controlling CS, so the logic can't work, and it's also a layering violation for the
> driver to look at the parent.
> 
> I would suggest fixing this using a new API function from the SPI core, if we
> don't already have a generic way to do it.
>
Originally this is what I have done until I was pointed to the generic cs-gpio mechanism 
in the SPI core. 
It is a generic mechanism already in the SPI core driver.
See: Documentation/devicetree/bindings/spi/spi-bus.txt

It is also part of the generic spi.h (include/Linux/spi/spi.h), already part of 
" struct spi_device" So it seemed redundant adding another mechanism for 
implementing the same.
Platform that interact with a wilink need to use it, and platforms that don't  
have this capability will probably not interact with a wilink device using SPI.

Best Regards,
Eyal



--
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: [PATCHv2] wlcore: spi: add wl18xx support

[Arnd Bergmann]

On Monday 18 April 2016 05:55:51 Reizer, Eyal wrote:
> > >
> > > - all wilink family needs special init command for entering wspi mode.
> > >   extra clock cycles should be sent after the spi init command while the
> > >   cs pin is high.
> > > - switch to controling the cs pin from the spi driver for achieveing the
> > >   above.
> > > - the selected cs gpio is read from the spi device-tree node using the
> > >   cs-gpios field and setup as a gpio.
> > > - See the example below for specifying the cs gpio using the cs-gpios entry
> > > &spi0   {
> > >         ...
> > >         cs-gpios = <&gpio0 5 0>;
> > >         ...
> > >         wlcore: wlcore@0 {
> > >                 compatible = "ti,wl1835";
> > >         ...
> > >         ...
> > >         };
> > > };
> > >
> > > Signed-off-by: Eyal Reizer <eyalr@ti.com>
> > 
> > I don't think this can work in general: not all SPI hosts uses GPIOs for
> > controlling CS, so the logic can't work, and it's also a layering violation for the
> > driver to look at the parent.
> > 
> > I would suggest fixing this using a new API function from the SPI core, if we
> > don't already have a generic way to do it.
> >
> Originally this is what I have done until I was pointed to the generic cs-gpio mechanism 
> in the SPI core. 
> It is a generic mechanism already in the SPI core driver.
> See: Documentation/devicetree/bindings/spi/spi-bus.txt

The cs-gpios property is documented as optional, it defines how you should
list the gpios if CS is implemented using gpio, but not all hardware does
it like this.

> It is also part of the generic spi.h (include/Linux/spi/spi.h), already part of 
> " struct spi_device" So it seemed redundant adding another mechanism for 
> implementing the same.
> Platform that interact with a wilink need to use it, and platforms that don't  
> have this capability will probably not interact with a wilink device using SPI.

The cs_gpio field in spi_device belongs to the spi host controller, no other
slave driver uses it.

I wasn't asking for a duplication of this mechanism, but an interface to
use it properly. Internally, the spi core uses the spi_set_cs() function to
pick a CS. Find a way to use that rather than reimplementing it incorrectly.

	Arnd

RE: [PATCHv2] wlcore: spi: add wl18xx support

[Reizer, Eyal]

Arnd,

> > > >
> > > > - all wilink family needs special init command for entering wspi mode.
> > > >   extra clock cycles should be sent after the spi init command while the
> > > >   cs pin is high.
> > > > - switch to controling the cs pin from the spi driver for achieveing the
> > > >   above.
> > > > - the selected cs gpio is read from the spi device-tree node using the
> > > >   cs-gpios field and setup as a gpio.
> > > > - See the example below for specifying the cs gpio using the cs-gpios
> entry
> > > > &spi0   {
> > > >         ...
> > > >         cs-gpios = <&gpio0 5 0>;
> > > >         ...
> > > >         wlcore: wlcore@0 {
> > > >                 compatible = "ti,wl1835";
> > > >         ...
> > > >         ...
> > > >         };
> > > > };
> > > >
> > > > Signed-off-by: Eyal Reizer <eyalr-l0cyMroinI0@public.gmane.org>
> > >
> > > I don't think this can work in general: not all SPI hosts uses GPIOs
> > > for controlling CS, so the logic can't work, and it's also a
> > > layering violation for the driver to look at the parent.
> > >
> > > I would suggest fixing this using a new API function from the SPI
> > > core, if we don't already have a generic way to do it.
> > >
> > Originally this is what I have done until I was pointed to the generic
> > cs-gpio mechanism in the SPI core.
> > It is a generic mechanism already in the SPI core driver.
> > See: Documentation/devicetree/bindings/spi/spi-bus.txt
> 
> The cs-gpios property is documented as optional, it defines how you should
> list the gpios if CS is implemented using gpio, but not all hardware does it like
> this.
> 
> > It is also part of the generic spi.h (include/Linux/spi/spi.h),
> > already part of " struct spi_device" So it seemed redundant adding
> > another mechanism for implementing the same.
> > Platform that interact with a wilink need to use it, and platforms
> > that don't have this capability will probably not interact with a wilink device
> using SPI.
> 
> The cs_gpio field in spi_device belongs to the spi host controller, no other
> slave driver uses it.
> 
> I wasn't asking for a duplication of this mechanism, but an interface to use it
> properly. Internally, the spi core uses the spi_set_cs() function to pick a CS.
> Find a way to use that rather than reimplementing it incorrectly.
> 

Understood. As this special CS manipulation is unique to wspi (wilink spi)  I think the 
best option is to move this gpio allocation into wlcore_spi as a new device tree entry
used only by this driver.
If you agree I will submit a v3.

Best Regards,
Eyal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Arnd Bergmann]

On Tuesday 19 April 2016 09:05:45 Reizer, Eyal wrote:
> > > It is also part of the generic spi.h (include/Linux/spi/spi.h),
> > > already part of " struct spi_device" So it seemed redundant adding
> > > another mechanism for implementing the same.
> > > Platform that interact with a wilink need to use it, and platforms
> > > that don't have this capability will probably not interact with a wilink device
> > using SPI.
> > 
> > The cs_gpio field in spi_device belongs to the spi host controller, no other
> > slave driver uses it.
> > 
> > I wasn't asking for a duplication of this mechanism, but an interface to use it
> > properly. Internally, the spi core uses the spi_set_cs() function to pick a CS.
> > Find a way to use that rather than reimplementing it incorrectly.
> > 
> 
> Understood. As this special CS manipulation is unique to wspi (wilink spi)  I think the 
> best option is to move this gpio allocation into wlcore_spi as a new device tree entry
> used only by this driver.
> If you agree I will submit a v3.

I don't think that can work either: aside of not solving the problem
of wilink devices on spi controllers that don't use gpio, it also doesn't
solve the problem of what happens when the driver manually triggers the
gpio to hold the CS signal while another driver talks to a different
device using another CS on the same controller.

	Arnd

RE: [PATCHv2] wlcore: spi: add wl18xx support

[Reizer, Eyal]

> > > > It is also part of the generic spi.h (include/Linux/spi/spi.h),
> > > > already part of " struct spi_device" So it seemed redundant adding
> > > > another mechanism for implementing the same.
> > > > Platform that interact with a wilink need to use it, and platforms
> > > > that don't have this capability will probably not interact with a
> > > > wilink device
> > > using SPI.
> > >
> > > The cs_gpio field in spi_device belongs to the spi host controller,
> > > no other slave driver uses it.
> > >
> > > I wasn't asking for a duplication of this mechanism, but an
> > > interface to use it properly. Internally, the spi core uses the spi_set_cs()
> function to pick a CS.
> > > Find a way to use that rather than reimplementing it incorrectly.
> > >
> >
> > Understood. As this special CS manipulation is unique to wspi (wilink
> > spi)  I think the best option is to move this gpio allocation into
> > wlcore_spi as a new device tree entry used only by this driver.
> > If you agree I will submit a v3.
> 
> I don't think that can work either: aside of not solving the problem of wilink
> devices on spi controllers that don't use gpio, it also doesn't solve the
> problem of what happens when the driver manually triggers the gpio to hold
> the CS signal while another driver talks to a different device using another CS
> on the same controller.
> 
Ok, understood. Will look into it.

Best Regards,
Eyal
--
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: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Tue, Apr 19, 2016 at 09:05:45AM +0000, Reizer, Eyal wrote:

> Understood. As this special CS manipulation is unique to wspi (wilink spi)  I think the 
> best option is to move this gpio allocation into wlcore_spi as a new device tree entry
> used only by this driver.

That sounds like it is going to break normal chip select operation which
doesn't seem like a good idea either.  What exactly are you trying to do
here?  If you need to send some extra bytes at the end of a transfer why
not just do that, or add an empty transfer with a delay.  It's hard to
see what more you could do with only control of the chip select...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Tue, Apr 19, 2016 at 05:21:01PM +0000, Reizer, Eyal wrote:

> The main quirk here is that i need to send extra clocks after the spi init command while the CS pin is "high" in order to put the wilink chip into SPI mode.
> So just sending an empty transfer wouldnt do the trick here.

A single byte transfer would result in extra clocks being sent so there
must be more to it than that...

> Do you have a recomendation on how to do it without changing the core logic. If it is possible it would be really great.

Please be explicit about what you're trying to do here.  I imagine you
may need to change the core logic but we need to know what it is that
the device needs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Tue, Apr 19, 2016 at 05:38:02PM +0000, Reizer, Eyal wrote:
> Hi Mark,
> 
> Hope you can see the attached picture that illustrates what need to sent for sucesfull SPI init.

I think what the picture shows is that you just need to send at least
one byte at the end of the transfer *after* deasserting /CS which is
totally not what I'd have understood from any of the previous
discussion.  That is already supported, just send an extra byte using
.cs_change to do what is says.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Tue, Apr 19, 2016 at 06:04:49PM +0000, Reizer, Eyal wrote:

> Thanks! Glad the illustration helped.
> I will try it out again as if i recall cotrectly, i did try that l, and it didnt produce the correct waveform, but perhaps i didnt understand the usage of .cs_change correctly.
> Will double check anyway.

It could also be a bug in your controller driver, it's common for them
to not handle cs_change correctly (at least those that open code the
message loop, obviously modern ones just factor this out into the core).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCHv2] wlcore: spi: add wl18xx support

[Reizer, Eyal]

> 
> > Thanks! Glad the illustration helped.
> > I will try it out again as if i recall cotrectly, i did try that l, and it didnt produce
> the correct waveform, but perhaps i didnt understand the usage of
> .cs_change correctly.
> > Will double check anyway.
> 
> It could also be a bug in your controller driver, it's common for them to not
> handle cs_change correctly (at least those that open code the message loop,
> obviously modern ones just factor this out into the core).

Tried looking into using cs_change and not sure I understand how it would do what I need.
According to, include/linux/spi/spi.h:

* All SPI transfers start with the relevant chipselect active.  Normally
* it stays selected until after the last transfer in a message.  Drivers
* can affect the chipselect signal using cs_change.
*
* (i) If the transfer isn't the last one in the message, this flag is
* used to make the chipselect briefly go inactive in the middle of the
* message.  Toggling chipselect in this way may be needed to terminate
* a chip command, letting a single spi_message perform all of group of
* chip transactions together.

Now, in my case I need the CS pin to go inactive and stay inactive during the transfer 
of the next byte for completing the SPI init correctly (sending the extra clock cycles while CS is inactive)
If I read the above correctly, CS will only briefly go inactive but will when the next byte 
is sent it will be active again.
What am I missing?

Best Regards,
Eyal

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Thu, Apr 21, 2016 at 11:07:37AM +0000, Reizer, Eyal wrote:

> * (i) If the transfer isn't the last one in the message, this flag is
> * used to make the chipselect briefly go inactive in the middle of the
> * message.  Toggling chipselect in this way may be needed to terminate
> * a chip command, letting a single spi_message perform all of group of
> * chip transactions together.

> Now, in my case I need the CS pin to go inactive and stay inactive during the transfer 
> of the next byte for completing the SPI init correctly (sending the extra clock cycles while CS is inactive)
> If I read the above correctly, CS will only briefly go inactive but will when the next byte 
> is sent it will be active again.
> What am I missing?

No, it changes the state.  The main use case is a brief bounce but
you can use it for other things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCHv2] wlcore: spi: add wl18xx support

[Mark Brown]

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

On Mon, Apr 18, 2016 at 05:55:51AM +0000, Reizer, Eyal wrote:

> > I would suggest fixing this using a new API function from the SPI core, if we
> > don't already have a generic way to do it.

> Originally this is what I have done until I was pointed to the generic cs-gpio mechanism 
> in the SPI core. 
> It is a generic mechanism already in the SPI core driver.
> See: Documentation/devicetree/bindings/spi/spi-bus.txt

> It is also part of the generic spi.h (include/Linux/spi/spi.h), already part of 
> " struct spi_device" So it seemed redundant adding another mechanism for 
> implementing the same.

No!  This is a *terrible* and broken idea.  Client drivers should *not*
be peering inside controller implementations like this, and they should
especially not be trying to change the chip select without the core
knowing about it.  This is at best going to be fragile, at worst it will
actively break some systems.  Whatever you are trying to do needs to go
through the SPI core with some degree of abstraction, the core needs to
know what's going on and the driver needs to support systems that don't
or can't mux the chip select out as a GPIO.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]