[PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Marek Vasut]

Document compatible string for the WILC3000 chip. The chip is similar
to WILC1000, except that the register layout is slightly different and
it does not support WPA3/SAE.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: - Use WILC1000 as fallback compatible string for WILC3000
V3: - Swap the wilc1000/wilc3000 compatible order
V4: - Add RB from Krzysztof
---
 .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
index 2460ccc082371..5d40f22765bb6 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
@@ -16,7 +16,11 @@ description:
 
 properties:
   compatible:
-    const: microchip,wilc1000
+    oneOf:
+      - items:
+          - const: microchip,wilc3000
+          - const: microchip,wilc1000
+      - const: microchip,wilc1000
 
   reg: true
 
-- 
2.45.2

[PATCH v4 2/5] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Marek Vasut]

Do not use wilc_get_chipid() outside of wlan.c . Instead, call
wilc_get_chipid() right after the SDIO/SPI interface has been
initialized to cache the device chipid, and then use the cached
chipid throughout the driver. Make wilc_get_chipid() static and
remove its prototype from wlan.h . Make wilc_get_chipid() return
a proper return value instead of a chipid.

Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: New patch
V3: - Undo setup in wilc_wlan_init() if chip is neither wilc1000 or wilc3000
    - Make wilc_get_chipid() return proper return value
V4: - Drop wilc_get_chipid() from netdev.c
    - Add RB from Alexis
---
 .../net/wireless/microchip/wilc1000/netdev.c  |  6 +-
 .../net/wireless/microchip/wilc1000/sdio.c    | 13 ----
 .../net/wireless/microchip/wilc1000/wlan.c    | 74 +++++++++++--------
 .../net/wireless/microchip/wilc1000/wlan.h    |  1 -
 4 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 9ecf3fb29b558..086e70d833e06 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -195,13 +195,13 @@ static int wilc_wlan_get_firmware(struct net_device *dev)
 {
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
-	int chip_id;
 	const struct firmware *wilc_fw;
 	int ret;
 
-	chip_id = wilc_get_chipid(wilc, false);
+	if (!is_wilc1000(wilc->chipid))
+		return -EINVAL;
 
-	netdev_info(dev, "ChipID [%x] loading firmware [%s]\n", chip_id,
+	netdev_info(dev, "WILC1000 loading firmware [%s]\n",
 		    WILC1000_FW(WILC1000_API_VER));
 
 	ret = request_firmware(&wilc_fw, WILC1000_FW(WILC1000_API_VER),
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 41122199d51eb..90357c89ae29b 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -671,7 +671,6 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
 	struct wilc_sdio *sdio_priv = wilc->bus_data;
 	struct sdio_cmd52 cmd;
 	int loop, ret;
-	u32 chipid;
 
 	/**
 	 *      function 0 csa enable
@@ -760,18 +759,6 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
 		return ret;
 	}
 
-	/**
-	 *      make sure can read back chip id correctly
-	 **/
-	if (!resume) {
-		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
-		if (ret) {
-			dev_err(&func->dev, "Fail cmd read chip id...\n");
-			return ret;
-		}
-		dev_err(&func->dev, "chipid (%08x)\n", chipid);
-	}
-
 	sdio_priv->isinit = true;
 	return 0;
 }
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 533939e71534a..5b7dd37267de0 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1402,9 +1402,37 @@ int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 	return ret;
 }
 
+static int wilc_get_chipid(struct wilc *wilc)
+{
+	u32 chipid = 0;
+	u32 rfrevid = 0;
+
+	if (wilc->chipid == 0) {
+		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
+		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
+					     &rfrevid);
+		if (!is_wilc1000(chipid)) {
+			wilc->chipid = 0;
+			return -EINVAL;
+		}
+		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
+			if (rfrevid != 0x1)
+				chipid = WILC_1000_BASE_ID_2A_REV1;
+		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
+			if (rfrevid == 0x4)
+				chipid = WILC_1000_BASE_ID_2B_REV1;
+			else if (rfrevid != 0x3)
+				chipid = WILC_1000_BASE_ID_2B_REV2;
+		}
+
+		wilc->chipid = chipid;
+	}
+
+	return 0;
+}
+
 static int init_chip(struct net_device *dev)
 {
-	u32 chipid;
 	u32 reg;
 	int ret = 0;
 	struct wilc_vif *vif = netdev_priv(dev);
@@ -1412,9 +1440,11 @@ static int init_chip(struct net_device *dev)
 
 	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 
-	chipid = wilc_get_chipid(wilc, true);
+	ret = wilc_get_chipid(wilc);
+	if (ret)
+		goto release;
 
-	if ((chipid & 0xfff) != 0xa0) {
+	if ((wilc->chipid & 0xfff) != 0xa0) {
 		ret = wilc->hif_func->hif_read_reg(wilc,
 						   WILC_CORTUS_RESET_MUX_SEL,
 						   &reg);
@@ -1445,34 +1475,6 @@ static int init_chip(struct net_device *dev)
 	return ret;
 }
 
-u32 wilc_get_chipid(struct wilc *wilc, bool update)
-{
-	u32 chipid = 0;
-	u32 rfrevid = 0;
-
-	if (wilc->chipid == 0 || update) {
-		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
-		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
-					     &rfrevid);
-		if (!is_wilc1000(chipid)) {
-			wilc->chipid = 0;
-			return wilc->chipid;
-		}
-		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
-			if (rfrevid != 0x1)
-				chipid = WILC_1000_BASE_ID_2A_REV1;
-		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
-			if (rfrevid == 0x4)
-				chipid = WILC_1000_BASE_ID_2B_REV1;
-			else if (rfrevid != 0x3)
-				chipid = WILC_1000_BASE_ID_2B_REV2;
-		}
-
-		wilc->chipid = chipid;
-	}
-	return wilc->chipid;
-}
-
 int wilc_load_mac_from_nv(struct wilc *wl)
 {
 	int ret = -EINVAL;
@@ -1535,9 +1537,19 @@ int wilc_wlan_init(struct net_device *dev)
 	if (!wilc->hif_func->hif_is_init(wilc)) {
 		acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
 		ret = wilc->hif_func->hif_init(wilc, false);
+		if (!ret)
+			ret = wilc_get_chipid(wilc);
 		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
 		if (ret)
 			goto fail;
+
+		if (!is_wilc1000(wilc->chipid)) {
+			netdev_err(dev, "Unsupported chipid: %x\n", wilc->chipid);
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		netdev_dbg(dev, "chipid (%08x)\n", wilc->chipid);
 	}
 
 	if (!wilc->vmm_table)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index dd2fb3c2f06a2..ae187192a79c6 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -443,6 +443,5 @@ void chip_wakeup(struct wilc *wilc);
 int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 			 u32 count);
 int wilc_wlan_init(struct net_device *dev);
-u32 wilc_get_chipid(struct wilc *wilc, bool update);
 int wilc_load_mac_from_nv(struct wilc *wilc);
 #endif
-- 
2.45.2

[PATCH v4 3/5] wifi: wilc1000: Fold chip_allow_sleep()/chip_wakeup() into wlan.c

[Marek Vasut]

Neither chip_allow_sleep()/chip_wakeup() is used outside of wlan.c .
Make both functions static and remove both the exported symbol and
entries from wlan.h .

Make chip_allow_sleep() return error code in preparation for the
follow up patches.

Move acquire_bus() and release_bus() to avoid forward declaration
of chip_allow_sleep()/chip_wakeup().

Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: New patch
V3: No change
V4: Add RB from Alexis
---
 .../net/wireless/microchip/wilc1000/wlan.c    | 47 +++++++++----------
 .../net/wireless/microchip/wilc1000/wlan.h    |  2 -
 2 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 5b7dd37267de0..d228c5df82628 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -12,20 +12,6 @@
 
 #define WAKE_UP_TRIAL_RETRY		10000
 
-static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
-{
-	mutex_lock(&wilc->hif_cs);
-	if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode)
-		chip_wakeup(wilc);
-}
-
-static inline void release_bus(struct wilc *wilc, enum bus_release release)
-{
-	if (release == WILC_BUS_RELEASE_ALLOW_SLEEP && wilc->power_save_mode)
-		chip_allow_sleep(wilc);
-	mutex_unlock(&wilc->hif_cs);
-}
-
 static void wilc_wlan_txq_remove(struct wilc *wilc, u8 q_num,
 				 struct txq_entry_t *tqe)
 {
@@ -555,7 +541,7 @@ static struct rxq_entry_t *wilc_wlan_rxq_remove(struct wilc *wilc)
 	return rqe;
 }
 
