[PATCH v4 1/3] mwifiex: Allow mwifiex early access to device structure

[PATCH v4 1/3] mwifiex: Allow mwifiex early access to device structure

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

Today all the interface drivers (usb/pcie/sdio) assign the
adapter->dev in the register_dev() callback, although they
have this piece of info well before hand.

This patch makes the device structure available for mwifiex
right at the beginning, so that it can be used for early
initialization if needed.

This is needed for subsequent patches in this patchset that
intend to unify and consolidate some of the code that would
otherwise have to be duplicated among the interface drivers
(sdio, pcie, usb).

Signed-off-by: Rajat Jain <rajatja@google.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1
v3: Fixed checkpatch warnings
WARNING: function definition argument 'void *' should also have an identifier name
#59: FILE: drivers/net/wireless/marvell/mwifiex/main.h:1415:
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,

WARNING: function definition argument 'struct semaphore *' should also have an identifier name
#59: FILE: drivers/net/wireless/marvell/mwifiex/main.h:1415:
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,

WARNING: function definition argument 'struct mwifiex_if_ops *' should also have an identifier name
#59: FILE: drivers/net/wireless/marvell/mwifiex/main.h:1415:
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,

WARNING: function definition argument 'u8' should also have an identifier name
#59: FILE: drivers/net/wireless/marvell/mwifiex/main.h:1415:
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,

WARNING: function definition argument 'struct device *' should also have an identifier name
#59: FILE: drivers/net/wireless/marvell/mwifiex/main.h:1415:
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
v4: Rebase v3 on top of "[v7] mwifiex: parse device tree node for PCIe"
---
 drivers/net/wireless/marvell/mwifiex/main.c | 4 +++-
 drivers/net/wireless/marvell/mwifiex/main.h | 4 +++-
 drivers/net/wireless/marvell/mwifiex/pcie.c | 4 +---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 5 +----
 drivers/net/wireless/marvell/mwifiex/usb.c  | 3 +--
 5 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 2478ccd..dcceab2 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,7 +1567,8 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
  */
 int
 mwifiex_add_card(void *card, struct semaphore *sem,
-		 struct mwifiex_if_ops *if_ops, u8 iface_type)
+		 struct mwifiex_if_ops *if_ops, u8 iface_type,
+		 struct device *dev)
 {
 	struct mwifiex_adapter *adapter;
 
@@ -1579,6 +1580,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 		goto err_init_sw;
 	}
 
+	adapter->dev = dev;
 	adapter->iface_type = iface_type;
 	adapter->card_sem = sem;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index d61fe3a..549e1ba 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1412,7 +1412,9 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
 
 int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
 			     u32 func_init_shutdown);
-int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8);
+int mwifiex_add_card(void *card, struct semaphore *sem,
+		     struct mwifiex_if_ops *if_ops, u8 iface_type,
+		     struct device *dev);
 int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
 
 void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 83a41b5..745ecd6 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -231,7 +231,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 	}
 
 	if (mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
-			     MWIFIEX_PCIE)) {
+			     MWIFIEX_PCIE, &pdev->dev)) {
 		pr_err("%s failed\n", __func__);
 		return -1;
 	}
@@ -2995,11 +2995,9 @@ static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
 static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
-	struct pci_dev *pdev = card->dev;
 
 	/* save adapter pointer in card */
 	card->adapter = adapter;
-	adapter->dev = &pdev->dev;
 
 	if (mwifiex_pcie_request_irq(adapter))
 		return -1;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 807af13..c95f41f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -206,7 +206,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
-			       MWIFIEX_SDIO);
+			       MWIFIEX_SDIO, &func->dev);
 	if (ret) {
 		dev_err(&func->dev, "add card failed\n");
 		goto err_disable;
@@ -2106,9 +2106,6 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 		return ret;
 	}
 
