* added acked-by for wl1251 patches - Kalle Valo <kvalo@codeaurora.org> * really removed old pdata-quirks code (not through #if 0) * splited out a partial revert of efdfeb079cc3b ("regulator: fixed: Convert to use GPIO descriptor only") because that was introduced after v4.19 and stops the removal of the pdata-quirks patch from cleanly applying to v4.9, v4.14, v4.19 - reported by Sasha Levin <sashal@kernel.org> * added a new patch to remove old omap hsmmc since pdata quirks were last user - suggested by Tony Lindgren <tony@atomide.com> PATCH V1 2019-10-18 22:25:39: Here we have a set of scattered patches to make the OpenPandora WiFi work again. v4.7 did break the pdata-quirks which made the mmc3 interface fail completely, because some code now assumes device tree based instantiation. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") v4.11 did break the sdio qirks for wl1251 which made the driver no longer load, although the device was found as an sdio client. Fixes: 884f38607897 ("mmc: core: move some sdio IDs out of quirks file") To solve these issues: * we convert mmc3 and wl1251 initialization from pdata-quirks to device tree * we make the wl1251 driver read properties from device tree * we fix the mmc core vendor ids and quirks * we fix the wl1251 (and wl1271) driver to use only vendor ids from header file instead of (potentially conflicting) local definitions H. Nikolaus Schaller (11): Documentation: dt: wireless: update wl1251 for sdio net: wireless: ti: wl1251 add device tree support DTS: ARM: pandora-common: define wl1251 as child node of mmc3 mmc: host: omap_hsmmc: add code for special init of wl1251 to get rid of pandora_wl1251_init_card omap: pdata-quirks: revert pandora specific gpiod additions omap: pdata-quirks: remove openpandora quirks for mmc3 and wl1251 omap: remove old hsmmc.[ch] and in Makefile mmc: sdio: fix wl1251 vendor id mmc: core: fix wl1251 sdio quirks net: wireless: ti: wl1251 use new SDIO_VENDOR_ID_TI_WL1251 definition net: wireless: ti: remove local VENDOR_ID and DEVICE_ID definitions .../bindings/net/wireless/ti,wl1251.txt | 26 +++ arch/arm/boot/dts/omap3-pandora-common.dtsi | 37 +++- arch/arm/mach-omap2/Makefile | 1 - arch/arm/mach-omap2/hsmmc.c | 171 ------------------ arch/arm/mach-omap2/hsmmc.h | 32 ---- arch/arm/mach-omap2/pdata-quirks.c | 105 ----------- drivers/mmc/core/quirks.h | 7 + drivers/mmc/host/omap_hsmmc.c | 21 +++ drivers/net/wireless/ti/wl1251/sdio.c | 23 ++- drivers/net/wireless/ti/wlcore/sdio.c | 8 - include/linux/mmc/sdio_ids.h | 2 + 11 files changed, 105 insertions(+), 328 deletions(-) delete mode 100644 arch/arm/mach-omap2/hsmmc.c delete mode 100644 arch/arm/mach-omap2/hsmmc.h -- 2.19.1
The standard method for sdio devices connected to an sdio interface is to define them as a child node like we can see with wlcore. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Acked-by: Kalle Valo <kvalo@codeaurora.org> --- .../bindings/net/wireless/ti,wl1251.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt index bb2fcde6f7ff..88612ff29f2d 100644 --- a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt @@ -35,3 +35,29 @@ Examples: ti,power-gpio = <&gpio3 23 GPIO_ACTIVE_HIGH>; /* 87 */ }; }; + +&mmc3 { + vmmc-supply = <&wlan_en>; + + bus-width = <4>; + non-removable; + ti,non-removable; + cap-power-off-card; + + pinctrl-names = "default"; + pinctrl-0 = <&mmc3_pins>; + + #address-cells = <1>; + #size-cells = <0>; + + wlan: wl1251@1 { + compatible = "ti,wl1251"; + + reg = <1>; + + interrupt-parent = <&gpio1>; + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ + + ti,wl1251-has-eeprom; + }; +}; -- 2.19.1
We will have the wl1251 defined as a child node of the mmc interface and can read setup for gpios, interrupts and the ti,use-eeprom property from there instead of pdata to be provided by pdata-quirks. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Acked-by: Kalle Valo <kvalo@codeaurora.org> --- drivers/net/wireless/ti/wl1251/sdio.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/net/wireless/ti/wl1251/sdio.c b/drivers/net/wireless/ti/wl1251/sdio.c index 677f1146ccf0..c54a273713ed 100644 --- a/drivers/net/wireless/ti/wl1251/sdio.c +++ b/drivers/net/wireless/ti/wl1251/sdio.c @@ -16,6 +16,9 @@ #include <linux/irq.h> #include <linux/pm_runtime.h> #include <linux/gpio.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/of_irq.h> #include "wl1251.h" @@ -217,6 +220,7 @@ static int wl1251_sdio_probe(struct sdio_func *func, struct ieee80211_hw *hw; struct wl1251_sdio *wl_sdio; const struct wl1251_platform_data *wl1251_board_data; + struct device_node *np = func->dev.of_node; hw = wl1251_alloc_hw(); if (IS_ERR(hw)) @@ -248,6 +252,15 @@ static int wl1251_sdio_probe(struct sdio_func *func, wl->power_gpio = wl1251_board_data->power_gpio; wl->irq = wl1251_board_data->irq; wl->use_eeprom = wl1251_board_data->use_eeprom; + } else if (np) { + wl->use_eeprom =of_property_read_bool(np, "ti,wl1251-has-eeprom"); + wl->power_gpio = of_get_named_gpio(np, "ti,power-gpio", 0); + wl->irq = of_irq_get(np, 0); + + if (wl->power_gpio == -EPROBE_DEFER || wl->irq == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto disable; + } } if (gpio_is_valid(wl->power_gpio)) { -- 2.19.1
Since v4.7 the dma initialization requires that there is a device tree property for "rx" and "tx" channels which is not provided by the pdata-quirks initialization. By conversion of the mmc3 setup to device tree this will finally allows to remove the OpenPandora wlan specific omap3 data-quirks. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.7.0 --- arch/arm/boot/dts/omap3-pandora-common.dtsi | 37 +++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi b/arch/arm/boot/dts/omap3-pandora-common.dtsi index ec5891718ae6..c595b3eb314d 100644 --- a/arch/arm/boot/dts/omap3-pandora-common.dtsi +++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi @@ -226,6 +226,18 @@ gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>; /* GPIO_164 */ }; + /* wl1251 wifi+bt module */ + wlan_en: fixed-regulator-wg7210_en { + compatible = "regulator-fixed"; + regulator-name = "vwlan"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + startup-delay-us = <50000>; + regulator-always-on; + enable-active-high; + gpio = <&gpio1 23 GPIO_ACTIVE_HIGH>; + }; + /* wg7210 (wifi+bt module) 32k clock buffer */ wg7210_32k: fixed-regulator-wg7210_32k { compatible = "regulator-fixed"; @@ -522,9 +534,30 @@ /*wp-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;*/ /* GPIO_127 */ }; -/* mmc3 is probed using pdata-quirks to pass wl1251 card data */ &mmc3 { - status = "disabled"; + vmmc-supply = <&wlan_en>; + + bus-width = <4>; + non-removable; + ti,non-removable; + cap-power-off-card; + + pinctrl-names = "default"; + pinctrl-0 = <&mmc3_pins>; + + #address-cells = <1>; + #size-cells = <0>; + + wlan: wl1251@1 { + compatible = "ti,wl1251"; + + reg = <1>; + + interrupt-parent = <&gpio1>; + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ + + ti,wl1251-has-eeprom; + }; }; /* bluetooth*/ -- 2.19.1
Pandora_wl1251_init_card was used to do special pdata based setup of the sdio mmc interface. This does no longer work with v4.7 and later. A fix requires a device tree based mmc3 setup. Therefore we move the special setup to omap_hsmmc.c instead of calling some pdata supplied init_card function. The new code checks for a DT child node compatible to wl1251 so it will not affect other MMC3 use cases. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.7.0 --- drivers/mmc/host/omap_hsmmc.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 952fa4063ff8..03ba80bcf319 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1512,6 +1512,27 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) if (mmc_pdata(host)->init_card) mmc_pdata(host)->init_card(card); + else if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { + struct device_node *np = mmc_dev(mmc)->of_node; + + np = of_get_compatible_child(np, "ti,wl1251"); + if (np) { + /* + * We have TI wl1251 attached to MMC3. Pass this information to + * SDIO core because it can't be probed by normal methods. + */ + + dev_info(host->dev, "found wl1251\n"); + card->quirks |= MMC_QUIRK_NONSTD_SDIO; + card->cccr.wide_bus = 1; + card->cis.vendor = 0x104c; + card->cis.device = 0x9066; + card->cis.blksize = 512; + card->cis.max_dtr = 24000000; + card->ocr = 0x80; + of_node_put(np); + } + } } static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable) -- 2.19.1
introduced by commit efdfeb079cc3b ("regulator: fixed: Convert to use GPIO descriptor only") We must remove this from mainline first, so that the following patch to remove the openpandora quirks for mmc3 and wl1251 cleanly applies to stable v4.9, v4.14, v4.19 where the above mentioned patch is not yet present. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- arch/arm/mach-omap2/pdata-quirks.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c index d942a3357090..89734ef9ab1e 100644 --- a/arch/arm/mach-omap2/pdata-quirks.c +++ b/arch/arm/mach-omap2/pdata-quirks.c @@ -7,7 +7,6 @@ #include <linux/clk.h> #include <linux/davinci_emac.h> #include <linux/gpio.h> -#include <linux/gpio/machine.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/of_platform.h> @@ -327,7 +326,9 @@ static struct regulator_init_data pandora_vmmc3 = { static struct fixed_voltage_config pandora_vwlan = { .supply_name = "vwlan", .microvolts = 1800000, /* 1.8V */ + .gpio = PANDORA_WIFI_NRESET_GPIO, .startup_delay = 50000, /* 50ms */ + .enable_high = 1, .init_data = &pandora_vmmc3, }; @@ -339,19 +340,6 @@ static struct platform_device pandora_vwlan_device = { }, }; -static struct gpiod_lookup_table pandora_vwlan_gpiod_table = { - .dev_id = "reg-fixed-voltage.1", - .table = { - /* - * As this is a low GPIO number it should be at the first - * GPIO bank. - */ - GPIO_LOOKUP("gpio-0-31", PANDORA_WIFI_NRESET_GPIO, - NULL, GPIO_ACTIVE_HIGH), - { }, - }, -}; - static void pandora_wl1251_init_card(struct mmc_card *card) { /* @@ -373,6 +361,8 @@ static struct omap2_hsmmc_info pandora_mmc3[] = { { .mmc = 3, .caps = MMC_CAP_4_BIT_DATA | MMC_CAP_POWER_OFF_CARD, + .gpio_cd = -EINVAL, + .gpio_wp = -EINVAL, .init_card = pandora_wl1251_init_card, }, {} /* Terminator */ @@ -411,7 +401,6 @@ static void __init pandora_wl1251_init(void) static void __init omap3_pandora_legacy_init(void) { platform_device_register(&pandora_backlight); - gpiod_add_lookup_table(&pandora_vwlan_gpiod_table); platform_device_register(&pandora_vwlan_device); omap_hsmmc_init(pandora_mmc3); omap_hsmmc_late_init(pandora_mmc3); -- 2.19.1
With a wl1251 child node of mmc3 in the device tree decoded in omap_hsmmc.c to handle special wl1251 initialization, we do no longer need to instantiate the mmc3 through pdata quirks. We also can remove the wlan regulator and reset/interrupt definitions and do them through device tree. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.7.0 --- arch/arm/mach-omap2/pdata-quirks.c | 93 ------------------------------ 1 file changed, 93 deletions(-) diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c index 89734ef9ab1e..ecc1ef632951 100644 --- a/arch/arm/mach-omap2/pdata-quirks.c +++ b/arch/arm/mach-omap2/pdata-quirks.c @@ -303,108 +303,15 @@ static void __init omap3_logicpd_torpedo_init(void) } /* omap3pandora legacy devices */ -#define PANDORA_WIFI_IRQ_GPIO 21 -#define PANDORA_WIFI_NRESET_GPIO 23 static struct platform_device pandora_backlight = { .name = "pandora-backlight", .id = -1, }; -static struct regulator_consumer_supply pandora_vmmc3_supply[] = { - REGULATOR_SUPPLY("vmmc", "omap_hsmmc.2"), -}; - -static struct regulator_init_data pandora_vmmc3 = { - .constraints = { - .valid_ops_mask = REGULATOR_CHANGE_STATUS, - }, - .num_consumer_supplies = ARRAY_SIZE(pandora_vmmc3_supply), - .consumer_supplies = pandora_vmmc3_supply, -}; - -static struct fixed_voltage_config pandora_vwlan = { - .supply_name = "vwlan", - .microvolts = 1800000, /* 1.8V */ - .gpio = PANDORA_WIFI_NRESET_GPIO, - .startup_delay = 50000, /* 50ms */ - .enable_high = 1, - .init_data = &pandora_vmmc3, -}; - -static struct platform_device pandora_vwlan_device = { - .name = "reg-fixed-voltage", - .id = 1, - .dev = { - .platform_data = &pandora_vwlan, - }, -}; - -static void pandora_wl1251_init_card(struct mmc_card *card) -{ - /* - * We have TI wl1251 attached to MMC3. Pass this information to - * SDIO core because it can't be probed by normal methods. - */ - if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { - card->quirks |= MMC_QUIRK_NONSTD_SDIO; - card->cccr.wide_bus = 1; - card->cis.vendor = 0x104c; - card->cis.device = 0x9066; - card->cis.blksize = 512; - card->cis.max_dtr = 24000000; - card->ocr = 0x80; - } -} - -static struct omap2_hsmmc_info pandora_mmc3[] = { - { - .mmc = 3, - .caps = MMC_CAP_4_BIT_DATA | MMC_CAP_POWER_OFF_CARD, - .gpio_cd = -EINVAL, - .gpio_wp = -EINVAL, - .init_card = pandora_wl1251_init_card, - }, - {} /* Terminator */ -}; - -static void __init pandora_wl1251_init(void) -{ - struct wl1251_platform_data pandora_wl1251_pdata; - int ret; - - memset(&pandora_wl1251_pdata, 0, sizeof(pandora_wl1251_pdata)); - - pandora_wl1251_pdata.power_gpio = -1; - - ret = gpio_request_one(PANDORA_WIFI_IRQ_GPIO, GPIOF_IN, "wl1251 irq"); - if (ret < 0) - goto fail; - - pandora_wl1251_pdata.irq = gpio_to_irq(PANDORA_WIFI_IRQ_GPIO); - if (pandora_wl1251_pdata.irq < 0) - goto fail_irq; - - pandora_wl1251_pdata.use_eeprom = true; - ret = wl1251_set_platform_data(&pandora_wl1251_pdata); - if (ret < 0) - goto fail_irq; - - return; - -fail_irq: - gpio_free(PANDORA_WIFI_IRQ_GPIO); -fail: - pr_err("wl1251 board initialisation failed\n"); -} - static void __init omap3_pandora_legacy_init(void) { platform_device_register(&pandora_backlight); - platform_device_register(&pandora_vwlan_device); - omap_hsmmc_init(pandora_mmc3); - omap_hsmmc_late_init(pandora_mmc3); - pandora_wl1251_init(); } #endif /* CONFIG_ARCH_OMAP3 */ -- 2.19.1
There is a new one in drivers/mmc/host/omap_hsmmc.c configured by CONFIG_MMC_OMAP_HS and the last user was the pdata-quirks for pandora. Suggested-by: Tony Lindgren <tony@atomide.com> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- arch/arm/mach-omap2/Makefile | 1 - arch/arm/mach-omap2/hsmmc.c | 171 ----------------------------- arch/arm/mach-omap2/hsmmc.h | 32 ------ arch/arm/mach-omap2/pdata-quirks.c | 1 - 4 files changed, 205 deletions(-) delete mode 100644 arch/arm/mach-omap2/hsmmc.c delete mode 100644 arch/arm/mach-omap2/hsmmc.h diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index 8f208197988f..1b3062f32899 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -216,7 +216,6 @@ obj-$(CONFIG_MACH_NOKIA_N8X0) += board-n8x0.o # Platform specific device init code -omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y) obj-y += omap_phy_internal.o diff --git a/arch/arm/mach-omap2/hsmmc.c b/arch/arm/mach-omap2/hsmmc.c deleted file mode 100644 index 63423ea6a240..000000000000 --- a/arch/arm/mach-omap2/hsmmc.c +++ /dev/null @@ -1,171 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * linux/arch/arm/mach-omap2/hsmmc.c - * - * Copyright (C) 2007-2008 Texas Instruments - * Copyright (C) 2008 Nokia Corporation - * Author: Texas Instruments - */ -#include <linux/kernel.h> -#include <linux/slab.h> -#include <linux/string.h> -#include <linux/delay.h> -#include <linux/mmc/host.h> -#include <linux/platform_data/hsmmc-omap.h> - -#include "soc.h" -#include "omap_device.h" - -#include "hsmmc.h" -#include "control.h" - -#if IS_ENABLED(CONFIG_MMC_OMAP_HS) - -static u16 control_pbias_offset; -static u16 control_devconf1_offset; - -#define HSMMC_NAME_LEN 9 - -static int __init omap_hsmmc_pdata_init(struct omap2_hsmmc_info *c, - struct omap_hsmmc_platform_data *mmc) -{ - char *hc_name; - - hc_name = kzalloc(HSMMC_NAME_LEN + 1, GFP_KERNEL); - if (!hc_name) - return -ENOMEM; - - snprintf(hc_name, (HSMMC_NAME_LEN + 1), "mmc%islot%i", c->mmc, 1); - mmc->name = hc_name; - mmc->caps = c->caps; - mmc->reg_offset = 0; - - return 0; -} - -static int omap_hsmmc_done; - -void omap_hsmmc_late_init(struct omap2_hsmmc_info *c) -{ - struct platform_device *pdev; - int res; - - if (omap_hsmmc_done) - return; - - omap_hsmmc_done = 1; - - for (; c->mmc; c++) { - pdev = c->pdev; - if (!pdev) - continue; - res = omap_device_register(pdev); - if (res) - pr_err("Could not late init MMC\n"); - } -} - -#define MAX_OMAP_MMC_HWMOD_NAME_LEN 16 - -static void __init omap_hsmmc_init_one(struct omap2_hsmmc_info *hsmmcinfo, - int ctrl_nr) -{ - struct omap_hwmod *oh; - struct omap_hwmod *ohs[1]; - struct omap_device *od; - struct platform_device *pdev; - char oh_name[MAX_OMAP_MMC_HWMOD_NAME_LEN]; - struct omap_hsmmc_platform_data *mmc_data; - struct omap_hsmmc_dev_attr *mmc_dev_attr; - char *name; - int res; - - mmc_data = kzalloc(sizeof(*mmc_data), GFP_KERNEL); - if (!mmc_data) - return; - - res = omap_hsmmc_pdata_init(hsmmcinfo, mmc_data); - if (res < 0) - goto free_mmc; - - name = "omap_hsmmc"; - res = snprintf(oh_name, MAX_OMAP_MMC_HWMOD_NAME_LEN, - "mmc%d", ctrl_nr); - WARN(res >= MAX_OMAP_MMC_HWMOD_NAME_LEN, - "String buffer overflow in MMC%d device setup\n", ctrl_nr); - - oh = omap_hwmod_lookup(oh_name); - if (!oh) { - pr_err("Could not look up %s\n", oh_name); - goto free_name; - } - ohs[0] = oh; - if (oh->dev_attr != NULL) { - mmc_dev_attr = oh->dev_attr; - mmc_data->controller_flags = mmc_dev_attr->flags; - } - - pdev = platform_device_alloc(name, ctrl_nr - 1); - if (!pdev) { - pr_err("Could not allocate pdev for %s\n", name); - goto free_name; - } - dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); - - od = omap_device_alloc(pdev, ohs, 1); - if (IS_ERR(od)) { - pr_err("Could not allocate od for %s\n", name); - goto put_pdev; - } - - res = platform_device_add_data(pdev, mmc_data, - sizeof(struct omap_hsmmc_platform_data)); - if (res) { - pr_err("Could not add pdata for %s\n", name); - goto put_pdev; - } - - hsmmcinfo->pdev = pdev; - - res = omap_device_register(pdev); - if (res) { - pr_err("Could not register od for %s\n", name); - goto free_od; - } - - goto free_mmc; - -free_od: - omap_device_delete(od); - -put_pdev: - platform_device_put(pdev); - -free_name: - kfree(mmc_data->name); - -free_mmc: - kfree(mmc_data); -} - -void __init omap_hsmmc_init(struct omap2_hsmmc_info *controllers) -{ - if (omap_hsmmc_done) - return; - - omap_hsmmc_done = 1; - - if (cpu_is_omap2430()) { - control_pbias_offset = OMAP243X_CONTROL_PBIAS_LITE; - control_devconf1_offset = OMAP243X_CONTROL_DEVCONF1; - } else { - control_pbias_offset = OMAP343X_CONTROL_PBIAS_LITE; - control_devconf1_offset = OMAP343X_CONTROL_DEVCONF1; - } - - for (; controllers->mmc; controllers++) - omap_hsmmc_init_one(controllers, controllers->mmc); - -} - -#endif diff --git a/arch/arm/mach-omap2/hsmmc.h b/arch/arm/mach-omap2/hsmmc.h deleted file mode 100644 index 76c5ed2afa72..000000000000 --- a/arch/arm/mach-omap2/hsmmc.h +++ /dev/null @@ -1,32 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * MMC definitions for OMAP2 - */ - -struct mmc_card; - -struct omap2_hsmmc_info { - u8 mmc; /* controller 1/2/3 */ - u32 caps; /* 4/8 wires and any additional host - * capabilities OR'd (ref. linux/mmc/host.h) */ - struct platform_device *pdev; /* mmc controller instance */ - /* init some special card */ - void (*init_card)(struct mmc_card *card); -}; - -#if IS_ENABLED(CONFIG_MMC_OMAP_HS) - -void omap_hsmmc_init(struct omap2_hsmmc_info *); -void omap_hsmmc_late_init(struct omap2_hsmmc_info *); - -#else - -static inline void omap_hsmmc_init(struct omap2_hsmmc_info *info) -{ -} - -static inline void omap_hsmmc_late_init(struct omap2_hsmmc_info *info) -{ -} - -#endif diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c index ecc1ef632951..4ce05d408a67 100644 --- a/arch/arm/mach-omap2/pdata-quirks.c +++ b/arch/arm/mach-omap2/pdata-quirks.c @@ -32,7 +32,6 @@ #include "omap_device.h" #include "omap-secure.h" #include "soc.h" -#include "hsmmc.h" static struct omap_hsmmc_platform_data __maybe_unused mmc_pdata[2]; -- 2.19.1
v4.11-rc1 did introduce a patch series that rearranged the sdio quirks into a header file. Unfortunately this did forget to handle SDIO_VENDOR_ID_TI differently between wl1251 and wl1271 with the result that although the wl1251 was found on the sdio bus, the firmware did not load any more and there was no interface registration. This patch defines separate constants to be used by sdio quirks and drivers. Fixes: 884f38607897 ("mmc: core: move some sdio IDs out of quirks file") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.11.0 --- include/linux/mmc/sdio_ids.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h index d1a5d5df02f5..08b25c02b5a1 100644 --- a/include/linux/mmc/sdio_ids.h +++ b/include/linux/mmc/sdio_ids.h @@ -71,6 +71,8 @@ #define SDIO_VENDOR_ID_TI 0x0097 #define SDIO_DEVICE_ID_TI_WL1271 0x4076 +#define SDIO_VENDOR_ID_TI_WL1251 0x104c +#define SDIO_DEVICE_ID_TI_WL1251 0x9066 #define SDIO_VENDOR_ID_STE 0x0020 #define SDIO_DEVICE_ID_STE_CW1200 0x2280 -- 2.19.1
wl1251 and wl1271 have different vendor id and device id. So we need to handle both with sdio quirks. Fixes: 884f38607897 ("mmc: core: move some sdio IDs out of quirks file") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.11.0 --- drivers/mmc/core/quirks.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h index 2d2d9ea8be4f..3dba15bccce2 100644 --- a/drivers/mmc/core/quirks.h +++ b/drivers/mmc/core/quirks.h @@ -119,7 +119,14 @@ static const struct mmc_fixup mmc_ext_csd_fixups[] = { END_FIXUP }; + static const struct mmc_fixup sdio_fixup_methods[] = { + SDIO_FIXUP(SDIO_VENDOR_ID_TI_WL1251, SDIO_DEVICE_ID_TI_WL1251, + add_quirk, MMC_QUIRK_NONSTD_FUNC_IF), + + SDIO_FIXUP(SDIO_VENDOR_ID_TI_WL1251, SDIO_DEVICE_ID_TI_WL1251, + add_quirk, MMC_QUIRK_DISABLE_CD), + SDIO_FIXUP(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271, add_quirk, MMC_QUIRK_NONSTD_FUNC_IF), -- 2.19.1
SDIO_VENDOR_ID_TI_WL1251 is now defined in mmc/sdio_ids.h separately from SDIO_VENDOR_ID_TI for wl1271. Fixes: 884f38607897 ("mmc: core: move some sdio IDs out of quirks file") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Acked-by: Kalle Valo <kvalo@codeaurora.org> Cc: <stable@vger.kernel.org> # 4.11.0 --- drivers/net/wireless/ti/wl1251/sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ti/wl1251/sdio.c b/drivers/net/wireless/ti/wl1251/sdio.c index c54a273713ed..42b55f3a50df 100644 --- a/drivers/net/wireless/ti/wl1251/sdio.c +++ b/drivers/net/wireless/ti/wl1251/sdio.c @@ -52,7 +52,7 @@ static void wl1251_sdio_interrupt(struct sdio_func *func) } static const struct sdio_device_id wl1251_devices[] = { - { SDIO_DEVICE(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1251) }, + { SDIO_DEVICE(SDIO_VENDOR_ID_TI_WL1251, SDIO_DEVICE_ID_TI_WL1251) }, {} }; MODULE_DEVICE_TABLE(sdio, wl1251_devices); -- 2.19.1
They are already included from mmc/sdio_ids.h and do not need a local definition. Fixes: 884f38607897 ("mmc: core: move some sdio IDs out of quirks file") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Acked-by: Kalle Valo <kvalo@codeaurora.org> Cc: <stable@vger.kernel.org> # 4.11.0 --- drivers/net/wireless/ti/wl1251/sdio.c | 8 -------- drivers/net/wireless/ti/wlcore/sdio.c | 8 -------- 2 files changed, 16 deletions(-) diff --git a/drivers/net/wireless/ti/wl1251/sdio.c b/drivers/net/wireless/ti/wl1251/sdio.c index 42b55f3a50df..3c4d5e38c66c 100644 --- a/drivers/net/wireless/ti/wl1251/sdio.c +++ b/drivers/net/wireless/ti/wl1251/sdio.c @@ -22,14 +22,6 @@ #include "wl1251.h" -#ifndef SDIO_VENDOR_ID_TI -#define SDIO_VENDOR_ID_TI 0x104c -#endif - -#ifndef SDIO_DEVICE_ID_TI_WL1251 -#define SDIO_DEVICE_ID_TI_WL1251 0x9066 -#endif - struct wl1251_sdio { struct sdio_func *func; u32 elp_val; diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c index 7afaf35f2453..9fd8cf2d270c 100644 --- a/drivers/net/wireless/ti/wlcore/sdio.c +++ b/drivers/net/wireless/ti/wlcore/sdio.c @@ -26,14 +26,6 @@ #include "wl12xx_80211.h" #include "io.h" -#ifndef SDIO_VENDOR_ID_TI -#define SDIO_VENDOR_ID_TI 0x0097 -#endif - -#ifndef SDIO_DEVICE_ID_TI_WL1271 -#define SDIO_DEVICE_ID_TI_WL1271 0x4076 -#endif - static bool dump = false; struct wl12xx_sdio_glue { -- 2.19.1
* H. Nikolaus Schaller <hns@goldelico.com> [191019 18:43]:
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -216,7 +216,6 @@ obj-$(CONFIG_MACH_NOKIA_N8X0) += board-n8x0.o
>
> # Platform specific device init code
>
> -omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o
> obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y)
The related obj-y line can go now too, right?
And looks like common.h also has struct omap2_hsmmc_info
so maybe check by grepping for hsmmc_info to see it's gone.
Regards,
Tony
> Am 21.10.2019 um 16:30 schrieb Tony Lindgren <tony@atomide.com>: > > * H. Nikolaus Schaller <hns@goldelico.com> [191019 18:43]: >> --- a/arch/arm/mach-omap2/Makefile >> +++ b/arch/arm/mach-omap2/Makefile >> @@ -216,7 +216,6 @@ obj-$(CONFIG_MACH_NOKIA_N8X0) += board-n8x0.o >> >> # Platform specific device init code >> >> -omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o >> obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y) > > The related obj-y line can go now too, right? Yes, I think so. It is a construction that I have never seen before :) Therefore I did not recognize that it is related. > And looks like common.h also has struct omap2_hsmmc_info > so maybe check by grepping for hsmmc_info to see it's gone. Yes, it is just a forward-declaration of the struct name with no user anywhere. Scheduled for v3. BTW: should this series go through your tree since it is an omap machine? BR and thanks, Nikolaus
* H. Nikolaus Schaller <hns@goldelico.com> [191021 17:08]:
>
> > Am 21.10.2019 um 16:30 schrieb Tony Lindgren <tony@atomide.com>:
> >
> > * H. Nikolaus Schaller <hns@goldelico.com> [191019 18:43]:
> >> --- a/arch/arm/mach-omap2/Makefile
> >> +++ b/arch/arm/mach-omap2/Makefile
> >> @@ -216,7 +216,6 @@ obj-$(CONFIG_MACH_NOKIA_N8X0) += board-n8x0.o
> >>
> >> # Platform specific device init code
> >>
> >> -omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o
> >> obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y)
> >
> > The related obj-y line can go now too, right?
>
> Yes, I think so. It is a construction that I have never seen before :)
> Therefore I did not recognize that it is related.
>
> > And looks like common.h also has struct omap2_hsmmc_info
> > so maybe check by grepping for hsmmc_info to see it's gone.
>
> Yes, it is just a forward-declaration of the struct name with
> no user anywhere.
>
> Scheduled for v3.
>
> BTW: should this series go through your tree since it is an
> omap machine?
Or MMC tree as that's where the code change really are.
Regards,
Tony
- Trimmed cc-list (could be a good idea for next submission as well)
On Mon, 21 Oct 2019 at 19:11, Tony Lindgren <tony@atomide.com> wrote:
>
> * H. Nikolaus Schaller <hns@goldelico.com> [191021 17:08]:
> >
> > > Am 21.10.2019 um 16:30 schrieb Tony Lindgren <tony@atomide.com>:
> > >
> > > * H. Nikolaus Schaller <hns@goldelico.com> [191019 18:43]:
> > >> --- a/arch/arm/mach-omap2/Makefile
> > >> +++ b/arch/arm/mach-omap2/Makefile
> > >> @@ -216,7 +216,6 @@ obj-$(CONFIG_MACH_NOKIA_N8X0) += board-n8x0.o
> > >>
> > >> # Platform specific device init code
> > >>
> > >> -omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o
> > >> obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y)
> > >
> > > The related obj-y line can go now too, right?
> >
> > Yes, I think so. It is a construction that I have never seen before :)
> > Therefore I did not recognize that it is related.
> >
> > > And looks like common.h also has struct omap2_hsmmc_info
> > > so maybe check by grepping for hsmmc_info to see it's gone.
> >
> > Yes, it is just a forward-declaration of the struct name with
> > no user anywhere.
> >
> > Scheduled for v3.
> >
> > BTW: should this series go through your tree since it is an
> > omap machine?
>
> Or MMC tree as that's where the code change really are.
I am okay with that. I will have a look at the series and provide some comments.
Kind regards
Uffe
On Sat, Oct 19, 2019 at 08:41:16PM +0200, H. Nikolaus Schaller wrote: > The standard method for sdio devices connected to > an sdio interface is to define them as a child node > like we can see with wlcore. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Acked-by: Kalle Valo <kvalo@codeaurora.org> > --- > .../bindings/net/wireless/ti,wl1251.txt | 26 +++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt > index bb2fcde6f7ff..88612ff29f2d 100644 > --- a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt > +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt > @@ -35,3 +35,29 @@ Examples: > ti,power-gpio = <&gpio3 23 GPIO_ACTIVE_HIGH>; /* 87 */ > }; > }; > + > +&mmc3 { > + vmmc-supply = <&wlan_en>; > + > + bus-width = <4>; > + non-removable; > + ti,non-removable; > + cap-power-off-card; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&mmc3_pins>; None of the above are really relevant to this binding. > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + wlan: wl1251@1 { wifi@1 > + compatible = "ti,wl1251"; > + > + reg = <1>; > + > + interrupt-parent = <&gpio1>; > + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ > + > + ti,wl1251-has-eeprom; > + }; > +}; > -- > 2.19.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> Am 25.10.2019 um 23:13 schrieb Rob Herring <robh@kernel.org>: > > On Sat, Oct 19, 2019 at 08:41:16PM +0200, H. Nikolaus Schaller wrote: >> The standard method for sdio devices connected to >> an sdio interface is to define them as a child node >> like we can see with wlcore. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> Acked-by: Kalle Valo <kvalo@codeaurora.org> >> --- >> .../bindings/net/wireless/ti,wl1251.txt | 26 +++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt >> index bb2fcde6f7ff..88612ff29f2d 100644 >> --- a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt >> +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt >> @@ -35,3 +35,29 @@ Examples: >> ti,power-gpio = <&gpio3 23 GPIO_ACTIVE_HIGH>; /* 87 */ >> }; >> }; >> + >> +&mmc3 { >> + vmmc-supply = <&wlan_en>; >> + >> + bus-width = <4>; >> + non-removable; >> + ti,non-removable; >> + cap-power-off-card; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mmc3_pins>; > > None of the above are really relevant to this binding. Ok, but how and where do we document that they are needed to make both ends of the interface work together? > >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + wlan: wl1251@1 { > > wifi@1 Ok. > >> + compatible = "ti,wl1251"; >> + >> + reg = <1>; >> + >> + interrupt-parent = <&gpio1>; >> + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ >> + >> + ti,wl1251-has-eeprom; >> + }; >> +}; >> -- >> 2.19.1 >> BR and thanks, Nikolaus
On Sat, 19 Oct 2019 at 20:42, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Pandora_wl1251_init_card was used to do special pdata based > setup of the sdio mmc interface. This does no longer work with > v4.7 and later. A fix requires a device tree based mmc3 setup. > > Therefore we move the special setup to omap_hsmmc.c instead > of calling some pdata supplied init_card function. > > The new code checks for a DT child node compatible to wl1251 > so it will not affect other MMC3 use cases. > > Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: <stable@vger.kernel.org> # 4.7.0 > --- > drivers/mmc/host/omap_hsmmc.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 952fa4063ff8..03ba80bcf319 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -1512,6 +1512,27 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) > > if (mmc_pdata(host)->init_card) > mmc_pdata(host)->init_card(card); > + else if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { > + struct device_node *np = mmc_dev(mmc)->of_node; > + > + np = of_get_compatible_child(np, "ti,wl1251"); > + if (np) { > + /* > + * We have TI wl1251 attached to MMC3. Pass this information to > + * SDIO core because it can't be probed by normal methods. > + */ > + > + dev_info(host->dev, "found wl1251\n"); > + card->quirks |= MMC_QUIRK_NONSTD_SDIO; > + card->cccr.wide_bus = 1; > + card->cis.vendor = 0x104c; > + card->cis.device = 0x9066; > + card->cis.blksize = 512; > + card->cis.max_dtr = 24000000; > + card->ocr = 0x80; These things should really be figured out by the mmc core during SDIO card initialization itself, not via the host ops ->init_card() callback. That is just poor hack, which in the long run should go away. Moreover, I think we should add a subnode to the host node in the DT, to describe the embedded SDIO card, rather than parsing the subnode for the SDIO func - as that seems wrong to me. To add a subnode for the SDIO card, we already have a binding that I think we should extend. Please have a look at Documentation/devicetree/bindings/mmc/mmc-card.txt. If you want an example of how to implement this for your case, do a git grep "broken-hpi" in the driver/mmc/core/, I think it will tell you more of what I have in mind. > + of_node_put(np); > + } > + } > } > > static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > -- > 2.19.1 > Kind regards Uffe
On Sat, 19 Oct 2019 at 20:42, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Since v4.7 the dma initialization requires that there is a > device tree property for "rx" and "tx" channels which is > not provided by the pdata-quirks initialization. > > By conversion of the mmc3 setup to device tree this will > finally allows to remove the OpenPandora wlan specific omap3 > data-quirks. > > Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: <stable@vger.kernel.org> # 4.7.0 > --- > arch/arm/boot/dts/omap3-pandora-common.dtsi | 37 +++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi b/arch/arm/boot/dts/omap3-pandora-common.dtsi > index ec5891718ae6..c595b3eb314d 100644 > --- a/arch/arm/boot/dts/omap3-pandora-common.dtsi > +++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi > @@ -226,6 +226,18 @@ > gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>; /* GPIO_164 */ > }; > > + /* wl1251 wifi+bt module */ > + wlan_en: fixed-regulator-wg7210_en { > + compatible = "regulator-fixed"; > + regulator-name = "vwlan"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; I doubt these are correct. I guess this should be in the range of 2.7V-3.6V. > + startup-delay-us = <50000>; > + regulator-always-on; Always on? > + enable-active-high; > + gpio = <&gpio1 23 GPIO_ACTIVE_HIGH>; > + }; > + > /* wg7210 (wifi+bt module) 32k clock buffer */ > wg7210_32k: fixed-regulator-wg7210_32k { > compatible = "regulator-fixed"; > @@ -522,9 +534,30 @@ > /*wp-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;*/ /* GPIO_127 */ > }; > > -/* mmc3 is probed using pdata-quirks to pass wl1251 card data */ > &mmc3 { > - status = "disabled"; > + vmmc-supply = <&wlan_en>; > + > + bus-width = <4>; > + non-removable; > + ti,non-removable; > + cap-power-off-card; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&mmc3_pins>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + wlan: wl1251@1 { > + compatible = "ti,wl1251"; > + > + reg = <1>; > + > + interrupt-parent = <&gpio1>; > + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ > + > + ti,wl1251-has-eeprom; > + }; > }; > > /* bluetooth*/ > -- > 2.19.1 > Kind regards Uffe
Hi Ulf, > Am 30.10.2019 um 16:51 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > >> + >> + np = of_get_compatible_child(np, "ti,wl1251"); >> + if (np) { >> + /* >> + * We have TI wl1251 attached to MMC3. Pass this information to >> + * SDIO core because it can't be probed by normal methods. >> + */ >> + >> + dev_info(host->dev, "found wl1251\n"); >> + card->quirks |= MMC_QUIRK_NONSTD_SDIO; >> + card->cccr.wide_bus = 1; >> + card->cis.vendor = 0x104c; >> + card->cis.device = 0x9066; >> + card->cis.blksize = 512; >> + card->cis.max_dtr = 24000000; >> + card->ocr = 0x80; > > These things should really be figured out by the mmc core during SDIO > card initialization itself, not via the host ops ->init_card() > callback. That is just poor hack, which in the long run should go > away. Yes, I agree. But I am just the poor guy who is trying to fix really broken code with as low effort as possible. I don't even have a significant clue what this code is exactly doing and what the magic values mean. They were setup by pandora_wl1251_init_card() in the same way so that I have just moved the code here and make it called in (almost) the same situation. > Moreover, I think we should add a subnode to the host node in the DT, > to describe the embedded SDIO card, rather than parsing the subnode > for the SDIO func - as that seems wrong to me. You mean a second subnode? The wl1251 is the child node of the mmc node and describes the SDIO card. We just check if it is a wl1251 or e.g. wl1837 or something else or even no child. > To add a subnode for the SDIO card, we already have a binding that I > think we should extend. Please have a look at > Documentation/devicetree/bindings/mmc/mmc-card.txt. > > If you want an example of how to implement this for your case, do a > git grep "broken-hpi" in the driver/mmc/core/, I think it will tell > you more of what I have in mind. So while I agree that it should be improved in the long run, we should IMHO fix the hack first (broken since v4.9!), even if it remains a hack for now. Improving this part seems to be quite independent and focussed on the mmc subsystem, while the other patches involve other subsystems. Maybe should we make a REVISIT note in the code? Or add something to the commit message about the idea how it should be done right? > >> + of_node_put(np); >> + } >> + } >> } >> >> static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable) >> -- >> 2.19.1 >> > > Kind regards > Uffe BR and thanks, Nikolaus
> Am 30.10.2019 um 17:44 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > On Sat, 19 Oct 2019 at 20:42, H. Nikolaus Schaller <hns@goldelico.com> wrote: >> >> Since v4.7 the dma initialization requires that there is a >> device tree property for "rx" and "tx" channels which is >> not provided by the pdata-quirks initialization. >> >> By conversion of the mmc3 setup to device tree this will >> finally allows to remove the OpenPandora wlan specific omap3 >> data-quirks. >> >> Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> Cc: <stable@vger.kernel.org> # 4.7.0 >> --- >> arch/arm/boot/dts/omap3-pandora-common.dtsi | 37 +++++++++++++++++++-- >> 1 file changed, 35 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi b/arch/arm/boot/dts/omap3-pandora-common.dtsi >> index ec5891718ae6..c595b3eb314d 100644 >> --- a/arch/arm/boot/dts/omap3-pandora-common.dtsi >> +++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi >> @@ -226,6 +226,18 @@ >> gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>; /* GPIO_164 */ >> }; >> >> + /* wl1251 wifi+bt module */ >> + wlan_en: fixed-regulator-wg7210_en { >> + compatible = "regulator-fixed"; >> + regulator-name = "vwlan"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; > > I doubt these are correct. > > I guess this should be in the range of 2.7V-3.6V. Well, it is a gpio which enables some LDO inside the wifi chip. We do not really know the voltage it produces and it does not matter. The gpio voltage is 1.8V. Basically we use a fixed-regulator to "translate" a regulator into a control gpio because the mmc interface wants to see a vmmc-supply. > >> + startup-delay-us = <50000>; >> + regulator-always-on; > > Always on? Oops. Yes, that is something to check! > >> + enable-active-high; >> + gpio = <&gpio1 23 GPIO_ACTIVE_HIGH>; >> + }; >> + >> /* wg7210 (wifi+bt module) 32k clock buffer */ >> wg7210_32k: fixed-regulator-wg7210_32k { >> compatible = "regulator-fixed"; >> @@ -522,9 +534,30 @@ >> /*wp-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;*/ /* GPIO_127 */ >> }; >> >> -/* mmc3 is probed using pdata-quirks to pass wl1251 card data */ >> &mmc3 { >> - status = "disabled"; >> + vmmc-supply = <&wlan_en>; >> + >> + bus-width = <4>; >> + non-removable; >> + ti,non-removable; >> + cap-power-off-card; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mmc3_pins>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + wlan: wl1251@1 { >> + compatible = "ti,wl1251"; >> + >> + reg = <1>; >> + >> + interrupt-parent = <&gpio1>; >> + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ >> + >> + ti,wl1251-has-eeprom; >> + }; >> }; >> >> /* bluetooth*/ >> -- >> 2.19.1 >> BR and thanks, Nikolaus
On Wed, 30 Oct 2019 at 18:25, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Hi Ulf, > > > Am 30.10.2019 um 16:51 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > > >> + > >> + np = of_get_compatible_child(np, "ti,wl1251"); > >> + if (np) { > >> + /* > >> + * We have TI wl1251 attached to MMC3. Pass this information to > >> + * SDIO core because it can't be probed by normal methods. > >> + */ > >> + > >> + dev_info(host->dev, "found wl1251\n"); > >> + card->quirks |= MMC_QUIRK_NONSTD_SDIO; > >> + card->cccr.wide_bus = 1; > >> + card->cis.vendor = 0x104c; > >> + card->cis.device = 0x9066; > >> + card->cis.blksize = 512; > >> + card->cis.max_dtr = 24000000; > >> + card->ocr = 0x80; > > > > These things should really be figured out by the mmc core during SDIO > > card initialization itself, not via the host ops ->init_card() > > callback. That is just poor hack, which in the long run should go > > away. > > Yes, I agree. > > But I am just the poor guy who is trying to fix really broken code with > as low effort as possible. I see. Thanks for looking at this mess! In general, as long as we improve the code, I am happy to move forward. However, my main concern at this point, is to make sure we get the DT bindings and the DTS files updated correctly. We don't want to come back to this again. > > I don't even have a significant clue what this code is exactly doing and what > the magic values mean. They were setup by pandora_wl1251_init_card() in the > same way so that I have just moved the code here and make it called in (almost) > the same situation. Okay! > > > Moreover, I think we should add a subnode to the host node in the DT, > > to describe the embedded SDIO card, rather than parsing the subnode > > for the SDIO func - as that seems wrong to me. > > You mean a second subnode? > > The wl1251 is the child node of the mmc node and describes the SDIO card. > We just check if it is a wl1251 or e.g. wl1837 or something else or even > no child. The reason why I brought this up, was because there are sometimes cases where an SDIO card is shared between more than one SDIO func. WiFi+Bluetooth for example, but if I am correct, that is not the case for wl1251? That said, I am happy to continue with your approach. > > > To add a subnode for the SDIO card, we already have a binding that I > > think we should extend. Please have a look at > > Documentation/devicetree/bindings/mmc/mmc-card.txt. > > > > If you want an example of how to implement this for your case, do a > > git grep "broken-hpi" in the driver/mmc/core/, I think it will tell > > you more of what I have in mind. > > So while I agree that it should be improved in the long run, we should > IMHO fix the hack first (broken since v4.9!), even if it remains a hack > for now. Improving this part seems to be quite independent and focussed > on the mmc subsystem, while the other patches involve other subsystems. I agree. > > Maybe should we make a REVISIT note in the code? Or add something to > the commit message about the idea how it should be done right? Just add a note that we should move this DT parsing of the subnode to the mmc core, but that we are leaving that as a future improvement. That's good enough. Then I can have a look as a second step, and when I get some time, to move this to the mmc core. However, there is one thing I would like you to add to the series. That is: In the struct omap_hsmmc_platform_data, there is an ->init_card() callback. Beyond the changes of this series, there is no longer any users of that, unless I am mistaken. Going forward, let's make sure it doesn't get used again, so can you please remove it!? [...] Kind regards Uffe
On Wed, 30 Oct 2019 at 18:25, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > > > Am 30.10.2019 um 17:44 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > > > On Sat, 19 Oct 2019 at 20:42, H. Nikolaus Schaller <hns@goldelico.com> wrote: > >> > >> Since v4.7 the dma initialization requires that there is a > >> device tree property for "rx" and "tx" channels which is > >> not provided by the pdata-quirks initialization. > >> > >> By conversion of the mmc3 setup to device tree this will > >> finally allows to remove the OpenPandora wlan specific omap3 > >> data-quirks. > >> > >> Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") > >> > >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > >> Cc: <stable@vger.kernel.org> # 4.7.0 > >> --- > >> arch/arm/boot/dts/omap3-pandora-common.dtsi | 37 +++++++++++++++++++-- > >> 1 file changed, 35 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi b/arch/arm/boot/dts/omap3-pandora-common.dtsi > >> index ec5891718ae6..c595b3eb314d 100644 > >> --- a/arch/arm/boot/dts/omap3-pandora-common.dtsi > >> +++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi > >> @@ -226,6 +226,18 @@ > >> gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>; /* GPIO_164 */ > >> }; > >> > >> + /* wl1251 wifi+bt module */ > >> + wlan_en: fixed-regulator-wg7210_en { > >> + compatible = "regulator-fixed"; > >> + regulator-name = "vwlan"; > >> + regulator-min-microvolt = <1800000>; > >> + regulator-max-microvolt = <1800000>; > > > > I doubt these are correct. > > > > I guess this should be in the range of 2.7V-3.6V. > > Well, it is a gpio which enables some LDO inside the > wifi chip. We do not really know the voltage it produces > and it does not matter. The gpio voltage is 1.8V. > > Basically we use a fixed-regulator to "translate" a > regulator into a control gpio because the mmc interface > wants to see a vmmc-supply. The vmmc supply represent the core power to the SDIO card (or SD/(e)MMC). Depending on what voltage range the vmmc supply supports, the so called OCR mask is created by the mmc core. The mask is then used to let the core negotiate the voltage level with the SDIO card, during the card initialization. This is not to confuse with the I/O voltage level, which is a different regulator. Anyway, according to the TI WiLink series specifications, it looks like vmmc should be a regulator supporting 3-3.3V (in many schematics it's called VBAT). Furthermore I decided to dig into various DTS files that specifies the vmmc regulator, of course for mmc nodes having a subnode specifying an SDIO card for a TI WiLink. In most cases a 1.8V fixed GPIO regulator is used. This looks wrong to me. The fixed GPIO regulator isn't really the one that should model vmmc. The proper solution, would rather be to use separate regulator for vmmc and instead use a so called mmc-pwrseq node to manage the GPIO. To conclude from my side, as we have lots of DTS that are wrong, I don't really care if we add another one in the way you suggest above. But feel free to look into the mmc-pwrseq option. > > > > >> + startup-delay-us = <50000>; > >> + regulator-always-on; > > > > Always on? > > Oops. Yes, that is something to check! As it's a GPIO regulator, for sure it's not always on. > > > > >> + enable-active-high; > >> + gpio = <&gpio1 23 GPIO_ACTIVE_HIGH>; > >> + }; > >> + > >> /* wg7210 (wifi+bt module) 32k clock buffer */ > >> wg7210_32k: fixed-regulator-wg7210_32k { > >> compatible = "regulator-fixed"; > >> @@ -522,9 +534,30 @@ > >> /*wp-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;*/ /* GPIO_127 */ > >> }; > >> > >> -/* mmc3 is probed using pdata-quirks to pass wl1251 card data */ > >> &mmc3 { > >> - status = "disabled"; > >> + vmmc-supply = <&wlan_en>; > >> + > >> + bus-width = <4>; > >> + non-removable; > >> + ti,non-removable; > >> + cap-power-off-card; > >> + > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&mmc3_pins>; > >> + > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + wlan: wl1251@1 { > >> + compatible = "ti,wl1251"; > >> + > >> + reg = <1>; > >> + > >> + interrupt-parent = <&gpio1>; > >> + interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* GPIO_21 */ > >> + > >> + ti,wl1251-has-eeprom; > >> + }; > >> }; > >> > >> /* bluetooth*/ > >> -- > >> 2.19.1 > >> > > BR and thanks, > Nikolaus > Kind regards Uffe