-void chip_allow_sleep(struct wilc *wilc)
+static int chip_allow_sleep(struct wilc *wilc)
 {
 	u32 reg = 0;
 	const struct wilc_hif_func *hif_func = wilc->hif_func;
@@ -584,7 +570,7 @@ void chip_allow_sleep(struct wilc *wilc)
 	while (--trials) {
 		ret = hif_func->hif_read_reg(wilc, to_host_from_fw_reg, &reg);
 		if (ret)
-			return;
+			return ret;
 		if ((reg & to_host_from_fw_bit) == 0)
 			break;
 	}
@@ -594,28 +580,28 @@ void chip_allow_sleep(struct wilc *wilc)
 	/* Clear bit 1 */
 	ret = hif_func->hif_read_reg(wilc, wakeup_reg, &reg);
 	if (ret)
-		return;
+		return ret;
 	if (reg & wakeup_bit) {
 		reg &= ~wakeup_bit;
 		ret = hif_func->hif_write_reg(wilc, wakeup_reg, reg);
 		if (ret)
-			return;
+			return ret;
 	}
 
 	ret = hif_func->hif_read_reg(wilc, from_host_to_fw_reg, &reg);
 	if (ret)
-		return;
+		return ret;
 	if (reg & from_host_to_fw_bit) {
 		reg &= ~from_host_to_fw_bit;
 		ret = hif_func->hif_write_reg(wilc, from_host_to_fw_reg, reg);
 		if (ret)
-			return;
-
+			return ret;
 	}
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(chip_allow_sleep);
 
-void chip_wakeup(struct wilc *wilc)
+static void chip_wakeup(struct wilc *wilc)
 {
 	u32 ret = 0;
 	u32 clk_status_val = 0, trials = 0;
@@ -674,7 +660,20 @@ void chip_wakeup(struct wilc *wilc)
 	if (wilc->io_type == WILC_HIF_SPI)
 		wilc->hif_func->hif_reset(wilc);
 }
-EXPORT_SYMBOL_GPL(chip_wakeup);
+
+static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
+{
+	mutex_lock(&wilc->hif_cs);
+	if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode)
+		chip_wakeup(wilc);
+}
+
+static inline void release_bus(struct wilc *wilc, enum bus_release release)
+{
+	if (release == WILC_BUS_RELEASE_ALLOW_SLEEP && wilc->power_save_mode)
+		chip_allow_sleep(wilc);
+	mutex_unlock(&wilc->hif_cs);
+}
 
 void host_wakeup_notify(struct wilc *wilc)
 {
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index ae187192a79c6..e75a5c8aaecec 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -438,8 +438,6 @@ void wilc_wfi_p2p_rx(struct wilc_vif *vif, u8 *buff, u32 size);
 bool wilc_wfi_mgmt_frame_rx(struct wilc_vif *vif, u8 *buff, u32 size);
 void host_wakeup_notify(struct wilc *wilc);
 void host_sleep_notify(struct wilc *wilc);
-void chip_allow_sleep(struct wilc *wilc);
-void chip_wakeup(struct wilc *wilc);
 int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 			 u32 count);
 int wilc_wlan_init(struct net_device *dev);
-- 
2.45.2

[PATCH v4 4/5] wifi: wilc1000: Fill in missing error handling

[Marek Vasut]

Add error handling to chip_wakeup() and propagate the errors throughout
the entire driver. Add error handling to acquire_bus()/release_bus() and
host_sleep_notify()/host_wakeup_notify() functions as a result as well.
Fill the error handling to all call sites.

Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V3: New patch
V4: No change
V4: Add RB from Alexis
---
 .../net/wireless/microchip/wilc1000/sdio.c    |  11 +-
 .../net/wireless/microchip/wilc1000/wlan.c    | 154 +++++++++++++-----
 .../net/wireless/microchip/wilc1000/wlan.h    |   4 +-
 3 files changed, 118 insertions(+), 51 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 90357c89ae29b..c26447289b71b 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -960,6 +960,7 @@ static int wilc_sdio_suspend(struct device *dev)
 {
 	struct sdio_func *func = dev_to_sdio_func(dev);
 	struct wilc *wilc = sdio_get_drvdata(func);
+	int ret;
 
 	dev_info(dev, "sdio suspend\n");
 
@@ -969,7 +970,11 @@ static int wilc_sdio_suspend(struct device *dev)
 	if (!IS_ERR(wilc->rtc_clk))
 		clk_disable_unprepare(wilc->rtc_clk);
 
-	host_sleep_notify(wilc);
+	ret = host_sleep_notify(wilc);
+	if (ret) {
+		clk_prepare_enable(wilc->rtc_clk);
+		return ret;
+	}
 
 	wilc_sdio_disable_interrupt(wilc);
 
@@ -992,9 +997,7 @@ static int wilc_sdio_resume(struct device *dev)
 	wilc_sdio_init(wilc, true);
 	wilc_sdio_enable_interrupt(wilc);
 
-	host_wakeup_notify(wilc);
-
-	return 0;
+	return host_wakeup_notify(wilc);
 }
 
 static const struct of_device_id wilc_of_match[] = {
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index d228c5df82628..01476f8ecc36f 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -601,7 +601,7 @@ static int chip_allow_sleep(struct wilc *wilc)
 	return 0;
 }
 
-static void chip_wakeup(struct wilc *wilc)
+static int chip_wakeup(struct wilc *wilc)
 {
 	u32 ret = 0;
 	u32 clk_status_val = 0, trials = 0;
@@ -630,20 +630,20 @@ static void chip_wakeup(struct wilc *wilc)
 	ret = hif_func->hif_write_reg(wilc, from_host_to_fw_reg,
 				      from_host_to_fw_bit);
 	if (ret)
-		return;
+		return ret;
 
 	/* Set wake-up bit */
 	ret = hif_func->hif_write_reg(wilc, wakeup_reg,
 				      wakeup_bit);
 	if (ret)
-		return;
+		return ret;
 
 	while (trials < WAKE_UP_TRIAL_RETRY) {
 		ret = hif_func->hif_read_reg(wilc, clk_status_reg,
 					     &clk_status_val);
 		if (ret) {
 			pr_err("Bus error %d %x\n", ret, clk_status_val);
-			return;
+			return ret;
 		}
 		if (clk_status_val & clk_status_bit)
 			break;
@@ -652,42 +652,63 @@ static void chip_wakeup(struct wilc *wilc)
 	}
 	if (trials >= WAKE_UP_TRIAL_RETRY) {
 		pr_err("Failed to wake-up the chip\n");
-		return;
+		return -ETIMEDOUT;
 	}
 	/* Sometimes spi fail to read clock regs after reading
 	 * writing clockless registers
 	 */
 	if (wilc->io_type == WILC_HIF_SPI)
 		wilc->hif_func->hif_reset(wilc);
+
+	return 0;
 }
 
-static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
+static inline int acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
 {
+	int ret = 0;
+
 	mutex_lock(&wilc->hif_cs);
-	if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode)
-		chip_wakeup(wilc);
+	if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode) {
+		ret = chip_wakeup(wilc);
+		if (ret)
+			mutex_unlock(&wilc->hif_cs);
+	}
+
+	return ret;
 }
 
-static inline void release_bus(struct wilc *wilc, enum bus_release release)
+static inline int release_bus(struct wilc *wilc, enum bus_release release)
 {
+	int ret = 0;
+
 	if (release == WILC_BUS_RELEASE_ALLOW_SLEEP && wilc->power_save_mode)
-		chip_allow_sleep(wilc);
+		ret = chip_allow_sleep(wilc);
 	mutex_unlock(&wilc->hif_cs);
+
+	return ret;
 }
 
-void host_wakeup_notify(struct wilc *wilc)
+int host_wakeup_notify(struct wilc *wilc)
 {
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	int ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+
+	if (ret)
+		return ret;
+
 	wilc->hif_func->hif_write_reg(wilc, WILC_CORTUS_INTERRUPT_2, 1);
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	return release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_wakeup_notify);
 
-void host_sleep_notify(struct wilc *wilc)
+int host_sleep_notify(struct wilc *wilc)
 {
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	int ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+
+	if (ret)
+		return ret;
+
 	wilc->hif_func->hif_write_reg(wilc, WILC_CORTUS_INTERRUPT_1, 1);
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	return release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_sleep_notify);
 
@@ -714,6 +735,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 	int srcu_idx;
 	u8 *txb = wilc->tx_buffer;
 	struct wilc_vif *vif;
+	int rv;
 
 	if (wilc->quit)
 		goto out_update_cnt;
@@ -784,7 +806,10 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 		goto out_unlock;
 	vmm_table[i] = 0x0;
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		goto out_unlock;
+
 	counter = 0;
 	func = wilc->hif_func;
 	do {
@@ -859,7 +884,9 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 		goto out_release_bus;
 	}
 
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	ret = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	if (ret)
+		goto out_unlock;
 
 	offset = 0;
 	i = 0;
@@ -921,7 +948,9 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 	for (i = 0; i < NQUEUES; i++)
 		wilc->txq[i].fw.count += ac_pkt_num_to_chip[i];
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		goto out_unlock;
 
 	ret = func->hif_clear_int_ext(wilc, ENABLE_TX_VMM);
 	if (ret)
@@ -930,7 +959,9 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 	ret = func->hif_block_tx_ext(wilc, 0, txb, offset);
 
 out_release_bus:
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	rv = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	if (!ret && rv)
+		ret = rv;
 
 out_unlock:
 	mutex_unlock(&wilc->txq_add_to_head_cs);
@@ -1059,8 +1090,14 @@ static void wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status)
 void wilc_handle_isr(struct wilc *wilc)
 {
 	u32 int_status;
+	int ret;
+
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret) {
+		dev_err_ratelimited(wilc->dev, "Cannot acquire bus\n");
+		return;
+	}
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 	wilc->hif_func->hif_read_int(wilc, &int_status);
 
 	if (int_status & DATA_INT_EXT)
@@ -1069,7 +1106,9 @@ void wilc_handle_isr(struct wilc *wilc)
 	if (!(int_status & (ALL_INT_EXT)))
 		wilc_unknown_isr_ext(wilc);
 
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	ret = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	if (ret)
+		dev_err_ratelimited(wilc->dev, "Cannot release bus\n");
 }
 EXPORT_SYMBOL_GPL(wilc_handle_isr);
 
@@ -1081,6 +1120,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 	u8 *dma_buffer;
 	int ret = 0;
 	u32 reg = 0;