-
-	adapter->dev = &func->dev;
-
 	strcpy(adapter->fw_name, card->firmware);
 	if (card->fw_dump_enh) {
 		adapter->mem_type_mapping_tbl = generic_mem_type_map;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 73eb084..f847fff 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, card);
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
-			       MWIFIEX_USB);
+			       MWIFIEX_USB, &card->udev->dev);
 	if (ret) {
 		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
 		usb_reset_device(udev);
@@ -932,7 +932,6 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
 	card->adapter = adapter;
-	adapter->dev = &card->udev->dev;
 
 	switch (le16_to_cpu(card->udev->descriptor.idProduct)) {
 	case USB8997_PID_1:
-- 
1.9.1

[PATCH v4 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

Introduce function mwifiex_probe_of() to parse common properties.
Interface drivers get to decide whether or not the device tree node
was a valid one (depending on the compatible property),
Lets fill "adapter->dt_node" in mwifiex_add_card().

The function mwifiex_probe_of() is currently only a place holder with
the next patch adding content to it.

Signed-off-by: Rajat Jain <rajatja@google.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1
v3: Redundant flag "of_node_valid" is removed (Brian)
v4: Same as v3
---
 drivers/net/wireless/marvell/mwifiex/main.c    | 12 ++++++++++++
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c |  5 +----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index dcceab2..835d330 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1552,6 +1552,16 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 }
 EXPORT_SYMBOL_GPL(mwifiex_do_flr);
 
+static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
+{
+	struct device *dev = adapter->dev;
+
+	if (!dev->of_node)
+		return;
+
+	adapter->dt_node = dev->of_node;
+}
+
 /*
  * This function adds the card.
  *
@@ -1581,6 +1591,8 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 	}
 
 	adapter->dev = dev;
+	mwifiex_probe_of(adapter);
+
 	adapter->iface_type = iface_type;
 	adapter->card_sem = sem;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index b697b61..bcd6408 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2235,10 +2235,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
 		 * The cal-data can be read from device tree and/or
 		 * a configuration file and downloaded to firmware.
 		 */
-		if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
-		    priv->adapter->iface_type == MWIFIEX_PCIE) &&
-		    adapter->dev->of_node) {
-			adapter->dt_node = adapter->dev->of_node;
+		if (adapter->dt_node) {
 			if (of_property_read_u32(adapter->dt_node,
 						 "marvell,wakeup-pin",
 						 &data) == 0) {
-- 
1.9.1

[PATCH v4 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
support") added WoWLAN feature only for sdio. This patch moves that
code to the common module so that all the interface drivers can use
it for free. It enables pcie and sdio for its use currently.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: v1 doesn't apply smoothly on latest code due to recently merged
patch "mwifiex: report error to PCIe for suspend failure". Minor
conflict is resolved in v2
v4: Same as v2, v3
---
 drivers/net/wireless/marvell/mwifiex/main.c | 41 ++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h | 25 ++++++++++
 drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 72 ++---------------------------
 drivers/net/wireless/marvell/mwifiex/sdio.h |  8 ----
 5 files changed, 73 insertions(+), 75 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 835d330..948f5c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 }
 EXPORT_SYMBOL_GPL(mwifiex_do_flr);
 
+static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
+{
+	struct mwifiex_adapter *adapter = priv;
+
+	if (adapter->irq_wakeup >= 0) {
+		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
+		adapter->wake_by_wifi = true;
+		disable_irq_nosync(irq);
+	}
+
+	/* Notify PM core we are wakeup source */
+	pm_wakeup_event(adapter->dev, 0);
+
+	return IRQ_HANDLED;
+}
+
 static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 {
+	int ret;
 	struct device *dev = adapter->dev;
 
 	if (!dev->of_node)
 		return;
 
 	adapter->dt_node = dev->of_node;
+	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
+	if (!adapter->irq_wakeup) {
+		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
+		return;
+	}
+
+	ret = devm_request_irq(dev, adapter->irq_wakeup,
+			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
+			       "wifi_wake", adapter);
+	if (ret) {
+		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
+			adapter->irq_wakeup, ret);
+		goto err_exit;
+	}
+
+	disable_irq(adapter->irq_wakeup);
+	if (device_init_wakeup(dev, true)) {
+		dev_err(dev, "fail to init wakeup for mwifiex\n");
+		goto err_exit;
+	}
+	return;
+
+err_exit:
+	adapter->irq_wakeup = 0;
 }
 
 /*
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 549e1ba..ae5afe5 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
 	bool usb_mc_setup;
 	struct cfg80211_wowlan_nd_info *nd_info;
 	struct ieee80211_regdomain *regd;
+
+	/* Wake-on-WLAN (WoWLAN) */
+	int irq_wakeup;
+	bool wake_by_wifi;
 };
 
 void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
@@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
 	return false;
 }
 
+/* Disable platform specific wakeup interrupt */
+static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
+{
+	if (adapter->irq_wakeup >= 0) {
+		disable_irq_wake(adapter->irq_wakeup);
+		if (!adapter->wake_by_wifi)
+			disable_irq(adapter->irq_wakeup);
+	}
+}
+
+/* Enable platform specific wakeup interrupt */
+static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
+{
+	/* Enable platform specific wakeup interrupt */
+	if (adapter->irq_wakeup >= 0) {
+		adapter->wake_by_wifi = false;
+		enable_irq(adapter->irq_wakeup);
+		enable_irq_wake(adapter->irq_wakeup);
+	}
+}
+
 int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
 			     u32 func_init_shutdown);
 int mwifiex_add_card(void *card, struct semaphore *sem,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 745ecd6..e8f4f90 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -131,6 +131,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	}
 
 	adapter = card->adapter;
+	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
 	if (!mwifiex_enable_hs(adapter)) {
@@ -186,6 +187,7 @@ static int mwifiex_pcie_resume(struct device *dev)
 
 	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
 			  MWIFIEX_ASYNC_CMD);
+	mwifiex_disable_wake(adapter);
 
 	return 0;
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index c95f41f..7055282 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -79,67 +79,18 @@
 	{ }
 };
 
-static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
-{
-	struct mwifiex_plt_wake_cfg *cfg = priv;
-
-	if (cfg->irq_wifi >= 0) {
-		pr_info("%s: wake by wifi", __func__);
-		cfg->wake_by_wifi = true;
-		disable_irq_nosync(irq);
-	}
-
-	/* Notify PM core we are wakeup source */
-	pm_wakeup_event(cfg->dev, 0);
-
-	return IRQ_HANDLED;
-}
-
 /* This function parse device tree node using mmc subnode devicetree API.
  * The device node is saved in card->plt_of_node.
  * if the device tree node exist and include interrupts attributes, this
  * function will also request platform specific wakeup interrupt.
  */
-static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
+static int mwifiex_sdio_probe_of(struct device *dev)
 {
-	struct mwifiex_plt_wake_cfg *cfg;
-	int ret;
-
 	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
 		dev_err(dev, "required compatible string missing\n");
 		return -EINVAL;
 	}
 
-	card->plt_of_node = dev->of_node;
-	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
-					  GFP_KERNEL);
-	cfg = card->plt_wake_cfg;
-	if (cfg && card->plt_of_node) {
-		cfg->dev = dev;
-		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
-		if (!cfg->irq_wifi) {
-			dev_dbg(dev,
-				"fail to parse irq_wifi from device tree\n");
-		} else {
-			ret = devm_request_irq(dev, cfg->irq_wifi,
-					       mwifiex_wake_irq_wifi,
-					       IRQF_TRIGGER_LOW,
-					       "wifi_wake", cfg);
-			if (ret) {
-				dev_dbg(dev,
-					"Failed to request irq_wifi %d (%d)\n",
-					cfg->irq_wifi, ret);
-				card->plt_wake_cfg = NULL;
-				return 0;
-			}
-			disable_irq(cfg->irq_wifi);
-		}
-	}
-
-	ret = device_init_wakeup(dev, true);
-	if (ret)
-		dev_err(dev, "fail to init wakeup for mwifiex");
-
 	return 0;
 }
 
