[PATCH v2 1/4] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

[PATCH v2 1/4] 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.

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
---
 .../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..b8ee6cdab3c25 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,wilc1000
+          - const: microchip,wilc3000
+      - const: microchip,wilc1000
 
   reg: true
 
-- 
2.43.0

[PATCH v2 2/4] 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 .

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
---
 .../net/wireless/microchip/wilc1000/sdio.c    | 13 ----
 .../net/wireless/microchip/wilc1000/wlan.c    | 69 +++++++++++--------
 .../net/wireless/microchip/wilc1000/wlan.h    |  1 -
 3 files changed, 40 insertions(+), 43 deletions(-)

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..1aab2f2dc159f 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1402,6 +1402,35 @@ int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 	return ret;
 }
 
+static u32 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 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;
+}
+
 static int init_chip(struct net_device *dev)
 {
 	u32 chipid;
@@ -1412,7 +1441,7 @@ static int init_chip(struct net_device *dev)
 
 	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 
-	chipid = wilc_get_chipid(wilc, true);
+	chipid = wilc_get_chipid(wilc);
 
 	if ((chipid & 0xfff) != 0xa0) {
 		ret = wilc->hif_func->hif_read_reg(wilc,
@@ -1445,34 +1474,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;
@@ -1527,6 +1528,7 @@ int wilc_wlan_init(struct net_device *dev)
 	int ret = 0;
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc;
+	u32 chipid = 0;
 
 	wilc = vif->wilc;
 
@@ -1535,9 +1537,18 @@ 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)
+			chipid = wilc_get_chipid(wilc);
 		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
 		if (ret)
 			goto fail;
+
+		if (!is_wilc1000(chipid)) {
+			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
+			return -EINVAL;
+		}
+
+		netdev_dbg(dev, "chipid (%08x)\n", 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.43.0

[PATCH v2 3/4] 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().

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
---
 .../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 1aab2f2dc159f..5fbba6876bd07 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.43.0

[PATCH v2 4/4] 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
---
 .../wireless/microchip/wilc1000/cfg80211.c    |   7 +
 .../net/wireless/microchip/wilc1000/netdev.c  |  29 ++-
 .../net/wireless/microchip/wilc1000/sdio.c    |  62 ++++-
 drivers/net/wireless/microchip/wilc1000/spi.c |   2 +-
 .../net/wireless/microchip/wilc1000/wlan.c    | 217 +++++++++++++++---
 .../net/wireless/microchip/wilc1000/wlan.h    |  43 +++-
 6 files changed, 298 insertions(+), 62 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 9ecf3fb29b558..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;
@@ -195,20 +201,24 @@ 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;
+	char *firmware;
 	int ret;
 
-	chip_id = wilc_get_chipid(wilc, false);
+	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, "ChipID [%x] loading firmware [%s]\n", chip_id,
+	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 90357c89ae29b..1f41dd72a885f 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 5fbba6876bd07..4ec6c79256667 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 void 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 void chip_allow_sleep(struct wilc *wilc)
+{
+	if (is_wilc1000(wilc->chipid))
+		chip_allow_sleep_wilc1000(wilc);
+	else
+		chip_allow_sleep_wilc3000(wilc);
+}
+
+static void chip_wakeup_wilc1000(struct wilc *wilc)
 {
 	u32 ret = 0;
 	u32 clk_status_val = 0, trials = 0;
@@ -613,15 +647,15 @@ static void 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;
 	}
@@ -661,6 +695,72 @@ static void chip_wakeup(struct wilc *wilc)
 		wilc->hif_func->hif_reset(wilc);
 }
 
+static void 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");
+}
+
+static void chip_wakeup(struct wilc *wilc)
+{
+	if (is_wilc1000(wilc->chipid))
+		chip_wakeup_wilc1000(wilc);
+	else
+		chip_wakeup_wilc3000(wilc);
+}
+
 static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
 {
 	mutex_lock(&wilc->hif_cs);
@@ -678,7 +778,9 @@ static inline void release_bus(struct wilc *wilc, enum bus_release release)
 void host_wakeup_notify(struct wilc *wilc)
 {
 	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
-	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);
 	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_wakeup_notify);
@@ -686,7 +788,9 @@ EXPORT_SYMBOL_GPL(host_wakeup_notify);
 void host_sleep_notify(struct wilc *wilc)
 {
 	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
-	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);
 	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_sleep_notify);