+	int rv;
 
 	blksz = BIT(12);
 
@@ -1091,7 +1131,9 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 	offset = 0;
 	pr_debug("%s: Downloading firmware size = %d\n", __func__, buffer_size);
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		return ret;
 
 	wilc->hif_func->hif_read_reg(wilc, WILC_GLB_RESET_0, &reg);
 	reg &= ~BIT(10);
@@ -1100,11 +1142,17 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 	if (reg & BIT(10))
 		pr_err("%s: Failed to reset\n", __func__);
 
-	release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+	ret = release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+	if (ret)
+		goto fail;
+
 	do {
 		addr = get_unaligned_le32(&buffer[offset]);
 		size = get_unaligned_le32(&buffer[offset + 4]);
-		acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+		ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+		if (ret)
+			goto fail;
+
 		offset += 8;
 		while (((int)size) && (offset < buffer_size)) {
 			if (size <= blksz)
@@ -1122,7 +1170,9 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 			offset += size2;
 			size -= size2;
 		}
-		release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+		rv = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+		if (!ret && rv)
+			ret = rv;
 
 		if (ret) {
 			pr_err("%s Bus error\n", __func__);
@@ -1141,7 +1191,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 int wilc_wlan_start(struct wilc *wilc)
 {
 	u32 reg = 0;
-	int ret;
+	int ret, rv;
 	u32 chipid;
 
 	if (wilc->io_type == WILC_HIF_SDIO) {
@@ -1150,7 +1200,10 @@ int wilc_wlan_start(struct wilc *wilc)
 	} else if (wilc->io_type == WILC_HIF_SPI) {
 		reg = 1;
 	}
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
+	if (ret)
+		return ret;
+
 	ret = wilc->hif_func->hif_write_reg(wilc, WILC_VMM_CORE_CFG, reg);
 	if (ret)
 		goto release;
@@ -1181,16 +1234,18 @@ int wilc_wlan_start(struct wilc *wilc)
 	wilc->hif_func->hif_read_reg(wilc, WILC_GLB_RESET_0, &reg);
 
 release:
-	release_bus(wilc, WILC_BUS_RELEASE_ONLY);
-	return ret;
+	rv = release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+	return ret ? ret : rv;
 }
 
 int wilc_wlan_stop(struct wilc *wilc, struct wilc_vif *vif)
 {
 	u32 reg = 0;
-	int ret;
+	int ret, rv;
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		return ret;
 
 	ret = wilc->hif_func->hif_read_reg(wilc, GLOBAL_MODE_CONTROL, &reg);
 	if (ret)
@@ -1226,9 +1281,9 @@ int wilc_wlan_stop(struct wilc *wilc, struct wilc_vif *vif)
 	ret = 0;
 release:
 	/* host comm is disabled - we can't issue sleep command anymore: */
-	release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+	rv = release_bus(wilc, WILC_BUS_RELEASE_ONLY);
 
-	return ret;
+	return ret ? ret : rv;
 }
 
 void wilc_wlan_cleanup(struct net_device *dev)
@@ -1433,11 +1488,13 @@ static int wilc_get_chipid(struct wilc *wilc)
 static int init_chip(struct net_device *dev)
 {
 	u32 reg;
-	int ret = 0;
+	int ret, rv;
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
 
-	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		return ret;
 
 	ret = wilc_get_chipid(wilc);
 	if (ret)
@@ -1469,17 +1526,19 @@ static int init_chip(struct net_device *dev)
 	}
 
 release:
-	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	rv = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 
-	return ret;
+	return ret ? ret : rv;
 }
 
 int wilc_load_mac_from_nv(struct wilc *wl)
 {
-	int ret = -EINVAL;
+	int ret, rv;
 	unsigned int i;
 
-	acquire_bus(wl, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	ret = acquire_bus(wl, WILC_BUS_ACQUIRE_AND_WAKEUP);
+	if (ret)
+		return ret;
 
 	for (i = 0; i < WILC_NVMEM_MAX_NUM_BANK; i++) {
 		int bank_offset = get_bank_offset_from_bank_index(i);
@@ -1518,14 +1577,14 @@ int wilc_load_mac_from_nv(struct wilc *wl)
 		break;
 	}
 
-	release_bus(wl, WILC_BUS_RELEASE_ALLOW_SLEEP);
-	return ret;
+	rv = release_bus(wl, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	return ret ? ret : rv;
 }
 EXPORT_SYMBOL_GPL(wilc_load_mac_from_nv);
 
 int wilc_wlan_init(struct net_device *dev)
 {
-	int ret = 0;
+	int ret = 0, rv;
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc;
 
@@ -1534,11 +1593,16 @@ int wilc_wlan_init(struct net_device *dev)
 	wilc->quit = 0;
 
 	if (!wilc->hif_func->hif_is_init(wilc)) {
-		acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
+		ret = acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
+		if (ret)
+			return ret;
+
 		ret = wilc->hif_func->hif_init(wilc, false);
 		if (!ret)
 			ret = wilc_get_chipid(wilc);
-		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+		rv = release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+		if (!ret && rv)
+			ret = rv;
 		if (ret)
 			goto fail;
 
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index e75a5c8aaecec..4e2b0c4ac1e21 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -436,8 +436,8 @@ netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *dev);
 
 void wilc_wfi_p2p_rx(struct wilc_vif *vif, u8 *buff, u32 size);
 bool wilc_wfi_mgmt_frame_rx(struct wilc_vif *vif, u8 *buff, u32 size);
-void host_wakeup_notify(struct wilc *wilc);
-void host_sleep_notify(struct wilc *wilc);
+int host_wakeup_notify(struct wilc *wilc);
+int host_sleep_notify(struct wilc *wilc);
 int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 			 u32 count);
 int wilc_wlan_init(struct net_device *dev);
-- 
2.45.2

[PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

From: Ajay Singh <ajay.kathat@microchip.com>

Add support for the WILC3000 chip. The chip is similar to WILC1000,
except that the register layout is slightly different and it does
not support WPA3/SAE.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Note: Squashed and updated from the following downstream patches:
wifi: wilc1000: wilc3000 support added
wifi: wilc1000: wilc3000 interrupt handling
wifi: wilc1000: wilc3000 added chip wake and sleep support
wifi: wilc1000: wilc3000 FW file sepecific changes
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: - Return -EINVAL in wilc_sdio_init() if chip ID is not supported
    - Dispose of wilc_chip_type, replace with is_wilc1000()/is_wilc3000()
    - Remove wilc3000 DT compatible string handling, match on wilc1000 only,
      the device type can be auto-detected based on chipID
V3: - Define and use WILC3000_BOOTROM_STATUS and WILC3000_CORTUS_BOOT_REGISTER_2
V4: - Fix up after dropping wilc_get_chipid() in netdev.c in 2/5
---
 .../wireless/microchip/wilc1000/cfg80211.c    |   7 +
 .../net/wireless/microchip/wilc1000/netdev.c  |  27 ++-
 .../net/wireless/microchip/wilc1000/sdio.c    |  62 ++++-
 drivers/net/wireless/microchip/wilc1000/spi.c |   2 +-
 .../net/wireless/microchip/wilc1000/wlan.c    | 220 +++++++++++++++---
 .../net/wireless/microchip/wilc1000/wlan.h    |  45 +++-
 6 files changed, 302 insertions(+), 61 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index eb37b228d54ea..69c66a7b41654 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 
 	vif->connecting = true;
 
+	if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
+	    is_wilc3000(vif->wilc->chipid)) {
+		netdev_err(dev, "WILC3000: WPA3 not supported\n");
+		ret = -EOPNOTSUPP;
+		goto out_error;
+	}
+
 	cipher_group = sme->crypto.cipher_group;
 	if (cipher_group != 0) {
 		if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_2) {
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 086e70d833e06..178f3241cef9f 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -23,6 +23,12 @@
 #define __WILC1000_FW(api)		WILC1000_FW_PREFIX #api ".bin"
 #define WILC1000_FW(api)		__WILC1000_FW(api)
 
+#define WILC3000_API_VER		1
+
+#define WILC3000_FW_PREFIX		"atmel/wilc3000_wifi_firmware-"
+#define __WILC3000_FW(api)		WILC3000_FW_PREFIX #api ".bin"
+#define WILC3000_FW(api)		__WILC3000_FW(api)
+
 static irqreturn_t isr_uh_routine(int irq, void *user_data)
 {
 	struct wilc *wilc = user_data;
@@ -196,19 +202,23 @@ static int wilc_wlan_get_firmware(struct net_device *dev)
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
 	const struct firmware *wilc_fw;
+	char *firmware;
 	int ret;
 
-	if (!is_wilc1000(wilc->chipid))
+	if (is_wilc1000(wilc->chipid))
+		firmware = WILC1000_FW(WILC1000_API_VER);
+	else if (is_wilc3000(wilc->chipid))
+		firmware = WILC3000_FW(WILC3000_API_VER);
+	else
 		return -EINVAL;
 
-	netdev_info(dev, "WILC1000 loading firmware [%s]\n",
+	netdev_info(dev, "WILC%d loading firmware [%s]\n",
+		    is_wilc1000(wilc->chipid) ? 1000 : 3000,
 		    WILC1000_FW(WILC1000_API_VER));
 
-	ret = request_firmware(&wilc_fw, WILC1000_FW(WILC1000_API_VER),
-			       wilc->dev);
+	ret = request_firmware(&wilc_fw, firmware, wilc->dev);
 	if (ret != 0) {
-		netdev_err(dev, "%s - firmware not available\n",
-			   WILC1000_FW(WILC1000_API_VER));
+		netdev_err(dev, "%s - firmware not available\n", firmware);
 		return -EINVAL;
 	}
 	wilc->firmware = wilc_fw;
@@ -233,7 +243,7 @@ static int wilc_start_firmware(struct net_device *dev)
 	return 0;
 }
 
-static int wilc1000_firmware_download(struct net_device *dev)
+static int wilc_firmware_download(struct net_device *dev)
 {
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
@@ -528,7 +538,7 @@ static int wilc_wlan_initialize(struct net_device *dev, struct wilc_vif *vif)
 		if (ret)
 			goto fail_irq_enable;
 
-		ret = wilc1000_firmware_download(dev);
+		ret = wilc_firmware_download(dev);
 		if (ret)
 			goto fail_irq_enable;
 
@@ -1014,3 +1024,4 @@ EXPORT_SYMBOL_GPL(wilc_netdev_ifc_init);
 MODULE_DESCRIPTION("Atmel WILC1000 core wireless driver");
 MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(WILC1000_FW(WILC1000_API_VER));
+MODULE_FIRMWARE(WILC3000_FW(WILC3000_API_VER));
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index c26447289b71b..983debb3c626b 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -806,13 +806,19 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status)
 		cmd.address = WILC_SDIO_EXT_IRQ_FLAG_REG;
 	} else {
 		cmd.function = 0;
-		cmd.address = WILC_SDIO_IRQ_FLAG_REG;
+		cmd.address = is_wilc1000(wilc->chipid) ?
+			      WILC1000_SDIO_IRQ_FLAG_REG :
+			      WILC3000_SDIO_IRQ_FLAG_REG;
 	}
 	cmd.raw = 0;
 	cmd.read_write = 0;
 	cmd.data = 0;
 	wilc_sdio_cmd52(wilc, &cmd);
 	irq_flags = cmd.data;
+
+	if (sdio_priv->irq_gpio)
+		irq_flags &= is_wilc1000(wilc->chipid) ? 0x1f : 0x0f;
+
 	tmp |= FIELD_PREP(IRG_FLAGS_MASK, cmd.data);
 
 	if (FIELD_GET(UNHANDLED_IRQ_MASK, irq_flags))
@@ -834,22 +840,56 @@ static int wilc_sdio_clear_int_ext(struct wilc *wilc, u32 val)
 	if (sdio_priv->irq_gpio)
 		reg = val & (BIT(MAX_NUM_INT) - 1);
 
-	/* select VMM table 0 */
-	if (val & SEL_VMM_TBL0)
-		reg |= BIT(5);
-	/* select VMM table 1 */
-	if (val & SEL_VMM_TBL1)
-		reg |= BIT(6);
-	/* enable VMM */
-	if (val & EN_VMM)
-		reg |= BIT(7);
+	if (is_wilc1000(wilc->chipid)) {
+		/* select VMM table 0 */
+		if (val & SEL_VMM_TBL0)
+			reg |= BIT(5);
+		/* select VMM table 1 */
+		if (val & SEL_VMM_TBL1)
+			reg |= BIT(6);
+		/* enable VMM */
+		if (val & EN_VMM)
+			reg |= BIT(7);
+	} else {
+		if (sdio_priv->irq_gpio && reg) {
+			struct sdio_cmd52 cmd;
+
+			cmd.read_write = 1;
+			cmd.function = 0;
+			cmd.raw = 0;
+			cmd.address = WILC3000_SDIO_IRQ_FLAG_REG;
+			cmd.data = reg;
+
+			ret = wilc_sdio_cmd52(wilc, &cmd);
+			if (ret) {
+				dev_err(&func->dev,
+					"Failed cmd52, set 0xfe data (%d) ...\n",
+					__LINE__);
+				return ret;
+			}
+		}
+
+		reg = 0;
+		/* select VMM table 0 */
+		if (val & SEL_VMM_TBL0)
+			reg |= BIT(0);
+		/* select VMM table 1 */
+		if (val & SEL_VMM_TBL1)
+			reg |= BIT(1);
+		/* enable VMM */
+		if (val & EN_VMM)
+			reg |= BIT(2);
+	}
+
 	if (reg) {
 		struct sdio_cmd52 cmd;
 
 		cmd.read_write = 1;
 		cmd.function = 0;
 		cmd.raw = 0;
-		cmd.address = WILC_SDIO_IRQ_CLEAR_FLAG_REG;
+		cmd.address = is_wilc1000(wilc->chipid) ?
+			      WILC1000_SDIO_IRQ_CLEAR_FLAG_REG :
+			      WILC3000_SDIO_VMM_TBL_CTRL_REG;
 		cmd.data = reg;
 
 		ret = wilc_sdio_cmd52(wilc, &cmd);
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 5ff940c53ad9c..8913703e10e26 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -1232,7 +1232,7 @@ static int wilc_validate_chipid(struct wilc *wilc)
 		dev_err(&spi->dev, "Fail cmd read chip id...\n");
 		return ret;
 	}
-	if (!is_wilc1000(chipid)) {
+	if (!is_wilc1000(chipid) && !is_wilc3000(chipid)) {
 		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
 		return -ENODEV;
 	}
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 01476f8ecc36f..721fc9c02de5c 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -541,7 +541,7 @@ static struct rxq_entry_t *wilc_wlan_rxq_remove(struct wilc *wilc)
 	return rqe;
 }
 
-static int chip_allow_sleep(struct wilc *wilc)
+static int chip_allow_sleep_wilc1000(struct wilc *wilc)
 {
 	u32 reg = 0;
 	const struct wilc_hif_func *hif_func = wilc->hif_func;
@@ -601,7 +601,41 @@ static int chip_allow_sleep(struct wilc *wilc)
 	return 0;
 }
 
-static int chip_wakeup(struct wilc *wilc)
+static int chip_allow_sleep_wilc3000(struct wilc *wilc)
+{
+	u32 reg = 0;
+	int ret;
+	const struct wilc_hif_func *hif_func = wilc->hif_func;
+
+	if (wilc->io_type == WILC_HIF_SDIO) {
+		ret = hif_func->hif_read_reg(wilc, WILC_SDIO_WAKEUP_REG, &reg);
+		if (ret)
+			return ret;
+		ret = hif_func->hif_write_reg(wilc, WILC_SDIO_WAKEUP_REG,
+					      reg & ~WILC_SDIO_WAKEUP_BIT);
+		if (ret)
+			return ret;
+	} else {
+		ret = hif_func->hif_read_reg(wilc, WILC_SPI_WAKEUP_REG, &reg);
+		if (ret)
+			return ret;
+		ret = hif_func->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG,
+					      reg & ~WILC_SPI_WAKEUP_BIT);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int chip_allow_sleep(struct wilc *wilc)
+{
+	if (is_wilc1000(wilc->chipid))
+		return chip_allow_sleep_wilc1000(wilc);
+	else
+		return chip_allow_sleep_wilc3000(wilc);
+}
+
+static int chip_wakeup_wilc1000(struct wilc *wilc)
 {
 	u32 ret = 0;
 	u32 clk_status_val = 0, trials = 0;
@@ -613,15 +647,15 @@ static int chip_wakeup(struct wilc *wilc)
 	if (wilc->io_type == WILC_HIF_SDIO) {
 		wakeup_reg = WILC_SDIO_WAKEUP_REG;
 		wakeup_bit = WILC_SDIO_WAKEUP_BIT;
-		clk_status_reg = WILC_SDIO_CLK_STATUS_REG;
-		clk_status_bit = WILC_SDIO_CLK_STATUS_BIT;
+		clk_status_reg = WILC1000_SDIO_CLK_STATUS_REG;
+		clk_status_bit = WILC1000_SDIO_CLK_STATUS_BIT;
 		from_host_to_fw_reg = WILC_SDIO_HOST_TO_FW_REG;
 		from_host_to_fw_bit = WILC_SDIO_HOST_TO_FW_BIT;
 	} else {
 		wakeup_reg = WILC_SPI_WAKEUP_REG;
 		wakeup_bit = WILC_SPI_WAKEUP_BIT;
-		clk_status_reg = WILC_SPI_CLK_STATUS_REG;
-		clk_status_bit = WILC_SPI_CLK_STATUS_BIT;
+		clk_status_reg = WILC1000_SPI_CLK_STATUS_REG;
+		clk_status_bit = WILC1000_SPI_CLK_STATUS_BIT;
 		from_host_to_fw_reg = WILC_SPI_HOST_TO_FW_REG;
 		from_host_to_fw_bit = WILC_SPI_HOST_TO_FW_BIT;
 	}
@@ -663,6 +697,74 @@ static int chip_wakeup(struct wilc *wilc)
 	return 0;
 }
 
+static int chip_wakeup_wilc3000(struct wilc *wilc)
+{
+	u32 wakeup_reg_val, clk_status_reg_val, trials = 0;
+	u32 wakeup_reg, wakeup_bit;
+	u32 clk_status_reg, clk_status_bit;
+	int wake_seq_trials = 5;
+	const struct wilc_hif_func *hif_func = wilc->hif_func;
+
+	if (wilc->io_type == WILC_HIF_SDIO) {
+		wakeup_reg = WILC_SDIO_WAKEUP_REG;
+		wakeup_bit = WILC_SDIO_WAKEUP_BIT;
+		clk_status_reg = WILC3000_SDIO_CLK_STATUS_REG;
+		clk_status_bit = WILC3000_SDIO_CLK_STATUS_BIT;
+	} else {
+		wakeup_reg = WILC_SPI_WAKEUP_REG;
+		wakeup_bit = WILC_SPI_WAKEUP_BIT;
+		clk_status_reg = WILC3000_SPI_CLK_STATUS_REG;
+		clk_status_bit = WILC3000_SPI_CLK_STATUS_BIT;
+	}
+
+	hif_func->hif_read_reg(wilc, wakeup_reg, &wakeup_reg_val);
+	do {
+		hif_func->hif_write_reg(wilc, wakeup_reg, wakeup_reg_val |
+							  wakeup_bit);
+		/* Check the clock status */
+		hif_func->hif_read_reg(wilc, clk_status_reg,
+				       &clk_status_reg_val);
+
+		/* In case of clocks off, wait 1ms, and check it again.
+		 * if still off, wait for another 1ms, for a total wait of 3ms.
+		 * If still off, redo the wake up sequence
+		 */
+		while ((clk_status_reg_val & clk_status_bit) == 0 &&
+		       (++trials % 4) != 0) {
+			/* Wait for the chip to stabilize*/
+			usleep_range(1000, 1100);
+
+			/* Make sure chip is awake. This is an extra step that
+			 * can be removed later to avoid the bus access
+			 * overhead
+			 */
+			hif_func->hif_read_reg(wilc, clk_status_reg,
+					       &clk_status_reg_val);
+		}
+		/* in case of failure, Reset the wakeup bit to introduce a new
+		 * edge on the next loop
+		 */
+		if ((clk_status_reg_val & clk_status_bit) == 0) {
+			hif_func->hif_write_reg(wilc, wakeup_reg,
+						wakeup_reg_val & (~wakeup_bit));
+			/* added wait before wakeup sequence retry */
+			usleep_range(200, 300);
+		}
+	} while ((clk_status_reg_val & clk_status_bit) == 0 && wake_seq_trials-- > 0);
+	if (!wake_seq_trials)
+		dev_err(wilc->dev, "clocks still OFF. Wake up failed\n");
+
+	return 0;
+}
+
+static int chip_wakeup(struct wilc *wilc)
+{
+	if (is_wilc1000(wilc->chipid))
+		return chip_wakeup_wilc1000(wilc);
+	else
+		return chip_wakeup_wilc3000(wilc);
+}
+
 static inline int acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
 {
 	int ret = 0;
@@ -695,7 +797,9 @@ int host_wakeup_notify(struct wilc *wilc)
 	if (ret)
 		return ret;
 
-	wilc->hif_func->hif_write_reg(wilc, WILC_CORTUS_INTERRUPT_2, 1);
+	wilc->hif_func->hif_write_reg(wilc, is_wilc1000(wilc->chipid) ?
+					    WILC1000_CORTUS_INTERRUPT_2 :
+					    WILC3000_CORTUS_INTERRUPT_2, 1);
 	return release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_wakeup_notify);
@@ -707,7 +811,9 @@ int host_sleep_notify(struct wilc *wilc)
 	if (ret)
 		return ret;
 
-	wilc->hif_func->hif_write_reg(wilc, WILC_CORTUS_INTERRUPT_1, 1);
+	wilc->hif_func->hif_write_reg(wilc, is_wilc1000(wilc->chipid) ?
+					    WILC1000_CORTUS_INTERRUPT_1 :
+					    WILC3000_CORTUS_INTERRUPT_1, 1);
 	return release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_sleep_notify);
@@ -842,19 +948,45 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 		if (ret)
 			break;
 
-		ret = func->hif_write_reg(wilc, WILC_HOST_VMM_CTL, 0x2);
-		if (ret)
-			break;
+		if (is_wilc1000(wilc->chipid)) {
+			ret = func->hif_write_reg(wilc, WILC_HOST_VMM_CTL, 0x2);
+			if (ret)
+				break;
 
-		do {
-			ret = func->hif_read_reg(wilc, WILC_HOST_VMM_CTL, &reg);
+			do {
+				ret = func->hif_read_reg(wilc, WILC_HOST_VMM_CTL, &reg);
+				if (ret)
+					break;
+				if (FIELD_GET(WILC_VMM_ENTRY_AVAILABLE, reg)) {
+					entries = FIELD_GET(WILC_VMM_ENTRY_COUNT, reg);
+					break;
+				}
+			} while (--timeout);
+		} else {
+			ret = func->hif_write_reg(wilc, WILC_HOST_VMM_CTL, 0);
 			if (ret)
 				break;
-			if (FIELD_GET(WILC_VMM_ENTRY_AVAILABLE, reg)) {
-				entries = FIELD_GET(WILC_VMM_ENTRY_COUNT, reg);
+
+			/* interrupt firmware */
+			ret = func->hif_write_reg(wilc, WILC_CORTUS_INTERRUPT_BASE, 1);
+			if (ret)
 				break;
-			}
-		} while (--timeout);
+
+			do {
+				ret = func->hif_read_reg(wilc, WILC_CORTUS_INTERRUPT_BASE, &reg);
+				if (ret)
+					break;
+				if (reg == 0) {
+					/* Get the entries */
+					ret = func->hif_read_reg(wilc, WILC_HOST_VMM_CTL, &reg);
+					if (ret)
+						break;
+
+					entries = FIELD_GET(WILC_VMM_ENTRY_COUNT, reg);
+					break;
+				}
+			} while (--timeout);
+		}
 		if (timeout <= 0) {
 			ret = func->hif_write_reg(wilc, WILC_HOST_VMM_CTL, 0x0);
 			break;
@@ -1212,6 +1344,9 @@ int wilc_wlan_start(struct wilc *wilc)
 	if (wilc->io_type == WILC_HIF_SDIO && wilc->dev_irq_num)
 		reg |= WILC_HAVE_SDIO_IRQ_GPIO;
 
+	if (is_wilc3000(wilc->chipid))
+		reg |= WILC_HAVE_SLEEP_CLK_SRC_RTC;
+
 	ret = wilc->hif_func->hif_write_reg(wilc, WILC_GP_REG_1, reg);
 	if (ret)
 		goto release;
@@ -1462,21 +1597,25 @@ static int wilc_get_chipid(struct wilc *wilc)
 	u32 rfrevid = 0;
 
 	if (wilc->chipid == 0) {
-		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
-		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
-					     &rfrevid);
-		if (!is_wilc1000(chipid)) {
-			wilc->chipid = 0;
-			return -EINVAL;
-		}
-		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
-			if (rfrevid != 0x1)
-				chipid = WILC_1000_BASE_ID_2A_REV1;
-		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
-			if (rfrevid == 0x4)
-				chipid = WILC_1000_BASE_ID_2B_REV1;
-			else if (rfrevid != 0x3)
-				chipid = WILC_1000_BASE_ID_2B_REV2;
+		wilc->hif_func->hif_read_reg(wilc, WILC3000_CHIP_ID, &chipid);
+		if (!is_wilc3000(chipid)) {
+			wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
+			wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
+						     &rfrevid);
+
+			if (!is_wilc1000(chipid)) {
+				wilc->chipid = 0;
+				return -EINVAL;
+			}
+			if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
+				if (rfrevid != 0x1)
+					chipid = WILC_1000_BASE_ID_2A_REV1;
+			} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
+				if (rfrevid == 0x4)
+					chipid = WILC_1000_BASE_ID_2B_REV1;
+				else if (rfrevid != 0x3)
+					chipid = WILC_1000_BASE_ID_2B_REV2;
+			}
 		}
 
 		wilc->chipid = chipid;
@@ -1525,6 +1664,21 @@ static int init_chip(struct net_device *dev)
 		}
 	}
 