@@ -198,11 +149,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 
 	/* device tree node parsing and platform specific configuration*/
 	if (func->dev.of_node) {
-		ret = mwifiex_sdio_probe_of(&func->dev, card);
-		if (ret) {
-			dev_err(&func->dev, "SDIO dt node parse failed\n");
+		ret = mwifiex_sdio_probe_of(&func->dev);
+		if (ret)
 			goto err_disable;
-		}
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
@@ -267,12 +216,7 @@ static int mwifiex_sdio_resume(struct device *dev)
 	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
 			  MWIFIEX_SYNC_CMD);
 
-	/* Disable platform specific wakeup interrupt */
-	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
-		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
-		if (!card->plt_wake_cfg->wake_by_wifi)
-			disable_irq(card->plt_wake_cfg->irq_wifi);
-	}
+	mwifiex_disable_wake(adapter);
 
 	return 0;
 }
@@ -352,13 +296,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
 	}
 
 	adapter = card->adapter;
-
-	/* Enable platform specific wakeup interrupt */
-	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
-		card->plt_wake_cfg->wake_by_wifi = false;
-		enable_irq(card->plt_wake_cfg->irq_wifi);
-		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
-	}
+	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
 	if (!mwifiex_enable_hs(adapter)) {
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index 07cdd23..b9fbc5c 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -154,12 +154,6 @@
 	a->mpa_rx.start_port = 0;					\
 } while (0)
 
