2014-09-30 15:49:11

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] wil6210: remove obsolete msm platform code

The wil6210 driver has a glue layer for the Qualcomm MSM platform
code, which apparently could never build and is unlikely to
ever be able to in the future, as the MSM platform is being phased
out in favor of the new QCOM platform, and the driver relies
on out-of-tree infrastructure. Trying to build it currently
results in this error:

drivers/net/wireless/ath/wil6210/wil_platform_msm.c:19:27: fatal error: linux/msm-bus.h: No such file or directory

Removing the driver fixes the build and makes it possible to
build an allmodconfig kernel for MSM.

Signed-off-by: Arnd Bergmann <[email protected]>
---
diff --git a/drivers/net/wireless/ath/wil6210/Kconfig b/drivers/net/wireless/ath/wil6210/Kconfig
index 481680a3aa55..ce8c0381825e 100644
--- a/drivers/net/wireless/ath/wil6210/Kconfig
+++ b/drivers/net/wireless/ath/wil6210/Kconfig
@@ -39,12 +39,3 @@ config WIL6210_TRACING
option if you are interested in debugging the driver.

If unsure, say Y to make it easier to debug problems.
-
-config WIL6210_PLATFORM_MSM
- bool "wil6210 MSM platform specific support"
- depends on WIL6210
- depends on ARCH_MSM
- default y
- ---help---
- Say Y here to enable wil6210 driver support for MSM
- platform specific features
diff --git a/drivers/net/wireless/ath/wil6210/Makefile b/drivers/net/wireless/ath/wil6210/Makefile
index a471d74ae409..944f300e2979 100644
--- a/drivers/net/wireless/ath/wil6210/Makefile
+++ b/drivers/net/wireless/ath/wil6210/Makefile
@@ -13,7 +13,6 @@ wil6210-y += rx_reorder.o
wil6210-y += fw.o
wil6210-$(CONFIG_WIL6210_TRACING) += trace.o
wil6210-y += wil_platform.o
-wil6210-$(CONFIG_WIL6210_PLATFORM_MSM) += wil_platform_msm.o