+	if (is_wilc3000(wilc->chipid)) {
+		ret = wilc->hif_func->hif_read_reg(wilc, WILC3000_BOOTROM_STATUS, &reg);
+		if (ret) {
+			netdev_err(dev, "failed to read WILC3000 BootROM status register\n");
+			goto release;
+		}
+
+		ret = wilc->hif_func->hif_write_reg(wilc, WILC3000_CORTUS_BOOT_REGISTER_2,
+						    WILC_CORTUS_BOOT_FROM_IRAM);
+		if (ret) {
+			netdev_err(dev, "failed to write WILC3000 Boot register\n");
+			goto release;
+		}
+	}
+
 release:
 	rv = release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 
@@ -1606,7 +1760,7 @@ int wilc_wlan_init(struct net_device *dev)
 		if (ret)
 			goto fail;
 
-		if (!is_wilc1000(wilc->chipid)) {
+		if (!is_wilc1000(wilc->chipid) && !is_wilc3000(wilc->chipid)) {
 			netdev_err(dev, "Unsupported chipid: %x\n", wilc->chipid);
 			ret = -EINVAL;
 			goto fail;
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index 4e2b0c4ac1e21..8fcae9d14550c 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -96,8 +96,14 @@
 #define WILC_SPI_WAKEUP_REG		0x1
 #define WILC_SPI_WAKEUP_BIT		BIT(1)
 
-#define WILC_SPI_CLK_STATUS_REG        0x0f
-#define WILC_SPI_CLK_STATUS_BIT        BIT(2)
+/* WILC1000 specific */
+#define WILC1000_SPI_CLK_STATUS_REG	0x0f
+#define WILC1000_SPI_CLK_STATUS_BIT	BIT(2)
+
+/* WILC3000 specific */
+#define WILC3000_SPI_CLK_STATUS_REG	0x13
+#define WILC3000_SPI_CLK_STATUS_BIT	BIT(2)
+
 #define WILC_SPI_HOST_TO_FW_REG		0x0b
 #define WILC_SPI_HOST_TO_FW_BIT		BIT(0)
 
@@ -123,14 +129,24 @@
 #define WILC_SDIO_WAKEUP_REG		0xf0
 #define WILC_SDIO_WAKEUP_BIT		BIT(0)
 
-#define WILC_SDIO_CLK_STATUS_REG	0xf1
-#define WILC_SDIO_CLK_STATUS_BIT	BIT(0)
+/* WILC1000 */
+#define WILC1000_SDIO_CLK_STATUS_REG	0xf1
+#define WILC1000_SDIO_CLK_STATUS_BIT	BIT(0)
+
+#define WILC1000_SDIO_IRQ_FLAG_REG	0xf7
+#define WILC1000_SDIO_IRQ_CLEAR_FLAG_REG	0xf8
+
+/* WILC3000 specific */
+#define WILC3000_SDIO_CLK_STATUS_REG	0xf0 /* clk & wakeup are on same reg */
+#define WILC3000_SDIO_CLK_STATUS_BIT	BIT(4)
+
+#define WILC3000_SDIO_VMM_TBL_CTRL_REG	0xf1
+#define WILC3000_SDIO_IRQ_FLAG_REG	0xfe
 
+/* Common vendor specific CCCR register */
 #define WILC_SDIO_INTERRUPT_DATA_SZ_REG	0xf2 /* Read size (2 bytes) */
 
 #define WILC_SDIO_VMM_TBL_CTRL_REG	0xf6
-#define WILC_SDIO_IRQ_FLAG_REG		0xf7
-#define WILC_SDIO_IRQ_CLEAR_FLAG_REG	0xf8
 
 #define WILC_SDIO_HOST_TO_FW_REG	0xfa
 #define WILC_SDIO_HOST_TO_FW_BIT	BIT(0)
@@ -172,8 +188,11 @@
 #define WILC_HAVE_USE_IRQ_AS_HOST_WAKE	BIT(8)
 
 #define WILC_CORTUS_INTERRUPT_BASE	0x10A8
-#define WILC_CORTUS_INTERRUPT_1		(WILC_CORTUS_INTERRUPT_BASE + 0x4)
-#define WILC_CORTUS_INTERRUPT_2		(WILC_CORTUS_INTERRUPT_BASE + 0x8)
+#define WILC1000_CORTUS_INTERRUPT_1	(WILC_CORTUS_INTERRUPT_BASE + 0x4)
+#define WILC3000_CORTUS_INTERRUPT_1	(WILC_CORTUS_INTERRUPT_BASE + 0x14)
+
+#define WILC1000_CORTUS_INTERRUPT_2	(WILC_CORTUS_INTERRUPT_BASE + 0x8)
+#define WILC3000_CORTUS_INTERRUPT_2	(WILC_CORTUS_INTERRUPT_BASE + 0x18)
 
 /* tx control register 1 to 4 for RX */
 #define WILC_REG_4_TO_1_RX		0x1e1c
@@ -183,6 +202,9 @@
 
 #define WILC_CORTUS_RESET_MUX_SEL	0x1118
 #define WILC_CORTUS_BOOT_REGISTER	0xc0000
+#define WILC3000_BOOTROM_STATUS		0x207ac
+#define WILC3000_CORTUS_BOOT_REGISTER_2	0x4f0000
+#define WILC3000_CHIP_ID		0x3b0000
 
 #define WILC_CORTUS_BOOT_FROM_IRAM	0x71
 
@@ -195,6 +217,8 @@
 #define WILC_1000_BASE_ID_2B_REV1	(WILC_1000_BASE_ID_2B + 1)
 #define WILC_1000_BASE_ID_2B_REV2	(WILC_1000_BASE_ID_2B + 2)
 
+#define WILC_3000_BASE_ID		0x300000
+
 #define WILC_CHIP_REV_FIELD		GENMASK(11, 0)
 
 /********************************************
@@ -413,6 +437,11 @@ static inline bool is_wilc1000(u32 id)
 	return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID;
 }
 
+static inline bool is_wilc3000(u32 id)
+{
+	return (id & (~WILC_CHIP_REV_FIELD)) == WILC_3000_BASE_ID;
+}
+
 int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
 				u32 buffer_size);
 int wilc_wlan_start(struct wilc *wilc);
-- 
2.45.2

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Alexis Lothoré]

Hello everyone,

On 8/29/24 02:44, Marek Vasut wrote:
> Document compatible string for the WILC3000 chip. The chip is similar
> to WILC1000, except that the register layout is slightly different and
> it does not support WPA3/SAE.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

>  .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
> index 2460ccc082371..5d40f22765bb6 100644
> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
> @@ -16,7 +16,11 @@ description:
>  
>  properties:
>    compatible:
> -    const: microchip,wilc1000
> +    oneOf:
> +      - items:
> +          - const: microchip,wilc3000
> +          - const: microchip,wilc1000
> +      - const: microchip,wilc1000
>  
>    reg: true

Following this series first revision, I have been taking a look at how to
implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through
a separated UART, see [1]), and I am facing some constraints. I feel like the
possible solutions would conflict with this new binding, so even if I am a bit
late to the party, I would like to expose the issue before the binding is merged
in case we can find something which would allow to add bluetooth support without
too much pain after the wlan part.

Downstream driver currently does not implement bluetooth as a standard bluetooth
driver (module in drivers/bluetooth, registering a HCI device) but only performs
a minimal set of operations directly in the wlan part ([2]). Getting a version
valid for upstream would need the following points to be addressed:
1. despite being controlled from a serial port for nominal operations, the
bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization
2. yet init steps are not performed on any kind of subsystem ops but through
writes to a custom chardev
3. the driver does not register itself a hci interface, it is expected to be
done by userspace (hciattach).

It is only after those 3 steps that the chip can be used with standard hci
commands over serial port. IMHO 1 is the biggest point, because it means that
**a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
(so only describing the bluetooth part of the chip as a child node of an uart
controller is not enough). Aside from bus access, I also expect some
interactions between bluetooth and wifi (eg: power management, sleep/wakeup)

After considering multiple solutions to try to share this bus between existing
wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some
handles, etc), my current best guess is to convert wilc driver to a MFD driver
for wilc3000. I guess some work can be done so that the driver can still be
shared between wilc1000 and wilc3000 _while_ remaining compatible with current
wilc1000 description, but it would impact the DT description for wilc3000, which
would need to switch from this:

  spi {
    wifi@0 {
      compatible = "microchip,wilc3000";
      [...]
    };
  };

To something like this:

  spi {
    wilc@0 {
      compatible = "microchip,wilc3000"; /* mfd driver */
      wifi {
        compatible = "microchip,wilc3000-wlan";
        [...]
      };
      bt {
        compatible = "microchip,wilc3000-bt";
        XXXX; /* some link to the uart controller connected to the chip */
        [...]
      };
    };
  };