-struct mwifiex_plt_wake_cfg {
-	struct device *dev;
-	int irq_wifi;
-	bool wake_by_wifi;
-};
-
 /* data structure for SDIO MPA TX */
 struct mwifiex_sdio_mpa_tx {
 	/* multiport tx aggregation buffer pointer */
@@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
 struct sdio_mmc_card {
 	struct sdio_func *func;
 	struct mwifiex_adapter *adapter;
-	struct device_node *plt_of_node;
-	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
 
 	const char *firmware;
 	const struct mwifiex_sdio_card_reg *reg;
-- 
1.9.1

Re: [PATCH v4 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Dmitry Torokhov]

On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
> 
> Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
> support") added WoWLAN feature only for sdio. This patch moves that
> code to the common module so that all the interface drivers can use
> it for free. It enables pcie and sdio for its use currently.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: v1 doesn't apply smoothly on latest code due to recently merged
> patch "mwifiex: report error to PCIe for suspend failure". Minor
> conflict is resolved in v2
> v4: Same as v2, v3
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 41 ++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/main.h | 25 ++++++++++
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 72 ++---------------------------
>  drivers/net/wireless/marvell/mwifiex/sdio.h |  8 ----
>  5 files changed, 73 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 835d330..948f5c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>  
> +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> +{
> +	struct mwifiex_adapter *adapter = priv;
> +
> +	if (adapter->irq_wakeup >= 0) {
> +		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> +		adapter->wake_by_wifi = true;
> +		disable_irq_nosync(irq);
> +	}
> +
> +	/* Notify PM core we are wakeup source */
> +	pm_wakeup_event(adapter->dev, 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
>  {
> +	int ret;
>  	struct device *dev = adapter->dev;
>  
>  	if (!dev->of_node)
>  		return;
>  
>  	adapter->dt_node = dev->of_node;
> +	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> +	if (!adapter->irq_wakeup) {
> +		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> +		return;
> +	}

I'd rather we used of_irq_get() here, because ti allows handling
deferrals. of_irq_get_byname() would be even better, but I guess we
already have bindings in the wild...

> +
> +	ret = devm_request_irq(dev, adapter->irq_wakeup,
> +			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,

irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
why do we override them?

> +			       "wifi_wake", adapter);
> +	if (ret) {
> +		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> +			adapter->irq_wakeup, ret);
> +		goto err_exit;
> +	}
> +
> +	disable_irq(adapter->irq_wakeup);
> +	if (device_init_wakeup(dev, true)) {
> +		dev_err(dev, "fail to init wakeup for mwifiex\n");
> +		goto err_exit;

Leaking interrupt (not forever, but if we are not using wakeup irq there
is no need to have it claimed).

> +	}
> +	return;
> +
> +err_exit:
> +	adapter->irq_wakeup = 0;
>  }

I also do not see anyone actually calling mwifiex_probe_of() in this
patch?

>  
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 549e1ba..ae5afe5 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
>  	bool usb_mc_setup;
>  	struct cfg80211_wowlan_nd_info *nd_info;
>  	struct ieee80211_regdomain *regd;
> +
> +	/* Wake-on-WLAN (WoWLAN) */
> +	int irq_wakeup;
> +	bool wake_by_wifi;
>  };
>  
>  void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
>  	return false;
>  }
>  
> +/* Disable platform specific wakeup interrupt */
> +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> +{
> +	if (adapter->irq_wakeup >= 0) {
> +		disable_irq_wake(adapter->irq_wakeup);
> +		if (!adapter->wake_by_wifi)
> +			disable_irq(adapter->irq_wakeup);
> +	}
> +}
> +
> +/* Enable platform specific wakeup interrupt */
> +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> +{
> +	/* Enable platform specific wakeup interrupt */
> +	if (adapter->irq_wakeup >= 0) {
> +		adapter->wake_by_wifi = false;
> +		enable_irq(adapter->irq_wakeup);
> +		enable_irq_wake(adapter->irq_wakeup);
> +	}
> +}
> +
>  int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
>  			     u32 func_init_shutdown);
>  int mwifiex_add_card(void *card, struct semaphore *sem,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 745ecd6..e8f4f90 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -131,6 +131,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
>  	}
>  
>  	adapter = card->adapter;
> +	mwifiex_enable_wake(adapter);
>  
>  	/* Enable the Host Sleep */
>  	if (!mwifiex_enable_hs(adapter)) {
> @@ -186,6 +187,7 @@ static int mwifiex_pcie_resume(struct device *dev)
>  
>  	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>  			  MWIFIEX_ASYNC_CMD);
> +	mwifiex_disable_wake(adapter);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..7055282 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -79,67 +79,18 @@
>  	{ }
>  };
>  
> -static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
> -{
> -	struct mwifiex_plt_wake_cfg *cfg = priv;
> -
> -	if (cfg->irq_wifi >= 0) {
> -		pr_info("%s: wake by wifi", __func__);
> -		cfg->wake_by_wifi = true;
> -		disable_irq_nosync(irq);
> -	}
> -
> -	/* Notify PM core we are wakeup source */
> -	pm_wakeup_event(cfg->dev, 0);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  /* This function parse device tree node using mmc subnode devicetree API.
>   * The device node is saved in card->plt_of_node.
>   * if the device tree node exist and include interrupts attributes, this
>   * function will also request platform specific wakeup interrupt.
>   */
> -static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> +static int mwifiex_sdio_probe_of(struct device *dev)
>  {
> -	struct mwifiex_plt_wake_cfg *cfg;
> -	int ret;
> -
>  	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
>  		dev_err(dev, "required compatible string missing\n");
>  		return -EINVAL;
>  	}
>  
> -	card->plt_of_node = dev->of_node;
> -	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
> -					  GFP_KERNEL);
> -	cfg = card->plt_wake_cfg;
> -	if (cfg && card->plt_of_node) {
> -		cfg->dev = dev;
> -		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
> -		if (!cfg->irq_wifi) {
> -			dev_dbg(dev,
> -				"fail to parse irq_wifi from device tree\n");
> -		} else {
> -			ret = devm_request_irq(dev, cfg->irq_wifi,
> -					       mwifiex_wake_irq_wifi,
> -					       IRQF_TRIGGER_LOW,
> -					       "wifi_wake", cfg);
> -			if (ret) {
> -				dev_dbg(dev,
> -					"Failed to request irq_wifi %d (%d)\n",
> -					cfg->irq_wifi, ret);
> -				card->plt_wake_cfg = NULL;
> -				return 0;
> -			}
> -			disable_irq(cfg->irq_wifi);
> -		}
> -	}
> -
> -	ret = device_init_wakeup(dev, true);
> -	if (ret)
> -		dev_err(dev, "fail to init wakeup for mwifiex");
> -
>  	return 0;
>  }
>  
> @@ -198,11 +149,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>  
>  	/* device tree node parsing and platform specific configuration*/
>  	if (func->dev.of_node) {
> -		ret = mwifiex_sdio_probe_of(&func->dev, card);
> -		if (ret) {
> -			dev_err(&func->dev, "SDIO dt node parse failed\n");
> +		ret = mwifiex_sdio_probe_of(&func->dev);
> +		if (ret)
>  			goto err_disable;
> -		}
>  	}
>  
>  	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> @@ -267,12 +216,7 @@ static int mwifiex_sdio_resume(struct device *dev)
>  	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>  			  MWIFIEX_SYNC_CMD);
>  
> -	/* Disable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -		if (!card->plt_wake_cfg->wake_by_wifi)
> -			disable_irq(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_disable_wake(adapter);
>  
>  	return 0;
>  }
> @@ -352,13 +296,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
>  	}
>  
>  	adapter = card->adapter;
> -
> -	/* Enable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		card->plt_wake_cfg->wake_by_wifi = false;
> -		enable_irq(card->plt_wake_cfg->irq_wifi);
> -		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_enable_wake(adapter);
>  
>  	/* Enable the Host Sleep */
>  	if (!mwifiex_enable_hs(adapter)) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 07cdd23..b9fbc5c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -154,12 +154,6 @@
>  	a->mpa_rx.start_port = 0;					\
>  } while (0)
>  
> -struct mwifiex_plt_wake_cfg {
> -	struct device *dev;
> -	int irq_wifi;
> -	bool wake_by_wifi;
> -};
> -
>  /* data structure for SDIO MPA TX */
>  struct mwifiex_sdio_mpa_tx {
>  	/* multiport tx aggregation buffer pointer */
> @@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
>  struct sdio_mmc_card {
>  	struct sdio_func *func;
>  	struct mwifiex_adapter *adapter;
> -	struct device_node *plt_of_node;
> -	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
>  
>  	const char *firmware;
>  	const struct mwifiex_sdio_card_reg *reg;
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

On Tue, Nov 15, 2016 at 09:35:07AM -0800, Dmitry Torokhov wrote:
> On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> >  }
> >  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
> >  
> > +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> > +{
> > +	struct mwifiex_adapter *adapter = priv;
> > +
> > +	if (adapter->irq_wakeup >= 0) {
> > +		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > +		adapter->wake_by_wifi = true;
> > +		disable_irq_nosync(irq);
> > +	}
> > +
> > +	/* Notify PM core we are wakeup source */
> > +	pm_wakeup_event(adapter->dev, 0);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> >  {
> > +	int ret;
> >  	struct device *dev = adapter->dev;
> >  
> >  	if (!dev->of_node)
> >  		return;
> >  
> >  	adapter->dt_node = dev->of_node;
> > +	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> > +	if (!adapter->irq_wakeup) {
> > +		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> > +		return;
> > +	}
> 
> I'd rather we used of_irq_get() here, because ti allows handling
> deferrals. of_irq_get_byname() would be even better, but I guess we
> already have bindings in the wild...

This function doesn't handle errors very gracefully at all; we just try,
and if we fail, we just skip the rest...

This could be an argument for rewriting the error handling to stop just
returning -1 in mwifiex_add_card() and use real Linux error codes.
Perhaps that can be a later cleanup?

> > +
> > +	ret = devm_request_irq(dev, adapter->irq_wakeup,
> > +			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> 
> irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
> why do we override them?
> 
> > +			       "wifi_wake", adapter);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> > +			adapter->irq_wakeup, ret);
> > +		goto err_exit;
> > +	}
> > +
> > +	disable_irq(adapter->irq_wakeup);
> > +	if (device_init_wakeup(dev, true)) {
> > +		dev_err(dev, "fail to init wakeup for mwifiex\n");
> > +		goto err_exit;
> 
> Leaking interrupt (not forever, but if we are not using wakeup irq there
> is no need to have it claimed).
> 
> > +	}
> > +	return;
> > +
> > +err_exit:
> > +	adapter->irq_wakeup = 0;
> >  }
> 
> I also do not see anyone actually calling mwifiex_probe_of() in this
> patch?