@@ -817,19 +921,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;
@@ -1159,6 +1289,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;
@@ -1407,21 +1540,25 @@ static u32 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 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->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 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;
@@ -1467,6 +1604,20 @@ static int init_chip(struct net_device *dev)
 		}
 	}
 
+	if (is_wilc3000(wilc->chipid)) {
+		ret = wilc->hif_func->hif_read_reg(wilc, 0x207ac, &reg);
+		if (ret) {
+			netdev_err(dev, "fail read reg 0x207ac\n");
+			goto release;
+		}
+
+		ret = wilc->hif_func->hif_write_reg(wilc, 0x4f0000, 0x71);
+		if (ret) {
+			netdev_err(dev, "fail write reg 0x4f0000\n");
+			goto release;
+		}
+	}
+
 release:
 	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 
@@ -1542,7 +1693,7 @@ int wilc_wlan_init(struct net_device *dev)
 		if (ret)
 			goto fail;
 
-		if (!is_wilc1000(chipid)) {
+		if (!is_wilc1000(chipid) && !is_wilc3000(chipid)) {
 			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
 			return -EINVAL;
 		}
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index e75a5c8aaecec..c69534ccba19c 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,7 @@
 
 #define WILC_CORTUS_RESET_MUX_SEL	0x1118
 #define WILC_CORTUS_BOOT_REGISTER	0xc0000
+#define WILC3000_CHIP_ID		0x3b0000
 
 #define WILC_CORTUS_BOOT_FROM_IRAM	0x71
 
@@ -195,6 +215,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 +435,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.43.0

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Simon Horman]

On Fri, Aug 23, 2024 at 06:08:57PM +0200, Marek Vasut wrote:
> 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 .
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

...

> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c

...

> @@ -1535,9 +1537,18 @@ 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)
> +			chipid = wilc_get_chipid(wilc);
>  		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
>  		if (ret)
>  			goto fail;
> +
> +		if (!is_wilc1000(chipid)) {
> +			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
> +			return -EINVAL;

Hi Marek,

Should this unwind as is the case elsewhere in this function?

			ret = -EINVAL;
			goto fail;

Flagged by Smatch.

> +		}
> +
> +		netdev_dbg(dev, "chipid (%08x)\n", chipid);
>  	}
>  
>  	if (!wilc->vmm_table)

...

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Marek Vasut]

On 8/23/24 7:46 PM, Simon Horman wrote:
> On Fri, Aug 23, 2024 at 06:08:57PM +0200, Marek Vasut wrote:
>> 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 .
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> ...
> 
>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> 
> ...
> 
>> @@ -1535,9 +1537,18 @@ 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)
>> +			chipid = wilc_get_chipid(wilc);
>>   		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
>>   		if (ret)
>>   			goto fail;
>> +
>> +		if (!is_wilc1000(chipid)) {
>> +			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
>> +			return -EINVAL;
> 
> Hi Marek,
> 
> Should this unwind as is the case elsewhere in this function?

It should, will fix in V3, thanks.

> 			ret = -EINVAL;
> 			goto fail;
> 
> Flagged by Smatch.

What's the trick here ?

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

[Krzysztof Kozlowski]

On 23/08/2024 18:08, 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.
> 
> 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
> ---
>  .../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..b8ee6cdab3c25 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,wilc1000
> +          - const: microchip,wilc3000

That's wrong order of compatibles. Fallback is wilc1000, so should be
the last item.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Simon Horman]

+ Dan

On Fri, Aug 23, 2024 at 10:38:59PM +0200, Marek Vasut wrote:
> On 8/23/24 7:46 PM, Simon Horman wrote:
> > On Fri, Aug 23, 2024 at 06:08:57PM +0200, Marek Vasut wrote:
> > > 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 .
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > 
> > ...
> > 
> > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> > 
> > ...
> > 
> > > @@ -1535,9 +1537,18 @@ 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)
> > > +			chipid = wilc_get_chipid(wilc);
> > >   		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
> > >   		if (ret)
> > >   			goto fail;
> > > +
> > > +		if (!is_wilc1000(chipid)) {
> > > +			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
> > > +			return -EINVAL;
> > 
> > Hi Marek,
> > 
> > Should this unwind as is the case elsewhere in this function?
> 
> It should, will fix in V3, thanks.
> 
> > 			ret = -EINVAL;
> > 			goto fail;
> > 
> > Flagged by Smatch.
> 
> What's the trick here ?

Smatch is here. I don't think there is much of a trick other than running it :)

https://github.com/error27/smatch

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Marek Vasut]