(and similar thing when wilc is driven over a sdio bus)

Any opinion on this ? Would it make sense to describe wilc3000 chip that way ?

Thanks,
Alexis

[1] https://www.microchip.com/en-us/product/atwilc3000
[2]
https://github.com/linux4sam/linux-at91/blob/linux-6.6-mchp/drivers/net/wireless/microchip/wilc1000/bt.c

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Krzysztof Kozlowski]

On 03/09/2024 18:09, Alexis Lothoré wrote:
> Hello everyone,
> 
> On 8/29/24 02:44, Marek Vasut wrote:
>> Document compatible string for the WILC3000 chip. The chip is similar
>> to WILC1000, except that the register layout is slightly different and
>> it does not support WPA3/SAE.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>>  .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> index 2460ccc082371..5d40f22765bb6 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> @@ -16,7 +16,11 @@ description:
>>  
>>  properties:
>>    compatible:
>> -    const: microchip,wilc1000
>> +    oneOf:
>> +      - items:
>> +          - const: microchip,wilc3000
>> +          - const: microchip,wilc1000
>> +      - const: microchip,wilc1000
>>  
>>    reg: true
> 
> Following this series first revision, I have been taking a look at how to
> implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through
> a separated UART, see [1]), and I am facing some constraints. I feel like the
> possible solutions would conflict with this new binding, so even if I am a bit
> late to the party, I would like to expose the issue before the binding is merged
> in case we can find something which would allow to add bluetooth support without
> too much pain after the wlan part.
> 
> Downstream driver currently does not implement bluetooth as a standard bluetooth
> driver (module in drivers/bluetooth, registering a HCI device) but only performs
> a minimal set of operations directly in the wlan part ([2]). Getting a version
> valid for upstream would need the following points to be addressed:
> 1. despite being controlled from a serial port for nominal operations, the
> bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization
> 2. yet init steps are not performed on any kind of subsystem ops but through
> writes to a custom chardev
> 3. the driver does not register itself a hci interface, it is expected to be
> done by userspace (hciattach).
> 
> It is only after those 3 steps that the chip can be used with standard hci
> commands over serial port. IMHO 1 is the biggest point, because it means that
> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
> (so only describing the bluetooth part of the chip as a child node of an uart
> controller is not enough). Aside from bus access, I also expect some
> interactions between bluetooth and wifi (eg: power management, sleep/wakeup)
> 
> After considering multiple solutions to try to share this bus between existing
> wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some

