[PATCHv2] wlcore: spi: add wl18xx support

[PATCHv2] wlcore: spi: add wl18xx support

[Reizer, Eyal]

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>
---
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:
 &spi1 {
-	wl1271@1 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi1_pins>;
+	cs-gpios = <&gpio0 5 0>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore@0 {
 		compatible = "ti,wl1271";
-
 		reg = <1>;
 		spi-max-frequency = <48000000>;
 		clock-xtal;
@@ -34,3 +52,21 @@ Examples:
 		vwlan-supply = <&vwlan_fixed>;
 	};
 };
+
+For wl18xx family:
+&spi0	{
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_pins>;
+	cs-gpios = <&gpio0 5 0>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore@0 {
+		compatible = "ti,wl1835";
+		vwlan-supply = <&wlan_en_reg>;
+		spi-max-frequency = <48000000>;
+		reg = <0>;
+		interrupt-parent = <&gpio0>;
+		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 <linux/platform_device.h>
 #include <linux/of_irq.h>
 #include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
 
 #include "wlcore.h"
 #include "wl12xx_80211.h"
@@ -70,16 +71,30 @@
 #define WSPI_MAX_CHUNK_SIZE    4092
 
 /*
- * 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 *dev;
 	struct platform_device *core;
@@ -120,6 +135,8 @@ static void wl12xx_spi_init(struct device *child)
 	struct spi_transfer t;
 	struct spi_message m;
 	u8 *cmd = kzalloc(WSPI_INIT_CMD_LEN, GFP_KERNEL);
+	struct spi_device *spi = (struct spi_device *)glue->dev;
+	struct spi_master *master = spi->master;
 
 	if (!cmd) {
 		dev_err(child->parent,
@@ -127,6 +144,15 @@ static void wl12xx_spi_init(struct device *child)
 		return;
 	}
 
+	if (!master->cs_gpios) {
+		dev_err(child->parent,
+			"spi chip select pin missing in platform data!\n");
+		return;
+	}
+
+	/* Drive CS line low */
+	gpio_direction_output(master->cs_gpios[0], 0);
+
 	memset(&t, 0, sizeof(t));
 	spi_message_init(&m);
 
@@ -163,6 +189,26 @@ static void wl12xx_spi_init(struct device *child)
 	spi_message_add_tail(&t, &m);
 
 	spi_sync(to_spi_device(glue->dev), &m);
+
+	/* Send extra clocks with CS high. this is required by the wilink
+	 * family in order for successfully enter WSPI mode
+	 */
+	gpio_direction_output(master->cs_gpios[0], 1);
+
+	memset(&m, 0, sizeof(m));
+	spi_message_init(&m);
+
+	cmd[0] = 0xff;
+	cmd[1] = 0xff;
+	cmd[2] = 0xff;
+	cmd[3] = 0xff;
+	swab32s((u32 *)cmd);
+
+	t.tx_buf = cmd;
+	t.len = 4;
+	spi_message_add_tail(&t, &m);
+	spi_sync(to_spi_device(glue->dev), &m);
+
 	kfree(cmd);
 }
 
@@ -213,6 +259,16 @@ static int __must_check wl12xx_spi_raw_read(struct device *child, int addr,
 	u32 *busy_buf;
 	u32 *cmd;
 	u32 chunk_len;
+	struct spi_device *spi = (struct spi_device *)glue->dev;
+	struct spi_master *master = spi->master;
+
+	if (!master->cs_gpios) {
+		dev_err(child->parent,
+			"spi chip select pin missing in platform data!\n");
+		return -EINVAL;
+	}
+	/* Drive CS line low */
+	gpio_direction_output(master->cs_gpios[0], 0);
 
 	while (len > 0) {
 		chunk_len = min_t(size_t, WSPI_MAX_CHUNK_SIZE, len);
@@ -267,25 +323,44 @@ static int __must_check wl12xx_spi_raw_read(struct device *child, int addr,
 		len -= chunk_len;
 	}
 
+	/* Drive CS line high */
+	gpio_direction_output(master->cs_gpios[0], 1);
 	return 0;
 }
 
-static int __must_check wl12xx_spi_raw_write(struct device *child, int addr,
-					     void *buf, size_t len, bool fixed)
+static int __wl12xx_spi_raw_write(struct device *child, int addr,
+				  void *buf, size_t len, bool fixed)
 {
 	struct wl12xx_spi_glue *glue = dev_get_drvdata(child->parent);
-	/* SPI write buffers - 2 for each chunk */
-	struct spi_transfer t[2 * WSPI_MAX_NUM_OF_CHUNKS];
+	struct spi_transfer *t;
 	struct spi_message m;
 	u32 commands[WSPI_MAX_NUM_OF_CHUNKS]; /* 1 command per chunk */
 	u32 *cmd;
 	u32 chunk_len;
 	int i;
+	struct spi_device *spi = (struct spi_device *)glue->dev;
+	struct spi_master *master = spi->master;
+
+	if (!master->cs_gpios) {
+		dev_err(child->parent,
+			"spi chip select pin missing in platform data!\n");
+		return -EINVAL;
+	}
+
+	/* SPI write buffers - 2 for each chunk */
+	t = kzalloc(sizeof(*t) * 2 * WSPI_MAX_NUM_OF_CHUNKS, GFP_KERNEL);
+	if (!t) {
+		dev_err(child->parent,
+			"could not allocate spi write buffer\n");
+		return -ENOMEM;
+	}
+
+	/* Drive CS line low */
+	gpio_direction_output(master->cs_gpios[0], 0);
 
 	WARN_ON(len > SPI_AGGR_BUFFER_SIZE);
 
 	spi_message_init(&m);
-	memset(t, 0, sizeof(t));
 
 	cmd = &commands[0];
 	i = 0;
@@ -318,9 +393,29 @@ static int __must_check wl12xx_spi_raw_write(struct device *child, int addr,
 
 	spi_sync(to_spi_device(glue->dev), &m);
 
+	/* Drive CS line high */
+	gpio_direction_output(master->cs_gpios[0], 1);
+
+	kfree(t);
 	return 0;
 }
 
+static int __must_check wl12xx_spi_raw_write(struct device *child, int addr,
+					     void *buf, size_t len, bool fixed)
+{
+	int ret;
+
+	/* The ELP wakeup write may fail the first time due to internal
+	 * hardware latency. It is safer to send the wakeup command twice to
+	 * avoid unexpected failures.
+	 */
+	if (addr == HW_ACCESS_ELP_CTRL_REG)
+		ret = __wl12xx_spi_raw_write(child, addr, buf, len, fixed);
+	ret = __wl12xx_spi_raw_write(child, addr, buf, len, fixed);
+
+	return ret;
+}
+
 /**
  * wl12xx_spi_set_power - power on/off the wl12xx unit
  * @child: wl12xx device handle.
@@ -349,17 +444,38 @@ static int wl12xx_spi_set_power(struct device *child, bool enable)
 	return ret;
 }
 
+/**
+ * wl12xx_spi_set_block_size
+ *
+ * This function is not needed for spi mode, but need to be present.
+ * Without it defined the wlcore fallback to use the wrong packet
+ * allignment on tx.
+ */
+static void wl12xx_spi_set_block_size(struct device *child,
+				      unsigned int blksz)
+{
+}
+
 static struct wl1271_if_operations spi_ops = {
 	.read		= wl12xx_spi_raw_read,
 	.write		= wl12xx_spi_raw_write,
 	.reset		= wl12xx_spi_reset,
 	.init		= wl12xx_spi_init,
 	.power		= wl12xx_spi_set_power,
-	.set_block_size = NULL,
+	.set_block_size = wl12xx_spi_set_block_size,
 };
 
 static const struct of_device_id wlcore_spi_of_match_table[] = {
-	{ .compatible = "ti,wl1271" },
+	{ .compatible = "ti,wl1271", .data = &wl12xx_data},
+	{ .compatible = "ti,wl1273", .data = &wl12xx_data},
+	{ .compatible = "ti,wl1281", .data = &wl12xx_data},
+	{ .compatible = "ti,wl1283", .data = &wl12xx_data},
+	{ .compatible = "ti,wl1801", .data = &wl18xx_data},
+	{ .compatible = "ti,wl1805", .data = &wl18xx_data},
+	{ .compatible = "ti,wl1807", .data = &wl18xx_data},
+	{ .compatible = "ti,wl1831", .data = &wl18xx_data},
+	{ .compatible = "ti,wl1835", .data = &wl18xx_data},
+	{ .compatible = "ti,wl1837", .data = &wl18xx_data},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, wlcore_spi_of_match_table);
@@ -375,18 +491,24 @@ static int wlcore_probe_of(struct spi_device *spi, struct wl12xx_spi_glue *glue,
 			   struct wlcore_platdev_data *pdev_data)
 {
 	struct device_node *dt_node = spi->dev.of_node;
-	int ret;
+	const struct of_device_id *of_id;
+
+	of_id = of_match_node(wlcore_spi_of_match_table, dt_node);
+	if (!of_id)
+		return -ENODEV;
+
+	wilink_data = of_id->data;
+	dev_info(&spi->dev, "selected chip familiy is %s\n",
+		 wilink_data->name);
 
 	if (of_find_property(dt_node, "clock-xtal", NULL))
 		pdev_data->ref_clock_xtal = true;
 
-	ret = of_property_read_u32(dt_node, "ref-clock-frequency",
-				   &pdev_data->ref_clock_freq);
-	if (IS_ERR_VALUE(ret)) {
-		dev_err(glue->dev,
-			"can't get reference clock frequency (%d)\n", ret);
-		return ret;
-	}
+	/* optional clock frequency params */
+	of_property_read_u32(dt_node, "ref-clock-frequency",
+			     &pdev_data->ref_clock_freq);
+	of_property_read_u32(dt_node, "tcxo-clock-frequency",
+			     &pdev_data->tcxo_clock_freq);
 
 	return 0;
 }
@@ -397,6 +519,7 @@ static int wl1271_probe(struct spi_device *spi)
 	struct wlcore_platdev_data pdev_data;
 	struct resource res[1];
 	int ret;
+	struct spi_master *master = spi->master;
 
 	memset(&pdev_data, 0x00, sizeof(pdev_data));
 
@@ -410,6 +533,12 @@ static int wl1271_probe(struct spi_device *spi)
 
 	glue->dev = &spi->dev;
 
+	if (!master->cs_gpios) {
+		dev_err(glue->dev,
+			"spi chip select pin missing in platform data!\n");
+		return -EINVAL;
+	}
+
 	spi_set_drvdata(spi, glue);
 
 	/* This is the only SPI value that we need to set here, the rest
@@ -431,15 +560,21 @@ static int wl1271_probe(struct spi_device *spi)
 		return ret;
 	}
 
+	if (gpio_request(master->cs_gpios[0], "spi1-cs0"))
+		return -EINVAL;
+
 	ret = spi_setup(spi);
 	if (ret < 0) {
 		dev_err(glue->dev, "spi_setup failed\n");
+		gpio_free(master->cs_gpios[0]);
 		return ret;
 	}
 
-	glue->core = platform_device_alloc("wl12xx", PLATFORM_DEVID_AUTO);
+	glue->core = platform_device_alloc(wilink_data->name,
+					   PLATFORM_DEVID_AUTO);
 	if (!glue->core) {
 		dev_err(glue->dev, "can't allocate platform_device\n");
+		gpio_free(master->cs_gpios[0]);
 		return -ENOMEM;
 	}
 
@@ -474,14 +609,17 @@ static int wl1271_probe(struct spi_device *spi)
 
 out_dev_put:
 	platform_device_put(glue->core);
+	gpio_free(master->cs_gpios[0]);
 	return ret;
 }
 
 static int wl1271_remove(struct spi_device *spi)
 {
 	struct wl12xx_spi_glue *glue = spi_get_drvdata(spi);
+	struct spi_master *master = spi->master;
 
 	platform_device_unregister(glue->core);
+	gpio_free(master->cs_gpios[0]);
 
 	return 0;
 }
-- 
1.7.9.5

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@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

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

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@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.
> 

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

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

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 --]