On 8/24/24 2:44 PM, Simon Horman wrote:
> + Dan
> 
> On Fri, Aug 23, 2024 at 10:38:59PM +0200, Marek Vasut wrote:
>> On 8/23/24 7:46 PM, Simon Horman wrote:
>>> On Fri, Aug 23, 2024 at 06:08:57PM +0200, Marek Vasut wrote:
>>>> 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 .
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> ...
>>>
>>>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
>>>
>>> ...
>>>
>>>> @@ -1535,9 +1537,18 @@ 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)
>>>> +			chipid = wilc_get_chipid(wilc);
>>>>    		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
>>>>    		if (ret)
>>>>    			goto fail;
>>>> +
>>>> +		if (!is_wilc1000(chipid)) {
>>>> +			netdev_err(dev, "Unsupported chipid: %x\n", chipid);
>>>> +			return -EINVAL;
>>>
>>> Hi Marek,
>>>
>>> Should this unwind as is the case elsewhere in this function?
>>
>> It should, will fix in V3, thanks.
>>
>>> 			ret = -EINVAL;
>>> 			goto fail;
>>>
>>> Flagged by Smatch.
>>
>> What's the trick here ?
> 
> Smatch is here. I don't think there is much of a trick other than running it :)
> 
> https://github.com/error27/smatch

Thanks !

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

[Marek Vasut]

On 8/24/24 8:22 AM, Krzysztof Kozlowski wrote:

Hi,

>> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> index 2460ccc082371..b8ee6cdab3c25 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,wilc1000
>> +          - const: microchip,wilc3000
> 
> That's wrong order of compatibles. Fallback is wilc1000, so should be
> the last item.

Right, fixed, thanks.

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Alexis Lothoré]

Hello Marek,

On 8/23/24 18:08, Marek Vasut wrote:
> 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 .
> 
> 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
> ---

[...]

> +static u32 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);
If we search for WILC_CHIPID in the whole driver, there are still two places
manually reading this register. Shouldn't those places also benefit from
wilc_get_chipid ?

> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
> +					     &rfrevid);
> +		if (!is_wilc1000(chipid)) {
> +			wilc->chipid = 0;

While at it, since you have trimmed the update parameter, it would be nice to
also fix this return value (ie make wilc_getchipid() not return 0 but a real
error code if we can not read the chip id.

Thanks,

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

Re: [PATCH v2 3/4] wifi: wilc1000: Fold chip_allow_sleep()/chip_wakeup() into wlan.c

[Alexis Lothoré]

On 8/23/24 18:08, Marek Vasut wrote:
> 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().
> 
> 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
> ---
>  .../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 1aab2f2dc159f..5fbba6876bd07 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;

Forwarding error codes sounds like a good idea, but neither this patch nor the
next one is reading the return value from any chip_allow_sleep[XXX] function, so
it does not bring much value.

Alexis

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

Re: [PATCH v2 4/4] wifi: wilc1000: Add WILC3000 support

[Alexis Lothoré]

On 8/23/24 18:08, Marek Vasut wrote:
> 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
> ---
>  .../wireless/microchip/wilc1000/cfg80211.c    |   7 +
>  .../net/wireless/microchip/wilc1000/netdev.c  |  29 ++-
>  .../net/wireless/microchip/wilc1000/sdio.c    |  62 ++++-
>  drivers/net/wireless/microchip/wilc1000/spi.c |   2 +-
>  .../net/wireless/microchip/wilc1000/wlan.c    | 217 +++++++++++++++---
>  .../net/wireless/microchip/wilc1000/wlan.h    |  43 +++-
>  6 files changed, 298 insertions(+), 62 deletions(-)
> 

[...]

> @@ -1467,6 +1604,20 @@ static int init_chip(struct net_device *dev)
>  		}
>  	}
>  
> +	if (is_wilc3000(wilc->chipid)) {
> +		ret = wilc->hif_func->hif_read_reg(wilc, 0x207ac, &reg);

Some defines would be nice here instead of hardcoded addresses. I have asked
Ajay about those while working on wilc3000, the meaning is roughly the following:
- 0x000207ac: WILC_3000_BOOTROM_STATUS_REGISTER
- 0x004f0000: WILC_3000_CORTUS_BOOT_REGISTER_2
- 0x71: WILC_CORTUS_BOOT_FROM_IRAM

Thanks,

Alexis

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

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Marek Vasut]

