[PATCH v5 1/3] dt: bindings: add wl18xx wireless device

[PATCH v5 1/3] dt: bindings: add wl18xx wireless device

[Eliad Peller]

Add device tree binding documentation for TI's wl18xx
wlan chip.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 .../devicetree/bindings/net/wireless/ti,wl18xx.txt | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt

diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt b/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt
new file mode 100644
index 0000000..9dcf535
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt
@@ -0,0 +1,39 @@
+TI Wilink8 (wl18xx) SDIO devices
+
+This node provides properties for controlling the wilink8 wireless device. The
+node is expected to be specified as a child node to the SDIO controller that
+connects the device to the system.
+
+Required properties:
+ - compatible : Should be "ti,wl18xx".
+ - compatible: should be one of the following:
+    * "ti,wl1801"
+    * "ti,wl1805"
+    * "ti,wl1807"
+    * "ti,wl1831"
+    * "ti,wl1835"
+    * "ti,wl1837"
+ - interrupts : specifies attributes for the out-of-band interrupt.
+
+Optional properties:
+ - interrupt-parent : the phandle for the interrupt controller to which the
+	device interrupts are connected.
+
+Example:
+
+&mmc3 {
+	status = "okay";
+	vmmc-supply = <&wlan_en_reg>;
+	bus-width = <4>;
+	cap-power-off-card;
+	keep-power-in-suspend;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore at 2 {
+		compatible = "ti,wl1835";
+		reg = <2>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <19 IRQ_TYPE_NONE>;
+	};
+};
-- 
1.8.5.2.229.g4448466.dirty

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Eliad Peller]

When running with device-tree, we no longer have a board file
that can set up the platform data for wlcore.
Allow this data to be passed from DT.

For now, parse only the irq used. Other (optional) properties
can be added later on.

Signed-off-by: Ido Yariv <ido@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/ti/wlcore/sdio.c | 80 ++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index d3dd7bf..ee556ac 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -34,6 +34,8 @@
 #include <linux/wl12xx.h>
 #include <linux/pm_runtime.h>
 #include <linux/printk.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "wlcore.h"
 #include "wl12xx_80211.h"
@@ -214,6 +216,69 @@ static struct wl1271_if_operations sdio_ops = {
 	.set_block_size = wl1271_sdio_set_block_size,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id wlcore_sdio_of_match_table[] = {
+	{ .compatible = "ti,wl1801" },
+	{ .compatible = "ti,wl1805" },
+	{ .compatible = "ti,wl1807" },
+	{ .compatible = "ti,wl1831" },
+	{ .compatible = "ti,wl1835" },
+	{ .compatible = "ti,wl1837" },
+	{ }
+};
+
+static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct wl12xx_platform_data *pdata;
+
+	if (!np || !of_match_node(wlcore_sdio_of_match_table, np))
+		return NULL;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->irq = irq_of_parse_and_map(np, 0);
+	if (!pdata->irq) {
+		dev_err(dev, "No irq in platform data\n");
+		kfree(pdata);
+		return NULL;
+	}
+
+	return pdata;
+}
+#else
+static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
+static struct wl12xx_platform_data *
+wlcore_get_platform_data(struct device *dev)
+{
+	struct wl12xx_platform_data *pdata;
+
+	/* first, look for DT data */
+	pdata = wlcore_probe_of(dev);
+	if (pdata)
+		return pdata;
+
+	/* if not found - fallback to static platform data */
+	pdata = wl12xx_get_platform_data();
+	if (!IS_ERR(pdata))
+		return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL);
+
+	dev_err(dev, "No platform data set\n");
+	return NULL;
+}
+
+static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
+{
+	kfree(pdata);
+}
+
 static int wl1271_probe(struct sdio_func *func,
 				  const struct sdio_device_id *id)
 {
@@ -245,12 +310,9 @@ static int wl1271_probe(struct sdio_func *func,
 	/* Use block mode for transferring over one block size of data */
 	func->card->quirks |= MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
 
-	pdev_data.pdata = wl12xx_get_platform_data();
-	if (IS_ERR(pdev_data.pdata)) {
-		ret = PTR_ERR(pdev_data.pdata);
-		dev_err(glue->dev, "missing wlan platform data: %d\n", ret);
+	pdev_data.pdata = wlcore_get_platform_data(&func->dev);
+	if (!pdev_data.pdata)
 		goto out_free_glue;
-	}
 
 	/* if sdio can keep power while host is suspended, enable wow */
 	mmcflags = sdio_get_host_pm_caps(func);
@@ -279,7 +341,7 @@ static int wl1271_probe(struct sdio_func *func,
 	if (!glue->core) {
 		dev_err(glue->dev, "can't allocate platform_device");
 		ret = -ENOMEM;
-		goto out_free_glue;
+		goto out_free_pdata;
 	}
 
 	glue->core->dev.parent = &func->dev;
@@ -313,6 +375,9 @@ static int wl1271_probe(struct sdio_func *func,
 out_dev_put:
 	platform_device_put(glue->core);
 
+out_free_pdata:
+	wlcore_del_platform_data(pdev_data.pdata);
+
 out_free_glue:
 	kfree(glue);
 
@@ -323,11 +388,14 @@ out:
 static void wl1271_remove(struct sdio_func *func)
 {
 	struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func);
+	struct wlcore_platdev_data *pdev_data = glue->core->dev.platform_data;
+	struct wl12xx_platform_data *pdata = pdev_data->pdata;
 
 	/* Undo decrement done above in wl1271_probe */
 	pm_runtime_get_noresume(&func->dev);
 
 	platform_device_unregister(glue->core);
+	wlcore_del_platform_data(pdata);
 	kfree(glue);
 }
 
-- 
1.8.5.2.229.g4448466.dirty

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Arnd Bergmann]

On Monday 09 March 2015 17:36:41 Eliad Peller wrote:
> @@ -323,11 +388,14 @@ out:
>  static void wl1271_remove(struct sdio_func *func)
>  {
>         struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func);
> +       struct wlcore_platdev_data *pdev_data = glue->core->dev.platform_data;
> +       struct wl12xx_platform_data *pdata = pdev_data->pdata;
>  
>         /* Undo decrement done above in wl1271_probe */
>         pm_runtime_get_noresume(&func->dev);
>  
>         platform_device_unregister(glue->core);
> +       wlcore_del_platform_data(pdata);
>         kfree(glue);
>  }
>  
> 

The third patch looks ok, but now you should remove the wl12xx_platform_data
from the wlcore code, since it's not used any more, it was broken to start
with (as it supports only one instance) and we want to prevent others from
adding new users of that. Since the only thing you need is the irq number,
you can directly add the irq number to struct wlcore_platdev_data and
remove the pdata pointer there.

	Arnd

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Javier Martinez Canillas]

Hello Eliad,

On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote:
> When running with device-tree, we no longer have a board file
> that can set up the platform data for wlcore.
> Allow this data to be passed from DT.
>
> For now, parse only the irq used. Other (optional) properties
> can be added later on.
>
> Signed-off-by: Ido Yariv <ido@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

I see this is a v5 but I don't know what was changed from prior
revisions. It would be good if the patches had a versions history.

>  drivers/net/wireless/ti/wlcore/sdio.c | 80 ++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> index d3dd7bf..ee556ac 100644
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
> @@ -34,6 +34,8 @@
>  #include <linux/wl12xx.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/printk.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
>
>  #include "wlcore.h"
>  #include "wl12xx_80211.h"
> @@ -214,6 +216,69 @@ static struct wl1271_if_operations sdio_ops = {
>         .set_block_size = wl1271_sdio_set_block_size,
>  };
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id wlcore_sdio_of_match_table[] = {
> +       { .compatible = "ti,wl1801" },
> +       { .compatible = "ti,wl1805" },
> +       { .compatible = "ti,wl1807" },
> +       { .compatible = "ti,wl1831" },
> +       { .compatible = "ti,wl1835" },
> +       { .compatible = "ti,wl1837" },
> +       { }
> +};
> +
> +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
> +{
> +       struct device_node *np = dev->of_node;
> +       struct wl12xx_platform_data *pdata;
> +
> +       if (!np || !of_match_node(wlcore_sdio_of_match_table, np))
> +               return NULL;
> +
> +       pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return NULL;
> +
> +       pdata->irq = irq_of_parse_and_map(np, 0);
> +       if (!pdata->irq) {
> +               dev_err(dev, "No irq in platform data\n");
> +               kfree(pdata);
> +               return NULL;
> +       }
> +
> +       return pdata;
> +}
> +#else
> +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
> +{
> +       return NULL;
> +}
> +#endif
> +
> +static struct wl12xx_platform_data *
> +wlcore_get_platform_data(struct device *dev)
> +{
> +       struct wl12xx_platform_data *pdata;
> +
> +       /* first, look for DT data */

I thought it was the opposite, that platform data should over-rule DT.
That way you can still use the data filled in
arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your
new DT binding.

> +       pdata = wlcore_probe_of(dev);
> +       if (pdata)
> +               return pdata;
> +
> +       /* if not found - fallback to static platform data */
> +       pdata = wl12xx_get_platform_data();
> +       if (!IS_ERR(pdata))
> +               return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL);
> +
> +       dev_err(dev, "No platform data set\n");
> +       return NULL;
> +}
> +
> +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
> +{
> +       kfree(pdata);
> +}
> +