Driver design should not have impact on bindings.

> handles, etc), my current best guess is to convert wilc driver to a MFD driver
> for wilc3000. I guess some work can be done so that the driver can still be
> shared between wilc1000 and wilc3000 _while_ remaining compatible with current
> wilc1000 description, but it would impact the DT description for wilc3000, which
> would need to switch from this:
> 
>   spi {
>     wifi@0 {
>       compatible = "microchip,wilc3000";
>       [...]
>     };
>   };
> 
> To something like this:
> 
>   spi {
>     wilc@0 {
>       compatible = "microchip,wilc3000"; /* mfd driver */

I do not see any reason why... or rather: What is MFD here? MFD is Linux
stuff and we talk about hardware.

>       wifi {
>         compatible = "microchip,wilc3000-wlan";

Why? Just merge it to parent...

>         [...]
>       };
>       bt {
>         compatible = "microchip,wilc3000-bt";
>         XXXX; /* some link to the uart controller connected to the chip */

That's not how we represent UART devices. I don't understand why do you
need these - if for power sequencing, then use power sequencing
framework and describe associated hardware (there are some talks coming
about it in 2 weeks). If for something else, then for what?

>         [...]
>       };
>     };
>   };
> 
> (and similar thing when wilc is driven over a sdio bus)
> 
> Any opinion on this ? Would it make sense to describe wilc3000 chip that way ?
> 