It was added to mwifiex_add_card() in the previous patch, so all users
with a proper ->dt_node would call it.

> >  
> >  /*

Brian

Re: [v4,1/3] mwifiex: Allow mwifiex early access to device structure

[Kalle Valo]

Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Rajat Jain <rajatja@google.com>
> 
> Today all the interface drivers (usb/pcie/sdio) assign the
> adapter->dev in the register_dev() callback, although they
> have this piece of info well before hand.
> 
> This patch makes the device structure available for mwifiex
> right at the beginning, so that it can be used for early
> initialization if needed.
> 
> This is needed for subsequent patches in this patchset that
> intend to unify and consolidate some of the code that would
> otherwise have to be duplicated among the interface drivers
> (sdio, pcie, usb).
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

This patchset doesn't apply:

Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 mwifiex: Introduce mwifiex_probe_of() to parse common properties

I guess it depends on this patch which is still under review:

mwifiex: parse device tree node for PCIe

-- 
https://patchwork.kernel.org/patch/9429835/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [v4,1/3] mwifiex: Allow mwifiex early access to device structure

[Kalle Valo]

Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Rajat Jain <rajatja@google.com>
> 
> Today all the interface drivers (usb/pcie/sdio) assign the
> adapter->dev in the register_dev() callback, although they
> have this piece of info well before hand.
> 
> This patch makes the device structure available for mwifiex
> right at the beginning, so that it can be used for early
> initialization if needed.
> 
> This is needed for subsequent patches in this patchset that
> intend to unify and consolidate some of the code that would
> otherwise have to be duplicated among the interface drivers
> (sdio, pcie, usb).
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

3 patches applied to wireless-drivers-next.git, thanks.

2e02b5814217 mwifiex: Allow mwifiex early access to device structure
5e28e5fbdcf0 mwifiex: Introduce mwifiex_probe_of() to parse common properties
853402a00823 mwifiex: Enable WoWLAN for both sdio and pcie

-- 
https://patchwork.kernel.org/patch/9429835/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[jeffy]

Hi guys,

with this patch, the pci device's irq might be override by this wakeup 
irq when not using msi:


/**
  * of_irq_parse_pci - Resolve the interrupt for a PCI device
  * @pdev:       the device whose interrupt is to be resolved
  * @out_irq:    structure of_irq filled by this function
  *
  * This function resolves the PCI interrupt for a given PCI device. If a
  * device-node exists for a given pci_dev, it will use normal OF tree
  * walking. If not, it will implement standard swizzling and walk up the
  * PCI tree until an device-node is found, at which point it will finish
  * resolving using the OF tree walking.
  */
int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args 
*out_irq)
{
...
         /* Check if we have a device node, if yes, fallback to standard
          * device tree parsing
          */
         dn = pci_device_to_OF_node(pdev);
         if (dn) {
                 rc = of_irq_parse_one(dn, 0, out_irq); <--- this would 
take wifi wake irq as pci irq instead of fallback to PCI_INTERRUPT_PIN
                 if (!rc)
                         return rc;
         }

         /* Ok, we don't, time to have fun. Let's start by building up an
          * interrupt spec.  we assume #interrupt-cells is 1, which is 
standard
          * for PCI. If you do different, then don't use that routine.
          */
         rc = pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);


but i have no idea how to fix it...