On 8/27/24 9:51 AM, Alexis Lothoré wrote:

Hi,

>> +static u32 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);
> If we search for WILC_CHIPID in the whole driver, there are still two places
> manually reading this register. Shouldn't those places also benefit from
> wilc_get_chipid ?

Both the one in wilc_wlan_start() and wilc_validate_chipid() look more 
like some sort of communication check attempt, rather than reading out 
the chipid for any sort of actual chip identification purpose. I could 
simply remove those ?

>> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
>> +					     &rfrevid);
>> +		if (!is_wilc1000(chipid)) {
>> +			wilc->chipid = 0;
> 
> While at it, since you have trimmed the update parameter, it would be nice to
> also fix this return value (ie make wilc_getchipid() not return 0 but a real
> error code if we can not read the chip id.

Fixed in V3, thanks .

Re: [PATCH v2 3/4] wifi: wilc1000: Fold chip_allow_sleep()/chip_wakeup() into wlan.c

[Marek Vasut]

On 8/27/24 10:14 AM, Alexis Lothoré wrote:
> On 8/23/24 18:08, Marek Vasut wrote:
>> 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().
>>
>> 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
>> ---
>>   .../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 1aab2f2dc159f..5fbba6876bd07 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;
> 
> Forwarding error codes sounds like a good idea, but neither this patch nor the
> next one is reading the return value from any chip_allow_sleep[XXX] function, so
> it does not bring much value.

I will add a follow up patch to this one which adds the error handling, 
since there is a lot of it to propagate the errors through. It will be 
in V3.

Re: [PATCH v2 4/4] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 8/27/24 10:28 AM, Alexis Lothoré wrote:

Hi,

>> @@ -1467,6 +1604,20 @@ static int init_chip(struct net_device *dev)
>>   		}
>>   	}
>>   
>> +	if (is_wilc3000(wilc->chipid)) {
>> +		ret = wilc->hif_func->hif_read_reg(wilc, 0x207ac, &reg);
> 
> Some defines would be nice here instead of hardcoded addresses. I have asked
> Ajay about those while working on wilc3000, the meaning is roughly the following:
> - 0x000207ac: WILC_3000_BOOTROM_STATUS_REGISTER
> - 0x004f0000: WILC_3000_CORTUS_BOOT_REGISTER_2
> - 0x71: WILC_CORTUS_BOOT_FROM_IRAM

Fixed in V3, thanks.

Re: [PATCH v2 2/4] wifi: wilc1000: Fold wilc_get_chipid() into wlan.c

[Alexis Lothoré]

On 8/27/24 17:34, Marek Vasut wrote:
> On 8/27/24 9:51 AM, Alexis Lothoré wrote:
> 
> Hi,
> 
>>> +static u32 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);
>> If we search for WILC_CHIPID in the whole driver, there are still two places
>> manually reading this register. Shouldn't those places also benefit from
>> wilc_get_chipid ?
> 
> Both the one in wilc_wlan_start() and wilc_validate_chipid() look more like some
> sort of communication check attempt, rather than reading out the chipid for any
> sort of actual chip identification purpose. I could simply remove those ?

Agree about the purpose of this reading in wilc_wlan_start and wilc_validate_chipid.
And about removing those: I would say why not. wilc_validate_chipid has proven
to be quite useful to diagnose some early communication failure, but I guess
there are enough communications attempts around
(wilc_spi_configure_bus_protocol, wilc_load_mac_from_nv) to still validate than
we are able to communicate with the chip at probe time.
> 
>>> +        wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
>>> +                         &rfrevid);
>>> +        if (!is_wilc1000(chipid)) {
>>> +            wilc->chipid = 0;
>>
>> While at it, since you have trimmed the update parameter, it would be nice to
>> also fix this return value (ie make wilc_getchipid() not return 0 but a real
>> error code if we can not read the chip id.
> 
> Fixed in V3, thanks .

Great, thanks

Alexis

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