You described drivers, not wilc3000 chip...

Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Marek Vasut]

On 9/3/24 6:09 PM, Alexis Lothoré wrote:
> Hello everyone,

Hi,

> On 8/29/24 02:44, Marek Vasut wrote:
>> Document compatible string for the WILC3000 chip. The chip is similar
>> to WILC1000, except that the register layout is slightly different and
>> it does not support WPA3/SAE.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>>   .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> index 2460ccc082371..5d40f22765bb6 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> @@ -16,7 +16,11 @@ description:
>>   
>>   properties:
>>     compatible:
>> -    const: microchip,wilc1000
>> +    oneOf:
>> +      - items:
>> +          - const: microchip,wilc3000
>> +          - const: microchip,wilc1000
>> +      - const: microchip,wilc1000
>>   
>>     reg: true
> 
> Following this series first revision, I have been taking a look at how to
> implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through
> a separated UART, see [1]), and I am facing some constraints. I feel like the
> possible solutions would conflict with this new binding, so even if I am a bit
> late to the party, I would like to expose the issue before the binding is merged
> in case we can find something which would allow to add bluetooth support without
> too much pain after the wlan part.
> 
> Downstream driver currently does not implement bluetooth as a standard bluetooth
> driver (module in drivers/bluetooth, registering a HCI device) but only performs
> a minimal set of operations directly in the wlan part ([2]). Getting a version
> valid for upstream would need the following points to be addressed:
> 1. despite being controlled from a serial port for nominal operations, the
> bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization
> 2. yet init steps are not performed on any kind of subsystem ops but through
> writes to a custom chardev
> 3. the driver does not register itself a hci interface, it is expected to be
> done by userspace (hciattach).
> 
> It is only after those 3 steps that the chip can be used with standard hci
> commands over serial port. IMHO 1 is the biggest point, because it means that
> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
> (so only describing the bluetooth part of the chip as a child node of an uart
> controller is not enough). Aside from bus access, I also expect some
> interactions between bluetooth and wifi (eg: power management, sleep/wakeup)

Just a quick idea -- what about having a phandle to the BT UART node in 
the wilc3000 node ? Then the wilc driver can check if the phandle is 
available and valid, and attach the BT part to the UART, while also 
doing all the necessary power sequencing and bus accesses via SDIO/SPI.

Like this:

&uart10 {
   status = "okay";
};

&mmc20 {
   ...
   wifi@0 {
     compatible = "microchip,wilc1000";
     microchip,bt-uart = <&uart10>; // OPTIONAL
     ...
   };
};

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Alexis Lothoré]

Hi Krzysztof,

On 9/3/24 20:47, Krzysztof Kozlowski wrote:
> On 03/09/2024 18:09, Alexis Lothoré wrote:

[...]

>> After considering multiple solutions to try to share this bus between existing
>> wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some
> 
> Driver design should not have impact on bindings.
> 
>> handles, etc), my current best guess is to convert wilc driver to a MFD driver
>> for wilc3000. I guess some work can be done so that the driver can still be
>> shared between wilc1000 and wilc3000 _while_ remaining compatible with current
>> wilc1000 description, but it would impact the DT description for wilc3000, which
>> would need to switch from this:
>>
>>   spi {
>>     wifi@0 {
>>       compatible = "microchip,wilc3000";
>>       [...]
>>     };
>>   };
>>
>> To something like this:
>>
>>   spi {
>>     wilc@0 {
>>       compatible = "microchip,wilc3000"; /* mfd driver */
> 
> I do not see any reason why... or rather: What is MFD here? MFD is Linux
> stuff and we talk about hardware.
> 
>>       wifi {
>>         compatible = "microchip,wilc3000-wlan";
> 
> Why? Just merge it to parent...
> 
>>         [...]
>>       };
>>       bt {
>>         compatible = "microchip,wilc3000-bt";
>>         XXXX; /* some link to the uart controller connected to the chip */
> 
> That's not how we represent UART devices. I don't understand why do you
> need these - if for power sequencing, then use power sequencing
> framework and describe associated hardware (there are some talks coming
> about it in 2 weeks). If for something else, then for what?

I have to check more for this power sequencing framework, it look likes it could
handle parts of the wifi/bt shared power management, but it will not cover
everything. The need for this bus on the BT side is not only for power
sequencing, there is some chip initialization to be performed over this bus,
like firmware upload to the chip (not the wifi firmware, it is an additional
bluetooth firmware).

I guess you are referring to Bartosz Golaszewski's talk at Plumbers.
Unfortunately I can not attend, but I'll make sure to check the materials once
available :)

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Alexis Lothoré]

Hello Marek,

On 9/3/24 21:30, Marek Vasut wrote:
> On 9/3/24 6:09 PM, Alexis Lothoré wrote:

[...]

>> It is only after those 3 steps that the chip can be used with standard hci
>> commands over serial port. IMHO 1 is the biggest point, because it means that
>> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
>> (so only describing the bluetooth part of the chip as a child node of an uart
>> controller is not enough). Aside from bus access, I also expect some
>> interactions between bluetooth and wifi (eg: power management, sleep/wakeup)
> 
> Just a quick idea -- what about having a phandle to the BT UART node in the
> wilc3000 node ? Then the wilc driver can check if the phandle is available and
> valid, and attach the BT part to the UART, while also doing all the necessary
> power sequencing and bus accesses via SDIO/SPI.
> 
> Like this:
> 
> &uart10 {
>   status = "okay";
> };
> 
> &mmc20 {
>   ...
>   wifi@0 {
>     compatible = "microchip,wilc1000";
>     microchip,bt-uart = <&uart10>; // OPTIONAL
>     ...
>   };
> };

I thought about something like this too, indeed (but somehow inverted, a
reference to wilc node in the bt node under uart, to allow the bluetooth part to
ask wilc to perform operations over sdio/spi). The design would likely be
simpler in this case, but some internal discussions with colleagues raised some
concerns, for example with power management (but Krzysztof's suggestion about
power sequencing may help with this).

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[Marek Vasut]

On 9/4/24 4:50 PM, Alexis Lothoré wrote:
> Hello Marek,

Hi,

>>> It is only after those 3 steps that the chip can be used with standard hci
>>> commands over serial port. IMHO 1 is the biggest point, because it means that
>>> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
>>> (so only describing the bluetooth part of the chip as a child node of an uart
>>> controller is not enough). Aside from bus access, I also expect some
>>> interactions between bluetooth and wifi (eg: power management, sleep/wakeup)
>>
>> Just a quick idea -- what about having a phandle to the BT UART node in the
>> wilc3000 node ? Then the wilc driver can check if the phandle is available and
>> valid, and attach the BT part to the UART, while also doing all the necessary
>> power sequencing and bus accesses via SDIO/SPI.
>>
>> Like this:
>>
>> &uart10 {
>>    status = "okay";
>> };
>>
>> &mmc20 {
>>    ...
>>    wifi@0 {
>>      compatible = "microchip,wilc1000";
>>      microchip,bt-uart = <&uart10>; // OPTIONAL
>>      ...
>>    };
>> };
> 
> I thought about something like this too, indeed (but somehow inverted, a
> reference to wilc node in the bt node under uart, to allow the bluetooth part to
> ask wilc to perform operations over sdio/spi). The design would likely be
> simpler in this case, but some internal discussions with colleagues raised some
> concerns, for example with power management (but Krzysztof's suggestion about
> power sequencing may help with this).

Maybe switching the current WILC power management to runtime PM would 
simplify things greatly ?

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

> From: Ajay Singh <ajay.kathat@microchip.com>
>
> Add support for the WILC3000 chip. The chip is similar to WILC1000,
> except that the register layout is slightly different and it does
> not support WPA3/SAE.
>
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>  
>  	vif->connecting = true;
>  
> +	if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
> +	    is_wilc3000(vif->wilc->chipid)) {
> +		netdev_err(dev, "WILC3000: WPA3 not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto out_error;
> +	}

This looks wrong. If wilc3000 doesn't support SAE you shouldn't
advertise NL80211_FEATURE_SAE to user space. I think the check for
wilc3000 should be in wilc_create_wiphy():

if (!is_wilc3000(vif->wilc->chipid))
	wiphy->features |= NL80211_FEATURE_SAE;

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 9/9/24 11:35 AM, Kalle Valo wrote:
> Marek Vasut <marex@denx.de> writes:
> 
>> From: Ajay Singh <ajay.kathat@microchip.com>
>>
>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>> except that the register layout is slightly different and it does
>> not support WPA3/SAE.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>   
>>   	vif->connecting = true;
>>   
>> +	if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>> +	    is_wilc3000(vif->wilc->chipid)) {
>> +		netdev_err(dev, "WILC3000: WPA3 not supported\n");
>> +		ret = -EOPNOTSUPP;
>> +		goto out_error;
>> +	}
> 
> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
> advertise NL80211_FEATURE_SAE to user space. I think the check for
> wilc3000 should be in wilc_create_wiphy():
> 
> if (!is_wilc3000(vif->wilc->chipid))