On 11/15/2016 09:36 PM, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
>
> Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
> support") added WoWLAN feature only for sdio. This patch moves that
> code to the common module so that all the interface drivers can use
> it for free. It enables pcie and sdio for its use currently.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: v1 doesn't apply smoothly on latest code due to recently merged
> patch "mwifiex: report error to PCIe for suspend failure". Minor
> conflict is resolved in v2
> v4: Same as v2, v3
> ---
>   drivers/net/wireless/marvell/mwifiex/main.c | 41 ++++++++++++++++
>   drivers/net/wireless/marvell/mwifiex/main.h | 25 ++++++++++
>   drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
>   drivers/net/wireless/marvell/mwifiex/sdio.c | 72 ++---------------------------
>   drivers/net/wireless/marvell/mwifiex/sdio.h |  8 ----
>   5 files changed, 73 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 835d330..948f5c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
>   }
>   EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>
> +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> +{
> +	struct mwifiex_adapter *adapter = priv;
> +
> +	if (adapter->irq_wakeup >= 0) {
> +		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> +		adapter->wake_by_wifi = true;
> +		disable_irq_nosync(irq);
> +	}
> +
> +	/* Notify PM core we are wakeup source */
> +	pm_wakeup_event(adapter->dev, 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
>   {
> +	int ret;
>   	struct device *dev = adapter->dev;
>
>   	if (!dev->of_node)
>   		return;
>
>   	adapter->dt_node = dev->of_node;
> +	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> +	if (!adapter->irq_wakeup) {
> +		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> +		return;
> +	}
> +
> +	ret = devm_request_irq(dev, adapter->irq_wakeup,
> +			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> +			       "wifi_wake", adapter);
> +	if (ret) {
> +		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> +			adapter->irq_wakeup, ret);
> +		goto err_exit;
> +	}
> +
> +	disable_irq(adapter->irq_wakeup);
> +	if (device_init_wakeup(dev, true)) {
> +		dev_err(dev, "fail to init wakeup for mwifiex\n");
> +		goto err_exit;
> +	}
> +	return;
> +
> +err_exit:
> +	adapter->irq_wakeup = 0;
>   }
>
>   /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 549e1ba..ae5afe5 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
>   	bool usb_mc_setup;
>   	struct cfg80211_wowlan_nd_info *nd_info;
>   	struct ieee80211_regdomain *regd;
> +
> +	/* Wake-on-WLAN (WoWLAN) */
> +	int irq_wakeup;
> +	bool wake_by_wifi;
>   };
>
>   void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
>   	return false;
>   }
>
> +/* Disable platform specific wakeup interrupt */
> +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> +{
> +	if (adapter->irq_wakeup >= 0) {
> +		disable_irq_wake(adapter->irq_wakeup);
> +		if (!adapter->wake_by_wifi)
> +			disable_irq(adapter->irq_wakeup);
> +	}
> +}
> +
> +/* Enable platform specific wakeup interrupt */
> +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> +{
> +	/* Enable platform specific wakeup interrupt */
> +	if (adapter->irq_wakeup >= 0) {
> +		adapter->wake_by_wifi = false;
> +		enable_irq(adapter->irq_wakeup);
> +		enable_irq_wake(adapter->irq_wakeup);
> +	}
> +}
> +
>   int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
>   			     u32 func_init_shutdown);
>   int mwifiex_add_card(void *card, struct semaphore *sem,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 745ecd6..e8f4f90 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -131,6 +131,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
>   	}
>
>   	adapter = card->adapter;
> +	mwifiex_enable_wake(adapter);
>
>   	/* Enable the Host Sleep */
>   	if (!mwifiex_enable_hs(adapter)) {
> @@ -186,6 +187,7 @@ static int mwifiex_pcie_resume(struct device *dev)
>
>   	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>   			  MWIFIEX_ASYNC_CMD);
> +	mwifiex_disable_wake(adapter);
>
>   	return 0;
>   }
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..7055282 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -79,67 +79,18 @@
>   	{ }
>   };
>
> -static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
> -{
> -	struct mwifiex_plt_wake_cfg *cfg = priv;
> -
> -	if (cfg->irq_wifi >= 0) {
> -		pr_info("%s: wake by wifi", __func__);
> -		cfg->wake_by_wifi = true;
> -		disable_irq_nosync(irq);
> -	}
> -
> -	/* Notify PM core we are wakeup source */
> -	pm_wakeup_event(cfg->dev, 0);
> -
> -	return IRQ_HANDLED;
> -}
> -
>   /* This function parse device tree node using mmc subnode devicetree API.
>    * The device node is saved in card->plt_of_node.
>    * if the device tree node exist and include interrupts attributes, this
>    * function will also request platform specific wakeup interrupt.
>    */
> -static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> +static int mwifiex_sdio_probe_of(struct device *dev)
>   {
> -	struct mwifiex_plt_wake_cfg *cfg;
> -	int ret;
> -
>   	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
>   		dev_err(dev, "required compatible string missing\n");
>   		return -EINVAL;
>   	}
>
> -	card->plt_of_node = dev->of_node;
> -	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
> -					  GFP_KERNEL);
> -	cfg = card->plt_wake_cfg;
> -	if (cfg && card->plt_of_node) {
> -		cfg->dev = dev;
> -		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
> -		if (!cfg->irq_wifi) {
> -			dev_dbg(dev,
> -				"fail to parse irq_wifi from device tree\n");
> -		} else {
> -			ret = devm_request_irq(dev, cfg->irq_wifi,
> -					       mwifiex_wake_irq_wifi,
> -					       IRQF_TRIGGER_LOW,
> -					       "wifi_wake", cfg);
> -			if (ret) {
> -				dev_dbg(dev,
> -					"Failed to request irq_wifi %d (%d)\n",
> -					cfg->irq_wifi, ret);
> -				card->plt_wake_cfg = NULL;
> -				return 0;
> -			}
> -			disable_irq(cfg->irq_wifi);
> -		}
> -	}
> -
> -	ret = device_init_wakeup(dev, true);
> -	if (ret)
> -		dev_err(dev, "fail to init wakeup for mwifiex");
> -
>   	return 0;
>   }
>
> @@ -198,11 +149,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>
>   	/* device tree node parsing and platform specific configuration*/
>   	if (func->dev.of_node) {
> -		ret = mwifiex_sdio_probe_of(&func->dev, card);
> -		if (ret) {
> -			dev_err(&func->dev, "SDIO dt node parse failed\n");
> +		ret = mwifiex_sdio_probe_of(&func->dev);
> +		if (ret)
>   			goto err_disable;
> -		}
>   	}
>
>   	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> @@ -267,12 +216,7 @@ static int mwifiex_sdio_resume(struct device *dev)
>   	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>   			  MWIFIEX_SYNC_CMD);
>
> -	/* Disable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -		if (!card->plt_wake_cfg->wake_by_wifi)
> -			disable_irq(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_disable_wake(adapter);
>
>   	return 0;
>   }
> @@ -352,13 +296,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
>   	}
>
>   	adapter = card->adapter;
> -
> -	/* Enable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		card->plt_wake_cfg->wake_by_wifi = false;
> -		enable_irq(card->plt_wake_cfg->irq_wifi);
> -		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_enable_wake(adapter);
>
>   	/* Enable the Host Sleep */
>   	if (!mwifiex_enable_hs(adapter)) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 07cdd23..b9fbc5c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -154,12 +154,6 @@
>   	a->mpa_rx.start_port = 0;					\
>   } while (0)
>
> -struct mwifiex_plt_wake_cfg {
> -	struct device *dev;
> -	int irq_wifi;
> -	bool wake_by_wifi;
> -};
> -
>   /* data structure for SDIO MPA TX */
>   struct mwifiex_sdio_mpa_tx {
>   	/* multiport tx aggregation buffer pointer */
> @@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
>   struct sdio_mmc_card {
>   	struct sdio_func *func;
>   	struct mwifiex_adapter *adapter;
> -	struct device_node *plt_of_node;
> -	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
>
>   	const char *firmware;
>   	const struct mwifiex_sdio_card_reg *reg;
>
>

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
> Hi guys,
> 
> with this patch, the pci device's irq might be override by this
> wakeup irq when not using msi:

Hmm, good point. I believe I noticed this one at some point and then
didn't get to investigate further...

It kind of seems like we inadvertently conflicted with the PCI OF
interrupt spec [1]. There, the "interrupts" property for a device (if
present) is supposed to represent INT{A...D} with values of {1...4}.
IIUC, there should only be a single entry in this property.

If we were to extend this properly, I guess that would mean we'd need a
second "interrupts" entry, with a different parent. I think we can use
"interrupts-extended" for that.

So we'd need to document an optional "interrupt-names" for Marvell, and
have the driver try that first. The rough outline would be something
like this.

For the device tree (e.g., rk3399-gru):

-			interrupt-parent = <&gpio0>;
-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+			interrupt-names = "int-A", "wake";

Then mwifiex would need to check "byname" before trying "by index":

	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
	if (!adapter->irq_wakeup) {
		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
		if (!adapter->irq_wakeup) {
			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
			goto err_exit;
		}
	}

Or if we want to suggest the original binding was wrong and that we
should just ignore existing device trees that tried to use it, we can
skip the by-index fallback.

Brian

[1] Documentation/devicetree/bindings/pci/pci.txt points to
http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
except that link is also dead now. I found the same doc here:
https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
Might want to update the binding doc... I've sent a patch for that
separately.

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
> Hi guys,
> 
> with this patch, the pci device's irq might be override by this
> wakeup irq when not using msi:

Hmm, good point. I believe I noticed this one at some point and then
didn't get to investigate further...

It kind of seems like we inadvertently conflicted with the PCI OF
interrupt spec [1]. There, the "interrupts" property for a device (if
present) is supposed to represent INT{A...D} with values of {1...4}.
IIUC, there should only be a single entry in this property.

If we were to extend this properly, I guess that would mean we'd need a
second "interrupts" entry, with a different parent. I think we can use
"interrupts-extended" for that.

So we'd need to document an optional "interrupt-names" for Marvell, and
have the driver try that first. The rough outline would be something
like this.

For the device tree (e.g., rk3399-gru):

-			interrupt-parent = <&gpio0>;
-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+			interrupt-names = "int-A", "wake";

Then mwifiex would need to check "byname" before trying "by index":

	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
	if (!adapter->irq_wakeup) {
		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
		if (!adapter->irq_wakeup) {
			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
			goto err_exit;
		}
	}

Or if we want to suggest the original binding was wrong and that we
should just ignore existing device trees that tried to use it, we can
skip the by-index fallback.

Brian

[1] Documentation/devicetree/bindings/pci/pci.txt points to
http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
except that link is also dead now. I found the same doc here:
https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
Might want to update the binding doc... I've sent a patch for that
separately.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[jeffy]

Hi brian,

On 07/07/2017 08:53 AM, Brian Norris wrote:
> On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
>> Hi guys,
>>
>> with this patch, the pci device's irq might be override by this
>> wakeup irq when not using msi:
>
> Hmm, good point. I believe I noticed this one at some point and then
> didn't get to investigate further...
>
> It kind of seems like we inadvertently conflicted with the PCI OF
> interrupt spec [1]. There, the "interrupts" property for a device (if
> present) is supposed to represent INT{A...D} with values of {1...4}.
> IIUC, there should only be a single entry in this property.
>
> If we were to extend this properly, I guess that would mean we'd need a
> second "interrupts" entry, with a different parent. I think we can use
> "interrupts-extended" for that.
>
> So we'd need to document an optional "interrupt-names" for Marvell, and
> have the driver try that first. The rough outline would be something
> like this.
>
> For the device tree (e.g., rk3399-gru):
>
> -			interrupt-parent = <&gpio0>;
> -			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupt-names = "int-A", "wake";
This is a great idea.

And how about also add a property to tell of_pci_irq to ignore of irq 
and force using PCI_INTERRUPT_PIN? Since there might be devices don't 
use pci irq, but using other irq(wowlan for example).

Then we can specify this property and add a name("wake") to the wifi 
wake irq here. And interrupts-extended would still be an available option.

>
> Then mwifiex would need to check "byname" before trying "by index":
>
> 	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
> 	if (!adapter->irq_wakeup) {
> 		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> 		if (!adapter->irq_wakeup) {
> 			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
> 			goto err_exit;
> 		}
> 	}
>
> Or if we want to suggest the original binding was wrong and that we
> should just ignore existing device trees that tried to use it, we can
> skip the by-index fallback.
>
> Brian
>
> [1] Documentation/devicetree/bindings/pci/pci.txt points to
> http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
> except that link is also dead now. I found the same doc here:
> https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
> Might want to update the binding doc... I've sent a patch for that
> separately.
>
>
>

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[jeffy]

Hi brian,

On 07/07/2017 08:53 AM, Brian Norris wrote:
> On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
>> Hi guys,
>>
>> with this patch, the pci device's irq might be override by this
>> wakeup irq when not using msi:
>
> Hmm, good point. I believe I noticed this one at some point and then
> didn't get to investigate further...
>
> It kind of seems like we inadvertently conflicted with the PCI OF
> interrupt spec [1]. There, the "interrupts" property for a device (if
> present) is supposed to represent INT{A...D} with values of {1...4}.
> IIUC, there should only be a single entry in this property.
>
> If we were to extend this properly, I guess that would mean we'd need a
> second "interrupts" entry, with a different parent. I think we can use
> "interrupts-extended" for that.
>
> So we'd need to document an optional "interrupt-names" for Marvell, and
> have the driver try that first. The rough outline would be something
> like this.
>
> For the device tree (e.g., rk3399-gru):
>
> -			interrupt-parent = <&gpio0>;
> -			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupt-names = "int-A", "wake";
This is a great idea.

And how about also add a property to tell of_pci_irq to ignore of irq 
and force using PCI_INTERRUPT_PIN? Since there might be devices don't 
use pci irq, but using other irq(wowlan for example).

Then we can specify this property and add a name("wake") to the wifi 
wake irq here. And interrupts-extended would still be an available option.

>
> Then mwifiex would need to check "byname" before trying "by index":
>
> 	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
> 	if (!adapter->irq_wakeup) {
> 		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> 		if (!adapter->irq_wakeup) {
> 			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
> 			goto err_exit;
> 		}
> 	}
>
> Or if we want to suggest the original binding was wrong and that we
> should just ignore existing device trees that tried to use it, we can
> skip the by-index fallback.
>
> Brian
>
> [1] Documentation/devicetree/bindings/pci/pci.txt points to
> http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
> except that link is also dead now. I found the same doc here:
> https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
> Might want to update the binding doc... I've sent a patch for that
> separately.
>
>
>


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

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

+ linux-pci
- linux-wireless

On Fri, Jul 07, 2017 at 11:04:32AM +0800, Jeffy Chen wrote:
> On 07/07/2017 08:53 AM, Brian Norris wrote:
> >It kind of seems like we inadvertently conflicted with the PCI OF
> >interrupt spec [1]. There, the "interrupts" property for a device (if
> >present) is supposed to represent INT{A...D} with values of {1...4}.
> >IIUC, there should only be a single entry in this property.
> >
> >If we were to extend this properly, I guess that would mean we'd need a
> >second "interrupts" entry, with a different parent. I think we can use
> >"interrupts-extended" for that.
> >
> >So we'd need to document an optional "interrupt-names" for Marvell, and
> >have the driver try that first. The rough outline would be something
> >like this.
> >
> >For the device tree (e.g., rk3399-gru):
> >
> >-			interrupt-parent = <&gpio0>;
> >-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> >+			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> >+			interrupt-names = "int-A", "wake";
> This is a great idea.
> 
> And how about also add a property to tell of_pci_irq to ignore of
> irq and force using PCI_INTERRUPT_PIN? Since there might be devices
> don't use pci irq, but using other irq(wowlan for example).

Even if they don't use the PCI IRQ, is there a way to provide an empty /
un-mapped entry?

I'm a little wary on trying to "extend" (which can sometimes be read
"break") the device tree spec here too much more. But perhaps DT or PCI
folks have a better recommendation (if they can hold their noses long
enough).

> Then we can specify this property and add a name("wake") to the wifi
> wake irq here. And interrupts-extended would still be an available
> option.

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

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

+ linux-pci
- linux-wireless

On Fri, Jul 07, 2017 at 11:04:32AM +0800, Jeffy Chen wrote:
> On 07/07/2017 08:53 AM, Brian Norris wrote:
> >It kind of seems like we inadvertently conflicted with the PCI OF
> >interrupt spec [1]. There, the "interrupts" property for a device (if
> >present) is supposed to represent INT{A...D} with values of {1...4}.
> >IIUC, there should only be a single entry in this property.
> >
> >If we were to extend this properly, I guess that would mean we'd need a
> >second "interrupts" entry, with a different parent. I think we can use
> >"interrupts-extended" for that.
> >
> >So we'd need to document an optional "interrupt-names" for Marvell, and
> >have the driver try that first. The rough outline would be something
> >like this.
> >
> >For the device tree (e.g., rk3399-gru):
> >
> >-			interrupt-parent = <&gpio0>;
> >-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> >+			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> >+			interrupt-names = "int-A", "wake";
> This is a great idea.
> 
> And how about also add a property to tell of_pci_irq to ignore of
> irq and force using PCI_INTERRUPT_PIN? Since there might be devices
> don't use pci irq, but using other irq(wowlan for example).

Even if they don't use the PCI IRQ, is there a way to provide an empty /
un-mapped entry?

I'm a little wary on trying to "extend" (which can sometimes be read
"break") the device tree spec here too much more. But perhaps DT or PCI
folks have a better recommendation (if they can hold their noses long
enough).

> Then we can specify this property and add a name("wake") to the wifi
> wake irq here. And interrupts-extended would still be an available
> option.

Brian