# for tracing framework to find trace.h
CFLAGS_trace.o := -I$(src)
diff --git a/drivers/net/wireless/ath/wil6210/wil_platform_msm.c b/drivers/net/wireless/ath/wil6210/wil_platform_msm.c
deleted file mode 100644
index b354a743240d..000000000000
--- a/drivers/net/wireless/ath/wil6210/wil_platform_msm.c
+++ /dev/null
@@ -1,257 +0,0 @@
-/*
- * Copyright (c) 2014 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/of.h>
-#include <linux/slab.h>
-#include <linux/msm-bus.h>
-
-#include "wil_platform.h"
-#include "wil_platform_msm.h"
-
-/**
- * struct wil_platform_msm - wil6210 msm platform module info
- *
- * @dev: device object
- * @msm_bus_handle: handle for using msm_bus API
- * @pdata: bus scale info retrieved from DT
- */
-struct wil_platform_msm {
- struct device *dev;
- uint32_t msm_bus_handle;
- struct msm_bus_scale_pdata *pdata;
-};
-
-#define KBTOB(a) (a * 1000ULL)
-
-/**
- * wil_platform_get_pdata() - Generate bus client data from device tree
- * provided by clients.
- *
- * dev: device object
- * of_node: Device tree node to extract information from
- *
- * The function returns a valid pointer to the allocated bus-scale-pdata
- * if the vectors were correctly read from the client's device node.
- * Any error in reading or parsing the device node will return NULL
- * to the caller.
- */
-static struct msm_bus_scale_pdata *wil_platform_get_pdata(
- struct device *dev,
- struct device_node *of_node)
-{
- struct msm_bus_scale_pdata *pdata;
- struct msm_bus_paths *usecase;
- int i, j, ret, len;
- unsigned int num_usecases, num_paths, mem_size;
- const uint32_t *vec_arr;
- struct msm_bus_vectors *vectors;
-
- /* first read num_usecases and num_paths so we can calculate
- * amount of memory to allocate
- */
- ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
- &num_usecases);
- if (ret) {
- dev_err(dev, "Error: num-usecases not found\n");
- return NULL;
- }
-
- ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
- &num_paths);
- if (ret) {
- dev_err(dev, "Error: num_paths not found\n");
- return NULL;
- }
-
- /* pdata memory layout:
- * msm_bus_scale_pdata
- * msm_bus_paths[num_usecases]
- * msm_bus_vectors[num_usecases][num_paths]
- */
- mem_size = sizeof(struct msm_bus_scale_pdata) +
- sizeof(struct msm_bus_paths) * num_usecases +
- sizeof(struct msm_bus_vectors) * num_usecases * num_paths;
-
- pdata = kzalloc(mem_size, GFP_KERNEL);
- if (!pdata)
- return NULL;
-
- ret = of_property_read_string(of_node, "qcom,msm-bus,name",
- &pdata->name);
- if (ret) {
- dev_err(dev, "Error: Client name not found\n");
- goto err;
- }
-
- if (of_property_read_bool(of_node, "qcom,msm-bus,active-only")) {
- pdata->active_only = 1;
- } else {
- dev_info(dev, "active_only flag absent.\n");
- dev_info(dev, "Using dual context by default\n");
- }
-
- pdata->num_usecases = num_usecases;
- pdata->usecase = (struct msm_bus_paths *)(pdata + 1);
-
- vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", &len);
- if (vec_arr == NULL) {
- dev_err(dev, "Error: Vector array not found\n");
- goto err;
- }
-
- if (len != num_usecases * num_paths * sizeof(uint32_t) * 4) {
- dev_err(dev, "Error: Length-error on getting vectors\n");
- goto err;
- }
-
- vectors = (struct msm_bus_vectors *)(pdata->usecase + num_usecases);
- for (i = 0; i < num_usecases; i++) {
- usecase = &pdata->usecase[i];
- usecase->num_paths = num_paths;
- usecase->vectors = &vectors[i];
-
- for (j = 0; j < num_paths; j++) {
- int index = ((i * num_paths) + j) * 4;
-
- usecase->vectors[j].src = be32_to_cpu(vec_arr[index]);
- usecase->vectors[j].dst =
- be32_to_cpu(vec_arr[index + 1]);
- usecase->vectors[j].ab = (uint64_t)
- KBTOB(be32_to_cpu(vec_arr[index + 2]));
- usecase->vectors[j].ib = (uint64_t)
- KBTOB(be32_to_cpu(vec_arr[index + 3]));
- }
- }
-
- return pdata;
-
-err:
- kfree(pdata);
-
- return NULL;
-}
-
-/* wil_platform API (callbacks) */
-
-static int wil_platform_bus_request(void *handle,
- uint32_t kbps /* KBytes/Sec */)
-{
- int rc, i;
- struct wil_platform_msm *msm = (struct wil_platform_msm *)handle;
- int vote = 0; /* vote 0 in case requested kbps cannot be satisfied */
- struct msm_bus_paths *usecase;
- uint32_t usecase_kbps;
- uint32_t min_kbps = ~0;
-
- /* find the lowest usecase that is bigger than requested kbps */
- for (i = 0; i < msm->pdata->num_usecases; i++) {
- usecase = &msm->pdata->usecase[i];
- /* assume we have single path (vectors[0]). If we ever
- * have multiple paths, need to define the behavior */
- usecase_kbps = div64_u64(usecase->vectors[0].ib, 1000);
- if (usecase_kbps >= kbps && usecase_kbps < min_kbps) {
- min_kbps = usecase_kbps;
- vote = i;
- }
- }
-
- rc = msm_bus_scale_client_update_request(msm->msm_bus_handle, vote);
- if (rc)
- dev_err(msm->dev, "Failed msm_bus voting. kbps=%d vote=%d, rc=%d\n",
- kbps, vote, rc);
- else
- /* TOOD: remove */
- dev_info(msm->dev, "msm_bus_scale_client_update_request succeeded. kbps=%d vote=%d\n",
- kbps, vote);
-
- return rc;
-}
-
-static void wil_platform_uninit(void *handle)
-{
- struct wil_platform_msm *msm = (struct wil_platform_msm *)handle;
-
- dev_info(msm->dev, "wil_platform_uninit\n");
-
- if (msm->msm_bus_handle)
- msm_bus_scale_unregister_client(msm->msm_bus_handle);
-
- kfree(msm->pdata);
- kfree(msm);
-}
-
-static int wil_platform_msm_bus_register(struct wil_platform_msm *msm,
- struct device_node *node)
-{
- msm->pdata = wil_platform_get_pdata(msm->dev, node);
- if (!msm->pdata) {
- dev_err(msm->dev, "Failed getting DT info\n");
- return -EINVAL;
- }
-
- msm->msm_bus_handle = msm_bus_scale_register_client(msm->pdata);
- if (!msm->msm_bus_handle) {
- dev_err(msm->dev, "Failed msm_bus registration\n");
- return -EINVAL;
- }
-
- dev_info(msm->dev, "msm_bus registration succeeded! handle 0x%x\n",
- msm->msm_bus_handle);
-
- return 0;
-}
-
-/**
- * wil_platform_msm_init() - wil6210 msm platform module init
- *
- * The function must be called before all other functions in this module.
- * It returns a handle which is used with the rest of the API
- *
- */
-void *wil_platform_msm_init(struct device *dev, struct wil_platform_ops *ops)
-{
- struct device_node *of_node;
- struct wil_platform_msm *msm;
- int rc;
-
- of_node = of_find_compatible_node(NULL, NULL, "qcom,wil6210");
- if (!of_node) {
- /* this could mean non-msm platform */
- dev_err(dev, "DT node not found\n");
- return NULL;
- }
-
- msm = kzalloc(sizeof(*msm), GFP_KERNEL);
- if (!msm)
- return NULL;
-
- msm->dev = dev;
-
- /* register with msm_bus module for scaling requests */
- rc = wil_platform_msm_bus_register(msm, of_node);
- if (rc)
- goto cleanup;
-
- memset(ops, 0, sizeof(*ops));
- ops->bus_request = wil_platform_bus_request;
- ops->uninit = wil_platform_uninit;
-
- return (void *)msm;
-
-cleanup:
- kfree(msm);
- return NULL;
-}
diff --git a/drivers/net/wireless/ath/wil6210/wil_platform_msm.h b/drivers/net/wireless/ath/wil6210/wil_platform_msm.h
deleted file mode 100644
index 2f2229edb498..000000000000
--- a/drivers/net/wireless/ath/wil6210/wil_platform_msm.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Copyright (c) 2014 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.
- */
-
-#ifndef __WIL_PLATFORM__MSM_H__
-#define __WIL_PLATFORM_MSM_H__
-
-#include "wil_platform.h"
-
-void *wil_platform_msm_init(struct device *dev, struct wil_platform_ops *ops);
-
-#endif /* __WIL_PLATFORM__MSM_H__ */