This function seems to be an unnecessary, why not just call kfree() directly?

Or better, maybe the resource-managed devm_*() functions can be used
so the data doesn't have to be explicitly freed?

Best regards,
Javier

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Arnd Bergmann]

On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote:
> > +
> > +static struct wl12xx_platform_data *
> > +wlcore_get_platform_data(struct device *dev)
> > +{
> > +       struct wl12xx_platform_data *pdata;
> > +
> > +       /* first, look for DT data */
> 
> I thought it was the opposite, that platform data should over-rule DT.
> That way you can still use the data filled in
> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your
> new DT binding.

No, the pdata-quirks stuff for this driver must die, it was a hack
that only exists because we previously could not attach data to an
sdio function.

> > +       pdata = wlcore_probe_of(dev);
> > +       if (pdata)
> > +               return pdata;
> > +
> > +       /* if not found - fallback to static platform data */
> > +       pdata = wl12xx_get_platform_data();
> > +       if (!IS_ERR(pdata))
> > +               return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL);
> > +
> > +       dev_err(dev, "No platform data set\n");
> > +       return NULL;
> > +}
> > +
> > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
> > +{
> > +       kfree(pdata);
> > +}
> > +
> 
> This function seems to be an unnecessary, why not just call kfree() directly?
> 
> Or better, maybe the resource-managed devm_*() functions can be used
> so the data doesn't have to be explicitly freed?

As I said earlier, I think it would be best not to dynamically allocate anything
here at all. As Eliad explained, the data is used by two different drivers:
wl12xx and wl18xx, and only the latter is converted for now, but after the
conversion, it should not need the platform data structure any more, only
the irq number that gets passed in from DT.

	Arnd

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Javier Martinez Canillas]

Hello Arnd,

On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote:
>> > +
>> > +static struct wl12xx_platform_data *
>> > +wlcore_get_platform_data(struct device *dev)
>> > +{
>> > +       struct wl12xx_platform_data *pdata;
>> > +
>> > +       /* first, look for DT data */
>>
>> I thought it was the opposite, that platform data should over-rule DT.
>> That way you can still use the data filled in
>> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your
>> new DT binding.
>
> No, the pdata-quirks stuff for this driver must die, it was a hack
> that only exists because we previously could not attach data to an
> sdio function.
>

Ok sorry, I misunderstood and thought that the output from the
discussion in patch 3/3 was that the pdata could not still be removed
due not having a way to configure the clocks for wl12xx.

I totally agree with removing the pdata on this driver, in fact I
would go an just remove all the remaining OMAP2+ board files and
pdata-quirks completely. Most OMAP2+ boards have been converted to DT
years ago and if someone really cares about mainline support for these
boards, they can add a DTS and DT bindings for the drivers that still
needed like this one.

Best regards,
Javier

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Arnd Bergmann]

On Wednesday 11 March 2015 11:05:47 Javier Martinez Canillas wrote:
> 
> On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote:
> >> > +
> >> > +static struct wl12xx_platform_data *
> >> > +wlcore_get_platform_data(struct device *dev)
> >> > +{
> >> > +       struct wl12xx_platform_data *pdata;
> >> > +
> >> > +       /* first, look for DT data */
> >>
> >> I thought it was the opposite, that platform data should over-rule DT.
> >> That way you can still use the data filled in
> >> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your
> >> new DT binding.
> >
> > No, the pdata-quirks stuff for this driver must die, it was a hack
> > that only exists because we previously could not attach data to an
> > sdio function.
> >
> 
> Ok sorry, I misunderstood and thought that the output from the
> discussion in patch 3/3 was that the pdata could not still be removed
> due not having a way to configure the clocks for wl12xx.

I think that is still the case, but we should never have both pdata
and DT, and if we ever did, I think the DT should take precedence
so we can be sure that the pdata is not used and can be removed.

	Arnd

[PATCH v5 2/3] wl18xx: add basic device-tree support

[Eliad Peller]

On Wed, Mar 11, 2015 at 11:51 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
>> > +{
>> > +       kfree(pdata);
>> > +}
>> > +
>>
>> This function seems to be an unnecessary, why not just call kfree() directly?
>>
>> Or better, maybe the resource-managed devm_*() functions can be used
>> so the data doesn't have to be explicitly freed?
>
> As I said earlier, I think it would be best not to dynamically allocate anything
> here at all. As Eliad explained, the data is used by two different drivers:
> wl12xx and wl18xx, and only the latter is converted for now, but after the
> conversion, it should not need the platform data structure any more, only
> the irq number that gets passed in from DT.
>
sure, i'll try taking care of it (probably with additional patch after
the conversion)

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and
omap3-igep0020-rev-f dts files, instead of defining the
platform data through the pdata-quirks.

The patch was compile-tested only.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +++++++++
 arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +++++++++
 arch/arm/mach-omap2/pdata-quirks.c         | 2 --
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
index cc8bd0c..8e5b44e 100644
--- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
+++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
@@ -42,4 +42,13 @@
 	vmmc-supply = <&lbep5clwmc_wlen>;
 	bus-width = <4>;
 	non-removable;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore at 2 {
+		compatible = "ti,wl1835";
+		reg = <2>;
+		interrupt-parent = <&gpio6>;
+		interrupts = <17 IRQ_TYPE_NONE>;
+	};
 };
diff --git a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
index 9326b28..02c4eec 100644
--- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
+++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
@@ -64,4 +64,13 @@
 	vmmc-supply = <&lbep5clwmc_wlen>;
 	bus-width = <4>;
 	non-removable;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore at 2 {
+		compatible = "ti,wl1835";
+		reg = <2>;
+		interrupt-parent = <&gpio5>;
+		interrupts = <8 IRQ_TYPE_NONE>;
+	};
 };
diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 190fa43..abc8a20 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -159,14 +159,12 @@ static struct platform_device btwilink_device = {
 
 static void __init omap3_igep0020_rev_f_legacy_init(void)
 {
-	legacy_init_wl12xx(0, 0, 177);
 	platform_device_register(&wl18xx_device);
 	platform_device_register(&btwilink_device);
 }
 
 static void __init omap3_igep0030_rev_g_legacy_init(void)
 {
-	legacy_init_wl12xx(0, 0, 136);
 	platform_device_register(&wl18xx_device);
 	platform_device_register(&btwilink_device);
 }
-- 
1.8.5.2.229.g4448466.dirty

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> @@ -64,4 +64,13 @@
>         vmmc-supply = <&lbep5clwmc_wlen>;
>         bus-width = <4>;
>         non-removable;
> +
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       wlcore: wlcore at 2 {
> +               compatible = "ti,wl1835";
> +               reg = <2>;
> +               interrupt-parent = <&gpio5>;
> +               interrupts = <8 IRQ_TYPE_NONE>;
> +       };
> 

Why IRQ_TYPE_NONE?

I was expecting you to remove all calls to legacy_init_wl12xx from this file,
including the ones for wl12xx aside from the wl18xx ones you removed, but
if that's enough to clean out the platform_data handling from the wlcore
driver, it's good enough as a start.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
>> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>> @@ -64,4 +64,13 @@
>>         vmmc-supply = <&lbep5clwmc_wlen>;
>>         bus-width = <4>;
>>         non-removable;
>> +
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       wlcore: wlcore at 2 {
>> +               compatible = "ti,wl1835";
>> +               reg = <2>;
>> +               interrupt-parent = <&gpio5>;
>> +               interrupts = <8 IRQ_TYPE_NONE>;
>> +       };
>>
>
> Why IRQ_TYPE_NONE?
>
i simply mirrored the current board file (which only sets the irq number).

> I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> including the ones for wl12xx aside from the wl18xx ones you removed, but
> if that's enough to clean out the platform_data handling from the wlcore
> driver, it's good enough as a start.
not sure i'm following - can you elaborate?

i'll summarize the way i see it. please correct me if i'm wrong.

both wl18xx and wl12xx use the platform data to get the irq number.
wl12xx (only) also needs some additional clock definitions to be
passed. there's currently some issue with specifying some the of clock
sources, so i preferred starting only with (the simpler) wl18xx
bindings.

for platforms with wl18xx, we can remove the pdata-quirk, as all the
data (i.e. irq) can be passed by the new DT bindings.
however, for platforms with wl12xx, we still need to pass the clock
definitions (along with the irq), so we have to keep
legacy_init_wl12xx for the time being (and that's also why we have to
currently keep the platform_data handling in the wlcore driver)

do you have something else in mind?

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Eliad Peller <eliad@wizery.com> [150309 14:03]:
> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> @@ -64,4 +64,13 @@
> >>         vmmc-supply = <&lbep5clwmc_wlen>;
> >>         bus-width = <4>;
> >>         non-removable;
> >> +
> >> +       #address-cells = <1>;
> >> +       #size-cells = <0>;
> >> +       wlcore: wlcore at 2 {
> >> +               compatible = "ti,wl1835";
> >> +               reg = <2>;
> >> +               interrupt-parent = <&gpio5>;
> >> +               interrupts = <8 IRQ_TYPE_NONE>;
> >> +       };
> >>
> >
> > Why IRQ_TYPE_NONE?
> >
> i simply mirrored the current board file (which only sets the irq number).
> 
> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> > if that's enough to clean out the platform_data handling from the wlcore
> > driver, it's good enough as a start.
> not sure i'm following - can you elaborate?
> 
> i'll summarize the way i see it. please correct me if i'm wrong.
> 
> both wl18xx and wl12xx use the platform data to get the irq number.
> wl12xx (only) also needs some additional clock definitions to be
> passed. there's currently some issue with specifying some the of clock
> sources, so i preferred starting only with (the simpler) wl18xx
> bindings.
> 
> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> data (i.e. irq) can be passed by the new DT bindings.
> however, for platforms with wl12xx, we still need to pass the clock
> definitions (along with the irq), so we have to keep
> legacy_init_wl12xx for the time being (and that's also why we have to
> currently keep the platform_data handling in the wlcore driver)
> 
> do you have something else in mind?

I think what Arnd is saying is we've now removed all the wl12xx using
legacy platforms, so all of them can boot with just data from dts.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Eliad Peller <eliad@wizery.com> [150309 14:03]:
>> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
>> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>> >> @@ -64,4 +64,13 @@
>> >>         vmmc-supply = <&lbep5clwmc_wlen>;
>> >>         bus-width = <4>;
>> >>         non-removable;
>> >> +
>> >> +       #address-cells = <1>;
>> >> +       #size-cells = <0>;
>> >> +       wlcore: wlcore at 2 {
>> >> +               compatible = "ti,wl1835";
>> >> +               reg = <2>;
>> >> +               interrupt-parent = <&gpio5>;
>> >> +               interrupts = <8 IRQ_TYPE_NONE>;
>> >> +       };
>> >>
>> >
>> > Why IRQ_TYPE_NONE?
>> >
>> i simply mirrored the current board file (which only sets the irq number).
>>
>> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
>> > including the ones for wl12xx aside from the wl18xx ones you removed, but
>> > if that's enough to clean out the platform_data handling from the wlcore
>> > driver, it's good enough as a start.
>> not sure i'm following - can you elaborate?
>>
>> i'll summarize the way i see it. please correct me if i'm wrong.
>>
>> both wl18xx and wl12xx use the platform data to get the irq number.
>> wl12xx (only) also needs some additional clock definitions to be
>> passed. there's currently some issue with specifying some the of clock
>> sources, so i preferred starting only with (the simpler) wl18xx
>> bindings.
>>
>> for platforms with wl18xx, we can remove the pdata-quirk, as all the
>> data (i.e. irq) can be passed by the new DT bindings.
>> however, for platforms with wl12xx, we still need to pass the clock
>> definitions (along with the irq), so we have to keep
>> legacy_init_wl12xx for the time being (and that's also why we have to
>> currently keep the platform_data handling in the wlcore driver)
>>
>> do you have something else in mind?
>
> I think what Arnd is saying is we've now removed all the wl12xx using
> legacy platforms, so all of them can boot with just data from dts.
>
I don't think that's the case (unless i'm missing something).
e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
initializes wl12xx device.

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Eliad Peller <eliad@wizery.com> [150309 14:03]:
> >> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
> >> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> >> @@ -64,4 +64,13 @@
> >> >>         vmmc-supply = <&lbep5clwmc_wlen>;
> >> >>         bus-width = <4>;
> >> >>         non-removable;
> >> >> +
> >> >> +       #address-cells = <1>;
> >> >> +       #size-cells = <0>;
> >> >> +       wlcore: wlcore at 2 {
> >> >> +               compatible = "ti,wl1835";
> >> >> +               reg = <2>;
> >> >> +               interrupt-parent = <&gpio5>;
> >> >> +               interrupts = <8 IRQ_TYPE_NONE>;
> >> >> +       };
> >> >>
> >> >
> >> > Why IRQ_TYPE_NONE?
> >> >
> >> i simply mirrored the current board file (which only sets the irq number).
> >>
> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> >> > if that's enough to clean out the platform_data handling from the wlcore
> >> > driver, it's good enough as a start.
> >> not sure i'm following - can you elaborate?
> >>
> >> i'll summarize the way i see it. please correct me if i'm wrong.
> >>
> >> both wl18xx and wl12xx use the platform data to get the irq number.
> >> wl12xx (only) also needs some additional clock definitions to be
> >> passed. there's currently some issue with specifying some the of clock
> >> sources, so i preferred starting only with (the simpler) wl18xx
> >> bindings.
> >>
> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> >> data (i.e. irq) can be passed by the new DT bindings.
> >> however, for platforms with wl12xx, we still need to pass the clock
> >> definitions (along with the irq), so we have to keep
> >> legacy_init_wl12xx for the time being (and that's also why we have to
> >> currently keep the platform_data handling in the wlcore driver)
> >>
> >> do you have something else in mind?
> >
> > I think what Arnd is saying is we've now removed all the wl12xx using
> > legacy platforms, so all of them can boot with just data from dts.

Right, that was my idea.

> I don't think that's the case (unless i'm missing something).
> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
> initializes wl12xx device.

This one is just like the igep0030, as Tony was saying: the board
boots from device tree already, so now that we have a binding for
it, we can remove the wl12xx_set_platform_data() for it.

Unfortunately, there is still one machine that uses a board file
in combination with this wl12xx: da850-evm calls
wl12xx_set_platform_data() from a legacy board file without DT.

We have DT support for this file as well, but if I remember correctly,
it is somewhat incomplete and we don't want to remove the file
yet. Adding Sekhar and Kevin to Cc here for confirmation.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [150310 07:11]:
> On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
> > On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Eliad Peller <eliad@wizery.com> [150309 14:03]:
> > >> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > >> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
> > >> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> > >> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> > >> >> @@ -64,4 +64,13 @@
> > >> >>         vmmc-supply = <&lbep5clwmc_wlen>;
> > >> >>         bus-width = <4>;
> > >> >>         non-removable;
> > >> >> +
> > >> >> +       #address-cells = <1>;
> > >> >> +       #size-cells = <0>;
> > >> >> +       wlcore: wlcore at 2 {
> > >> >> +               compatible = "ti,wl1835";
> > >> >> +               reg = <2>;
> > >> >> +               interrupt-parent = <&gpio5>;
> > >> >> +               interrupts = <8 IRQ_TYPE_NONE>;
> > >> >> +       };
> > >> >>
> > >> >
> > >> > Why IRQ_TYPE_NONE?
> > >> >
> > >> i simply mirrored the current board file (which only sets the irq number).
> > >>
> > >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> > >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> > >> > if that's enough to clean out the platform_data handling from the wlcore
> > >> > driver, it's good enough as a start.
> > >> not sure i'm following - can you elaborate?
> > >>
> > >> i'll summarize the way i see it. please correct me if i'm wrong.
> > >>
> > >> both wl18xx and wl12xx use the platform data to get the irq number.
> > >> wl12xx (only) also needs some additional clock definitions to be
> > >> passed. there's currently some issue with specifying some the of clock
> > >> sources, so i preferred starting only with (the simpler) wl18xx
> > >> bindings.
> > >>
> > >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> > >> data (i.e. irq) can be passed by the new DT bindings.
> > >> however, for platforms with wl12xx, we still need to pass the clock
> > >> definitions (along with the irq), so we have to keep
> > >> legacy_init_wl12xx for the time being (and that's also why we have to
> > >> currently keep the platform_data handling in the wlcore driver)
> > >>
> > >> do you have something else in mind?
> > >
> > > I think what Arnd is saying is we've now removed all the wl12xx using
> > > legacy platforms, so all of them can boot with just data from dts.
> 
> Right, that was my idea.
> 
> > I don't think that's the case (unless i'm missing something).
> > e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
> > initializes wl12xx device.
> 
> This one is just like the igep0030, as Tony was saying: the board
> boots from device tree already, so now that we have a binding for
> it, we can remove the wl12xx_set_platform_data() for it.
> 
> Unfortunately, there is still one machine that uses a board file
> in combination with this wl12xx: da850-evm calls
> wl12xx_set_platform_data() from a legacy board file without DT.
> 
> We have DT support for this file as well, but if I remember correctly,
> it is somewhat incomplete and we don't want to remove the file
> yet. Adding Sekhar and Kevin to Cc here for confirmation.

Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the
platform data a little bit longer. But nothing stopping us moving
all the other ones to use a proper device tree based configuration.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Tuesday 10 March 2015 07:28:05 Tony Lindgren wrote:
> 
> Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the
> platform data a little bit longer. But nothing stopping us moving
> all the other ones to use a proper device tree based configuration.
> 

For all I can tell, the t3730 board file does not support wl12xx
at the moment, only the dts file does.

If we do what Sekhar suggested and drop wl12xx support from
the DA850 EVM board file, we can fix all wlcore users.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [150310 08:48]:
> On Tuesday 10 March 2015 07:28:05 Tony Lindgren wrote:
> > 
> > Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the
> > platform data a little bit longer. But nothing stopping us moving
> > all the other ones to use a proper device tree based configuration.
> > 
> 
> For all I can tell, the t3730 board file does not support wl12xx
> at the moment, only the dts file does.

Hmm strange, it seems to configure the wlan_rst pin. Maybe there
are variants with different WLAN module. But yeah, seems to be
unused.

That still leaves configuring all the dts users of the
pdata-quirks.c legacy_init_wl12xx() with proper dts before
removing it:

$ git grep legacy_init_wl12xx pdata-quirks.c
pdata-quirks.c:static void __init __used legacy_init_wl12xx(unsigned ref_clock,
pdata-quirks.c:static inline void legacy_init_wl12xx(unsigned ref_clock,
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 136);
pdata-quirks.c: legacy_init_wl12xx(0, 0, 177);
pdata-quirks.c: legacy_init_wl12xx(0, 0, 136);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 149);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_26, 0, 162);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 145);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_26,
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 53);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 41);
pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 31);

