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

[PATCH 1/2] 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
---
 .../devicetree/bindings/net/wireless/microchip,wilc1000.yaml  | 4 +++-
 1 file changed, 3 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..1bf3496c21e64 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
@@ -16,7 +16,9 @@ description:
 
 properties:
   compatible:
-    const: microchip,wilc1000
+    enum:
+      - microchip,wilc1000
+      - microchip,wilc3000
 
   reg: true
 
-- 
2.43.0

[PATCH 2/2] 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
---
 .../wireless/microchip/wilc1000/cfg80211.c    |   7 +
 .../net/wireless/microchip/wilc1000/netdev.c  |  28 ++-
 .../net/wireless/microchip/wilc1000/netdev.h  |   1 +
 .../net/wireless/microchip/wilc1000/sdio.c    |  76 ++++--
 drivers/net/wireless/microchip/wilc1000/spi.c |   7 +-
 .../net/wireless/microchip/wilc1000/wlan.c    | 226 +++++++++++++++---
 .../net/wireless/microchip/wilc1000/wlan.h    |  48 +++-
 7 files changed, 321 insertions(+), 72 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index eb37b228d54ea..beba11b9b8888 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 &&
+	    vif->wilc->chip == WILC_3000) {
+		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..1a119045a33be 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,23 @@ 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 (wilc->chip == WILC_3000)
+		firmware = WILC3000_FW(WILC3000_API_VER);
+	else if (wilc->chip == WILC_1000)
+		firmware = WILC1000_FW(WILC1000_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", wilc->chip,
 		    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 +242,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 +537,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 +1023,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/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index 95bc8b8fe65a5..03644e68740be 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -273,6 +273,7 @@ struct wilc {
 
 	struct device *dev;
 
+	enum wilc_chip_type chip;
 	struct workqueue_struct *hif_workqueue;
 	struct wilc_cfg cfg;
 	void *bus_data;
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 41122199d51eb..7b99fcc450fd3 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -764,9 +764,13 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
 	 *      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");
+		chipid = wilc_get_chipid(wilc, true);
+		if (is_wilc3000(chipid)) {
+			wilc->chip = WILC_3000;
+		} else if (is_wilc1000(chipid)) {
+			wilc->chip = WILC_1000;
+		} else {
+			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
 			return ret;
 		}
 		dev_err(&func->dev, "chipid (%08x)\n", chipid);
@@ -819,13 +823,22 @@ 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;
+		if (wilc->chip == WILC_1000)
+			cmd.address = WILC1000_SDIO_IRQ_FLAG_REG;
+		else
+			cmd.address = 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) {
+		if (wilc->chip == WILC_1000)
+			irq_flags = cmd.data & 0x1f;
+		else
+			irq_flags = cmd.data & 0x0f;
+	}
 	tmp |= FIELD_PREP(IRG_FLAGS_MASK, cmd.data);
 
 	if (FIELD_GET(UNHANDLED_IRQ_MASK, irq_flags))
@@ -847,22 +860,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 (wilc->chip == WILC_1000) {
+		/* 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 = (wilc->chip == WILC_1000) ?
+			      WILC1000_SDIO_IRQ_CLEAR_FLAG_REG :
+			      WILC3000_SDIO_VMM_TBL_CTRL_REG;
 		cmd.data = reg;
 
 		ret = wilc_sdio_cmd52(wilc, &cmd);
@@ -1012,6 +1059,7 @@ static int wilc_sdio_resume(struct device *dev)
 
 static const struct of_device_id wilc_of_match[] = {
 	{ .compatible = "microchip,wilc1000", },
+	{ .compatible = "microchip,wilc3000", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, wilc_of_match);
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 5ff940c53ad9c..40c93e127b804 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -287,12 +287,14 @@ static void wilc_bus_remove(struct spi_device *spi)
 
 static const struct of_device_id wilc_of_match[] = {
 	{ .compatible = "microchip,wilc1000", },
+	{ .compatible = "microchip,wilc3000", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, wilc_of_match);
 
 static const struct spi_device_id wilc_spi_id[] = {
 	{ "wilc1000", 0 },
+	{ "wilc3000", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, wilc_spi_id);
@@ -1232,10 +1234,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)) {
-		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
-		return -ENODEV;
-	}
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 533939e71534a..a7cc8c0ea5de4 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -555,7 +555,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_wilc1000(struct wilc *wilc)
 {
 	u32 reg = 0;
 	const struct wilc_hif_func *hif_func = wilc->hif_func;
@@ -584,7 +584,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 +594,63 @@ 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;
+}
+
+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;
+}
+
+void chip_allow_sleep(struct wilc *wilc)
+{
+	if (wilc->chip == WILC_1000)
+		chip_allow_sleep_wilc1000(wilc);
+	else
+		chip_allow_sleep_wilc3000(wilc);
 }
 EXPORT_SYMBOL_GPL(chip_allow_sleep);
 
-void chip_wakeup(struct wilc *wilc)
+static void chip_wakeup_wilc1000(struct wilc *wilc)
 {
 	u32 ret = 0;
 	u32 clk_status_val = 0, trials = 0;
@@ -627,15 +662,15 @@ 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;
 	}
@@ -674,12 +709,80 @@ void chip_wakeup(struct wilc *wilc)
 	if (wilc->io_type == WILC_HIF_SPI)
 		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");
+}
+
+void chip_wakeup(struct wilc *wilc)
+{
+	if (wilc->chip == WILC_1000)
+		chip_wakeup_wilc1000(wilc);
+	else
+		chip_wakeup_wilc3000(wilc);
+}
 EXPORT_SYMBOL_GPL(chip_wakeup);
 
 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, (wilc->chip == WILC_1000) ?
+					    WILC1000_CORTUS_INTERRUPT_2 :
+					    WILC3000_CORTUS_INTERRUPT_2, 1);
 	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_wakeup_notify);
@@ -687,7 +790,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, (wilc->chip == WILC_1000) ?
+					    WILC1000_CORTUS_INTERRUPT_1 :
+					    WILC3000_CORTUS_INTERRUPT_1, 1);
 	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 EXPORT_SYMBOL_GPL(host_sleep_notify);
@@ -818,19 +923,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 (wilc->chip == WILC_1000) {
+			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;
@@ -1160,6 +1291,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 (wilc->chip == WILC_3000)
+		reg |= WILC_HAVE_SLEEP_CLK_SRC_RTC;
+
 	ret = wilc->hif_func->hif_write_reg(wilc, WILC_GP_REG_1, reg);
 	if (ret)
 		goto release;
@@ -1439,6 +1573,20 @@ static int init_chip(struct net_device *dev)
 		}
 	}
 
+	if (wilc->chip == WILC_3000) {
+		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);
 
@@ -1451,21 +1599,25 @@ u32 wilc_get_chipid(struct wilc *wilc, bool update)
 	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->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;
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index dd2fb3c2f06a2..21b925e6a92f2 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)
 
 /********************************************
@@ -356,6 +378,11 @@ struct rxq_entry_t {
 	int buffer_size;
 };
 
+enum wilc_chip_type {
+	WILC_1000 = 1000,
+	WILC_3000 = 3000,
+};
+
 /********************************************
  *
  *      Host IF Structure
@@ -413,6 +440,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 2/2] wifi: wilc1000: Add WILC3000 support

[Simon Horman]

On Wed, Aug 21, 2024 at 08:42:33PM +0200, 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>

...

> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
> index 41122199d51eb..7b99fcc450fd3 100644
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -764,9 +764,13 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
>  	 *      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");
> +		chipid = wilc_get_chipid(wilc, true);
> +		if (is_wilc3000(chipid)) {
> +			wilc->chip = WILC_3000;
> +		} else if (is_wilc1000(chipid)) {
> +			wilc->chip = WILC_1000;
> +		} else {
> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>  			return ret;

Hi Marek and Ajay,

It seems that with this change ret will be 0 here.
Perhaps an negative error code should be returned instead?

Flagged by Smatch.

>  		}
>  		dev_err(&func->dev, "chipid (%08x)\n", chipid);

...

Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 8/22/24 12:26 PM, Simon Horman wrote:
> On Wed, Aug 21, 2024 at 08:42:33PM +0200, 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>
> 
> ...
> 
>> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> index 41122199d51eb..7b99fcc450fd3 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> @@ -764,9 +764,13 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
>>   	 *      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");
>> +		chipid = wilc_get_chipid(wilc, true);
>> +		if (is_wilc3000(chipid)) {
>> +			wilc->chip = WILC_3000;
>> +		} else if (is_wilc1000(chipid)) {
>> +			wilc->chip = WILC_1000;
>> +		} else {
>> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>>   			return ret;
> 
> Hi Marek and Ajay,
> 
> It seems that with this change ret will be 0 here.
> Perhaps an negative error code should be returned instead?

Fixed by returning -EINVAL , thanks .

Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

[Alexis Lothoré]

Hello Marek,

I was coincidentally working on adding wilc3000 support upstream too. My work is
also based on downstream tree, so my comments will likely reflect the reworks I
was doing or intended to do.
For the record, I have some wilc1000 and wilc3000 modules, in both  sdio and spi
setups.

On 8/21/24 20:42, Marek Vasut wrote:
> From: Ajay Singh <ajay.kathat@microchip.com>

[...]

>  	if (!resume) {
> -		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
> -		if (ret) {
> -			dev_err(&func->dev, "Fail cmd read chip id...\n");
> +		chipid = wilc_get_chipid(wilc, true);
> +		if (is_wilc3000(chipid)) {
> +			wilc->chip = WILC_3000;
> +		} else if (is_wilc1000(chipid)) {
> +			wilc->chip = WILC_1000;
> +		} else {
> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>  			return ret;
>  		}

I wonder if this additional enum (enum wilc_chip_type)  is really useful. We
already store the raw chipid, which just needs to be masked to know about the
device type. We should likely store one or the other but not both, otherwise we
may just risk to create desync without really saving useful info.

Also, this change makes wilc1000-sdio failing to build as module (missing symbol
export on wilc_get_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);
> +	if (wilc->chip == WILC_1000) {

wilc1000 should likely remain the default/fallback ?

[...]

> @@ -1232,10 +1234,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)) {
> -		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
> -		return -ENODEV;
> -	}
> +

Instead of dropping any filtering (and then making the function name become
irrelevant), why not ensuring that it is at least either a wilc1000 or a wilc3000 ?

>  	return 0;
>  }
>  
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 533939e71534a..a7cc8c0ea5de4 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -555,7 +555,7 @@ static struct rxq_entry_t *wilc_wlan_rxq_remove(struct wilc *wilc)
>  	return rqe;
>  }

[...]

> +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;
> +}
> +
> +void chip_allow_sleep(struct wilc *wilc)
> +{
> +	if (wilc->chip == WILC_1000)
> +		chip_allow_sleep_wilc1000(wilc);
> +	else
> +		chip_allow_sleep_wilc3000(wilc);
>  }
>  EXPORT_SYMBOL_GPL(chip_allow_sleep);
>  
> -void chip_wakeup(struct wilc *wilc)
> +static void chip_wakeup_wilc1000(struct wilc *wilc)
>  {
>  	u32 ret = 0;
>  	u32 clk_status_val = 0, trials = 0;
> @@ -627,15 +662,15 @@ 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;
>  	}
> @@ -674,12 +709,80 @@ void chip_wakeup(struct wilc *wilc)
>  	if (wilc->io_type == WILC_HIF_SPI)
>  		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");
> +}
> +
> +void chip_wakeup(struct wilc *wilc)
> +{
> +	if (wilc->chip == WILC_1000)
> +		chip_wakeup_wilc1000(wilc);
> +	else
> +		chip_wakeup_wilc3000(wilc);
> +}
>  EXPORT_SYMBOL_GPL(chip_wakeup);

This new support makes a few places in wlan.c, netdev.c and in bus files
(sdio.c, spi.c) install (sometimes big) branches on the device type (chip init,
sleep, wakeup, read interrupt, clear interrupt, txq handling, etc), because the
registers are different, the masks are different, the number of involved
registers may not be the same, wilc3000 may need more operations to perform the
same thing... I feel like it will make it harder in the long run to maintain the
driver, especially if some new variants are added later. Those branches tend to
show that some operations in those files are too specific to the targeted
device. I was examining the possibility to start creating device-type specific
files (wilc1000.c, wilc3000.c) and move those operations as "device-specific"
ops. Then wlan/netdev would call those chip-specific ops, which in turn may call
the hif_func ops. It may need some rework in the bus files to fit this new
hierarchy, but it may allow to keep netdev and wlan unaware of the device type,
and since wilc3000 has bluetooth, it may also make it easier to introduce the
corresponding support later. What do you think about it ? Ajay, any opinion on
this ?

Thanks,

Alexis

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

Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

[Marek Vasut]

On 8/22/24 2:10 PM, Alexis Lothoré wrote:
> Hello Marek,

Hi,

> I was coincidentally working on adding wilc3000 support upstream too.

I hope you weren't too far along with that and I didn't waste too much 
of your time/effort here.

> My work is
> also based on downstream tree, so my comments will likely reflect the reworks I
> was doing or intended to do.
> For the record, I have some wilc1000 and wilc3000 modules, in both  sdio and spi
> setups.

Nice, I only have this WILC3000 SDIO device .

> On 8/21/24 20:42, Marek Vasut wrote:
>> From: Ajay Singh <ajay.kathat@microchip.com>
> 
> [...]
> 
>>   	if (!resume) {
>> -		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
>> -		if (ret) {
>> -			dev_err(&func->dev, "Fail cmd read chip id...\n");
>> +		chipid = wilc_get_chipid(wilc, true);
>> +		if (is_wilc3000(chipid)) {
>> +			wilc->chip = WILC_3000;
>> +		} else if (is_wilc1000(chipid)) {
>> +			wilc->chip = WILC_1000;
>> +		} else {
>> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>>   			return ret;
>>   		}
> 
> I wonder if this additional enum (enum wilc_chip_type)  is really useful. We
> already store the raw chipid, which just needs to be masked to know about the
> device type. We should likely store one or the other but not both, otherwise we
> may just risk to create desync without really saving useful info.
> 
> Also, this change makes wilc1000-sdio failing to build as module (missing symbol
> export on wilc_get_chipid)

I think I have a separate patch for this, one which folds 
wilc_get_chipid() entirely into wlan.c , and then follow up which uses 
is_wilc1000() / is_wilc3000() all over the place to discern the two MACs 
based on cached chip ID . That should work, I'll test it and submit it 
later today I hope.

> [...]
> 
>> -	/* 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 (wilc->chip == WILC_1000) {
> 
> wilc1000 should likely remain the default/fallback ?

I am now validating whether the hardware is either wilc1000 or wilc3000 
up front based on the chip ID early in init, so no other option can 
occur here, so there is no need for fallback, it is either wilc1000 or 
wilc3000 now (*). I think keeping them ordered alphanumerically is the 
nicer option.

> [...]
> 
>> @@ -1232,10 +1234,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)) {
>> -		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
>> -		return -ENODEV;
>> -	}
>> +
> 
> Instead of dropping any filtering (and then making the function name become
> irrelevant), why not ensuring that it is at least either a wilc1000 or a wilc3000 ?

Right, done.

[...]

>> +void chip_wakeup(struct wilc *wilc)
>> +{
>> +	if (wilc->chip == WILC_1000)
>> +		chip_wakeup_wilc1000(wilc);
>> +	else
>> +		chip_wakeup_wilc3000(wilc);
>> +}
>>   EXPORT_SYMBOL_GPL(chip_wakeup);
> 
> This new support makes a few places in wlan.c, netdev.c and in bus files
> (sdio.c, spi.c) install (sometimes big) branches on the device type (chip init,
> sleep, wakeup, read interrupt, clear interrupt, txq handling, etc), because the
> registers are different, the masks are different, the number of involved
> registers may not be the same, wilc3000 may need more operations to perform the
> same thing... I feel like it will make it harder in the long run to maintain the
> driver, especially if some new variants are added later.

I agree the code is ugly. Looking at the roadmap, it seems the next 
thing is WILCS02 which has its own driver, and for the WILC1000/3000 
inherited from atmel this seems to be the end of the road.

> Those branches tend to
> show that some operations in those files are too specific to the targeted
> device. I was examining the possibility to start creating device-type specific
> files (wilc1000.c, wilc3000.c) and move those operations as "device-specific"
> ops. Then wlan/netdev would call those chip-specific ops, which in turn may call
> the hif_func ops. It may need some rework in the bus files to fit this new
> hierarchy, but it may allow to keep netdev and wlan unaware of the device type,
> and since wilc3000 has bluetooth, it may also make it easier to introduce the
> corresponding support later. What do you think about it ? Ajay, any opinion on
> this ?

I did something like that for KSZ8851, that had bus-specific ops. I 
vaguely recall there was feedback that the function pointer indirection 
had non-trivial overhead due to spectre mitigations, and in case of the 
handle_txq() here, the chip specific ops would be called in a while() {} 
loop.

I can imagine some of the long functions like wilc_sdio_clear_int_ext or 
the handle_txq could be split up a bit, but likely only by factoring out 
some of the code into static functions. But looking at this closer, both 
pieces which are wilc1000/3000 specific in those functions manipulate 
with variables which would have to be passed in into that factored out 
code as function arguments, so I am not sure if this would improve 
readability by much either.

[...]

Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

[Ajay.Kathat]

On 8/22/24 19:46, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 8/22/24 2:10 PM, Alexis Lothoré wrote:
>> Hello Marek,
> 
> Hi,
> 
>> I was coincidentally working on adding wilc3000 support upstream too.
> 
> I hope you weren't too far along with that and I didn't waste too much
> of your time/effort here.
> 
>> My work is
>> also based on downstream tree, so my comments will likely reflect the reworks I
>> was doing or intended to do.
>> For the record, I have some wilc1000 and wilc3000 modules, in both  sdio and
>> spi
>> setups.
> 
> Nice, I only have this WILC3000 SDIO device .
> 
>> On 8/21/24 20:42, Marek Vasut wrote:
>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>
>> [...]
>>
>>>      if (!resume) {
>>> -            ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
>>> -            if (ret) {
>>> -                    dev_err(&func->dev, "Fail cmd read chip id...\n");
>>> +            chipid = wilc_get_chipid(wilc, true);
>>> +            if (is_wilc3000(chipid)) {
>>> +                    wilc->chip = WILC_3000;
>>> +            } else if (is_wilc1000(chipid)) {
>>> +                    wilc->chip = WILC_1000;
>>> +            } else {
>>> +                    dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>>>                      return ret;
>>>              }
>>
>> I wonder if this additional enum (enum wilc_chip_type)  is really useful. We
>> already store the raw chipid, which just needs to be masked to know about the
>> device type. We should likely store one or the other but not both, otherwise we
>> may just risk to create desync without really saving useful info.
>>
>> Also, this change makes wilc1000-sdio failing to build as module (missing
>> symbol
>> export on wilc_get_chipid)
> 
> I think I have a separate patch for this, one which folds
> wilc_get_chipid() entirely into wlan.c , and then follow up which uses
> is_wilc1000() / is_wilc3000() all over the place to discern the two MACs
> based on cached chip ID . That should work, I'll test it and submit it
> later today I hope.
> 
>> [...]
>>
>>> -    /* 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 (wilc->chip == WILC_1000) {
>>
>> wilc1000 should likely remain the default/fallback ?
> 
> I am now validating whether the hardware is either wilc1000 or wilc3000
> up front based on the chip ID early in init, so no other option can
> occur here, so there is no need for fallback, it is either wilc1000 or
> wilc3000 now (*). I think keeping them ordered alphanumerically is the
> nicer option.
> 
>> [...]
>>
>>> @@ -1232,10 +1234,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)) {
>>> -            dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
>>> -            return -ENODEV;
>>> -    }
>>> +
>>
>> Instead of dropping any filtering (and then making the function name become
>> irrelevant), why not ensuring that it is at least either a wilc1000 or a
>> wilc3000 ?
> 
> Right, done.
> 
> [...]
> 
>>> +void chip_wakeup(struct wilc *wilc)
>>> +{
>>> +    if (wilc->chip == WILC_1000)
>>> +            chip_wakeup_wilc1000(wilc);
>>> +    else
>>> +            chip_wakeup_wilc3000(wilc);
>>> +}
>>>   EXPORT_SYMBOL_GPL(chip_wakeup);
>>
>> This new support makes a few places in wlan.c, netdev.c and in bus files
>> (sdio.c, spi.c) install (sometimes big) branches on the device type (chip init,
>> sleep, wakeup, read interrupt, clear interrupt, txq handling, etc), because the
>> registers are different, the masks are different, the number of involved
>> registers may not be the same, wilc3000 may need more operations to perform the
>> same thing... I feel like it will make it harder in the long run to maintain
>> the
>> driver, especially if some new variants are added later.
> 
> I agree the code is ugly. Looking at the roadmap, it seems the next
> thing is WILCS02 which has its own driver, and for the WILC1000/3000
> inherited from atmel this seems to be the end of the road

yes, WILCS02 is the next-generation product of WILC family. It supports both
SDIO and SPI(mmc-over-spi) and the driver is same as  WILC1000/3000. The same
sdio driver will be used for mmc-over-spi interface. Similar to wilc3000, the
wilcs02 have different register sets and minor difference in firmware start
and txq_handling flow. The single driver will support all WILC variants.

> 
>> Those branches tend to
>> show that some operations in those files are too specific to the targeted
>> device. I was examining the possibility to start creating device-type specific
>> files (wilc1000.c, wilc3000.c) and move those operations as "device-specific"
>> ops. Then wlan/netdev would call those chip-specific ops, which in turn may
>> call
>> the hif_func ops. It may need some rework in the bus files to fit this new
>> hierarchy, but it may allow to keep netdev and wlan unaware of the device type,
>> and since wilc3000 has bluetooth, it may also make it easier to introduce the
>> corresponding support later. What do you think about it ? Ajay, any opinion on
>> this ?
> 
> I did something like that for KSZ8851, that had bus-specific ops. I
> vaguely recall there was feedback that the function pointer indirection
> had non-trivial overhead due to spectre mitigations, and in case of the
> handle_txq() here, the chip specific ops would be called in a while() {}
> loop.
> 
> I can imagine some of the long functions like wilc_sdio_clear_int_ext or
> the handle_txq could be split up a bit, but likely only by factoring out
> some of the code into static functions. But looking at this closer, both
> pieces which are wilc1000/3000 specific in those functions manipulate
> with variables which would have to be passed in into that factored out
> code as function arguments, so I am not sure if this would improve
> readability by much either.
> 

I also think adding wilc1000/wilc3000 specific may not improve much but some
of the large functions can be refactored if it improves the readability. For
most part, wilc1000 and wilc3000 has similar code but it mainly differs in
txq_handle handling, which may not be improved even after separating the code
to wilc1000 and wilc3000 specific files.

Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

[kernel test robot]

Hi Marek,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main net-next/main net/main linus/master v6.11-rc4 next-20240823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/wifi-wilc1000-Add-WILC3000-support/20240822-024553
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20240821184356.163816-2-marex%40denx.de
patch subject: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support
config: i386-randconfig-054-20240823 (https://download.01.org/0day-ci/archive/20240824/202408240116.5VwA6jhI-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240824/202408240116.5VwA6jhI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408240116.5VwA6jhI-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_powersave.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
>> ERROR: modpost: "wilc_get_chipid" [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
ERROR: modpost: "usb_alloc_urb" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_free_urb" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_anchor_urb" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_submit_urb" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_unanchor_urb" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_kill_anchored_urbs" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_control_msg" [drivers/bluetooth/btmtk.ko] undefined!
ERROR: modpost: "usb_set_interface" [drivers/bluetooth/btmtk.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Krzysztof Kozlowski]

On Wed, Aug 21, 2024 at 08:42:32PM +0200, 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
> ---
>  .../devicetree/bindings/net/wireless/microchip,wilc1000.yaml  | 4 +++-
>  1 file changed, 3 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..1bf3496c21e64 100644
> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
> @@ -16,7 +16,9 @@ description:
>  
>  properties:
>    compatible:
> -    const: microchip,wilc1000
> +    enum:
> +      - microchip,wilc1000
> +      - microchip,wilc3000

Your driver change suggests device type is fully auto-detectable, thus
they are compatible.

Best regards,
Krzysztof

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

[Marek Vasut]

On 8/22/24 10:07 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..1bf3496c21e64 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> @@ -16,7 +16,9 @@ description:
>>   
>>   properties:
>>     compatible:
>> -    const: microchip,wilc1000
>> +    enum:
>> +      - microchip,wilc1000
>> +      - microchip,wilc3000
> 
> Your driver change suggests device type is fully auto-detectable

It seems that way, yes.

>, thus
> they are compatible.

I _think_ the hardware is internally somewhat different, the WILC1000 is 
WiFi-only device and the WILC3000 has some extra Bluetooth part (which 
is currently not supported).

Maybe a fallback compatible string would be better here ?

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

[Krzysztof Kozlowski]

On 23/08/2024 03:38, Marek Vasut wrote:
> On 8/22/24 10:07 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..1bf3496c21e64 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>>> @@ -16,7 +16,9 @@ description:
>>>   
>>>   properties:
>>>     compatible:
>>> -    const: microchip,wilc1000
>>> +    enum:
>>> +      - microchip,wilc1000
>>> +      - microchip,wilc3000
>>
>> Your driver change suggests device type is fully auto-detectable
> 
> It seems that way, yes.
> 
>> , thus
>> they are compatible.
> 
> I _think_ the hardware is internally somewhat different, the WILC1000 is 
> WiFi-only device and the WILC3000 has some extra Bluetooth part (which 
> is currently not supported).
> 
> Maybe a fallback compatible string would be better here ?

Yes, that's what I meant by compatibility. 3000 followed by 1000.

Best regards,
Krzysztof