[PATCH 0/2] net: wireless: wcn36xx: OOT platform reference patch for msm8916 / wcn36xx

[PATCH 0/2] net: wireless: wcn36xx: OOT platform reference patch for msm8916 / wcn36xx

[Andy Green]

These patches are not intended for upstreaming.  They are
included as a reference to show how to hook up wcn36xx to
msm platforms using the OOT PIL support needed.

The following series adds Eugene's OOT msm platform shim to
wcn36xx and modifies it to provide wcn36xx platform data
about chip type from Device Tree.

It's useful as an example for how to implement on msm
platforms that do not have all the necessary support
upstreamed yet.

This was tested on msm8916-QRD "phone" dev platform.

---

Andy Green (1):
      net wireless wcn36xx adapt wcnss platform to select module by DT

Eugene Krasnikov (1):
      net wireless wcn36xx add wcnss platform code


 drivers/net/wireless/ath/wcn36xx/Makefile      |    2 
 drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  360 ++++++++++++++++++++++++
 2 files changed, 361 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c

--

[PATCH 1/2] net wireless wcn36xx add wcnss platform code

[Andy Green]

From: Eugene Krasnikov <k.eugene.e@gmail.com>

AG modified to remove regulator handling not needed on msm8916-qrd

Signed-off-by: Eugene Krasnikov <k.eugene.e@gmail.com>
Signed-off-by: Andy Green <andy.green@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/Makefile      |    2 
 drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  357 ++++++++++++++++++++++++
 2 files changed, 358 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c