> If we do what Sekhar suggested and drop wl12xx support from
> the DA850 EVM board file, we can fix all wlcore users.

I guess Sekhar knows the da850 evm status the best. So up to
you guys if wl12xx is not being used on legacy da850.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
>> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
>> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
>> >> > if that's enough to clean out the platform_data handling from the wlcore
>> >> > driver, it's good enough as a start.
>> >> not sure i'm following - can you elaborate?
>> >>
>> >> i'll summarize the way i see it. please correct me if i'm wrong.
>> >>
>> >> both wl18xx and wl12xx use the platform data to get the irq number.
>> >> wl12xx (only) also needs some additional clock definitions to be
>> >> passed. there's currently some issue with specifying some the of clock
>> >> sources, so i preferred starting only with (the simpler) wl18xx
>> >> bindings.
>> >>
>> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
>> >> data (i.e. irq) can be passed by the new DT bindings.
>> >> however, for platforms with wl12xx, we still need to pass the clock
>> >> definitions (along with the irq), so we have to keep
>> >> legacy_init_wl12xx for the time being (and that's also why we have to
>> >> currently keep the platform_data handling in the wlcore driver)
>> >>
>> >> do you have something else in mind?
>> >
>> > I think what Arnd is saying is we've now removed all the wl12xx using
>> > legacy platforms, so all of them can boot with just data from dts.
>
> Right, that was my idea.
>
>> I don't think that's the case (unless i'm missing something).
>> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
>> initializes wl12xx device.
>
> This one is just like the igep0030, as Tony was saying: the board
> boots from device tree already, so now that we have a binding for
> it, we can remove the wl12xx_set_platform_data() for it.
>
i think the wl12xx_set_platform_data() name created some confusion -
it is used to pass platform data for both wl12xx and wl18xx devices.
(this confusion is all around the wlcore driver as well, due to the
code evolution)

the binding i added is for wl18xx only (there is no wl12xx binding yet).
the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
so i don't see how we can remove these wl12xx_set_platform_data()
calls before we have wl12xx bindings in-place as well.

> Unfortunately, there is still one machine that uses a board file
> in combination with this wl12xx: da850-evm calls
> wl12xx_set_platform_data() from a legacy board file without DT.
>
ditto.

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote:
> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> >> >> > if that's enough to clean out the platform_data handling from the wlcore
> >> >> > driver, it's good enough as a start.
> >> >> not sure i'm following - can you elaborate?
> >> >>
> >> >> i'll summarize the way i see it. please correct me if i'm wrong.
> >> >>
> >> >> both wl18xx and wl12xx use the platform data to get the irq number.
> >> >> wl12xx (only) also needs some additional clock definitions to be
> >> >> passed. there's currently some issue with specifying some the of clock
> >> >> sources, so i preferred starting only with (the simpler) wl18xx
> >> >> bindings.
> >> >>
> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> >> >> data (i.e. irq) can be passed by the new DT bindings.
> >> >> however, for platforms with wl12xx, we still need to pass the clock
> >> >> definitions (along with the irq), so we have to keep
> >> >> legacy_init_wl12xx for the time being (and that's also why we have to
> >> >> currently keep the platform_data handling in the wlcore driver)
> >> >>
> >> >> do you have something else in mind?
> >> >
> >> > I think what Arnd is saying is we've now removed all the wl12xx using
> >> > legacy platforms, so all of them can boot with just data from dts.
> >
> > Right, that was my idea.
> >
> >> I don't think that's the case (unless i'm missing something).
> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
> >> initializes wl12xx device.
> >
> > This one is just like the igep0030, as Tony was saying: the board
> > boots from device tree already, so now that we have a binding for
> > it, we can remove the wl12xx_set_platform_data() for it.
> >
> i think the wl12xx_set_platform_data() name created some confusion -
> it is used to pass platform data for both wl12xx and wl18xx devices.
> (this confusion is all around the wlcore driver as well, due to the
> code evolution)
> 
> the binding i added is for wl18xx only (there is no wl12xx binding yet).
> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
> so i don't see how we can remove these wl12xx_set_platform_data()
> calls before we have wl12xx bindings in-place as well.

What is missing for that binding then? I keep getting confused here,
but I thought that they share the implementation that looks at the
platform data.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote:
>> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
>> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
>> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
>> >> >> > if that's enough to clean out the platform_data handling from the wlcore
>> >> >> > driver, it's good enough as a start.
>> >> >> not sure i'm following - can you elaborate?
>> >> >>
>> >> >> i'll summarize the way i see it. please correct me if i'm wrong.
>> >> >>
>> >> >> both wl18xx and wl12xx use the platform data to get the irq number.
>> >> >> wl12xx (only) also needs some additional clock definitions to be
>> >> >> passed. there's currently some issue with specifying some the of clock
>> >> >> sources, so i preferred starting only with (the simpler) wl18xx
>> >> >> bindings.
>> >> >>
>> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
>> >> >> data (i.e. irq) can be passed by the new DT bindings.
>> >> >> however, for platforms with wl12xx, we still need to pass the clock
>> >> >> definitions (along with the irq), so we have to keep
>> >> >> legacy_init_wl12xx for the time being (and that's also why we have to
>> >> >> currently keep the platform_data handling in the wlcore driver)
>> >> >>
>> >> >> do you have something else in mind?
>> >> >
>> >> > I think what Arnd is saying is we've now removed all the wl12xx using
>> >> > legacy platforms, so all of them can boot with just data from dts.
>> >
>> > Right, that was my idea.
>> >
>> >> I don't think that's the case (unless i'm missing something).
>> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
>> >> initializes wl12xx device.
>> >
>> > This one is just like the igep0030, as Tony was saying: the board
>> > boots from device tree already, so now that we have a binding for
>> > it, we can remove the wl12xx_set_platform_data() for it.
>> >
>> i think the wl12xx_set_platform_data() name created some confusion -
>> it is used to pass platform data for both wl12xx and wl18xx devices.
>> (this confusion is all around the wlcore driver as well, due to the
>> code evolution)
>>
>> the binding i added is for wl18xx only (there is no wl12xx binding yet).
>> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
>> so i don't see how we can remove these wl12xx_set_platform_data()
>> calls before we have wl12xx bindings in-place as well.
>
> What is missing for that binding then? I keep getting confused here,
> but I thought that they share the implementation that looks at the
> platform data.
>
they both get the same wl12xx_platform_data struct, but only wl12xx
cares about the clocks-related fields.
the bindings i added parses only the irq.

(Luca tried previously to upstream wl12xx DT support along with the
required clock DT changes, but got some rejections, mainly wrt. clock
stuff.
e.g. http://thread.gmane.org/gmane.linux.kernel/1520752
that's why i preferred starting with "easier" wl18xx bindings only)

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Eliad Peller <eliad@wizery.com> [150310 09:11]:
> On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote:
> >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
> >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> >> >> >> > if that's enough to clean out the platform_data handling from the wlcore
> >> >> >> > driver, it's good enough as a start.
> >> >> >> not sure i'm following - can you elaborate?
> >> >> >>
> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong.
> >> >> >>
> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number.
> >> >> >> wl12xx (only) also needs some additional clock definitions to be
> >> >> >> passed. there's currently some issue with specifying some the of clock
> >> >> >> sources, so i preferred starting only with (the simpler) wl18xx
> >> >> >> bindings.
> >> >> >>
> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> >> >> >> data (i.e. irq) can be passed by the new DT bindings.
> >> >> >> however, for platforms with wl12xx, we still need to pass the clock
> >> >> >> definitions (along with the irq), so we have to keep
> >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to
> >> >> >> currently keep the platform_data handling in the wlcore driver)
> >> >> >>
> >> >> >> do you have something else in mind?
> >> >> >
> >> >> > I think what Arnd is saying is we've now removed all the wl12xx using
> >> >> > legacy platforms, so all of them can boot with just data from dts.
> >> >
> >> > Right, that was my idea.
> >> >
> >> >> I don't think that's the case (unless i'm missing something).
> >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
> >> >> initializes wl12xx device.
> >> >
> >> > This one is just like the igep0030, as Tony was saying: the board
> >> > boots from device tree already, so now that we have a binding for
> >> > it, we can remove the wl12xx_set_platform_data() for it.
> >> >
> >> i think the wl12xx_set_platform_data() name created some confusion -
> >> it is used to pass platform data for both wl12xx and wl18xx devices.
> >> (this confusion is all around the wlcore driver as well, due to the
> >> code evolution)
> >>
> >> the binding i added is for wl18xx only (there is no wl12xx binding yet).
> >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
> >> so i don't see how we can remove these wl12xx_set_platform_data()
> >> calls before we have wl12xx bindings in-place as well.
> >
> > What is missing for that binding then? I keep getting confused here,
> > but I thought that they share the implementation that looks at the
> > platform data.
> >
> they both get the same wl12xx_platform_data struct, but only wl12xx
> cares about the clocks-related fields.
> the bindings i added parses only the irq.
> 
> (Luca tried previously to upstream wl12xx DT support along with the
> required clock DT changes, but got some rejections, mainly wrt. clock
> stuff.
> e.g. http://thread.gmane.org/gmane.linux.kernel/1520752
> that's why i preferred starting with "easier" wl18xx bindings only)

I believe we did not have clock bindings back then, now it's simple
to get the clock. If it's some internal clock to the wl12xx, then
that's a different story, it should be just hidden behind a compatible
flag then.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Tue, Mar 10, 2015 at 6:18 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Eliad Peller <eliad@wizery.com> [150310 09:11]:
>> On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote:
>> >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
>> >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
>> >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
>> >> >> >> > if that's enough to clean out the platform_data handling from the wlcore
>> >> >> >> > driver, it's good enough as a start.
>> >> >> >> not sure i'm following - can you elaborate?
>> >> >> >>
>> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong.
>> >> >> >>
>> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number.
>> >> >> >> wl12xx (only) also needs some additional clock definitions to be
>> >> >> >> passed. there's currently some issue with specifying some the of clock
>> >> >> >> sources, so i preferred starting only with (the simpler) wl18xx
>> >> >> >> bindings.
>> >> >> >>
>> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
>> >> >> >> data (i.e. irq) can be passed by the new DT bindings.
>> >> >> >> however, for platforms with wl12xx, we still need to pass the clock
>> >> >> >> definitions (along with the irq), so we have to keep
>> >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to
>> >> >> >> currently keep the platform_data handling in the wlcore driver)
>> >> >> >>
>> >> >> >> do you have something else in mind?
>> >> >> >
>> >> >> > I think what Arnd is saying is we've now removed all the wl12xx using
>> >> >> > legacy platforms, so all of them can boot with just data from dts.
>> >> >
>> >> > Right, that was my idea.
>> >> >
>> >> >> I don't think that's the case (unless i'm missing something).
>> >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
>> >> >> initializes wl12xx device.
>> >> >
>> >> > This one is just like the igep0030, as Tony was saying: the board
>> >> > boots from device tree already, so now that we have a binding for
>> >> > it, we can remove the wl12xx_set_platform_data() for it.
>> >> >
>> >> i think the wl12xx_set_platform_data() name created some confusion -
>> >> it is used to pass platform data for both wl12xx and wl18xx devices.
>> >> (this confusion is all around the wlcore driver as well, due to the
>> >> code evolution)
>> >>
>> >> the binding i added is for wl18xx only (there is no wl12xx binding yet).
>> >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
>> >> so i don't see how we can remove these wl12xx_set_platform_data()
>> >> calls before we have wl12xx bindings in-place as well.
>> >
>> > What is missing for that binding then? I keep getting confused here,
>> > but I thought that they share the implementation that looks at the
>> > platform data.
>> >
>> they both get the same wl12xx_platform_data struct, but only wl12xx
>> cares about the clocks-related fields.
>> the bindings i added parses only the irq.
>>
>> (Luca tried previously to upstream wl12xx DT support along with the
>> required clock DT changes, but got some rejections, mainly wrt. clock
>> stuff.
>> e.g. http://thread.gmane.org/gmane.linux.kernel/1520752
>> that's why i preferred starting with "easier" wl18xx bindings only)
>
> I believe we did not have clock bindings back then, now it's simple
> to get the clock. If it's some internal clock to the wl12xx, then
> that's a different story, it should be just hidden behind a compatible
> flag then.
>
i'm really not familiar with the common clock framework, but there
still doesn't seem to be a way to determine whether a clock is XTAL or
not (which is what Luca's patch was about). should we use compatible
flag in such case? i'm not sure it sounds right.

anyway, i'd really prefer, if possible, starting with the wl18xx
bindings, and defer the wl12xx complications to later on.
(i also need to find some wl12xx card in order to actually test the
patches once i'll have them...)

we do have to make sure these wl18xx bindings are future-compatible
with the wl12xx ones, but i think the current bindings are pretty much
standard (and are actually a subset of the bindings needed by wl12xx),
so it should be fine.

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Eliad Peller <eliad@wizery.com> [150310 10:01]:
> On Tue, Mar 10, 2015 at 6:18 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Eliad Peller <eliad@wizery.com> [150310 09:11]:
> >> On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote:
> >> >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
> >> >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file,
> >> >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but
> >> >> >> >> > if that's enough to clean out the platform_data handling from the wlcore
> >> >> >> >> > driver, it's good enough as a start.
> >> >> >> >> not sure i'm following - can you elaborate?
> >> >> >> >>
> >> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong.
> >> >> >> >>
> >> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number.
> >> >> >> >> wl12xx (only) also needs some additional clock definitions to be
> >> >> >> >> passed. there's currently some issue with specifying some the of clock
> >> >> >> >> sources, so i preferred starting only with (the simpler) wl18xx
> >> >> >> >> bindings.
> >> >> >> >>
> >> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the
> >> >> >> >> data (i.e. irq) can be passed by the new DT bindings.
> >> >> >> >> however, for platforms with wl12xx, we still need to pass the clock
> >> >> >> >> definitions (along with the irq), so we have to keep
> >> >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to
> >> >> >> >> currently keep the platform_data handling in the wlcore driver)
> >> >> >> >>
> >> >> >> >> do you have something else in mind?
> >> >> >> >
> >> >> >> > I think what Arnd is saying is we've now removed all the wl12xx using
> >> >> >> > legacy platforms, so all of them can boot with just data from dts.
> >> >> >
> >> >> > Right, that was my idea.
> >> >> >
> >> >> >> I don't think that's the case (unless i'm missing something).
> >> >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
> >> >> >> initializes wl12xx device.
> >> >> >
> >> >> > This one is just like the igep0030, as Tony was saying: the board
> >> >> > boots from device tree already, so now that we have a binding for
> >> >> > it, we can remove the wl12xx_set_platform_data() for it.
> >> >> >
> >> >> i think the wl12xx_set_platform_data() name created some confusion -
> >> >> it is used to pass platform data for both wl12xx and wl18xx devices.
> >> >> (this confusion is all around the wlcore driver as well, due to the
> >> >> code evolution)
> >> >>
> >> >> the binding i added is for wl18xx only (there is no wl12xx binding yet).
> >> >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards.
> >> >> so i don't see how we can remove these wl12xx_set_platform_data()
> >> >> calls before we have wl12xx bindings in-place as well.
> >> >
> >> > What is missing for that binding then? I keep getting confused here,
> >> > but I thought that they share the implementation that looks at the
> >> > platform data.
> >> >
> >> they both get the same wl12xx_platform_data struct, but only wl12xx
> >> cares about the clocks-related fields.
> >> the bindings i added parses only the irq.
> >>
> >> (Luca tried previously to upstream wl12xx DT support along with the
> >> required clock DT changes, but got some rejections, mainly wrt. clock
> >> stuff.
> >> e.g. http://thread.gmane.org/gmane.linux.kernel/1520752
> >> that's why i preferred starting with "easier" wl18xx bindings only)
> >
> > I believe we did not have clock bindings back then, now it's simple
> > to get the clock. If it's some internal clock to the wl12xx, then
> > that's a different story, it should be just hidden behind a compatible
> > flag then.
> >
> i'm really not familiar with the common clock framework, but there
> still doesn't seem to be a way to determine whether a clock is XTAL or
> not (which is what Luca's patch was about). should we use compatible
> flag in such case? i'm not sure it sounds right.
> 
> anyway, i'd really prefer, if possible, starting with the wl18xx
> bindings, and defer the wl12xx complications to later on.
> (i also need to find some wl12xx card in order to actually test the
> patches once i'll have them...)
> 
> we do have to make sure these wl18xx bindings are future-compatible
> with the wl12xx ones, but i think the current bindings are pretty much
> standard (and are actually a subset of the bindings needed by wl12xx),
> so it should be fine.

Well how about add just the parsing of the clock and assume it's always
WL12XX_REFCLOCK_38 for now as that's the only thing we're currently
using. Then we can add a property or compatible value if using something
else as needed.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Tuesday 10 March 2015 10:35:50 Tony Lindgren wrote:
> * Eliad Peller <eliad@wizery.com> [150310 10:01]:
> > i'm really not familiar with the common clock framework, but there
> > still doesn't seem to be a way to determine whether a clock is XTAL or
> > not (which is what Luca's patch was about). should we use compatible
> > flag in such case? i'm not sure it sounds right.
> > 
> > anyway, i'd really prefer, if possible, starting with the wl18xx
> > bindings, and defer the wl12xx complications to later on.
> > (i also need to find some wl12xx card in order to actually test the
> > patches once i'll have them...)
> > 
> > we do have to make sure these wl18xx bindings are future-compatible
> > with the wl12xx ones, but i think the current bindings are pretty much
> > standard (and are actually a subset of the bindings needed by wl12xx),
> > so it should be fine.
> 
> Well how about add just the parsing of the clock and assume it's always
> WL12XX_REFCLOCK_38 for now as that's the only thing we're currently
> using. Then we can add a property or compatible value if using something
> else as needed.
> 

We have two exceptions:

static void __init omap3_zoom_legacy_init(void)
{
        legacy_init_wl12xx(WL12XX_REFCLOCK_26, 0, 162);
}

static void __init omap4_sdp_legacy_init(void)
{
        legacy_init_wl12xx(WL12XX_REFCLOCK_26,
                           WL12XX_TCXOCLOCK_26, 53);
}

Where do those clocks come from? If these are always fixed-rate
clocks coming from an external clock provider, using a separate
compatible string in the clock provider would seem reasonable to
me, but we can do that once we have to: none of the machines
we support use an XTAL clock input.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Tony,

On Tue, Mar 10, 2015 at 6:35 PM, Tony Lindgren <tony@atomide.com> wrote:
>>
>> we do have to make sure these wl18xx bindings are future-compatible
>> with the wl12xx ones, but i think the current bindings are pretty much
>> standard (and are actually a subset of the bindings needed by wl12xx),
>> so it should be fine.
>
> Well how about add just the parsing of the clock and assume it's always
> WL12XX_REFCLOCK_38 for now as that's the only thing we're currently
> using. Then we can add a property or compatible value if using something
> else as needed.
>

What do you mean by parsing here? IIUC there isn't a clock driver for
these clocks and are setup directly in the
drivers/net/wireless/ti/wl12xx/main.c driver.

So you can't make the WLAN chip dev node a consumer of these clocks by
adding a phandle to a clock provider and clock specifiers since there
isn't a provider to be referenced in the DT. Or did I misunderstand?

> Regards,
>
> Tony

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote:
> Hello Tony,
> 
> On Tue, Mar 10, 2015 at 6:35 PM, Tony Lindgren <tony@atomide.com> wrote:
> >>
> >> we do have to make sure these wl18xx bindings are future-compatible
> >> with the wl12xx ones, but i think the current bindings are pretty much
> >> standard (and are actually a subset of the bindings needed by wl12xx),
> >> so it should be fine.
> >
> > Well how about add just the parsing of the clock and assume it's always
> > WL12XX_REFCLOCK_38 for now as that's the only thing we're currently
> > using. Then we can add a property or compatible value if using something
> > else as needed.
> >
> 
> What do you mean by parsing here? IIUC there isn't a clock driver for
> these clocks and are setup directly in the
> drivers/net/wireless/ti/wl12xx/main.c driver.
> 
> So you can't make the WLAN chip dev node a consumer of these clocks by
> adding a phandle to a clock provider and clock specifiers since there
> isn't a provider to be referenced in the DT. Or did I misunderstand?

As I understand it, the clock signal is provided by an external oscillator,
which we can easily model in DT, and then you call clk_get_rate on that.

If there is no external clock provider for this chip and the clocks
are provided by the device itself, then all we need is a clock-frequency
property in the device node.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Arnd,

On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote:
>>
>> What do you mean by parsing here? IIUC there isn't a clock driver for
>> these clocks and are setup directly in the
>> drivers/net/wireless/ti/wl12xx/main.c driver.
>>
>> So you can't make the WLAN chip dev node a consumer of these clocks by
>> adding a phandle to a clock provider and clock specifiers since there
>> isn't a provider to be referenced in the DT. Or did I misunderstand?
>
> As I understand it, the clock signal is provided by an external oscillator,

According to [0], it seems the chip can be connected to both external
oscillators or internal clocks provided by the chip itself.

> which we can easily model in DT, and then you call clk_get_rate on that.
>

Right, my point wast that this can be done only if the external
oscillator have a proper clock driver / provider which I don't think
is the case here. Most of this stuff predates the common clock
framework.

Or at least Luciano Coelho had a patch on his series to make the
wilink driver a clock provider itself by registering the refclock and
tcxoclock clocks [0].

Luciano also had patches for:

* Adding the clock provider dev node in the DTS [1]
* Have a table to map the clock rate with the FW configuration values [2]
* Getting the clock from DT and the rate as you said to configure the
firmware accordingly [3]

I think that patch [0] should not be needed since for external clocks,
the IP providing the clocks should have its own clock driver and for
internal clocks, a property should be used instead as you said.

> If there is no external clock provider for this chip and the clocks
> are provided by the device itself, then all we need is a clock-frequency
> property in the device node.
>

Agreed, IIUC Luciano wanted to expose the internal clocks by
registering in the common clock framework but if those clocks are not
really accessible from outside the wlan chip, then I also think that a
device node property should be used instead.

>         Arnd
> --

Best regards,
Javier

[0]: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-July/037139.html
[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187794.html
[2]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181594.html
[3]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181591.html

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Wed, Mar 11, 2015 at 1:34 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> I think that patch [0] should not be needed since for external clocks,
> the IP providing the clocks should have its own clock driver and for
> internal clocks, a property should be used instead as you said.
>
>> If there is no external clock provider for this chip and the clocks
>> are provided by the device itself, then all we need is a clock-frequency
>> property in the device node.
>>
>
> Agreed, IIUC Luciano wanted to expose the internal clocks by
> registering in the common clock framework but if those clocks are not
> really accessible from outside the wlan chip, then I also think that a
> device node property should be used instead.
>
how should i describe multiple clock-frequency properties (there are 2
relevant clocks) in this case?

does something like the following makes sense?

wlcore: wlcore at 2 {
    ...
    refclock: refclock {
        compatible = "fixed-clock";
        #clock-cells = <0>;
        clock-frequency = <38400000>;
    };
}

Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Wednesday 11 March 2015 14:12:54 Eliad Peller wrote:
> On Wed, Mar 11, 2015 at 1:34 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
> > I think that patch [0] should not be needed since for external clocks,
> > the IP providing the clocks should have its own clock driver and for
> > internal clocks, a property should be used instead as you said.
> >
> >> If there is no external clock provider for this chip and the clocks
> >> are provided by the device itself, then all we need is a clock-frequency
> >> property in the device node.
> >>
> >
> > Agreed, IIUC Luciano wanted to expose the internal clocks by
> > registering in the common clock framework but if those clocks are not
> > really accessible from outside the wlan chip, then I also think that a
> > device node property should be used instead.
> >
> how should i describe multiple clock-frequency properties (there are 2
> relevant clocks) in this case?
> 
> does something like the following makes sense?
> 
> wlcore: wlcore at 2 {
>     ...
>     refclock: refclock {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <38400000>;
>     };
> }

I would put that clock node on the top level of the DT, as you are
describing an external clock input here, but other than that, it looks
good.

I would do the binding in a way that mandates either a "clocks"
reference to an external clock provider in case of a XTAL or
a "clock-frequency" property. In the first case, you can use
the "clock-names" property to identify the "ref" and "txco"
clock inputs, in the second case we could either decide
to have a single property with two cells, or have named
properties like

	wlcore at 2 {
		interrupts = < ... >;
		tcxo-clock-frequency = <38400000>;
		ref-clock-frequency = <19200000>;
	}

I don't know which combinations are possible here. I did notice
that most of the references in the board hacks use '0' as the
tcxo clock value, which happens to be the same as
WL12XX_TCXOCLOCK_19_2, but could also meant that no tcxo clock
is used.

	Arnd 

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Wednesday 11 March 2015 12:34:03 Javier Martinez Canillas wrote:
> Hello Arnd,
> 
> On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote:
> >>
> >> What do you mean by parsing here? IIUC there isn't a clock driver for
> >> these clocks and are setup directly in the
> >> drivers/net/wireless/ti/wl12xx/main.c driver.
> >>
> >> So you can't make the WLAN chip dev node a consumer of these clocks by
> >> adding a phandle to a clock provider and clock specifiers since there
> >> isn't a provider to be referenced in the DT. Or did I misunderstand?
> >
> > As I understand it, the clock signal is provided by an external oscillator,
> 
> According to [0], it seems the chip can be connected to both external
> oscillators or internal clocks provided by the chip itself.

I see, that part wasn't clear to me.

> > which we can easily model in DT, and then you call clk_get_rate on that.
> >
> 
> Right, my point wast that this can be done only if the external
> oscillator have a proper clock driver / provider which I don't think
> is the case here. Most of this stuff predates the common clock
> framework.
> 
> Or at least Luciano Coelho had a patch on his series to make the
> wilink driver a clock provider itself by registering the refclock and
> tcxoclock clocks [0].

I guess we should only do this if the clocks from the wilink device
might be used by some other device.

> Luciano also had patches for:
> 
> * Adding the clock provider dev node in the DTS [1]
> * Have a table to map the clock rate with the FW configuration values [2]
> * Getting the clock from DT and the rate as you said to configure the
> firmware accordingly [3]
> 
> I think that patch [0] should not be needed since for external clocks,
> the IP providing the clocks should have its own clock driver and for
> internal clocks, a property should be used instead as you said.

Right.

> > If there is no external clock provider for this chip and the clocks
> > are provided by the device itself, then all we need is a clock-frequency
> > property in the device node.
> >
> 
> Agreed, IIUC Luciano wanted to expose the internal clocks by
> registering in the common clock framework but if those clocks are not
> really accessible from outside the wlan chip, then I also think that a
> device node property should be used instead.

If I understand this right, that measn we can easily distinguish between
an external XTAL clock and the internal clock by they way they are
described in the DT: for the internal clock, we just provide a clock-frequency
property, while the external clock would be referenced through a clocks
property.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Arnd,

On Wed, Mar 11, 2015 at 1:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 11 March 2015 12:34:03 Javier Martinez Canillas wrote:
>> Hello Arnd,
>>
>> On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote:
>> >>
>> >> What do you mean by parsing here? IIUC there isn't a clock driver for
>> >> these clocks and are setup directly in the
>> >> drivers/net/wireless/ti/wl12xx/main.c driver.
>> >>
>> >> So you can't make the WLAN chip dev node a consumer of these clocks by
>> >> adding a phandle to a clock provider and clock specifiers since there
>> >> isn't a provider to be referenced in the DT. Or did I misunderstand?
>> >
>> > As I understand it, the clock signal is provided by an external oscillator,
>>
>> According to [0], it seems the chip can be connected to both external
>> oscillators or internal clocks provided by the chip itself.
>
> I see, that part wasn't clear to me.
>

Yeah, it was not clear to me either before reading Luciano's commit message.

[snip]

>> > If there is no external clock provider for this chip and the clocks
>> > are provided by the device itself, then all we need is a clock-frequency
>> > property in the device node.
>> >
>>
>> Agreed, IIUC Luciano wanted to expose the internal clocks by
>> registering in the common clock framework but if those clocks are not
>> really accessible from outside the wlan chip, then I also think that a
>> device node property should be used instead.
>
> If I understand this right, that measn we can easily distinguish between
> an external XTAL clock and the internal clock by they way they are
> described in the DT: for the internal clock, we just provide a clock-frequency
> property, while the external clock would be referenced through a clocks
> property.
>

That's my understanding as well.

Right now it seems that all boards in mainline with a WiLink6 part are
using internal clocks. So as a first step I think that adding an
optional refclock-frequency and tcxoclock-frequency properties should
be enough.

It would be good if the driver supports getting the refclock and
tcxoclock from an external provider in case a board gets these from
external clocks but that can be done as a followup if there are boards
in the future using that design.

But please bear in mind that I'm not familiar with the clock handling
in WiLink6 since the WiLink8 part used in the IGEP boards does not
need these clocks and I only looked at Luciano's previous patches and
the WiLink today driver today. So it would be good if Eliad can double
check my assumptions to see if those are correct.

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote:
> 
> Right now it seems that all boards in mainline with a WiLink6 part are
> using internal clocks. So as a first step I think that adding an
> optional refclock-frequency and tcxoclock-frequency properties should
> be enough.
> 
> It would be good if the driver supports getting the refclock and
> tcxoclock from an external provider in case a board gets these from
> external clocks but that can be done as a followup if there are boards
> in the future using that design.
> 
> But please bear in mind that I'm not familiar with the clock handling
> in WiLink6 since the WiLink8 part used in the IGEP boards does not
> need these clocks and I only looked at Luciano's previous patches and
> the WiLink today driver today. So it would be good if Eliad can double
> check my assumptions to see if those are correct.

Sounds good. I'd also be fine with not implementing the case for
external clocks in the code until we need (and can test) it, but
I think it should be specified in the binding from the start.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote:
>>
>> Right now it seems that all boards in mainline with a WiLink6 part are
>> using internal clocks. So as a first step I think that adding an
>> optional refclock-frequency and tcxoclock-frequency properties should
>> be enough.
>>
>> It would be good if the driver supports getting the refclock and
>> tcxoclock from an external provider in case a board gets these from
>> external clocks but that can be done as a followup if there are boards
>> in the future using that design.
>>
>> But please bear in mind that I'm not familiar with the clock handling
>> in WiLink6 since the WiLink8 part used in the IGEP boards does not
>> need these clocks and I only looked at Luciano's previous patches and
>> the WiLink today driver today. So it would be good if Eliad can double
>> check my assumptions to see if those are correct.
>
> Sounds good. I'd also be fine with not implementing the case for
> external clocks in the code until we need (and can test) it, but
> I think it should be specified in the binding from the start.
>

Agreed.

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

On Wed, Mar 11, 2015 at 3:21 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote:
>>>
>>> Right now it seems that all boards in mainline with a WiLink6 part are
>>> using internal clocks. So as a first step I think that adding an
>>> optional refclock-frequency and tcxoclock-frequency properties should
>>> be enough.
>>>
>>> It would be good if the driver supports getting the refclock and
>>> tcxoclock from an external provider in case a board gets these from
>>> external clocks but that can be done as a followup if there are boards
>>> in the future using that design.
>>>
>>> But please bear in mind that I'm not familiar with the clock handling
>>> in WiLink6 since the WiLink8 part used in the IGEP boards does not
>>> need these clocks and I only looked at Luciano's previous patches and
>>> the WiLink today driver today. So it would be good if Eliad can double
>>> check my assumptions to see if those are correct.
>>
sounds right. that's what i know as well.

>> Sounds good. I'd also be fine with not implementing the case for
>> external clocks in the code until we need (and can test) it, but
>> I think it should be specified in the binding from the start.
>>
> Agreed.
>
great. so i'll implement the internal clocks case only.

thanks,
Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Tony Lindgren]

* Eliad Peller <eliad@wizery.com> [150311 06:39]:
> On Wed, Mar 11, 2015 at 3:21 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
> > On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote:
> >>>
> >>> Right now it seems that all boards in mainline with a WiLink6 part are
> >>> using internal clocks. So as a first step I think that adding an
> >>> optional refclock-frequency and tcxoclock-frequency properties should
> >>> be enough.
> >>>
> >>> It would be good if the driver supports getting the refclock and
> >>> tcxoclock from an external provider in case a board gets these from
> >>> external clocks but that can be done as a followup if there are boards
> >>> in the future using that design.
> >>>
> >>> But please bear in mind that I'm not familiar with the clock handling
> >>> in WiLink6 since the WiLink8 part used in the IGEP boards does not
> >>> need these clocks and I only looked at Luciano's previous patches and
> >>> the WiLink today driver today. So it would be good if Eliad can double
> >>> check my assumptions to see if those are correct.
> >>
> sounds right. that's what i know as well.
> 
> >> Sounds good. I'd also be fine with not implementing the case for
> >> external clocks in the code until we need (and can test) it, but
> >> I think it should be specified in the binding from the start.
> >>
> > Agreed.
> >
> great. so i'll implement the internal clocks case only.

OK great sounds good to me also.

Regards,

Tony

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Sekhar Nori]

On Tuesday 10 March 2015 07:41 PM, Arnd Bergmann wrote:
> On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote:
>> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote:
>>> * Eliad Peller <eliad@wizery.com> [150309 14:03]:
>>>> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
>>>>>> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>>>>>> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
>>>>>> @@ -64,4 +64,13 @@
>>>>>>         vmmc-supply = <&lbep5clwmc_wlen>;
>>>>>>         bus-width = <4>;
>>>>>>         non-removable;
>>>>>> +
>>>>>> +       #address-cells = <1>;
>>>>>> +       #size-cells = <0>;
>>>>>> +       wlcore: wlcore at 2 {
>>>>>> +               compatible = "ti,wl1835";
>>>>>> +               reg = <2>;
>>>>>> +               interrupt-parent = <&gpio5>;
>>>>>> +               interrupts = <8 IRQ_TYPE_NONE>;
>>>>>> +       };
>>>>>>
>>>>>
>>>>> Why IRQ_TYPE_NONE?
>>>>>
>>>> i simply mirrored the current board file (which only sets the irq number).
>>>>
>>>>> I was expecting you to remove all calls to legacy_init_wl12xx from this file,
>>>>> including the ones for wl12xx aside from the wl18xx ones you removed, but
>>>>> if that's enough to clean out the platform_data handling from the wlcore
>>>>> driver, it's good enough as a start.
>>>> not sure i'm following - can you elaborate?
>>>>
>>>> i'll summarize the way i see it. please correct me if i'm wrong.
>>>>
>>>> both wl18xx and wl12xx use the platform data to get the irq number.
>>>> wl12xx (only) also needs some additional clock definitions to be
>>>> passed. there's currently some issue with specifying some the of clock
>>>> sources, so i preferred starting only with (the simpler) wl18xx
>>>> bindings.
>>>>
>>>> for platforms with wl18xx, we can remove the pdata-quirk, as all the
>>>> data (i.e. irq) can be passed by the new DT bindings.
>>>> however, for platforms with wl12xx, we still need to pass the clock
>>>> definitions (along with the irq), so we have to keep
>>>> legacy_init_wl12xx for the time being (and that's also why we have to
>>>> currently keep the platform_data handling in the wlcore driver)
>>>>
>>>> do you have something else in mind?
>>>
>>> I think what Arnd is saying is we've now removed all the wl12xx using
>>> legacy platforms, so all of them can boot with just data from dts.
> 
> Right, that was my idea.
> 
>> I don't think that's the case (unless i'm missing something).
>> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which
>> initializes wl12xx device.
> 
> This one is just like the igep0030, as Tony was saying: the board
> boots from device tree already, so now that we have a binding for
> it, we can remove the wl12xx_set_platform_data() for it.
> 
> Unfortunately, there is still one machine that uses a board file
> in combination with this wl12xx: da850-evm calls
> wl12xx_set_platform_data() from a legacy board file without DT.
> 
> We have DT support for this file as well, but if I remember correctly,
> it is somewhat incomplete and we don't want to remove the file
> yet. Adding Sekhar and Kevin to Cc here for confirmation.

Thats true, we do not want to drop legacy booting for DA850 EVM yet.

But if its coming in the way of code consolidation, I do not mind
dropping WLAN support for that board. Anyone who really needs it can add
support to the DA850 EVM DT file.

Thanks,
Sekhar

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Arnd Bergmann]

On Monday 09 March 2015 23:03:30 Eliad Peller wrote:
> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote:
> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts
> >> @@ -64,4 +64,13 @@
> >>         vmmc-supply = <&lbep5clwmc_wlen>;
> >>         bus-width = <4>;
> >>         non-removable;
> >> +
> >> +       #address-cells = <1>;
> >> +       #size-cells = <0>;
> >> +       wlcore: wlcore at 2 {
> >> +               compatible = "ti,wl1835";
> >> +               reg = <2>;
> >> +               interrupt-parent = <&gpio5>;
> >> +               interrupts = <8 IRQ_TYPE_NONE>;
> >> +       };
> >>
> >
> > Why IRQ_TYPE_NONE?
> >
> i simply mirrored the current board file (which only sets the irq number).

The irq type is set in this chunk of code from wlcore_nvs_cb:

        if (wl->platform_quirks & WL12XX_PLATFORM_QUIRK_EDGE_IRQ) {
                irqflags = IRQF_TRIGGER_RISING;
                hardirq_fn = wlcore_hardirq;
        } else {
                irqflags = IRQF_TRIGGER_HIGH | IRQF_ONESHOT;
        }

This means you would replace the platform_quirks with setting the
correct irq type.

	Arnd

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote:
> Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and
> omap3-igep0020-rev-f dts files, instead of defining the
> platform data through the pdata-quirks.
>
> The patch was compile-tested only.
>

You should look at MAINTAINERS or use ./scripts/get_maintainer.pl to
know who should be cc'ed. Specially for this kind of patches where you
are not able to test on the actual platform. Added Enric to cc as well
since he also maintains the IGEP boards an has access to the hw too.

> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---
>  arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +++++++++
>  arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +++++++++
>  arch/arm/mach-omap2/pdata-quirks.c         | 2 --
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
> index cc8bd0c..8e5b44e 100644
> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
> @@ -42,4 +42,13 @@
>         vmmc-supply = <&lbep5clwmc_wlen>;
>         bus-width = <4>;
>         non-removable;
> +
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       wlcore: wlcore at 2 {
> +               compatible = "ti,wl1835";
> +               reg = <2>;
> +               interrupt-parent = <&gpio6>;
> +               interrupts = <17 IRQ_TYPE_NONE>;

As Arnd said, it seems this should be IRQ_TYPE_LEVEL_HIGH to match
what the platform code is currently doing.

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Wed, Mar 11, 2015 at 1:28 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote:
>> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>> @@ -42,4 +42,13 @@
>>         vmmc-supply = <&lbep5clwmc_wlen>;

Something I forgot to mention is that this vmmc-supply is a DT hack
since the WLAN SDIO chip needs a WL_EN signal to be asserted as a part
of the chip power sequencing.

But there wasn't a DT binding to describe that so most boards use the
same hack which is to define a fake fixed-regulator with an enable
GPIO just to toggle that pin.

But now the MMC subsystem has support for power sequence providers and
the pwrseq-simple driver
(Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has
support for reset GPIOs so you may want to change that as well.

In fact, the pwrseq-simple also has support for an external clock (and
can be extended to support more than one) so if the clocks are not
internal to the wl12xx, maybe these should be defined in the
pwrseq-simple dev node assuming that there is a clock driver for them.

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

hi Javier,

On Wed, Mar 11, 2015 at 3:19 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Eliad,
>
> On Wed, Mar 11, 2015 at 1:28 AM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>> On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote:
>>> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>>> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>>> @@ -42,4 +42,13 @@
>>>         vmmc-supply = <&lbep5clwmc_wlen>;
>
> Something I forgot to mention is that this vmmc-supply is a DT hack
> since the WLAN SDIO chip needs a WL_EN signal to be asserted as a part
> of the chip power sequencing.
>
> But there wasn't a DT binding to describe that so most boards use the
> same hack which is to define a fake fixed-regulator with an enable
> GPIO just to toggle that pin.
>
> But now the MMC subsystem has support for power sequence providers and
> the pwrseq-simple driver
> (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has
> support for reset GPIOs so you may want to change that as well.
>
interesting. i wasn't aware of it.
but that's for a different time i guess :)

thanks,
Eliad.

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Wed, Mar 11, 2015 at 12:59 PM, Eliad Peller <eliad@wizery.com> wrote:
> On Wed, Mar 11, 2015 at 3:19 AM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>>
>> But there wasn't a DT binding to describe that so most boards use the
>> same hack which is to define a fake fixed-regulator with an enable
>> GPIO just to toggle that pin.
>>
>> But now the MMC subsystem has support for power sequence providers and
>> the pwrseq-simple driver
>> (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has
>> support for reset GPIOs so you may want to change that as well.
>>
> interesting. i wasn't aware of it.
> but that's for a different time i guess :)
>

Indeed, I've on my TODO anyways so I can post a patch on top of your series :-)

> thanks,
> Eliad.

Best regards,
Javier

[PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Eliad Peller]

hi Javier,

On Wed, Mar 11, 2015 at 2:28 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Eliad,
>
> On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote:
>> Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and
>> omap3-igep0020-rev-f dts files, instead of defining the
>> platform data through the pdata-quirks.
>>
>> The patch was compile-tested only.
>>
>
> You should look at MAINTAINERS or use ./scripts/get_maintainer.pl to
> know who should be cc'ed. Specially for this kind of patches where you
> are not able to test on the actual platform. Added Enric to cc as well
> since he also maintains the IGEP boards an has access to the hw too.
>
right. sorry about that.
i assumed adding the relevant mailing lists should be enough, but i
guess i should have added the relevant maintainers as well.

>>  arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +++++++++
>>  arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +++++++++
>>  arch/arm/mach-omap2/pdata-quirks.c         | 2 --
>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>> index cc8bd0c..8e5b44e 100644
>> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
>> @@ -42,4 +42,13 @@
>>         vmmc-supply = <&lbep5clwmc_wlen>;
>>         bus-width = <4>;
>>         non-removable;
>> +
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       wlcore: wlcore at 2 {
>> +               compatible = "ti,wl1835";
>> +               reg = <2>;
>> +               interrupt-parent = <&gpio6>;
>> +               interrupts = <17 IRQ_TYPE_NONE>;
>
> As Arnd said, it seems this should be IRQ_TYPE_LEVEL_HIGH to match
> what the platform code is currently doing.
>
will be fixed.

thanks,
Eliad.