2014-10-02 12:12:00

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] wil6210: remove obsolete msm platform code

On Thursday 02 October 2014 14:29:06 Vladimir Kondratiev wrote:
> On Tuesday, September 30, 2014 05:48:20 PM Arnd Bergmann wrote:
> > The wil6210 driver has a glue layer for the Qualcomm MSM platform
> > code, which apparently could never build and is unlikely to
> > ever be able to in the future, as the MSM platform is being phased
> > out in favor of the new QCOM platform, and the driver relies
> > on out-of-tree infrastructure. Trying to build it currently
> > results in this error:
> >
> > drivers/net/wireless/ath/wil6210/wil_platform_msm.c:19:27: fatal error: linux/msm-bus.h: No such file or directory
> >
> > Removing the driver fixes the build and makes it possible to
> > build an allmodconfig kernel for MSM.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
> >
>
> We are investigating this, looking for the appropriate solution.
> Thanks for pointing to this.

FWIW, I took a closer look at the actual driver code now. It's
actually worse than I thought and I don't see any possible way
out other than removing this driver completely and starting a
new one, if you want to use it for mach-qcom. I hadn't realized
before that this is actually new code rather than ancient bitrot
that suddenly started causing problems.

Some of the worst problems are:

- the driver scans for DT properties that are completely
undocumented and have not been reviewed

- if they had been reviewed, the first comment you would have
received is that the binding makes no sense: you scan for
another node rather than taking the node you already have for
the PCI device, then pass data into a random soc-specific
driver API that is not part of the upstream kernel.

- as mentioned in the my patch above, the code has never been
compiled on a mainline kernel and now breaks builds.

I think the only way forward is to have the platform support for
whatever chip this is meant for reviewed and merged first and
then you can write a new glue driver for the interfaces we end
up putting into the kernel.
My guess is that with proper interfaces between the platform
bus driver and the wil driver glue, there would be very little
left in terms of interface and you can just call a generic
interface from the PCI driver, passing the 'struct device'
for the PCI dev in to an exported interface when provided.

Arnd

2014-10-02 11:29:13

by Vladimir Kondratiev

[permalink] [raw]
Subject: Re: [PATCH] wil6210: remove obsolete msm platform code

On Tuesday, September 30, 2014 05:48:20 PM Arnd Bergmann wrote:
> The wil6210 driver has a glue layer for the Qualcomm MSM platform
> code, which apparently could never build and is unlikely to
> ever be able to in the future, as the MSM platform is being phased
> out in favor of the new QCOM platform, and the driver relies
> on out-of-tree infrastructure. Trying to build it currently
> results in this error:
>
> drivers/net/wireless/ath/wil6210/wil_platform_msm.c:19:27: fatal error: linux/msm-bus.h: No such file or directory
>
> Removing the driver fixes the build and makes it possible to
> build an allmodconfig kernel for MSM.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
>

We are investigating this, looking for the appropriate solution.
Thanks for pointing to this.

Thanks, Vladimir