diff --git a/drivers/net/wireless/ath/wcn36xx/Makefile b/drivers/net/wireless/ath/wcn36xx/Makefile
index 50c43b4..e889f2c 100644
--- a/drivers/net/wireless/ath/wcn36xx/Makefile
+++ b/drivers/net/wireless/ath/wcn36xx/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_WCN36XX) := wcn36xx.o
+obj-$(CONFIG_WCN36XX) := wcn36xx.o wcn36xx-msm.o
 wcn36xx-y +=   main.o \
                dxe.o \
                txrx.o \
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
new file mode 100644
index 0000000..f6f6c83
--- /dev/null
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright (c) 2013 Eugene Krasnikov <k.eugene.e@gmail.com>
+ * Copyright (c) 2013 Qualcomm Atheros, Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <linux/completion.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <soc/qcom/smd.h>
+#include <soc/qcom/smsm.h>
+#include "wcn36xx.h"
+
+#include <soc/qcom/subsystem_restart.h>
+#include <soc/qcom/subsystem_notif.h>
+
+#define MAC_ADDR_0 "wlan/macaddr0"
+
+static void *pil;
+
+struct wcn36xx_msm {
+	struct wcn36xx_platform_ctrl_ops ctrl_ops;
+	struct platform_device *core;
+	void *drv_priv;
+	void (*rsp_cb)(void *drv_priv, void *buf, size_t len);
+	/* SMD related */
+	struct workqueue_struct	*wq;
+	struct work_struct	smd_work;
+	struct completion	smd_compl;
+	smd_channel_t		*smd_ch;
+	struct pinctrl *pinctrl;
+} wmsm;
+
+static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
+{
+	 return smsm_change_state(SMSM_APPS_STATE, clear_mask, set_mask);
+}
+
+static int wcn36xx_msm_get_hw_mac(u8 *addr)
+{
+	const struct firmware *addr_file = NULL;
+	int status;
+	u8 tmp[18];
+	static const u8 qcom_oui[3] = {0x00, 0x0A, 0xF5};
+	static const char *files = {MAC_ADDR_0};
+
+	status = request_firmware(&addr_file, files, &wmsm.core->dev);
+
+	if (status < 0) {
+		/* Assign a random mac with Qualcomm oui */
+		dev_err(&wmsm.core->dev, "Failed (%d) to read macaddress file %s, using a random address instead", status,
+			     files);
+		memcpy(addr, qcom_oui, 3);
+		get_random_bytes(addr + 3, 3);
+	} else {
+		memset(tmp, 0, sizeof(tmp));
+		memcpy(tmp, addr_file->data, sizeof(tmp) - 1);
+		sscanf(tmp, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
+		       &addr[0],
+		       &addr[1],
+		       &addr[2],
+		       &addr[3],
+		       &addr[4],
+		       &addr[5]);
+
+		release_firmware(addr_file);
+	}
+
+	return 0;
+}
+
+static int wcn36xx_msm_smd_send_and_wait(char *buf, size_t len)
+{
+	int avail;
+	int ret = 0;
+
+	avail = smd_write_avail(wmsm.smd_ch);
+
+	if (avail >= len) {
+		avail = smd_write(wmsm.smd_ch, buf, len);
+		if (avail != len) {
+			dev_err(&wmsm.core->dev,
+				"Cannot write to SMD channel\n");
+			ret = -EAGAIN;
+			goto out;
+		}
+	} else {
+		dev_err(&wmsm.core->dev,
+			"SMD channel can accept only %d bytes\n", avail);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static void wcn36xx_msm_smd_notify(void *data, unsigned event)
+{
+	struct wcn36xx_msm *wmsm_priv = (struct wcn36xx_msm *)data;
+
+	switch (event) {
+	case SMD_EVENT_OPEN:
+		complete(&wmsm_priv->smd_compl);
+		break;
+	case SMD_EVENT_DATA:
+		queue_work(wmsm_priv->wq, &wmsm_priv->smd_work);
+		break;
+	case SMD_EVENT_CLOSE:
+		break;
+	case SMD_EVENT_STATUS:
+		break;
+	case SMD_EVENT_REOPEN_READY:
+		break;
+	default:
+		dev_err(&wmsm_priv->core->dev,
+			"%s: SMD_EVENT (%d) not supported\n", __func__, event);
+		break;
+	}
+}
+
+static void wcn36xx_msm_smd_work(struct work_struct *work)
+{
+	int avail;
+	int msg_len;
+	void *msg;
+	int ret;
+	struct wcn36xx_msm *wmsm_priv =
+		container_of(work, struct wcn36xx_msm, smd_work);
+
+	while (1) {
+		msg_len = smd_cur_packet_size(wmsm_priv->smd_ch);
+		if (0 == msg_len) {
+			return;
+		}
+		avail = smd_read_avail(wmsm_priv->smd_ch);
+		if (avail < msg_len) {
+			return;
+		}
+		msg = kmalloc(msg_len, GFP_KERNEL);
+		if (NULL == msg) {
+			return;
+		}
+
+		ret = smd_read(wmsm_priv->smd_ch, msg, msg_len);
+		if (ret != msg_len) {
+			return;
+		}
+		wmsm_priv->rsp_cb(wmsm_priv->drv_priv, msg, msg_len);
+		kfree(msg);
+	}
+}
+
+int wcn36xx_msm_smd_open(void *drv_priv, void *rsp_cb)
+{
+	int ret, left;
+	wmsm.drv_priv = drv_priv;
+	wmsm.rsp_cb = rsp_cb;
+	INIT_WORK(&wmsm.smd_work, wcn36xx_msm_smd_work);
+	init_completion(&wmsm.smd_compl);
+
+	wmsm.wq = create_workqueue("wcn36xx_msm_smd_wq");
+	if (!wmsm.wq) {
+		dev_err(&wmsm.core->dev, "failed to allocate wq");
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	ret = smd_named_open_on_edge("WLAN_CTRL", SMD_APPS_WCNSS,
+		&wmsm.smd_ch, &wmsm, wcn36xx_msm_smd_notify);
+	if (ret) {
+		dev_err(&wmsm.core->dev,
+			"smd_named_open_on_edge failed: %d\n", ret);
+		return ret;
+	}
+
+	left = wait_for_completion_interruptible_timeout(&wmsm.smd_compl,
+		msecs_to_jiffies(HAL_MSG_TIMEOUT));
+	if (left <= 0) {
+		dev_err(&wmsm.core->dev,
+			"timeout waiting for smd open: %d\n", ret);
+		return left;
+	}
+
+	/* Not to receive INT until the whole buf from SMD is read */
+	smd_disable_read_intr(wmsm.smd_ch);
+
+	return 0;
+}
+
+void wcn36xx_msm_smd_close(void)
+{
+	smd_close(wmsm.smd_ch);
+	flush_workqueue(wmsm.wq);
+	destroy_workqueue(wmsm.wq);
+}
+
+int wcn36xx_msm_shutdown(const struct subsys_desc *desc, bool force_stop)
+{
+	return 0;
+}
+int wcn36xx_msm_powerup(const struct subsys_desc *desc)
+{
+	return 0;
+}
+
+static int wcn36xx_msm_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *wcnss_memory;
+	struct resource *tx_irq;
+	struct resource *rx_irq;
+	struct resource res[3];
+	struct pinctrl_state *ps;
+
+	wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR_OR_NULL(wmsm.pinctrl))
+		return PTR_ERR(wmsm.pinctrl);
+
+	ps = pinctrl_lookup_state(wmsm.pinctrl, "wcnss_default");
+	if (IS_ERR_OR_NULL(ps))
+			return PTR_ERR(ps);
+
+	ret = pinctrl_select_state(wmsm.pinctrl, ps);
+	if (ret)
+		return ret;
+
+	if (IS_ERR_OR_NULL(pil))
+		pil = subsystem_get("wcnss");
+		if (IS_ERR_OR_NULL(pil))
+			return PTR_ERR(pil);
+
+	wmsm.core = platform_device_alloc("wcn36xx", -1);
+
+	//dev_err(&pdev->dev, "%s starting\n", __func__);
+
+	memset(res, 0x00, sizeof(res));
+	wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
+	wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
+	wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
+	wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
+	wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
+	wcnss_memory =
+		platform_get_resource_byname(pdev,
+					      IORESOURCE_MEM,
+					      "wcnss_mmio");
+	if (wcnss_memory == NULL) {
+		dev_err(&wmsm.core->dev,
+			"Failed to get wcnss wlan memory map.\n");
+		ret = -ENOMEM;
+		return ret;
+	}
+	memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
+
+	tx_irq = platform_get_resource_byname(pdev,
+					      IORESOURCE_IRQ,
+					      "wcnss_wlantx_irq");
+	if (tx_irq == NULL) {
+		dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
+		ret = -ENOMEM;
+		return ret;
+	}
+	memcpy(&res[1], tx_irq, sizeof(*tx_irq));
+
+	rx_irq = platform_get_resource_byname(pdev,
+					      IORESOURCE_IRQ,
+					      "wcnss_wlanrx_irq");
+	if (rx_irq == NULL) {
+		dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
+		ret = -ENOMEM;
+		return ret;
+	}
+	memcpy(&res[2], rx_irq, sizeof(*rx_irq));
+
+	platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
+
+	ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
+				       sizeof(wmsm.ctrl_ops));
+	if (ret) {
+		dev_err(&wmsm.core->dev, "Can't add platform data\n");
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	platform_device_add(wmsm.core);
+
+	dev_info(&pdev->dev, "%s initialized\n", __func__);
+
+	return 0;
+}
+static int wcn36xx_msm_remove(struct platform_device *pdev)
+{
+        struct pinctrl_state *ps;
+
+	platform_device_del(wmsm.core);
+	platform_device_put(wmsm.core);
+
+	if (wmsm.pinctrl) {
+		ps = pinctrl_lookup_state(wmsm.pinctrl, "wcnss_sleep");
+		if (IS_ERR_OR_NULL(ps))
+			return PTR_ERR(ps);
+
+		pinctrl_select_state(wmsm.pinctrl, ps);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id wcn36xx_msm_match_table[] = {
+	{ .compatible = "qcom,wcn36xx" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
+
+static struct platform_driver wcn36xx_msm_driver = {
+	.probe		= wcn36xx_msm_probe,
+	.remove		= wcn36xx_msm_remove,
+	.driver		= {
+		.name	= "wcn36xx-msm",
+		.owner	= THIS_MODULE,
+		.of_match_table = wcn36xx_msm_match_table,
+	},
+};
+
+static int __init wcn36xx_msm_init(void)
+{
+	return platform_driver_register(&wcn36xx_msm_driver);
+}
+module_init(wcn36xx_msm_init);
+
+static void __exit wcn36xx_msm_exit(void)
+{
+	platform_driver_unregister(&wcn36xx_msm_driver);
+	if (pil)
+		subsystem_put(pil);
+
+
+}
+module_exit(wcn36xx_msm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eugene Krasnikov k.eugene.e@gmail.com");
+MODULE_FIRMWARE(MAC_ADDR_0);
+

Re: [PATCH 1/2] net wireless wcn36xx add wcnss platform code

[Bjorn Andersson]

On Sat, Jan 17, 2015 at 9:16 PM, Andy Green <andy.green@linaro.org> wrote:
> From: Eugene Krasnikov <k.eugene.e@gmail.com>
>
> AG modified to remove regulator handling not needed on msm8916-qrd
>
> Signed-off-by: Eugene Krasnikov <k.eugene.e@gmail.com>
> Signed-off-by: Andy Green <andy.green@linaro.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/Makefile      |    2
>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  357 ++++++++++++++++++++++++

It seems like we're finally getting somewhere with the hwspinlock
devicetree binding, which will allow us to go ahead with the work on
getting smem, smd, smsm and smp2p into mainline.

With that in place we have fixed apis that this driver will run upon,
so I would like to suggest that we at that point fold this logic into
the driver itself rather than having this skim.

Regards,
Bjorn

Re: [PATCH 1/2] net wireless wcn36xx add wcnss platform code

[Kalle Valo]

Bjorn Andersson <bjorn@kryo.se> writes:

> On Sat, Jan 17, 2015 at 9:16 PM, Andy Green <andy.green@linaro.org> wrote:
>> From: Eugene Krasnikov <k.eugene.e@gmail.com>
>>
>> AG modified to remove regulator handling not needed on msm8916-qrd
>>
>> Signed-off-by: Eugene Krasnikov <k.eugene.e@gmail.com>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> ---
>>  drivers/net/wireless/ath/wcn36xx/Makefile      |    2
>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  357 ++++++++++++++++++++++++
>
> It seems like we're finally getting somewhere with the hwspinlock
> devicetree binding, which will allow us to go ahead with the work on
> getting smem, smd, smsm and smp2p into mainline.
>
> With that in place we have fixed apis that this driver will run upon,
> so I would like to suggest that we at that point fold this logic into
> the driver itself rather than having this skim.

Yeah, this approach would be much better.

-- 
Kalle Valo

[PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Andy Green]

Simplify the resource handling and use DT to indicate which chip type
we are dealing with

Signed-off-by: Andy Green <andy.green@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
 1 file changed, 52 insertions(+), 49 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
index f6f6c83..c9250e0 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
@@ -42,7 +42,10 @@ struct wcn36xx_msm {
 	struct completion	smd_compl;
 	smd_channel_t		*smd_ch;
 	struct pinctrl *pinctrl;
-} wmsm;
+	enum wcn36xx_chip_type chip_type;
+};
+
+static struct wcn36xx_msm wmsm;
 
 static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
 {
@@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
 	return 0;
 }
 
+static const struct of_device_id wcn36xx_msm_match_table[] = {
+	{ .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
+	{ .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
+	{ .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
+	{ }
+};
+
+static int wcn36xx_msm_get_chip_type(void)
+{
+	return wmsm.chip_type;
+}
+
+static struct wcn36xx_msm wmsm = {
+	.ctrl_ops = {
+		.open = wcn36xx_msm_smd_open,
+		.close = wcn36xx_msm_smd_close,
+		.tx = wcn36xx_msm_smd_send_and_wait,
+		.get_hw_mac = wcn36xx_msm_get_hw_mac,
+		.smsm_change_state = wcn36xx_msm_smsm_change_state,
+		.get_chip_type = wcn36xx_msm_get_chip_type,
+	},
+};
+
 static int wcn36xx_msm_probe(struct platform_device *pdev)
 {
 	int ret;
-	struct resource *wcnss_memory;
-	struct resource *tx_irq;
-	struct resource *rx_irq;
+	const struct of_device_id *of_id;
+	struct resource *r;
 	struct resource res[3];
 	struct pinctrl_state *ps;
+	static const char const *rnames[] = {
+		"wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
+	static const int rtype[] = {
+		IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
+	int n;
+
+	of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
+	if (!of_id)
+		return -EINVAL;
+
+	wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
 
 	wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (IS_ERR_OR_NULL(wmsm.pinctrl))
@@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
 
 	if (IS_ERR_OR_NULL(pil))
 		pil = subsystem_get("wcnss");
-		if (IS_ERR_OR_NULL(pil))
-			return PTR_ERR(pil);
+	if (IS_ERR_OR_NULL(pil))
+		return PTR_ERR(pil);
 
 	wmsm.core = platform_device_alloc("wcn36xx", -1);
 
-	//dev_err(&pdev->dev, "%s starting\n", __func__);
-
-	memset(res, 0x00, sizeof(res));
-	wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
-	wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
-	wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
-	wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
-	wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
-	wcnss_memory =
-		platform_get_resource_byname(pdev,
-					      IORESOURCE_MEM,
-					      "wcnss_mmio");
-	if (wcnss_memory == NULL) {
-		dev_err(&wmsm.core->dev,
-			"Failed to get wcnss wlan memory map.\n");
-		ret = -ENOMEM;
-		return ret;
-	}
-	memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
-
-	tx_irq = platform_get_resource_byname(pdev,
-					      IORESOURCE_IRQ,
-					      "wcnss_wlantx_irq");
-	if (tx_irq == NULL) {
-		dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
-		ret = -ENOMEM;
-		return ret;
-	}
-	memcpy(&res[1], tx_irq, sizeof(*tx_irq));
-
-	rx_irq = platform_get_resource_byname(pdev,
-					      IORESOURCE_IRQ,
-					      "wcnss_wlanrx_irq");
-	if (rx_irq == NULL) {
-		dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
-		ret = -ENOMEM;
-		return ret;
+	for (n = 0; n < ARRAY_SIZE(rnames); n++) {
+		r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
+		if (!r) {
+			dev_err(&wmsm.core->dev,
+				"Missing resource %s'\n", rnames[n]);
+			ret = -ENOMEM;
+			return ret;
+		}
+		res[n] = *r;
 	}
-	memcpy(&res[2], rx_irq, sizeof(*rx_irq));
 
-	platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
+	platform_device_add_resources(wmsm.core, res, n);
 
 	ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
 				       sizeof(wmsm.ctrl_ops));
@@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id wcn36xx_msm_match_table[] = {
-	{ .compatible = "qcom,wcn36xx" },
-	{ }
-};
 MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
 
 static struct platform_driver wcn36xx_msm_driver = {

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Eugene Krasnikov]

So how do we insmod wcn36xx_msm with a parameter specifying what type
of hardware do we use?

2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
> Simplify the resource handling and use DT to indicate which chip type
> we are dealing with
>
> Signed-off-by: Andy Green <andy.green@linaro.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>  1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
> index f6f6c83..c9250e0 100644
> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>         struct completion       smd_compl;
>         smd_channel_t           *smd_ch;
>         struct pinctrl *pinctrl;
> -} wmsm;
> +       enum wcn36xx_chip_type chip_type;
> +};
> +
> +static struct wcn36xx_msm wmsm;
>
>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>  {
> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>         return 0;
>  }
>
> +static const struct of_device_id wcn36xx_msm_match_table[] = {
> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
> +       { }
> +};
> +
> +static int wcn36xx_msm_get_chip_type(void)
> +{
> +       return wmsm.chip_type;
> +}
> +
> +static struct wcn36xx_msm wmsm = {
> +       .ctrl_ops = {
> +               .open = wcn36xx_msm_smd_open,
> +               .close = wcn36xx_msm_smd_close,
> +               .tx = wcn36xx_msm_smd_send_and_wait,
> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
> +               .get_chip_type = wcn36xx_msm_get_chip_type,
> +       },
> +};
> +
>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>  {
>         int ret;
> -       struct resource *wcnss_memory;
> -       struct resource *tx_irq;
> -       struct resource *rx_irq;
> +       const struct of_device_id *of_id;
> +       struct resource *r;
>         struct resource res[3];
>         struct pinctrl_state *ps;
> +       static const char const *rnames[] = {
> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
> +       static const int rtype[] = {
> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
> +       int n;
> +
> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
> +       if (!of_id)
> +               return -EINVAL;
> +
> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>
>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>
>         if (IS_ERR_OR_NULL(pil))
>                 pil = subsystem_get("wcnss");
> -               if (IS_ERR_OR_NULL(pil))
> -                       return PTR_ERR(pil);
> +       if (IS_ERR_OR_NULL(pil))
> +               return PTR_ERR(pil);
>
>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>
> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
> -
> -       memset(res, 0x00, sizeof(res));
> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
> -       wcnss_memory =
> -               platform_get_resource_byname(pdev,
> -                                             IORESOURCE_MEM,
> -                                             "wcnss_mmio");
> -       if (wcnss_memory == NULL) {
> -               dev_err(&wmsm.core->dev,
> -                       "Failed to get wcnss wlan memory map.\n");
> -               ret = -ENOMEM;
> -               return ret;
> -       }
> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
> -
> -       tx_irq = platform_get_resource_byname(pdev,
> -                                             IORESOURCE_IRQ,
> -                                             "wcnss_wlantx_irq");
> -       if (tx_irq == NULL) {
> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
> -               ret = -ENOMEM;
> -               return ret;
> -       }
> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
> -
> -       rx_irq = platform_get_resource_byname(pdev,
> -                                             IORESOURCE_IRQ,
> -                                             "wcnss_wlanrx_irq");
> -       if (rx_irq == NULL) {
> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
> -               ret = -ENOMEM;
> -               return ret;
> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
> +               if (!r) {
> +                       dev_err(&wmsm.core->dev,
> +                               "Missing resource %s'\n", rnames[n]);
> +                       ret = -ENOMEM;
> +                       return ret;
> +               }
> +               res[n] = *r;
>         }
> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>
> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
> +       platform_device_add_resources(wmsm.core, res, n);
>
>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>                                        sizeof(wmsm.ctrl_ops));
> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> -static const struct of_device_id wcn36xx_msm_match_table[] = {
> -       { .compatible = "qcom,wcn36xx" },
> -       { }
> -};
>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>
>  static struct platform_driver wcn36xx_msm_driver = {
>



-- 
Best regards,
Eugene

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Andy Green]

On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:

> So how do we insmod wcn36xx_msm with a parameter specifying what type
> of hardware do we use?

The type of chip is defined in the DT "compatible" which also delivers
the resource information.

        qcom,wcn36xx@0a000000 {
                compatible = "qcom,wcn3620";
                reg = <0x0a000000 0x280000>;
                reg-names = "wcnss_mmio";

                interrupts = <0 145 0 0 146 0>;
                interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
...

This bit based on your code can't go in mainline until there's some
kind of PIL support.

So the only things we can discuss about it for mainline purpose is
whether using a platform ops is a good way to interface to the
mainline driver.

If you're OK with that and you want a module parameter then this can
grow a module parameter and prefer to deliver the chip type from that
if given, without modifying the platform op interface.

But with or without a module parameter this can't be upstreamed right
now due to PIL.

-Andy

> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
>> Simplify the resource handling and use DT to indicate which chip type
>> we are dealing with
>>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> ---
>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>> index f6f6c83..c9250e0 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>         struct completion       smd_compl;
>>         smd_channel_t           *smd_ch;
>>         struct pinctrl *pinctrl;
>> -} wmsm;
>> +       enum wcn36xx_chip_type chip_type;
>> +};
>> +
>> +static struct wcn36xx_msm wmsm;
>>
>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>  {
>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>         return 0;
>>  }
>>
>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>> +       { }
>> +};
>> +
>> +static int wcn36xx_msm_get_chip_type(void)
>> +{
>> +       return wmsm.chip_type;
>> +}
>> +
>> +static struct wcn36xx_msm wmsm = {
>> +       .ctrl_ops = {
>> +               .open = wcn36xx_msm_smd_open,
>> +               .close = wcn36xx_msm_smd_close,
>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>> +       },
>> +};
>> +
>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>  {
>>         int ret;
>> -       struct resource *wcnss_memory;
>> -       struct resource *tx_irq;
>> -       struct resource *rx_irq;
>> +       const struct of_device_id *of_id;
>> +       struct resource *r;
>>         struct resource res[3];
>>         struct pinctrl_state *ps;
>> +       static const char const *rnames[] = {
>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>> +       static const int rtype[] = {
>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>> +       int n;
>> +
>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>> +       if (!of_id)
>> +               return -EINVAL;
>> +
>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>
>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>
>>         if (IS_ERR_OR_NULL(pil))
>>                 pil = subsystem_get("wcnss");
>> -               if (IS_ERR_OR_NULL(pil))
>> -                       return PTR_ERR(pil);
>> +       if (IS_ERR_OR_NULL(pil))
>> +               return PTR_ERR(pil);
>>
>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>
>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>> -
>> -       memset(res, 0x00, sizeof(res));
>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>> -       wcnss_memory =
>> -               platform_get_resource_byname(pdev,
>> -                                             IORESOURCE_MEM,
>> -                                             "wcnss_mmio");
>> -       if (wcnss_memory == NULL) {
>> -               dev_err(&wmsm.core->dev,
>> -                       "Failed to get wcnss wlan memory map.\n");
>> -               ret = -ENOMEM;
>> -               return ret;
>> -       }
>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>> -
>> -       tx_irq = platform_get_resource_byname(pdev,
>> -                                             IORESOURCE_IRQ,
>> -                                             "wcnss_wlantx_irq");
>> -       if (tx_irq == NULL) {
>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>> -               ret = -ENOMEM;
>> -               return ret;
>> -       }
>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>> -
>> -       rx_irq = platform_get_resource_byname(pdev,
>> -                                             IORESOURCE_IRQ,
>> -                                             "wcnss_wlanrx_irq");
>> -       if (rx_irq == NULL) {
>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>> -               ret = -ENOMEM;
>> -               return ret;
>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>> +               if (!r) {
>> +                       dev_err(&wmsm.core->dev,
>> +                               "Missing resource %s'\n", rnames[n]);
>> +                       ret = -ENOMEM;
>> +                       return ret;
>> +               }
>> +               res[n] = *r;
>>         }
>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>
>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>> +       platform_device_add_resources(wmsm.core, res, n);
>>
>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>                                        sizeof(wmsm.ctrl_ops));
>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>         return 0;
>>  }
>>
>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>> -       { .compatible = "qcom,wcn36xx" },
>> -       { }
>> -};
>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>
>>  static struct platform_driver wcn36xx_msm_driver = {
>>
>
>
>
> --
> Best regards,
> Eugene

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Eugene Krasnikov]

Have you tested this code on any device other than wcn3620?

2015-01-19 8:44 GMT+00:00 Andy Green <andy.green@linaro.org>:
> On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>
>> So how do we insmod wcn36xx_msm with a parameter specifying what type
>> of hardware do we use?
>
> The type of chip is defined in the DT "compatible" which also delivers
> the resource information.
>
>         qcom,wcn36xx@0a000000 {
>                 compatible = "qcom,wcn3620";
>                 reg = <0x0a000000 0x280000>;
>                 reg-names = "wcnss_mmio";
>
>                 interrupts = <0 145 0 0 146 0>;
>                 interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
> ...
>
> This bit based on your code can't go in mainline until there's some
> kind of PIL support.
>
> So the only things we can discuss about it for mainline purpose is
> whether using a platform ops is a good way to interface to the
> mainline driver.
>
> If you're OK with that and you want a module parameter then this can
> grow a module parameter and prefer to deliver the chip type from that
> if given, without modifying the platform op interface.
>
> But with or without a module parameter this can't be upstreamed right
> now due to PIL.
>
> -Andy
>
>> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>> Simplify the resource handling and use DT to indicate which chip type
>>> we are dealing with
>>>
>>> Signed-off-by: Andy Green <andy.green@linaro.org>
>>> ---
>>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>> index f6f6c83..c9250e0 100644
>>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>>         struct completion       smd_compl;
>>>         smd_channel_t           *smd_ch;
>>>         struct pinctrl *pinctrl;
>>> -} wmsm;
>>> +       enum wcn36xx_chip_type chip_type;
>>> +};
>>> +
>>> +static struct wcn36xx_msm wmsm;
>>>
>>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>>  {
>>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>>         return 0;
>>>  }
>>>
>>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>>> +       { }
>>> +};
>>> +
>>> +static int wcn36xx_msm_get_chip_type(void)
>>> +{
>>> +       return wmsm.chip_type;
>>> +}
>>> +
>>> +static struct wcn36xx_msm wmsm = {
>>> +       .ctrl_ops = {
>>> +               .open = wcn36xx_msm_smd_open,
>>> +               .close = wcn36xx_msm_smd_close,
>>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>>> +       },
>>> +};
>>> +
>>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>  {
>>>         int ret;
>>> -       struct resource *wcnss_memory;
>>> -       struct resource *tx_irq;
>>> -       struct resource *rx_irq;
>>> +       const struct of_device_id *of_id;
>>> +       struct resource *r;
>>>         struct resource res[3];
>>>         struct pinctrl_state *ps;
>>> +       static const char const *rnames[] = {
>>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>>> +       static const int rtype[] = {
>>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>>> +       int n;
>>> +
>>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>>> +       if (!of_id)
>>> +               return -EINVAL;
>>> +
>>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>>
>>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>
>>>         if (IS_ERR_OR_NULL(pil))
>>>                 pil = subsystem_get("wcnss");
>>> -               if (IS_ERR_OR_NULL(pil))
>>> -                       return PTR_ERR(pil);
>>> +       if (IS_ERR_OR_NULL(pil))
>>> +               return PTR_ERR(pil);
>>>
>>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>>
>>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>>> -
>>> -       memset(res, 0x00, sizeof(res));
>>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>>> -       wcnss_memory =
>>> -               platform_get_resource_byname(pdev,
>>> -                                             IORESOURCE_MEM,
>>> -                                             "wcnss_mmio");
>>> -       if (wcnss_memory == NULL) {
>>> -               dev_err(&wmsm.core->dev,
>>> -                       "Failed to get wcnss wlan memory map.\n");
>>> -               ret = -ENOMEM;
>>> -               return ret;
>>> -       }
>>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>>> -
>>> -       tx_irq = platform_get_resource_byname(pdev,
>>> -                                             IORESOURCE_IRQ,
>>> -                                             "wcnss_wlantx_irq");
>>> -       if (tx_irq == NULL) {
>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>>> -               ret = -ENOMEM;
>>> -               return ret;
>>> -       }
>>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>>> -
>>> -       rx_irq = platform_get_resource_byname(pdev,
>>> -                                             IORESOURCE_IRQ,
>>> -                                             "wcnss_wlanrx_irq");
>>> -       if (rx_irq == NULL) {
>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>>> -               ret = -ENOMEM;
>>> -               return ret;
>>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>>> +               if (!r) {
>>> +                       dev_err(&wmsm.core->dev,
>>> +                               "Missing resource %s'\n", rnames[n]);
>>> +                       ret = -ENOMEM;
>>> +                       return ret;
>>> +               }
>>> +               res[n] = *r;
>>>         }
>>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>>
>>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>>> +       platform_device_add_resources(wmsm.core, res, n);
>>>
>>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>>                                        sizeof(wmsm.ctrl_ops));
>>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>>         return 0;
>>>  }
>>>
>>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>>> -       { .compatible = "qcom,wcn36xx" },
>>> -       { }
>>> -};
>>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>>
>>>  static struct platform_driver wcn36xx_msm_driver = {
>>>
>>
>>
>>
>> --
>> Best regards,
>> Eugene



-- 
Best regards,
Eugene

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Andy Green]

On 19 January 2015 at 16:49, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
> Have you tested this code on any device other than wcn3620?

No... the only hardware I have is 3620.  But the only code we're
adding to mainline is the patch with the ops to get the chip type.

The two-patch series just shows one way to set that (which will
certainly work for all three defined compatible types, if it works for
one).  And this code cannot go upstream.

So the only decision to make is around whether adding the platform op
is a good way (or, eg, directly add DT support to wcn36xx and
eliminate the 'device regeneration' part of the OOT -msm code).

At the moment the detect code does not work for 3620, so something
needs to be done.

-Andy

> 2015-01-19 8:44 GMT+00:00 Andy Green <andy.green@linaro.org>:
>> On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>>
>>> So how do we insmod wcn36xx_msm with a parameter specifying what type
>>> of hardware do we use?
>>
>> The type of chip is defined in the DT "compatible" which also delivers
>> the resource information.
>>
>>         qcom,wcn36xx@0a000000 {
>>                 compatible = "qcom,wcn3620";
>>                 reg = <0x0a000000 0x280000>;
>>                 reg-names = "wcnss_mmio";
>>
>>                 interrupts = <0 145 0 0 146 0>;
>>                 interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
>> ...
>>
>> This bit based on your code can't go in mainline until there's some
>> kind of PIL support.
>>
>> So the only things we can discuss about it for mainline purpose is
>> whether using a platform ops is a good way to interface to the
>> mainline driver.
>>
>> If you're OK with that and you want a module parameter then this can
>> grow a module parameter and prefer to deliver the chip type from that
>> if given, without modifying the platform op interface.
>>
>> But with or without a module parameter this can't be upstreamed right
>> now due to PIL.
>>
>> -Andy
>>
>>> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>>> Simplify the resource handling and use DT to indicate which chip type
>>>> we are dealing with
>>>>
>>>> Signed-off-by: Andy Green <andy.green@linaro.org>
>>>> ---
>>>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>> index f6f6c83..c9250e0 100644
>>>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>>>         struct completion       smd_compl;
>>>>         smd_channel_t           *smd_ch;
>>>>         struct pinctrl *pinctrl;
>>>> -} wmsm;
>>>> +       enum wcn36xx_chip_type chip_type;
>>>> +};
>>>> +
>>>> +static struct wcn36xx_msm wmsm;
>>>>
>>>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>>>  {
>>>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>>>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>>>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>>>> +       { }
>>>> +};
>>>> +
>>>> +static int wcn36xx_msm_get_chip_type(void)
>>>> +{
>>>> +       return wmsm.chip_type;
>>>> +}
>>>> +
>>>> +static struct wcn36xx_msm wmsm = {
>>>> +       .ctrl_ops = {
>>>> +               .open = wcn36xx_msm_smd_open,
>>>> +               .close = wcn36xx_msm_smd_close,
>>>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>>>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>>>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>>>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>>>> +       },
>>>> +};
>>>> +
>>>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>  {
>>>>         int ret;
>>>> -       struct resource *wcnss_memory;
>>>> -       struct resource *tx_irq;
>>>> -       struct resource *rx_irq;
>>>> +       const struct of_device_id *of_id;
>>>> +       struct resource *r;
>>>>         struct resource res[3];
>>>>         struct pinctrl_state *ps;
>>>> +       static const char const *rnames[] = {
>>>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>>>> +       static const int rtype[] = {
>>>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>>>> +       int n;
>>>> +
>>>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>>>> +       if (!of_id)
>>>> +               return -EINVAL;
>>>> +
>>>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>>>
>>>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>>>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>
>>>>         if (IS_ERR_OR_NULL(pil))
>>>>                 pil = subsystem_get("wcnss");
>>>> -               if (IS_ERR_OR_NULL(pil))
>>>> -                       return PTR_ERR(pil);
>>>> +       if (IS_ERR_OR_NULL(pil))
>>>> +               return PTR_ERR(pil);
>>>>
>>>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>>>
>>>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>>>> -
>>>> -       memset(res, 0x00, sizeof(res));
>>>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>>>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>>>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>>>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>>>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>>>> -       wcnss_memory =
>>>> -               platform_get_resource_byname(pdev,
>>>> -                                             IORESOURCE_MEM,
>>>> -                                             "wcnss_mmio");
>>>> -       if (wcnss_memory == NULL) {
>>>> -               dev_err(&wmsm.core->dev,
>>>> -                       "Failed to get wcnss wlan memory map.\n");
>>>> -               ret = -ENOMEM;
>>>> -               return ret;
>>>> -       }
>>>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>>>> -
>>>> -       tx_irq = platform_get_resource_byname(pdev,
>>>> -                                             IORESOURCE_IRQ,
>>>> -                                             "wcnss_wlantx_irq");
>>>> -       if (tx_irq == NULL) {
>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>>>> -               ret = -ENOMEM;
>>>> -               return ret;
>>>> -       }
>>>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>>>> -
>>>> -       rx_irq = platform_get_resource_byname(pdev,
>>>> -                                             IORESOURCE_IRQ,
>>>> -                                             "wcnss_wlanrx_irq");
>>>> -       if (rx_irq == NULL) {
>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>>>> -               ret = -ENOMEM;
>>>> -               return ret;
>>>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>>>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>>>> +               if (!r) {
>>>> +                       dev_err(&wmsm.core->dev,
>>>> +                               "Missing resource %s'\n", rnames[n]);
>>>> +                       ret = -ENOMEM;
>>>> +                       return ret;
>>>> +               }
>>>> +               res[n] = *r;
>>>>         }
>>>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>>>
>>>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>>>> +       platform_device_add_resources(wmsm.core, res, n);
>>>>
>>>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>>>                                        sizeof(wmsm.ctrl_ops));
>>>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>> -       { .compatible = "qcom,wcn36xx" },
>>>> -       { }
>>>> -};
>>>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>>>
>>>>  static struct platform_driver wcn36xx_msm_driver = {
>>>>
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Eugene
>
>
>
> --
> Best regards,
> Eugene

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Eugene Krasnikov]

The idea is definitely better than just checking for AC support. But
the question is whether platform/hardware/firmware support that?

2015-01-19 9:00 GMT+00:00 Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> On 19 January 2015 at 16:49, Eugene Krasnikov <k.eugene.e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Have you tested this code on any device other than wcn3620?
>
> No... the only hardware I have is 3620.  But the only code we're
> adding to mainline is the patch with the ops to get the chip type.
>
> The two-patch series just shows one way to set that (which will
> certainly work for all three defined compatible types, if it works for
> one).  And this code cannot go upstream.
>
> So the only decision to make is around whether adding the platform op
> is a good way (or, eg, directly add DT support to wcn36xx and
> eliminate the 'device regeneration' part of the OOT -msm code).
>
> At the moment the detect code does not work for 3620, so something
> needs to be done.
>
> -Andy
>
>> 2015-01-19 8:44 GMT+00:00 Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>>> On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> So how do we insmod wcn36xx_msm with a parameter specifying what type
>>>> of hardware do we use?
>>>
>>> The type of chip is defined in the DT "compatible" which also delivers
>>> the resource information.
>>>
>>>         qcom,wcn36xx@0a000000 {
>>>                 compatible = "qcom,wcn3620";
>>>                 reg = <0x0a000000 0x280000>;
>>>                 reg-names = "wcnss_mmio";
>>>
>>>                 interrupts = <0 145 0 0 146 0>;
>>>                 interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
>>> ...
>>>
>>> This bit based on your code can't go in mainline until there's some
>>> kind of PIL support.
>>>
>>> So the only things we can discuss about it for mainline purpose is
>>> whether using a platform ops is a good way to interface to the
>>> mainline driver.
>>>
>>> If you're OK with that and you want a module parameter then this can
>>> grow a module parameter and prefer to deliver the chip type from that
>>> if given, without modifying the platform op interface.
>>>
>>> But with or without a module parameter this can't be upstreamed right
>>> now due to PIL.
>>>
>>> -Andy
>>>
>>>> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>>>>> Simplify the resource handling and use DT to indicate which chip type
>>>>> we are dealing with
>>>>>
>>>>> Signed-off-by: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> ---
>>>>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>> index f6f6c83..c9250e0 100644
>>>>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>>>>         struct completion       smd_compl;
>>>>>         smd_channel_t           *smd_ch;
>>>>>         struct pinctrl *pinctrl;
>>>>> -} wmsm;
>>>>> +       enum wcn36xx_chip_type chip_type;
>>>>> +};
>>>>> +
>>>>> +static struct wcn36xx_msm wmsm;
>>>>>
>>>>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>>>>  {
>>>>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>>>>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>>>>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>>>>> +       { }
>>>>> +};
>>>>> +
>>>>> +static int wcn36xx_msm_get_chip_type(void)
>>>>> +{
>>>>> +       return wmsm.chip_type;
>>>>> +}
>>>>> +
>>>>> +static struct wcn36xx_msm wmsm = {
>>>>> +       .ctrl_ops = {
>>>>> +               .open = wcn36xx_msm_smd_open,
>>>>> +               .close = wcn36xx_msm_smd_close,
>>>>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>>>>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>>>>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>>>>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>>>>> +       },
>>>>> +};
>>>>> +
>>>>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>  {
>>>>>         int ret;
>>>>> -       struct resource *wcnss_memory;
>>>>> -       struct resource *tx_irq;
>>>>> -       struct resource *rx_irq;
>>>>> +       const struct of_device_id *of_id;
>>>>> +       struct resource *r;
>>>>>         struct resource res[3];
>>>>>         struct pinctrl_state *ps;
>>>>> +       static const char const *rnames[] = {
>>>>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>>>>> +       static const int rtype[] = {
>>>>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>>>>> +       int n;
>>>>> +
>>>>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>>>>> +       if (!of_id)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>>>>
>>>>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>>>>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>
>>>>>         if (IS_ERR_OR_NULL(pil))
>>>>>                 pil = subsystem_get("wcnss");
>>>>> -               if (IS_ERR_OR_NULL(pil))
>>>>> -                       return PTR_ERR(pil);
>>>>> +       if (IS_ERR_OR_NULL(pil))
>>>>> +               return PTR_ERR(pil);
>>>>>
>>>>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>>>>
>>>>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>>>>> -
>>>>> -       memset(res, 0x00, sizeof(res));
>>>>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>>>>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>>>>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>>>>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>>>>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>>>>> -       wcnss_memory =
>>>>> -               platform_get_resource_byname(pdev,
>>>>> -                                             IORESOURCE_MEM,
>>>>> -                                             "wcnss_mmio");
>>>>> -       if (wcnss_memory == NULL) {
>>>>> -               dev_err(&wmsm.core->dev,
>>>>> -                       "Failed to get wcnss wlan memory map.\n");
>>>>> -               ret = -ENOMEM;
>>>>> -               return ret;
>>>>> -       }
>>>>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>>>>> -
>>>>> -       tx_irq = platform_get_resource_byname(pdev,
>>>>> -                                             IORESOURCE_IRQ,
>>>>> -                                             "wcnss_wlantx_irq");
>>>>> -       if (tx_irq == NULL) {
>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>>>>> -               ret = -ENOMEM;
>>>>> -               return ret;
>>>>> -       }
>>>>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>>>>> -
>>>>> -       rx_irq = platform_get_resource_byname(pdev,
>>>>> -                                             IORESOURCE_IRQ,
>>>>> -                                             "wcnss_wlanrx_irq");
>>>>> -       if (rx_irq == NULL) {
>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>>>>> -               ret = -ENOMEM;
>>>>> -               return ret;
>>>>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>>>>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>>>>> +               if (!r) {
>>>>> +                       dev_err(&wmsm.core->dev,
>>>>> +                               "Missing resource %s'\n", rnames[n]);
>>>>> +                       ret = -ENOMEM;
>>>>> +                       return ret;
>>>>> +               }
>>>>> +               res[n] = *r;
>>>>>         }
>>>>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>>>>
>>>>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>>>>> +       platform_device_add_resources(wmsm.core, res, n);
>>>>>
>>>>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>>>>                                        sizeof(wmsm.ctrl_ops));
>>>>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>> -       { .compatible = "qcom,wcn36xx" },
>>>>> -       { }
>>>>> -};
>>>>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>>>>
>>>>>  static struct platform_driver wcn36xx_msm_driver = {
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Eugene
>>
>>
>>
>> --
>> Best regards,
>> Eugene



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

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Andy Green]

On 19 January 2015 at 17:02, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
> The idea is definitely better than just checking for AC support. But
> the question is whether platform/hardware/firmware support that?

Sorry I don't understand what might be unsupported.

 - We don't ask the firmware, we tell the driver what chip it is from
the outside.  There's nothing for the firmware to support.

 - Platform supports a set of ops via platform_data already.  This
just adds one op to get the chip type from the platform code.

 - What can't the hardware support?  The hardware physically is a
3620, 3660 or 3680.  We just tell the driver what it is when we
instantiate the device.  We don't ask anything of the hardware.

I expected to have a debate about whether to move the dt support to
wcn36xx directly which is also reasonable... there's no question
adding an op will work or not, it will work for all cases like this.
But it also implies there must be the "device faking" business in -msm
code, one day that will also go upstream and then it might be
considered a bit strange.

I did it like this now because it's the minimum change from the
current situation.

-Andy

> 2015-01-19 9:00 GMT+00:00 Andy Green <andy.green@linaro.org>:
>> On 19 January 2015 at 16:49, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>>> Have you tested this code on any device other than wcn3620?
>>
>> No... the only hardware I have is 3620.  But the only code we're
>> adding to mainline is the patch with the ops to get the chip type.
>>
>> The two-patch series just shows one way to set that (which will
>> certainly work for all three defined compatible types, if it works for
>> one).  And this code cannot go upstream.
>>
>> So the only decision to make is around whether adding the platform op
>> is a good way (or, eg, directly add DT support to wcn36xx and
>> eliminate the 'device regeneration' part of the OOT -msm code).
>>
>> At the moment the detect code does not work for 3620, so something
>> needs to be done.
>>
>> -Andy
>>
>>> 2015-01-19 8:44 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>>> On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>>>>
>>>>> So how do we insmod wcn36xx_msm with a parameter specifying what type
>>>>> of hardware do we use?
>>>>
>>>> The type of chip is defined in the DT "compatible" which also delivers
>>>> the resource information.
>>>>
>>>>         qcom,wcn36xx@0a000000 {
>>>>                 compatible = "qcom,wcn3620";
>>>>                 reg = <0x0a000000 0x280000>;
>>>>                 reg-names = "wcnss_mmio";
>>>>
>>>>                 interrupts = <0 145 0 0 146 0>;
>>>>                 interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
>>>> ...
>>>>
>>>> This bit based on your code can't go in mainline until there's some
>>>> kind of PIL support.
>>>>
>>>> So the only things we can discuss about it for mainline purpose is
>>>> whether using a platform ops is a good way to interface to the
>>>> mainline driver.
>>>>
>>>> If you're OK with that and you want a module parameter then this can
>>>> grow a module parameter and prefer to deliver the chip type from that
>>>> if given, without modifying the platform op interface.
>>>>
>>>> But with or without a module parameter this can't be upstreamed right
>>>> now due to PIL.
>>>>
>>>> -Andy
>>>>
>>>>> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>>>>> Simplify the resource handling and use DT to indicate which chip type
>>>>>> we are dealing with
>>>>>>
>>>>>> Signed-off-by: Andy Green <andy.green@linaro.org>
>>>>>> ---
>>>>>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>> index f6f6c83..c9250e0 100644
>>>>>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>>>>>         struct completion       smd_compl;
>>>>>>         smd_channel_t           *smd_ch;
>>>>>>         struct pinctrl *pinctrl;
>>>>>> -} wmsm;
>>>>>> +       enum wcn36xx_chip_type chip_type;
>>>>>> +};
>>>>>> +
>>>>>> +static struct wcn36xx_msm wmsm;
>>>>>>
>>>>>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>>>>>  {
>>>>>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>>>>>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>>>>>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>>>>>> +       { }
>>>>>> +};
>>>>>> +
>>>>>> +static int wcn36xx_msm_get_chip_type(void)
>>>>>> +{
>>>>>> +       return wmsm.chip_type;
>>>>>> +}
>>>>>> +
>>>>>> +static struct wcn36xx_msm wmsm = {
>>>>>> +       .ctrl_ops = {
>>>>>> +               .open = wcn36xx_msm_smd_open,
>>>>>> +               .close = wcn36xx_msm_smd_close,
>>>>>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>>>>>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>>>>>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>>>>>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>>>>>> +       },
>>>>>> +};
>>>>>> +
>>>>>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>>  {
>>>>>>         int ret;
>>>>>> -       struct resource *wcnss_memory;
>>>>>> -       struct resource *tx_irq;
>>>>>> -       struct resource *rx_irq;
>>>>>> +       const struct of_device_id *of_id;
>>>>>> +       struct resource *r;
>>>>>>         struct resource res[3];
>>>>>>         struct pinctrl_state *ps;
>>>>>> +       static const char const *rnames[] = {
>>>>>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>>>>>> +       static const int rtype[] = {
>>>>>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>>>>>> +       int n;
>>>>>> +
>>>>>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>>>>>> +       if (!of_id)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>>>>>
>>>>>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>>>>>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>>
>>>>>>         if (IS_ERR_OR_NULL(pil))
>>>>>>                 pil = subsystem_get("wcnss");
>>>>>> -               if (IS_ERR_OR_NULL(pil))
>>>>>> -                       return PTR_ERR(pil);
>>>>>> +       if (IS_ERR_OR_NULL(pil))
>>>>>> +               return PTR_ERR(pil);
>>>>>>
>>>>>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>>>>>
>>>>>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>>>>>> -
>>>>>> -       memset(res, 0x00, sizeof(res));
>>>>>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>>>>>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>>>>>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>>>>>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>>>>>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>>>>>> -       wcnss_memory =
>>>>>> -               platform_get_resource_byname(pdev,
>>>>>> -                                             IORESOURCE_MEM,
>>>>>> -                                             "wcnss_mmio");
>>>>>> -       if (wcnss_memory == NULL) {
>>>>>> -               dev_err(&wmsm.core->dev,
>>>>>> -                       "Failed to get wcnss wlan memory map.\n");
>>>>>> -               ret = -ENOMEM;
>>>>>> -               return ret;
>>>>>> -       }
>>>>>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>>>>>> -
>>>>>> -       tx_irq = platform_get_resource_byname(pdev,
>>>>>> -                                             IORESOURCE_IRQ,
>>>>>> -                                             "wcnss_wlantx_irq");
>>>>>> -       if (tx_irq == NULL) {
>>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>>>>>> -               ret = -ENOMEM;
>>>>>> -               return ret;
>>>>>> -       }
>>>>>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>>>>>> -
>>>>>> -       rx_irq = platform_get_resource_byname(pdev,
>>>>>> -                                             IORESOURCE_IRQ,
>>>>>> -                                             "wcnss_wlanrx_irq");
>>>>>> -       if (rx_irq == NULL) {
>>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>>>>>> -               ret = -ENOMEM;
>>>>>> -               return ret;
>>>>>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>>>>>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>>>>>> +               if (!r) {
>>>>>> +                       dev_err(&wmsm.core->dev,
>>>>>> +                               "Missing resource %s'\n", rnames[n]);
>>>>>> +                       ret = -ENOMEM;
>>>>>> +                       return ret;
>>>>>> +               }
>>>>>> +               res[n] = *r;
>>>>>>         }
>>>>>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>>>>>
>>>>>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>>>>>> +       platform_device_add_resources(wmsm.core, res, n);
>>>>>>
>>>>>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>>>>>                                        sizeof(wmsm.ctrl_ops));
>>>>>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>>> -       { .compatible = "qcom,wcn36xx" },
>>>>>> -       { }
>>>>>> -};
>>>>>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>>>>>
>>>>>>  static struct platform_driver wcn36xx_msm_driver = {
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Eugene
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Eugene
>
>
>
> --
> Best regards,
> Eugene

Re: [PATCH 2/2] net wireless wcn36xx adapt wcnss platform to select module by DT

[Eugene Krasnikov]

What i mean is that it's not clear who knows what chip is this,
whether wcn36xx or wcn36xx_msm. Previously the assumption was that SMD
command will tell what interface to use. Now we are moving towards
wcn36xx_msm telling what chip is installed. Both approaches will work.
If it less work to do then fine.

Sorry for any confusion.

2015-01-19 9:34 GMT+00:00 Andy Green <andy.green@linaro.org>:
> On 19 January 2015 at 17:02, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>> The idea is definitely better than just checking for AC support. But
>> the question is whether platform/hardware/firmware support that?
>
> Sorry I don't understand what might be unsupported.
>
>  - We don't ask the firmware, we tell the driver what chip it is from
> the outside.  There's nothing for the firmware to support.
>
>  - Platform supports a set of ops via platform_data already.  This
> just adds one op to get the chip type from the platform code.
>
>  - What can't the hardware support?  The hardware physically is a
> 3620, 3660 or 3680.  We just tell the driver what it is when we
> instantiate the device.  We don't ask anything of the hardware.
>
> I expected to have a debate about whether to move the dt support to
> wcn36xx directly which is also reasonable... there's no question
> adding an op will work or not, it will work for all cases like this.
> But it also implies there must be the "device faking" business in -msm
> code, one day that will also go upstream and then it might be
> considered a bit strange.
>
> I did it like this now because it's the minimum change from the
> current situation.
>
> -Andy
>
>> 2015-01-19 9:00 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>> On 19 January 2015 at 16:49, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>>>> Have you tested this code on any device other than wcn3620?
>>>
>>> No... the only hardware I have is 3620.  But the only code we're
>>> adding to mainline is the patch with the ops to get the chip type.
>>>
>>> The two-patch series just shows one way to set that (which will
>>> certainly work for all three defined compatible types, if it works for
>>> one).  And this code cannot go upstream.
>>>
>>> So the only decision to make is around whether adding the platform op
>>> is a good way (or, eg, directly add DT support to wcn36xx and
>>> eliminate the 'device regeneration' part of the OOT -msm code).
>>>
>>> At the moment the detect code does not work for 3620, so something
>>> needs to be done.
>>>
>>> -Andy
>>>
>>>> 2015-01-19 8:44 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>>>> On 19 January 2015 at 16:34, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>>>>>
>>>>>> So how do we insmod wcn36xx_msm with a parameter specifying what type
>>>>>> of hardware do we use?
>>>>>
>>>>> The type of chip is defined in the DT "compatible" which also delivers
>>>>> the resource information.
>>>>>
>>>>>         qcom,wcn36xx@0a000000 {
>>>>>                 compatible = "qcom,wcn3620";
>>>>>                 reg = <0x0a000000 0x280000>;
>>>>>                 reg-names = "wcnss_mmio";
>>>>>
>>>>>                 interrupts = <0 145 0 0 146 0>;
>>>>>                 interrupt-names = "wcnss_wlantx_irq", "wcnss_wlanrx_irq";
>>>>> ...
>>>>>
>>>>> This bit based on your code can't go in mainline until there's some
>>>>> kind of PIL support.
>>>>>
>>>>> So the only things we can discuss about it for mainline purpose is
>>>>> whether using a platform ops is a good way to interface to the
>>>>> mainline driver.
>>>>>
>>>>> If you're OK with that and you want a module parameter then this can
>>>>> grow a module parameter and prefer to deliver the chip type from that
>>>>> if given, without modifying the platform op interface.
>>>>>
>>>>> But with or without a module parameter this can't be upstreamed right
>>>>> now due to PIL.
>>>>>
>>>>> -Andy
>>>>>
>>>>>> 2015-01-18 5:16 GMT+00:00 Andy Green <andy.green@linaro.org>:
>>>>>>> Simplify the resource handling and use DT to indicate which chip type
>>>>>>> we are dealing with
>>>>>>>
>>>>>>> Signed-off-by: Andy Green <andy.green@linaro.org>
>>>>>>> ---
>>>>>>>  drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c |  101 ++++++++++++------------
>>>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>>> index f6f6c83..c9250e0 100644
>>>>>>> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>>> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx-msm.c
>>>>>>> @@ -42,7 +42,10 @@ struct wcn36xx_msm {
>>>>>>>         struct completion       smd_compl;
>>>>>>>         smd_channel_t           *smd_ch;
>>>>>>>         struct pinctrl *pinctrl;
>>>>>>> -} wmsm;
>>>>>>> +       enum wcn36xx_chip_type chip_type;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct wcn36xx_msm wmsm;
>>>>>>>
>>>>>>>  static int wcn36xx_msm_smsm_change_state(u32 clear_mask, u32 set_mask)
>>>>>>>  {
>>>>>>> @@ -217,14 +220,47 @@ int wcn36xx_msm_powerup(const struct subsys_desc *desc)
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>>>> +       { .compatible = "qcom,wcn3660", .data = (void *)WCN36XX_CHIP_3660 },
>>>>>>> +       { .compatible = "qcom,wcn3680", .data = (void *)WCN36XX_CHIP_3680 },
>>>>>>> +       { .compatible = "qcom,wcn3620", .data = (void *)WCN36XX_CHIP_3620 },
>>>>>>> +       { }
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int wcn36xx_msm_get_chip_type(void)
>>>>>>> +{
>>>>>>> +       return wmsm.chip_type;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static struct wcn36xx_msm wmsm = {
>>>>>>> +       .ctrl_ops = {
>>>>>>> +               .open = wcn36xx_msm_smd_open,
>>>>>>> +               .close = wcn36xx_msm_smd_close,
>>>>>>> +               .tx = wcn36xx_msm_smd_send_and_wait,
>>>>>>> +               .get_hw_mac = wcn36xx_msm_get_hw_mac,
>>>>>>> +               .smsm_change_state = wcn36xx_msm_smsm_change_state,
>>>>>>> +               .get_chip_type = wcn36xx_msm_get_chip_type,
>>>>>>> +       },
>>>>>>> +};
>>>>>>> +
>>>>>>>  static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>>>  {
>>>>>>>         int ret;
>>>>>>> -       struct resource *wcnss_memory;
>>>>>>> -       struct resource *tx_irq;
>>>>>>> -       struct resource *rx_irq;
>>>>>>> +       const struct of_device_id *of_id;
>>>>>>> +       struct resource *r;
>>>>>>>         struct resource res[3];
>>>>>>>         struct pinctrl_state *ps;
>>>>>>> +       static const char const *rnames[] = {
>>>>>>> +               "wcnss_mmio", "wcnss_wlantx_irq", "wcnss_wlanrx_irq" };
>>>>>>> +       static const int rtype[] = {
>>>>>>> +               IORESOURCE_MEM, IORESOURCE_IRQ, IORESOURCE_IRQ };
>>>>>>> +       int n;
>>>>>>> +
>>>>>>> +       of_id = of_match_node(wcn36xx_msm_match_table, pdev->dev.of_node);
>>>>>>> +       if (!of_id)
>>>>>>> +               return -EINVAL;
>>>>>>> +
>>>>>>> +       wmsm.chip_type = (enum wcn36xx_chip_type)of_id->data;
>>>>>>>
>>>>>>>         wmsm.pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>>>>         if (IS_ERR_OR_NULL(wmsm.pinctrl))
>>>>>>> @@ -240,52 +276,23 @@ static int wcn36xx_msm_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>>         if (IS_ERR_OR_NULL(pil))
>>>>>>>                 pil = subsystem_get("wcnss");
>>>>>>> -               if (IS_ERR_OR_NULL(pil))
>>>>>>> -                       return PTR_ERR(pil);
>>>>>>> +       if (IS_ERR_OR_NULL(pil))
>>>>>>> +               return PTR_ERR(pil);
>>>>>>>
>>>>>>>         wmsm.core = platform_device_alloc("wcn36xx", -1);
>>>>>>>
>>>>>>> -       //dev_err(&pdev->dev, "%s starting\n", __func__);
>>>>>>> -
>>>>>>> -       memset(res, 0x00, sizeof(res));
>>>>>>> -       wmsm.ctrl_ops.open = wcn36xx_msm_smd_open;
>>>>>>> -       wmsm.ctrl_ops.close = wcn36xx_msm_smd_close;
>>>>>>> -       wmsm.ctrl_ops.tx = wcn36xx_msm_smd_send_and_wait;
>>>>>>> -       wmsm.ctrl_ops.get_hw_mac = wcn36xx_msm_get_hw_mac;
>>>>>>> -       wmsm.ctrl_ops.smsm_change_state = wcn36xx_msm_smsm_change_state;
>>>>>>> -       wcnss_memory =
>>>>>>> -               platform_get_resource_byname(pdev,
>>>>>>> -                                             IORESOURCE_MEM,
>>>>>>> -                                             "wcnss_mmio");
>>>>>>> -       if (wcnss_memory == NULL) {
>>>>>>> -               dev_err(&wmsm.core->dev,
>>>>>>> -                       "Failed to get wcnss wlan memory map.\n");
>>>>>>> -               ret = -ENOMEM;
>>>>>>> -               return ret;
>>>>>>> -       }
>>>>>>> -       memcpy(&res[0], wcnss_memory, sizeof(*wcnss_memory));
>>>>>>> -
>>>>>>> -       tx_irq = platform_get_resource_byname(pdev,
>>>>>>> -                                             IORESOURCE_IRQ,
>>>>>>> -                                             "wcnss_wlantx_irq");
>>>>>>> -       if (tx_irq == NULL) {
>>>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss tx_irq");
>>>>>>> -               ret = -ENOMEM;
>>>>>>> -               return ret;
>>>>>>> -       }
>>>>>>> -       memcpy(&res[1], tx_irq, sizeof(*tx_irq));
>>>>>>> -
>>>>>>> -       rx_irq = platform_get_resource_byname(pdev,
>>>>>>> -                                             IORESOURCE_IRQ,
>>>>>>> -                                             "wcnss_wlanrx_irq");
>>>>>>> -       if (rx_irq == NULL) {
>>>>>>> -               dev_err(&wmsm.core->dev, "Failed to get wcnss rx_irq");
>>>>>>> -               ret = -ENOMEM;
>>>>>>> -               return ret;
>>>>>>> +       for (n = 0; n < ARRAY_SIZE(rnames); n++) {
>>>>>>> +               r = platform_get_resource_byname(pdev, rtype[n], rnames[n]);
>>>>>>> +               if (!r) {
>>>>>>> +                       dev_err(&wmsm.core->dev,
>>>>>>> +                               "Missing resource %s'\n", rnames[n]);
>>>>>>> +                       ret = -ENOMEM;
>>>>>>> +                       return ret;
>>>>>>> +               }
>>>>>>> +               res[n] = *r;
>>>>>>>         }
>>>>>>> -       memcpy(&res[2], rx_irq, sizeof(*rx_irq));
>>>>>>>
>>>>>>> -       platform_device_add_resources(wmsm.core, res, ARRAY_SIZE(res));
>>>>>>> +       platform_device_add_resources(wmsm.core, res, n);
>>>>>>>
>>>>>>>         ret = platform_device_add_data(wmsm.core, &wmsm.ctrl_ops,
>>>>>>>                                        sizeof(wmsm.ctrl_ops));
>>>>>>> @@ -319,10 +326,6 @@ static int wcn36xx_msm_remove(struct platform_device *pdev)
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> -static const struct of_device_id wcn36xx_msm_match_table[] = {
>>>>>>> -       { .compatible = "qcom,wcn36xx" },
>>>>>>> -       { }
>>>>>>> -};
>>>>>>>  MODULE_DEVICE_TABLE(of, wcn36xx_msm_match_table);
>>>>>>>
>>>>>>>  static struct platform_driver wcn36xx_msm_driver = {
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards,
>>>>>> Eugene
>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Eugene
>>
>>
>>
>> --
>> Best regards,
>> Eugene



-- 
Best regards,
Eugene