Re: [PATCHv2] wlcore: spi: add wl18xx support
On Thu, Apr 21, 2016 at 11:07:37AM +, 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. signature.asc Description: PGP signature
RE: [PATCHv2] wlcore: spi: add wl18xx support
> > > 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
On Tue, Apr 19, 2016 at 06:04:49PM +, 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). signature.asc Description: PGP signature
Re: [PATCHv2] wlcore: spi: add wl18xx support
On Tue, Apr 19, 2016 at 05:38:02PM +, 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. signature.asc Description: PGP signature
Re: [PATCHv2] wlcore: spi: add wl18xx support
On Tue, Apr 19, 2016 at 05:21:01PM +, 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. signature.asc Description: PGP signature
Re: [PATCHv2] wlcore: spi: add wl18xx support
On Tue, Apr 19, 2016 at 09:05:45AM +, 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... signature.asc Description: PGP signature
Re: [PATCHv2] wlcore: spi: add wl18xx support
On Mon, Apr 18, 2016 at 05:55:51AM +, 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. signature.asc Description: PGP signature
RE: [PATCHv2] wlcore: spi: add wl18xx support
> > > > 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 linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] wlcore: spi: add wl18xx support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] wlcore: spi: add wl18xx support
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
> > > >{
> > > > ...
> > > > cs-gpios = < 5 0>;
> > > > ...
> > > > wlcore: wlcore@0 {
> > > > compatible = "ti,wl1835";
> > > > ...
> > > > ...
> > > > };
> > > > };
> > > >
> > > > Signed-off-by: Eyal Reizer
> > >
> > > 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-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] wlcore: spi: add wl18xx support
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
> > >{
> > > ...
> > > cs-gpios = < 5 0>;
> > > ...
> > > wlcore: wlcore@0 {
> > > compatible = "ti,wl1835";
> > > ...
> > > ...
> > > };
> > > };
> > >
> > > Signed-off-by: Eyal Reizer
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] wlcore: spi: add wl18xx support
> >
> > - 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
> >{
> > ...
> > cs-gpios = < 5 0>;
> > ...
> > wlcore: wlcore@0 {
> > compatible = "ti,wl1835";
> > ...
> > ...
> > };
> > };
> >
> > Signed-off-by: Eyal Reizer
>
> 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 linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] wlcore: spi: add wl18xx support
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
>{
> ...
> cs-gpios = < 5 0>;
> ...
> wlcore: wlcore@0 {
> compatible = "ti,wl1835";
> ...
> ...
> };
> };
>
> Signed-off-by: Eyal Reizer
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] wlcore: spi: add wl18xx support
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
{
...
cs-gpios = < 5 0>;
...
wlcore: wlcore@0 {
compatible = "ti,wl1835";
...
...
};
};
Signed-off-by: Eyal Reizer
---
v1 -> v2: update device tree bindings documentation
.../bindings/net/wireless/ti,wlcore,spi.txt| 50 +-
drivers/net/wireless/ti/wlcore/spi.c | 176 +---
2 files changed, 200 insertions(+), 26 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
b/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
index 9180724..912ab0c 100644
--- a/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
+++ b/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
@@ -1,19 +1,31 @@
-* Texas Instruments wl1271 wireless lan controller
+* Texas Instruments wl12xx/wl18xx wireless lan controller
-The wl1271 chip can be connected via SPI or via SDIO. This
+The wl12xx/wl18xx chips can be connected via SPI or via SDIO. This
document describes the binding for the SPI connected chip.
Required properties:
-- compatible : Should be "ti,wl1271"
+- compatible : Should be one of the following:
+* "ti,wl1271"
+* "ti,wl1273"
+* "ti,wl1281"
+* "ti,wl1283"
+* "ti,wl1801"
+* "ti,wl1805"
+* "ti,wl1807"
+* "ti,wl1831"
+* "ti,wl1835"
+* "ti,wl1837"
- reg : Chip select address of device
- spi-max-frequency : Maximum SPI clocking speed of device in Hz
-- ref-clock-frequency : Reference clock frequency
- interrupt-parent, interrupts :
Should contain parameters for 1 interrupt line.
Interrupt parameters: parent, line number, type.
-- vwlan-supply :Point the node of the regulator that powers/enable the
wl1271 chip
+- vwlan-supply :Point the node of the regulator that powers/enable the
+wl12xx/wl18xx chip
+- cs-gpios :GPIO pin used as the spi chip select
Optional properties:
+- ref-clock-frequency : Reference clock frequency (should be set for wl12xx)
- clock-xtal : boolean, clock is generated from XTAL
- Please consult Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -21,10 +33,16 @@ Optional properties:
Examples:
+For wl12xx family:
{
- wl1271@1 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ cs-gpios = < 5 0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ wlcore: wlcore@0 {
compatible = "ti,wl1271";
-
reg = <1>;
spi-max-frequency = <4800>;
clock-xtal;
@@ -34,3 +52,21 @@ Examples:
vwlan-supply = <_fixed>;
};
};
+
+For wl18xx family:
+ {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ cs-gpios = < 5 0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ wlcore: wlcore@0 {
+ compatible = "ti,wl1835";
+ vwlan-supply = <_en_reg>;
+ spi-max-frequency = <4800>;
+ reg = <0>;
+ interrupt-parent = <>;
+ interrupts = <27 IRQ_TYPE_EDGE_RISING>;
+ };
+};
diff --git a/drivers/net/wireless/ti/wlcore/spi.c
b/drivers/net/wireless/ti/wlcore/spi.c
index 020ac1a..fb48a0d 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include "wlcore.h"
#include "wl12xx_80211.h"
@@ -70,16 +71,30 @@
#define WSPI_MAX_CHUNK_SIZE4092
/*
- * only support SPI for 12xx - this code should be reworked when 18xx
- * support is introduced
+ * wl18xx driver aggregation buffer size is (13 * PAGE_SIZE) compared to
+ * (4 * PAGE_SIZE) for wl12xx, so use the larger buffer needed for wl18xx
*/
-#define SPI_AGGR_BUFFER_SIZE (4 * PAGE_SIZE)
+#define SPI_AGGR_BUFFER_SIZE (13 * PAGE_SIZE)
/* Maximum number of SPI write chunks */
#define WSPI_MAX_NUM_OF_CHUNKS \
((SPI_AGGR_BUFFER_SIZE / WSPI_MAX_CHUNK_SIZE) + 1)
+struct wilink_familiy_data {
+ char name[8];
+};
+
+const struct wilink_familiy_data *wilink_data;
+
+static const struct wilink_familiy_data wl18xx_data = {
+ .name = "wl18xx",
+};
+
+static const struct wilink_familiy_data wl12xx_data = {
+ .name = "wl12xx",
+};
+
struct wl12xx_spi_glue {
struct device