It is probably better to do "if (is_wilc1000(wl->chipid))" here. This 
way, fixed in V5, thanks.

> 	wiphy->features |= NL80211_FEATURE_SAE;

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

> On 9/9/24 11:35 AM, Kalle Valo wrote:
>
>> Marek Vasut <marex@denx.de> writes:
>> 
>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>
>>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>>> except that the register layout is slightly different and it does
>>> not support WPA3/SAE.
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>> [...]
>> 
>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>>     	vif->connecting = true;
>>>   +	if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>>> +	    is_wilc3000(vif->wilc->chipid)) {
>>> +		netdev_err(dev, "WILC3000: WPA3 not supported\n");
>>> +		ret = -EOPNOTSUPP;
>>> +		goto out_error;
>>> +	}
>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
>> advertise NL80211_FEATURE_SAE to user space. I think the check for
>> wilc3000 should be in wilc_create_wiphy():
>> if (!is_wilc3000(vif->wilc->chipid))
>
> It is probably better to do "if (is_wilc1000(wl->chipid))" here.

Good point.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Ajay.Kathat]

On 9/9/24 02:35, Kalle Valo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Marek Vasut <marex@denx.de> writes:
> 
>> From: Ajay Singh <ajay.kathat@microchip.com>
>>
>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>> except that the register layout is slightly different and it does
>> not support WPA3/SAE.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>
>>       vif->connecting = true;
>>
>> +     if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>> +         is_wilc3000(vif->wilc->chipid)) {
>> +             netdev_err(dev, "WILC3000: WPA3 not supported\n");
>> +             ret = -EOPNOTSUPP;
>> +             goto out_error;
>> +     }
> 
> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
> advertise NL80211_FEATURE_SAE to user space. I think the check for
> wilc3000 should be in wilc_create_wiphy():
> 

Actually, the chip ID is not available when wilc_create_wiphy() is called but
is set later in the device probe function. Therefore, adding the
'is_wilc3000(vif->wilc->chipid)' condition may not work as expected.
Also, I think there is no API to change "wiphy->features" after wiphy is
registered to set it later when chip ID information is available.

Does it make sense to add a module parameter for device type(wilc1000 or
wilc3000) to address device-specific featurization.

> if (!is_wilc3000(vif->wilc->chipid))
>         wiphy->features |= NL80211_FEATURE_SAE;
> 
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Kalle Valo]

<Ajay.Kathat@microchip.com> writes:

> On 9/9/24 02:35, Kalle Valo wrote:
>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> 
>> Marek Vasut <marex@denx.de> writes:
>> 
>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>
>>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>>> except that the register layout is slightly different and it does
>>> not support WPA3/SAE.
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>> 
>> [...]
>> 
>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>>
>>>       vif->connecting = true;
>>>
>>> +     if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>>> +         is_wilc3000(vif->wilc->chipid)) {
>>> +             netdev_err(dev, "WILC3000: WPA3 not supported\n");
>>> +             ret = -EOPNOTSUPP;
>>> +             goto out_error;
>>> +     }
>> 
>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
>> advertise NL80211_FEATURE_SAE to user space. I think the check for
>> wilc3000 should be in wilc_create_wiphy():
>> 
>
> Actually, the chip ID is not available when wilc_create_wiphy() is called but
> is set later in the device probe function. Therefore, adding the
> 'is_wilc3000(vif->wilc->chipid)' condition may not work as expected.
> Also, I think there is no API to change "wiphy->features" after wiphy is
> registered to set it later when chip ID information is available.

Sounds like the driver is doing something funky in the registration, the
idea is that the device capabilities are probed before calling
wiphy_register().

> Does it make sense to add a module parameter for device type(wilc1000 or
> wilc3000) to address device-specific featurization.

We don't do hacks like that in upstream, it's expected that the driver
does this all automatically.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 9/9/24 5:04 PM, Kalle Valo wrote:
> Marek Vasut <marex@denx.de> writes:
> 
>> On 9/9/24 11:35 AM, Kalle Valo wrote:
>>
>>> Marek Vasut <marex@denx.de> writes:
>>>
>>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>>
>>>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>>>> except that the register layout is slightly different and it does
>>>> not support WPA3/SAE.
>>>>
>>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> [...]
>>>
>>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>>>      	vif->connecting = true;
>>>>    +	if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>>>> +	    is_wilc3000(vif->wilc->chipid)) {
>>>> +		netdev_err(dev, "WILC3000: WPA3 not supported\n");
>>>> +		ret = -EOPNOTSUPP;
>>>> +		goto out_error;
>>>> +	}
>>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
>>> advertise NL80211_FEATURE_SAE to user space. I think the check for
>>> wilc3000 should be in wilc_create_wiphy():
>>> if (!is_wilc3000(vif->wilc->chipid))
>>
>> It is probably better to do "if (is_wilc1000(wl->chipid))" here.
> 
> Good point.
I did send v5 which grew a few more patches to address getting chipid early.

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Ajay.Kathat]

On 9/9/24 10:31, Kalle Valo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> <Ajay.Kathat@microchip.com> writes:
> 
>> On 9/9/24 02:35, Kalle Valo wrote:
>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Marek Vasut <marex@denx.de> writes:
>>>
>>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>>
>>>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>>>> except that the register layout is slightly different and it does
>>>> not support WPA3/SAE.
>>>>
>>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> [...]
>>>
>>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>>>
>>>>       vif->connecting = true;
>>>>
>>>> +     if (sme->auth_type == NL80211_AUTHTYPE_SAE &&
>>>> +         is_wilc3000(vif->wilc->chipid)) {
>>>> +             netdev_err(dev, "WILC3000: WPA3 not supported\n");
>>>> +             ret = -EOPNOTSUPP;
>>>> +             goto out_error;
>>>> +     }
>>>
>>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't
>>> advertise NL80211_FEATURE_SAE to user space. I think the check for
>>> wilc3000 should be in wilc_create_wiphy():
>>>
>>
>> Actually, the chip ID is not available when wilc_create_wiphy() is called but
>> is set later in the device probe function. Therefore, adding the
>> 'is_wilc3000(vif->wilc->chipid)' condition may not work as expected.
>> Also, I think there is no API to change "wiphy->features" after wiphy is
>> registered to set it later when chip ID information is available.
> 
> Sounds like the driver is doing something funky in the registration, the
> idea is that the device capabilities are probed before calling
> wiphy_register().
> 

Agree, this approach will configure the wiphy based on connected device.

>> Does it make sense to add a module parameter for device type(wilc1000 or
>> wilc3000) to address device-specific featurization.
> 
> We don't do hacks like that in upstream, it's expected that the driver
> does this all automatically.

Marek has already submitted the patch to delay calling wiphy_register() so it
should work at run time for both the devices.
I'm just curious to know for which scenario the module parameters should be
used. Can it be used for enabling or disabling any feature, configuring any
parameters, instead of complete device-specific featurization.


Regards,
Ajay

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 9/9/24 11:11 PM, Ajay.Kathat@microchip.com wrote:

[...]

>>> Does it make sense to add a module parameter for device type(wilc1000 or
>>> wilc3000) to address device-specific featurization.
>>
>> We don't do hacks like that in upstream, it's expected that the driver
>> does this all automatically.
> 
> Marek has already submitted the patch to delay calling wiphy_register() so it
> should work at run time for both the devices.
> I'm just curious to know for which scenario the module parameters should be
> used. Can it be used for enabling or disabling any feature, configuring any
> parameters, instead of complete device-specific featurization.
Module parameters are applicable for tunables which cannot be otherwise 
configured at runtime. Runtime configuration is always preferable. For 
the wilc, I don't think there is anything which has to be a module 
parameter, maybe firmware filename could be it.

Re: [PATCH v4 5/5] wifi: wilc1000: Add WILC3000 support

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

> On 9/9/24 11:11 PM, Ajay.Kathat@microchip.com wrote:
>
> [...]
>
>>>> Does it make sense to add a module parameter for device type(wilc1000 or
>>>> wilc3000) to address device-specific featurization.
>>>
>>> We don't do hacks like that in upstream, it's expected that the driver
>>> does this all automatically.
>>
>> Marek has already submitted the patch to delay calling
>> wiphy_register() so it
>> should work at run time for both the devices.
>> I'm just curious to know for which scenario the module parameters should be
>> used. Can it be used for enabling or disabling any feature, configuring any
>> parameters, instead of complete device-specific featurization.
>
> Module parameters are applicable for tunables which cannot be
> otherwise configured at runtime. Runtime configuration is always
> preferable. For the wilc, I don't think there is anything which has to
> be a module parameter, maybe firmware filename could be it.

Nowadays module parameters are frowned upon, apparently some subsystems
have even banned adding new module parameters. In wireless we have been
more lax and have allowed new module parameters in some cases with good
justification, but it's still rare.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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