[PATCH v11 0/4] Add AP6275P wireless support

[PATCH v11 0/4] Add AP6275P wireless support

[Jacobe Zang]

These add AP6275P wireless support on Khadas Edge2. Enable 32k clock
for Wi-Fi module and extend the hardware IDs table in the brcmfmac
driver for it to attach.

Changes in v11:
 - Retain interrupt check in of.c
 - Split DTS and submit separately 

 - Link to v10: https://lore.kernel.org/all/20240813082007.2625841-1-jacobe.zang@wesion.com/

Changes in v10:
 - Use ret instead unused probe_attach_result in sdio.c 

 - Link to v9: https://lore.kernel.org/all/20240810035141.439024-1-jacobe.zang@wesion.com/

Changes in v9:
 - Add return -ENODEV error pointer from brcmf_sdio_probe as the default for the fail path
 - Add if statement for brcmf_of_probe in common.c
 - Retain modifications to of.c other than the return values

 - Link to v8: https://lore.kernel.org/all/20240805073425.3492078-1-jacobe.zang@wesion.com/

Changes in v8:
 - Add appropriate errno's for return values that will be
    send to bus when error occurred.
 
 - Link to v7: https://lore.kernel.org/all/20240802025715.2360456-1-jacobe.zang@wesion.com/

Changes in v7:
 - Change brcmf_of_probe prototypes from void to int, add appropriate errno's for return
    value, move clock check to the end of brcmf_of_probe
 - Add "brcm,bcm4329-fmac" compatible for wifi node

 - Link to v6: https://lore.kernel.org/all/20240731061132.703368-1-jacobe.zang@wesion.com/

Changes in v6:
 - Move "brcm,bcm4329-fmac" check to the top of brcmf_of_probe in of.c
 - Add return if clk didn't set in DTS

 -Link to v5: https://lore.kernel.org/all/20240730033053.4092132-1-jacobe.zang@wesion.com/

Changes in v5:
 - Add more commit message to the clock in bindings
 - Use IS_ERR_OR_NULL as a judgment condition of clk

 - Link to v4: https://lore.kernel.org/all/20240729070102.3770318-1-jacobe.zang@wesion.com/

Changes in v4:
 - Change clock description in dt-bindings
 - Move enable clk from pcie.c to of.c
 - Add compatible for wifi node in DTS
 - Add random seed flag for firmware download

 - Link to v3: https://lore.kernel.org/all/20240630073605.2164346-1-jacobe.zang@wesion.com/

Changes in v3:
 - Dropped redundant parts in dt-bindings.
 - Change driver patch title prefix as 'wifi: brcmfmac:'.
 - Change DTS Wi-Fi node clock-name as 'lpo'.
 
 - Link to v2: https://lore.kernel.org/all/20240624081906.1399447-1-jacobe.zang@wesion.com/

Changes in v2:
 - Add SoB tags for original developer.
 - Add dt-bindings for pci14e4,449d and clocks.
 - Replace dev_info to brcmf_dbg in pcie.c

 - Link to v1: https://lore.kernel.org/all/20240620020015.4021696-1-jacobe.zang@wesion.com/

Jacobe Zang (4):
  dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d
  dt-bindings: net: wireless: brcm4329-fmac: add clock description for
    AP6275P
  wifi: brcmfmac: Add optional lpo clock enable support
  wifi: brcmfmac: add flag for random seed during firmware download

 .../net/wireless/brcm,bcm4329-fmac.yaml       |  9 +++
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +-
 .../broadcom/brcm80211/brcmfmac/common.c      |  3 +-
 .../wireless/broadcom/brcm80211/brcmfmac/of.c | 29 +++++++---
 .../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 +--
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 55 ++++++++++++++++---
 .../broadcom/brcm80211/brcmfmac/sdio.c        | 22 +++++---
 .../broadcom/brcm80211/brcmfmac/usb.c         |  3 +
 .../broadcom/brcm80211/include/brcm_hw_ids.h  |  2 +
 9 files changed, 104 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v11 1/4] dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d

[Jacobe Zang]

It's the device id used by AP6275P which is the Wi-Fi module
used by Rockchip's RK3588 evaluation board and also used in
some other RK3588 boards.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml      | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
index e564f20d8f415..2c2093c77ec9a 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
@@ -53,6 +53,7 @@ properties:
           - pci14e4,4488  # BCM4377
           - pci14e4,4425  # BCM4378
           - pci14e4,4433  # BCM4387
+          - pci14e4,449d  # BCM43752
 
   reg:
     description: SDIO function number for the device (for most cases
-- 
2.34.1

Re: [PATCH v11 1/4] dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d

[Kalle Valo]

Jacobe Zang <jacobe.zang@wesion.com> wrote:

> It's the device id used by AP6275P which is the Wi-Fi module
> used by Rockchip's RK3588 evaluation board and also used in
> some other RK3588 boards.
> 
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>

Fails to apply, please rebase on top of wireless-next.

Recorded preimage for 'drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c'
Recorded preimage for 'drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c'
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: wifi: brcmfmac: Add optional lpo clock enable support
Using index info to reconstruct a base tree...
M	drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
M	drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
M	drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
M	drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
M	drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
Auto-merging drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
CONFLICT (content): Merge conflict in drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
Auto-merging drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
Auto-merging drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
CONFLICT (content): Merge conflict in drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
Auto-merging drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
Patch failed at 0001 wifi: brcmfmac: Add optional lpo clock enable support

4 patches set to Changes Requested.

13765357 [v11,1/4] dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d
13765358 [v11,2/4] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P
13765359 [v11,3/4] wifi: brcmfmac: Add optional lpo clock enable support
13765360 [v11,4/4] wifi: brcmfmac: add flag for random seed during firmware download

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240816020635.1273911-2-jacobe.zang@wesion.com/

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

[PATCH v11 2/4] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P

[Jacobe Zang]

Not only AP6275P Wi-Fi device but also all Broadcom wireless devices allow
external low power clock input. In DTS the clock as an optional choice in
the absence of an internal clock.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../bindings/net/wireless/brcm,bcm4329-fmac.yaml          | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
index 2c2093c77ec9a..a3607d55ef367 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
@@ -122,6 +122,14 @@ properties:
       NVRAM. This would normally be filled in by the bootloader from platform
       configuration data.
 
+  clocks:
+    items:
+      - description: External Low Power Clock input (32.768KHz)
+
+  clock-names:
+    items:
+      - const: lpo
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH v11 3/4] wifi: brcmfmac: Add optional lpo clock enable support

[Jacobe Zang]

WiFi modules often require 32kHz clock to function. Add support to
enable the clock to PCIe driver and move "brcm,bcm4329-fmac" check
to the top of brcmf_of_probe. Change function prototypes from void
to int and add appropriate errno's for return values that will be
send to bus when error occurred.

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Reviewed-by: Sai Krishna <saikrishnag@marvell.com>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +--
 .../broadcom/brcm80211/brcmfmac/common.c      |  3 +-
 .../wireless/broadcom/brcm80211/brcmfmac/of.c | 29 +++++++++++++------
 .../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 +++---
 .../broadcom/brcm80211/brcmfmac/pcie.c        |  3 ++
 .../broadcom/brcm80211/brcmfmac/sdio.c        | 22 +++++++++-----
 .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
 7 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 13391c2d82aae..b2ede4e579c5c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -947,8 +947,8 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
 
 	/* try to attach to the target device */
 	sdiodev->bus = brcmf_sdio_probe(sdiodev);
-	if (!sdiodev->bus) {
-		ret = -ENODEV;
+	if (IS_ERR(sdiodev->bus)) {
+		ret = PTR_ERR(sdiodev->bus);
 		goto out;
 	}
 	brcmf_sdiod_host_fixup(sdiodev->func2->card->host);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..58d50918dd177 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -561,7 +561,8 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev,
 	if (!found) {
 		/* No platform data for this device, try OF and DMI data */
 		brcmf_dmi_probe(settings, chip, chiprev);
-		brcmf_of_probe(dev, bus_type, settings);
+		if (brcmf_of_probe(dev, bus_type, settings) == -EPROBE_DEFER)
+			return ERR_PTR(-EPROBE_DEFER);
 		brcmf_acpi_probe(dev, bus_type, settings);
 	}
 	return settings;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index e406e11481a62..5a66d64919209 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
@@ -6,6 +6,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_net.h>
+#include <linux/clk.h>
 
 #include <defs.h>
 #include "debug.h"
@@ -65,17 +66,21 @@ static int brcmf_of_get_country_codes(struct device *dev,
 	return 0;
 }
 
-void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
-		    struct brcmf_mp_device *settings)
+int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
+		   struct brcmf_mp_device *settings)
 {
 	struct brcmfmac_sdio_pd *sdio = &settings->bus.sdio;
 	struct device_node *root, *np = dev->of_node;
+	struct clk *clk;
 	const char *prop;
 	int irq;
 	int err;
 	u32 irqf;
 	u32 val;
 
+	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
+		return 0;
+
 	/* Apple ARM64 platforms have their own idea of board type, passed in
 	 * via the device tree. They also have an antenna SKU parameter
 	 */
@@ -105,7 +110,7 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
 		if (!board_type) {
 			of_node_put(root);
-			return;
+			return 0;
 		}
 		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
@@ -113,33 +118,39 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 		of_node_put(root);
 	}
 
-	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
-		return;
-
 	err = brcmf_of_get_country_codes(dev, settings);
 	if (err)
 		brcmf_err("failed to get OF country code map (err=%d)\n", err);
 
 	of_get_mac_address(np, settings->mac);
 
+	clk = devm_clk_get_optional_enabled(dev, "lpo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	brcmf_dbg(INFO, "%s LPO clock\n", clk ? "enable" : "no");
+	clk_set_rate(clk, 32768);
+
 	if (bus_type != BRCMF_BUSTYPE_SDIO)
-		return;
+		return 0;
 
 	if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
 		sdio->drive_strength = val;
 
 	/* make sure there are interrupts defined in the node */
 	if (!of_property_present(np, "interrupts"))
-		return;
+		return 0;
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
 		brcmf_err("interrupt could not be mapped\n");
-		return;
+		return 0;
 	}
 	irqf = irqd_get_trigger_type(irq_get_irq_data(irq));
 
 	sdio->oob_irq_supported = true;
 	sdio->oob_irq_nr = irq;
 	sdio->oob_irq_flags = irqf;
+
+	return 0;
 }
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h
index 10bf52253337e..ae124c73fc3b7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h
@@ -3,11 +3,12 @@
  * Copyright (c) 2014 Broadcom Corporation
  */
 #ifdef CONFIG_OF
-void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
-		    struct brcmf_mp_device *settings);
+int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
+		   struct brcmf_mp_device *settings);
 #else
-static void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
-			   struct brcmf_mp_device *settings)
+static int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
+			  struct brcmf_mp_device *settings)
 {
+	return 0;
 }
 #endif /* CONFIG_OF */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 06698a714b523..c34405a6d38b8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -2457,6 +2457,9 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		ret = -ENOMEM;
 		goto fail;
 	}
+	ret = PTR_ERR_OR_ZERO(devinfo->settings);
+	if (ret < 0)
+		goto fail;
 
 	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
 	if (!bus) {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 6b38d9de71af6..462fc669b959c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3943,7 +3943,7 @@ static const struct brcmf_buscore_ops brcmf_sdio_buscore_ops = {
 	.write32 = brcmf_sdio_buscore_write32,
 };
 
-static bool
+static int
 brcmf_sdio_probe_attach(struct brcmf_sdio *bus)
 {
 	struct brcmf_sdio_dev *sdiodev;
@@ -3953,6 +3953,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus)
 	u32 reg_val;
 	u32 drivestrength;
 	u32 enum_base;
+	int ret = -EBADE;
 
 	sdiodev = bus->sdiodev;
 	sdio_claim_host(sdiodev->func1);
@@ -4001,8 +4002,9 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus)
 						   BRCMF_BUSTYPE_SDIO,
 						   bus->ci->chip,
 						   bus->ci->chiprev);
-	if (!sdiodev->settings) {
+	if (IS_ERR_OR_NULL(sdiodev->settings)) {
 		brcmf_err("Failed to get device parameters\n");
+		ret = PTR_ERR_OR_ZERO(sdiodev->settings);
 		goto fail;
 	}
 	/* platform specific configuration:
@@ -4071,7 +4073,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus)
 	/* allocate header buffer */
 	bus->hdrbuf = kzalloc(MAX_HDR_READ + bus->head_align, GFP_KERNEL);
 	if (!bus->hdrbuf)
-		return false;
+		return -ENOMEM;
 	/* Locate an appropriately-aligned portion of hdrbuf */
 	bus->rxhdr = (u8 *) roundup((unsigned long)&bus->hdrbuf[0],
 				    bus->head_align);
@@ -4082,11 +4084,11 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus)
 	if (bus->poll)
 		bus->pollrate = 1;
 
-	return true;
+	return 0;
 
 fail:
 	sdio_release_host(sdiodev->func1);
-	return false;
+	return ret;
 }
 
 static int
@@ -4451,8 +4453,10 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 
 	/* Allocate private bus interface state */
 	bus = kzalloc(sizeof(struct brcmf_sdio), GFP_ATOMIC);
-	if (!bus)
+	if (!bus) {
+		ret = -ENOMEM;
 		goto fail;
+	}
 
 	bus->sdiodev = sdiodev;
 	sdiodev->bus = bus;
@@ -4467,6 +4471,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 				     dev_name(&sdiodev->func1->dev));
 	if (!wq) {
 		brcmf_err("insufficient memory to create txworkqueue\n");
+		ret = -ENOMEM;
 		goto fail;
 	}
 	brcmf_sdiod_freezer_count(sdiodev);
@@ -4474,7 +4479,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	bus->brcmf_wq = wq;
 
 	/* attempt to attach to the dongle */
-	if (!(brcmf_sdio_probe_attach(bus))) {
+	ret = brcmf_sdio_probe_attach(bus);
+	if (ret < 0) {
 		brcmf_err("brcmf_sdio_probe_attach failed\n");
 		goto fail;
 	}
@@ -4546,7 +4552,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 
 fail:
 	brcmf_sdio_remove(bus);
-	return NULL;
+	return ERR_PTR(ret);
 }
 
 /* Detach and free everything */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 9a105e6debe1f..f7db46ae44906 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1272,6 +1272,9 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo,
 		ret = -ENOMEM;
 		goto fail;
 	}
+	ret = PTR_ERR_OR_ZERO(devinfo->settings);
+	if (ret < 0)
+		goto fail;
 
 	if (!brcmf_usb_dlneeded(devinfo)) {
 		ret = brcmf_alloc(devinfo->dev, devinfo->settings);
-- 
2.34.1

Re: [PATCH v11 3/4] wifi: brcmfmac: Add optional lpo clock enable support

[Arend van Spriel]

On 8/16/2024 4:06 AM, Jacobe Zang wrote:
> WiFi modules often require 32kHz clock to function. Add support to
> enable the clock to PCIe driver and move "brcm,bcm4329-fmac" check
> to the top of brcmf_of_probe. Change function prototypes from void
> to int and add appropriate errno's for return values that will be
> send to bus when error occurred.

Looks good to me. Not sure if I should Ack this as my SoB is there as 
well. If that is not a problem than here it is:

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Reviewed-by: Sai Krishna <saikrishnag@marvell.com>
> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> ---
>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +--
>   .../broadcom/brcm80211/brcmfmac/common.c      |  3 +-
>   .../wireless/broadcom/brcm80211/brcmfmac/of.c | 29 +++++++++++++------
>   .../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 +++---
>   .../broadcom/brcm80211/brcmfmac/pcie.c        |  3 ++
>   .../broadcom/brcm80211/brcmfmac/sdio.c        | 22 +++++++++-----
>   .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
>   7 files changed, 49 insertions(+), 24 deletions(-)

[PATCH v11 4/4] wifi: brcmfmac: add flag for random seed during firmware download

[Jacobe Zang]

Providing the random seed to firmware was tied to the fact that the
device has a valid OTP, which worked for some Apple chips. However,
it turns out the BCM43752 device also needs the random seed in order
to get firmware running. Suspect it is simply tied to the firmware
branch used for the device. Introducing a mechanism to allow setting
it for a device through the device table.

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 52 ++++++++++++++++---
 .../broadcom/brcm80211/include/brcm_hw_ids.h  |  2 +
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index c34405a6d38b8..e88fa4cd62a1d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -66,6 +66,7 @@ BRCMF_FW_DEF(4365C, "brcmfmac4365c-pcie");
 BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie");
 BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie");
 BRCMF_FW_DEF(4371, "brcmfmac4371-pcie");
+BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-pcie");
 BRCMF_FW_CLM_DEF(4377B3, "brcmfmac4377b3-pcie");
 BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie");
 BRCMF_FW_CLM_DEF(4378B3, "brcmfmac4378b3-pcie");
@@ -104,6 +105,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = {
 	BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C),
 	BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C),
 	BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371),
+	BRCMF_FW_ENTRY(BRCM_CC_43752_CHIP_ID, 0xFFFFFFFF, 43752),
 	BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* revision ID 4 */
 	BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0x0000000F, 4378B1), /* revision ID 3 */
 	BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFE0, 4378B3), /* revision ID 5 */
@@ -358,6 +360,7 @@ struct brcmf_pciedev_info {
 			  u16 value);
 	struct brcmf_mp_device *settings;
 	struct brcmf_otp_params otp;
+	bool fwseed;
 #ifdef DEBUG
 	u32 console_interval;
 	bool console_active;
@@ -1720,14 +1723,14 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
 		memcpy_toio(devinfo->tcm + address, nvram, nvram_len);
 		brcmf_fw_nvram_free(nvram);
 
-		if (devinfo->otp.valid) {
+		if (devinfo->fwseed) {
 			size_t rand_len = BRCMF_RANDOM_SEED_LENGTH;
 			struct brcmf_random_seed_footer footer = {
 				.length = cpu_to_le32(rand_len),
 				.magic = cpu_to_le32(BRCMF_RANDOM_SEED_MAGIC),
 			};
 
-			/* Some Apple chips/firmwares expect a buffer of random
+			/* Some chips/firmwares expect a buffer of random
 			 * data to be present before NVRAM
 			 */
 			brcmf_dbg(PCIE, "Download random seed\n");
@@ -2399,6 +2402,37 @@ static void brcmf_pcie_debugfs_create(struct device *dev)
 }
 #endif
 
+struct brcmf_pcie_drvdata {
+	enum brcmf_fwvendor vendor;
+	bool fw_seed;
+};
+
+enum {
+	BRCMF_DRVDATA_CYW,
+	BRCMF_DRVDATA_BCA,
+	BRCMF_DRVDATA_WCC,
+	BRCMF_DRVDATA_WCC_SEED,
+};
+
+static const struct brcmf_pcie_drvdata drvdata[] = {
+	[BRCMF_DRVDATA_CYW] = {
+		.vendor = BRCMF_FWVENDOR_CYW,
+		.fw_seed = false,
+	},
+	[BRCMF_DRVDATA_BCA] = {
+		.vendor = BRCMF_FWVENDOR_BCA,
+		.fw_seed = false,
+	},
+	[BRCMF_DRVDATA_WCC] = {
+		.vendor = BRCMF_FWVENDOR_WCC,
+		.fw_seed = false,
+	},
+	[BRCMF_DRVDATA_WCC_SEED] = {
+		.vendor = BRCMF_FWVENDOR_WCC,
+		.fw_seed = true,
+	},
+};
+
 /* Forward declaration for pci_match_id() call */
 static const struct pci_device_id brcmf_pcie_devid_table[];
 
@@ -2480,9 +2514,10 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	bus->bus_priv.pcie = pcie_bus_dev;
 	bus->ops = &brcmf_pcie_bus_ops;
 	bus->proto_type = BRCMF_PROTO_MSGBUF;
-	bus->fwvid = id->driver_data;
 	bus->chip = devinfo->coreid;
 	bus->wowl_supported = pci_pme_capable(pdev, PCI_D3hot);
+	bus->fwvid = drvdata[id->driver_data].vendor;
+	devinfo->fwseed = drvdata[id->driver_data].fw_seed;
 	dev_set_drvdata(&pdev->dev, bus);
 
 	ret = brcmf_alloc(&devinfo->pdev->dev, devinfo->settings);
@@ -2668,14 +2703,14 @@ static const struct dev_pm_ops brcmf_pciedrvr_pm = {
 		BRCM_PCIE_VENDOR_ID_BROADCOM, (dev_id), \
 		PCI_ANY_ID, PCI_ANY_ID, \
 		PCI_CLASS_NETWORK_OTHER << 8, 0xffff00, \
-		BRCMF_FWVENDOR_ ## fw_vend \
+		BRCMF_DRVDATA_ ## fw_vend \
 	}
 #define BRCMF_PCIE_DEVICE_SUB(dev_id, subvend, subdev, fw_vend) \
 	{ \
 		BRCM_PCIE_VENDOR_ID_BROADCOM, (dev_id), \
 		(subvend), (subdev), \
 		PCI_CLASS_NETWORK_OTHER << 8, 0xffff00, \
-		BRCMF_FWVENDOR_ ## fw_vend \
+		BRCMF_DRVDATA_ ## fw_vend \
 	}
 
 static const struct pci_device_id brcmf_pcie_devid_table[] = {
@@ -2703,9 +2738,10 @@ static const struct pci_device_id brcmf_pcie_devid_table[] = {
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW),
-	BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC),
-	BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC),
-	BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_DEVICE_ID, WCC),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC_SEED),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC_SEED),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_DEVICE_ID, WCC_SEED),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC_SEED),
 
 	{ /* end: all zeroes */ }
 };
diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
index 44684bf1b9acc..c1e22c589d85e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
+++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
@@ -52,6 +52,7 @@
 #define BRCM_CC_43664_CHIP_ID		43664
 #define BRCM_CC_43666_CHIP_ID		43666
 #define BRCM_CC_4371_CHIP_ID		0x4371
+#define BRCM_CC_43752_CHIP_ID		43752
 #define BRCM_CC_4377_CHIP_ID		0x4377
 #define BRCM_CC_4378_CHIP_ID		0x4378
 #define BRCM_CC_4387_CHIP_ID		0x4387
@@ -94,6 +95,7 @@
 #define BRCM_PCIE_4366_5G_DEVICE_ID	0x43c5
 #define BRCM_PCIE_4371_DEVICE_ID	0x440d
 #define BRCM_PCIE_43596_DEVICE_ID	0x4415
+#define BRCM_PCIE_43752_DEVICE_ID	0x449d
 #define BRCM_PCIE_4377_DEVICE_ID	0x4488
 #define BRCM_PCIE_4378_DEVICE_ID	0x4425
 #define BRCM_PCIE_4387_DEVICE_ID	0x4433
-- 
2.34.1

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Hi,

I tested this on RK3588 EVB1 and the driver is working fine. The DT
bindings are not correct, though:

linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
compatible: 'oneOf' conditional failed, one must be fixed:

['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#

It's easy to see the problem in the binding. It does not expect a
fallback string after the PCI ID based compatible. Either the
pci14e4,449d entry must be added to the first enum in the binding,
which has the fallback compatible, or the fallback compatible
should not be added to DTS.

If the fallback compatible is missing in DTS, the compatible check in
brcmf_of_probe() fails and the lpo clock is not requested resulting
in the firmware startup failing. So that would require further
driver changes.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Arend van Spriel]

On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
> Hi,
> 
> I tested this on RK3588 EVB1 and the driver is working fine. The DT
> bindings are not correct, though:
> 
> linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
> compatible: 'oneOf' conditional failed, one must be fixed:
> 
> ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
> 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
> 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
> 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
> 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
> 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
> 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
> 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
> 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
> from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#
> 
> It's easy to see the problem in the binding. It does not expect a
> fallback string after the PCI ID based compatible. Either the
> pci14e4,449d entry must be added to the first enum in the binding,
> which has the fallback compatible, or the fallback compatible
> should not be added to DTS.

Never understood why we ended up with such a large list. When the 
binding was introduced there was one compatible, ie. brcm,bcm4329-fmac. 
People wanted all the other flavors because it described a specific wifi 
chip and no other reason whatsoever. The PCI ID based compatible do 
obfuscate that info so those are even less useful in my opinion.

> If the fallback compatible is missing in DTS, the compatible check in
> brcmf_of_probe() fails and the lpo clock is not requested resulting
> in the firmware startup failing. So that would require further
> driver changes.

Right. The text based bindings file in 5.12 kernel clearly says:

Required properties:

  - compatible : Should be "brcm,bcm4329-fmac".

In 5.13 kernel this was replaced by the json-schema yaml file. The PCI 
ID based enum which was added later does also list brcm,bcm4329-fmac so 
why does that not work for the compatible list ['pci14e4,449d', 
'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I 
stripped a bit for brevity:

properties:
   compatible:
     oneOf:
       - items:
           - enum:
               - brcm,bcm43143-fmac
               - brcm,bcm4329-fmac
               - infineon,cyw43439-fmac
           - const: brcm,bcm4329-fmac
       - enum:
           - brcm,bcm4329-fmac
           - pci14e4,43dc  # BCM4355
           - pci14e4,4464  # BCM4364
           - pci14e4,4488  # BCM4377
           - pci14e4,4425  # BCM4378
           - pci14e4,4433  # BCM4387

So how should I read this. Searching for some sort of syntax description 
I found [1] which has an example schema with description that has a 
similarly complicated compatible property. From that I think the above 
should be changed to:

  properties:
    compatible:
      oneOf:
        - items:
            - enum:
                - brcm,bcm43143-fmac
-              - brcm,bcm4329-fmac
                - infineon,cyw43439-fmac
            - const: brcm,bcm4329-fmac
+      - items:
            - enum:
-              - brcm,bcm4329-fmac
                - pci14e4,43dc  # BCM4355
                - pci14e4,4464  # BCM4364
                - pci14e4,4488  # BCM4377
                - pci14e4,4425  # BCM4378
                - pci14e4,4433  # BCM4387
+          - const: brcm,bcm4329-fmac
+      - const: brcm,bcm4329-fmac

This poses a constraint in which the last string in the compatible list 
is always 'brcm,bcm4329-fmac' even if it is the only string. At least 
that is my understanding so if my understanding is wrong feel free to 
correct me on this.

[1] https://docs.kernel.org/devicetree/bindings/writing-schema.html
> Greetings,
> 
> -- Sebastian

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5222 bytes --]

Hello,

On Mon, Aug 19, 2024 at 09:35:12PM GMT, Arend van Spriel wrote:
> On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
> > I tested this on RK3588 EVB1 and the driver is working fine. The DT
> > bindings are not correct, though:
> > 
> > linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
> > compatible: 'oneOf' conditional failed, one must be fixed:
> > 
> > ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
> > 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
> > 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
> > 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
> > 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
> > 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
> > 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
> > 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
> > 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
> > from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#
> > 
> > It's easy to see the problem in the binding. It does not expect a
> > fallback string after the PCI ID based compatible. Either the
> > pci14e4,449d entry must be added to the first enum in the binding,
> > which has the fallback compatible, or the fallback compatible
> > should not be added to DTS.
> 
> Never understood why we ended up with such a large list. When the binding
> was introduced there was one compatible, ie. brcm,bcm4329-fmac. People
> wanted all the other flavors because it described a specific wifi chip and
> no other reason whatsoever. The PCI ID based compatible do obfuscate that
> info so those are even less useful in my opinion.
> 
> > If the fallback compatible is missing in DTS, the compatible check in
> > brcmf_of_probe() fails and the lpo clock is not requested resulting
> > in the firmware startup failing. So that would require further
> > driver changes.
> 
> Right. The text based bindings file in 5.12 kernel clearly says:
> 
> Required properties:
> 
>  - compatible : Should be "brcm,bcm4329-fmac".
> 
> In 5.13 kernel this was replaced by the json-schema yaml file. The PCI ID
> based enum which was added later does also list brcm,bcm4329-fmac so why
> does that not work for the compatible list ['pci14e4,449d',
> 'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I
> stripped a bit for brevity:
> 
> properties:
>   compatible:
>     oneOf:
>       - items:
>           - enum:
>               - brcm,bcm43143-fmac
>               - brcm,bcm4329-fmac
>               - infineon,cyw43439-fmac
>           - const: brcm,bcm4329-fmac
>       - enum:
>           - brcm,bcm4329-fmac
>           - pci14e4,43dc  # BCM4355
>           - pci14e4,4464  # BCM4364
>           - pci14e4,4488  # BCM4377
>           - pci14e4,4425  # BCM4378
>           - pci14e4,4433  # BCM4387
> 
> So how should I read this. Searching for some sort of syntax description I
> found [1] which has an example schema with description that has a similarly
> complicated compatible property. From that I think the above should be
> changed to:
> 
>  properties:
>    compatible:
>      oneOf:
>        - items:
>            - enum:
>                - brcm,bcm43143-fmac
> -              - brcm,bcm4329-fmac
>                - infineon,cyw43439-fmac
>            - const: brcm,bcm4329-fmac
> +      - items:
>            - enum:
> -              - brcm,bcm4329-fmac
>                - pci14e4,43dc  # BCM4355
>                - pci14e4,4464  # BCM4364
>                - pci14e4,4488  # BCM4377
>                - pci14e4,4425  # BCM4378
>                - pci14e4,4433  # BCM4387
> +          - const: brcm,bcm4329-fmac
> +      - const: brcm,bcm4329-fmac
> 
> This poses a constraint in which the last string in the compatible list is
> always 'brcm,bcm4329-fmac' even if it is the only string. At least that is
> my understanding so if my understanding is wrong feel free to correct me on
> this.
> 
> [1] https://docs.kernel.org/devicetree/bindings/writing-schema.html

Your proposed change should work as you describe. But it will result
in DT check errors for some Apple devices, which followed the
current binding and do not have the "brcm,bcm4329-fmac" fallback
compatible:

$ git grep -E "(pci14e4,43dc)|(pci14e4,4464)|(pci14e4,4488)|(pci14e4,4425)|(pci14e4,4433)" arch/
arch/arm64/boot/dts/apple/t8103-jxxx.dtsi:           compatible = "pci14e4,4425";
arch/arm64/boot/dts/apple/t8112-j413.dts:            compatible = "pci14e4,4433";
arch/arm64/boot/dts/apple/t8112-j493.dts:            compatible = "pci14e4,4425";

I guess patch 3/4 from this series will also introduce some
regressions for these devices by moving the check. What is the
purpose of the compatible check in brcmf_of_probe() in the first
place? Can it just be dropped?

I see it was introduced 10 years ago in 61f663dfc1a09, probably to
avoid a spurious error message for systems not having the IRQ
described in DT? The current code exits quietly when none of the
optional resources are defined.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Arend van Spriel]

On 8/19/2024 10:33 PM, Sebastian Reichel wrote:
> Hello,
> 
> On Mon, Aug 19, 2024 at 09:35:12PM GMT, Arend van Spriel wrote:
>> On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
>>> I tested this on RK3588 EVB1 and the driver is working fine. The DT
>>> bindings are not correct, though:
>>>
>>> linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
>>> compatible: 'oneOf' conditional failed, one must be fixed:
>>>
>>> ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
>>> 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
>>> 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
>>> 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
>>> 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
>>> 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
>>> 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
>>> 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
>>> 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
>>> from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#
>>>
>>> It's easy to see the problem in the binding. It does not expect a
>>> fallback string after the PCI ID based compatible. Either the
>>> pci14e4,449d entry must be added to the first enum in the binding,
>>> which has the fallback compatible, or the fallback compatible
>>> should not be added to DTS.
>>
>> Never understood why we ended up with such a large list. When the binding
>> was introduced there was one compatible, ie. brcm,bcm4329-fmac. People
>> wanted all the other flavors because it described a specific wifi chip and
>> no other reason whatsoever. The PCI ID based compatible do obfuscate that
>> info so those are even less useful in my opinion.
>>
>>> If the fallback compatible is missing in DTS, the compatible check in
>>> brcmf_of_probe() fails and the lpo clock is not requested resulting
>>> in the firmware startup failing. So that would require further
>>> driver changes.
>>
>> Right. The text based bindings file in 5.12 kernel clearly says:
>>
>> Required properties:
>>
>>   - compatible : Should be "brcm,bcm4329-fmac".
>>
>> In 5.13 kernel this was replaced by the json-schema yaml file. The PCI ID
>> based enum which was added later does also list brcm,bcm4329-fmac so why
>> does that not work for the compatible list ['pci14e4,449d',
>> 'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I
>> stripped a bit for brevity:
>>
>> properties:
>>    compatible:
>>      oneOf:
>>        - items:
>>            - enum:
>>                - brcm,bcm43143-fmac
>>                - brcm,bcm4329-fmac
>>                - infineon,cyw43439-fmac
>>            - const: brcm,bcm4329-fmac
>>        - enum:
>>            - brcm,bcm4329-fmac
>>            - pci14e4,43dc  # BCM4355
>>            - pci14e4,4464  # BCM4364
>>            - pci14e4,4488  # BCM4377
>>            - pci14e4,4425  # BCM4378
>>            - pci14e4,4433  # BCM4387
>>
>> So how should I read this. Searching for some sort of syntax description I
>> found [1] which has an example schema with description that has a similarly
>> complicated compatible property. From that I think the above should be
>> changed to:
>>
>>   properties:
>>     compatible:
>>       oneOf:
>>         - items:
>>             - enum:
>>                 - brcm,bcm43143-fmac
>> -              - brcm,bcm4329-fmac
>>                 - infineon,cyw43439-fmac
>>             - const: brcm,bcm4329-fmac
>> +      - items:
>>             - enum:
>> -              - brcm,bcm4329-fmac
>>                 - pci14e4,43dc  # BCM4355
>>                 - pci14e4,4464  # BCM4364
>>                 - pci14e4,4488  # BCM4377
>>                 - pci14e4,4425  # BCM4378
>>                 - pci14e4,4433  # BCM4387
>> +          - const: brcm,bcm4329-fmac
>> +      - const: brcm,bcm4329-fmac
>>
>> This poses a constraint in which the last string in the compatible list is
>> always 'brcm,bcm4329-fmac' even if it is the only string. At least that is
>> my understanding so if my understanding is wrong feel free to correct me on
>> this.
>>
>> [1] https://docs.kernel.org/devicetree/bindings/writing-schema.html
> 
> Your proposed change should work as you describe. But it will result
> in DT check errors for some Apple devices, which followed the
> current binding and do not have the "brcm,bcm4329-fmac" fallback
> compatible:
> 
> $ git grep -E "(pci14e4,43dc)|(pci14e4,4464)|(pci14e4,4488)|(pci14e4,4425)|(pci14e4,4433)" arch/
> arch/arm64/boot/dts/apple/t8103-jxxx.dtsi:           compatible = "pci14e4,4425";
> arch/arm64/boot/dts/apple/t8112-j413.dts:            compatible = "pci14e4,4433";
> arch/arm64/boot/dts/apple/t8112-j493.dts:            compatible = "pci14e4,4425";

> I guess patch 3/4 from this series will also introduce some
> regressions for these devices by moving the check. What is the
> purpose of the compatible check in brcmf_of_probe() in the first
> place? Can it just be dropped?
> 
> I see it was introduced 10 years ago in 61f663dfc1a09, probably to
> avoid a spurious error message for systems not having the IRQ
> described in DT? The current code exits quietly when none of the
> optional resources are defined.

It was introduced simply because the compatible property has a meaning 
that goes beyond informational. It is a claim that the properties of the 
node comply to the bindings specification. I would really want to keep 
the sanity check event though all properties are optional. The 
constraint keeps the compatible matching in the driver relatively simple.

Regards,
Arend

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Jacobe Zang]

On 2024/8/20 17:53, Arend van Spriel wrote:
> On 8/19/2024 10:33 PM, Sebastian Reichel wrote:
>> Hello,
>>
>> On Mon, Aug 19, 2024 at 09:35:12PM GMT, Arend van Spriel wrote:
>>> On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
>>>> I tested this on RK3588 EVB1 and the driver is working fine. The DT
>>>> bindings are not correct, though:
>>>>
>>>> linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
>>>> compatible: 'oneOf' conditional failed, one must be fixed:
>>>>
>>>> ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
>>>> 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0- 
>>>> fmac',
>>>> 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
>>>> 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
>>>> 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
>>>> 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
>>>> 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
>>>> 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
>>>> 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
>>>> from schema $id: http://devicetree.org/schemas/net/wireless/ 
>>>> brcm,bcm4329-fmac.yaml#
>>>>
>>>> It's easy to see the problem in the binding. It does not expect a
>>>> fallback string after the PCI ID based compatible. Either the
>>>> pci14e4,449d entry must be added to the first enum in the binding,
>>>> which has the fallback compatible, or the fallback compatible
>>>> should not be added to DTS.
>>>
>>> Never understood why we ended up with such a large list. When the 
>>> binding
>>> was introduced there was one compatible, ie. brcm,bcm4329-fmac. People
>>> wanted all the other flavors because it described a specific wifi 
>>> chip and
>>> no other reason whatsoever. The PCI ID based compatible do obfuscate 
>>> that
>>> info so those are even less useful in my opinion.
>>>
>>>> If the fallback compatible is missing in DTS, the compatible check in
>>>> brcmf_of_probe() fails and the lpo clock is not requested resulting
>>>> in the firmware startup failing. So that would require further
>>>> driver changes.
>>>
>>> Right. The text based bindings file in 5.12 kernel clearly says:
>>>
>>> Required properties:
>>>
>>>   - compatible : Should be "brcm,bcm4329-fmac".
>>>
>>> In 5.13 kernel this was replaced by the json-schema yaml file. The 
>>> PCI ID
>>> based enum which was added later does also list brcm,bcm4329-fmac so why
>>> does that not work for the compatible list ['pci14e4,449d',
>>> 'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I
>>> stripped a bit for brevity:
>>>
>>> properties:
>>>    compatible:
>>>      oneOf:
>>>        - items:
>>>            - enum:
>>>                - brcm,bcm43143-fmac
>>>                - brcm,bcm4329-fmac
>>>                - infineon,cyw43439-fmac
>>>            - const: brcm,bcm4329-fmac
>>>        - enum:
>>>            - brcm,bcm4329-fmac
>>>            - pci14e4,43dc  # BCM4355
>>>            - pci14e4,4464  # BCM4364
>>>            - pci14e4,4488  # BCM4377
>>>            - pci14e4,4425  # BCM4378
>>>            - pci14e4,4433  # BCM4387
>>>
>>> So how should I read this. Searching for some sort of syntax 
>>> description I
>>> found [1] which has an example schema with description that has a 
>>> similarly
>>> complicated compatible property. From that I think the above should be
>>> changed to:
>>>
>>>   properties:
>>>     compatible:
>>>       oneOf:
>>>         - items:
>>>             - enum:
>>>                 - brcm,bcm43143-fmac
>>> -              - brcm,bcm4329-fmac
>>>                 - infineon,cyw43439-fmac
>>>             - const: brcm,bcm4329-fmac
>>> +      - items:
>>>             - enum:
>>> -              - brcm,bcm4329-fmac
>>>                 - pci14e4,43dc  # BCM4355
>>>                 - pci14e4,4464  # BCM4364
>>>                 - pci14e4,4488  # BCM4377
>>>                 - pci14e4,4425  # BCM4378
>>>                 - pci14e4,4433  # BCM4387
>>> +          - const: brcm,bcm4329-fmac
>>> +      - const: brcm,bcm4329-fmac
>>>
>>> This poses a constraint in which the last string in the compatible 
>>> list is
>>> always 'brcm,bcm4329-fmac' even if it is the only string. At least 
>>> that is
>>> my understanding so if my understanding is wrong feel free to correct 
>>> me on
>>> this.
>>>
>>> [1] https://docs.kernel.org/devicetree/bindings/writing-schema.html
>>
>> Your proposed change should work as you describe. But it will result
>> in DT check errors for some Apple devices, which followed the
>> current binding and do not have the "brcm,bcm4329-fmac" fallback
>> compatible:
>>
>> $ git grep -E "(pci14e4,43dc)|(pci14e4,4464)|(pci14e4,4488)| 
>> (pci14e4,4425)|(pci14e4,4433)" arch/
>> arch/arm64/boot/dts/apple/t8103-jxxx.dtsi:           compatible = 
>> "pci14e4,4425";
>> arch/arm64/boot/dts/apple/t8112-j413.dts:            compatible = 
>> "pci14e4,4433";
>> arch/arm64/boot/dts/apple/t8112-j493.dts:            compatible = 
>> "pci14e4,4425";
> 
>> I guess patch 3/4 from this series will also introduce some
>> regressions for these devices by moving the check. What is the
>> purpose of the compatible check in brcmf_of_probe() in the first
>> place? Can it just be dropped?
>>
>> I see it was introduced 10 years ago in 61f663dfc1a09, probably to
>> avoid a spurious error message for systems not having the IRQ
>> described in DT? The current code exits quietly when none of the
>> optional resources are defined.
> 
> It was introduced simply because the compatible property has a meaning 
> that goes beyond informational. It is a claim that the properties of the 
> node comply to the bindings specification. I would really want to keep 
> the sanity check event though all properties are optional. The 
> constraint keeps the compatible matching in the driver relatively simple.
> 

I see;-)

-- 
Best Regards
Jacobe

Re: [PATCH v11 0/4] Add AP6275P wireless support

[Jacobe Zang]

On 2024/8/20 4:33, Sebastian Reichel wrote:
> Hello,
> 
> On Mon, Aug 19, 2024 at 09:35:12PM GMT, Arend van Spriel wrote:
>> On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
>>> I tested this on RK3588 EVB1 and the driver is working fine. The DT
>>> bindings are not correct, though:
>>>
>>> linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
>>> compatible: 'oneOf' conditional failed, one must be fixed:
>>>
>>> ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
>>> 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
>>> 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
>>> 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
>>> 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
>>> 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
>>> 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
>>> 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
>>> 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
>>> from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#
>>>
>>> It's easy to see the problem in the binding. It does not expect a
>>> fallback string after the PCI ID based compatible. Either the
>>> pci14e4,449d entry must be added to the first enum in the binding,
>>> which has the fallback compatible, or the fallback compatible
>>> should not be added to DTS.
>>
>> Never understood why we ended up with such a large list. When the binding
>> was introduced there was one compatible, ie. brcm,bcm4329-fmac. People
>> wanted all the other flavors because it described a specific wifi chip and
>> no other reason whatsoever. The PCI ID based compatible do obfuscate that
>> info so those are even less useful in my opinion.
>>
>>> If the fallback compatible is missing in DTS, the compatible check in
>>> brcmf_of_probe() fails and the lpo clock is not requested resulting
>>> in the firmware startup failing. So that would require further
>>> driver changes.
>>
>> Right. The text based bindings file in 5.12 kernel clearly says:
>>
>> Required properties:
>>
>>   - compatible : Should be "brcm,bcm4329-fmac".
>>
>> In 5.13 kernel this was replaced by the json-schema yaml file. The PCI ID
>> based enum which was added later does also list brcm,bcm4329-fmac so why
>> does that not work for the compatible list ['pci14e4,449d',
>> 'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I
>> stripped a bit for brevity:
>>
>> properties:
>>    compatible:
>>      oneOf:
>>        - items:
>>            - enum:
>>                - brcm,bcm43143-fmac
>>                - brcm,bcm4329-fmac
>>                - infineon,cyw43439-fmac
>>            - const: brcm,bcm4329-fmac
>>        - enum:
>>            - brcm,bcm4329-fmac
>>            - pci14e4,43dc  # BCM4355
>>            - pci14e4,4464  # BCM4364
>>            - pci14e4,4488  # BCM4377
>>            - pci14e4,4425  # BCM4378
>>            - pci14e4,4433  # BCM4387
>>
>> So how should I read this. Searching for some sort of syntax description I
>> found [1] which has an example schema with description that has a similarly
>> complicated compatible property. From that I think the above should be
>> changed to:
>>
>>   properties:
>>     compatible:
>>       oneOf:
>>         - items:
>>             - enum:
>>                 - brcm,bcm43143-fmac
>> -              - brcm,bcm4329-fmac
>>                 - infineon,cyw43439-fmac
>>             - const: brcm,bcm4329-fmac
>> +      - items:
>>             - enum:
>> -              - brcm,bcm4329-fmac
>>                 - pci14e4,43dc  # BCM4355
>>                 - pci14e4,4464  # BCM4364
>>                 - pci14e4,4488  # BCM4377
>>                 - pci14e4,4425  # BCM4378
>>                 - pci14e4,4433  # BCM4387
>> +          - const: brcm,bcm4329-fmac
>> +      - const: brcm,bcm4329-fmac
>>
>> This poses a constraint in which the last string in the compatible list is
>> always 'brcm,bcm4329-fmac' even if it is the only string. At least that is
>> my understanding so if my understanding is wrong feel free to correct me on
>> this.
>>
>> [1] https://docs.kernel.org/devicetree/bindings/writing-schema.html
> 
> Your proposed change should work as you describe. But it will result
> in DT check errors for some Apple devices, which followed the
> current binding and do not have the "brcm,bcm4329-fmac" fallback
> compatible:
> 
> $ git grep -E "(pci14e4,43dc)|(pci14e4,4464)|(pci14e4,4488)|(pci14e4,4425)|(pci14e4,4433)" arch/
> arch/arm64/boot/dts/apple/t8103-jxxx.dtsi:           compatible = "pci14e4,4425";
> arch/arm64/boot/dts/apple/t8112-j413.dts:            compatible = "pci14e4,4433";
> arch/arm64/boot/dts/apple/t8112-j493.dts:            compatible = "pci14e4,4425";
> 
> I guess patch 3/4 from this series will also introduce some
> regressions for these devices by moving the check. What is the
> purpose of the compatible check in brcmf_of_probe() in the first
> place? Can it just be dropped?
> 
> I see it was introduced 10 years ago in 61f663dfc1a09, probably to
> avoid a spurious error message for systems not having the IRQ
> described in DT? The current code exits quietly when none of the

Before these patches, the compatible check in brcmf_of_probe() is only 
used for interrupt configuration in DTS. Wi-Fi compatible in some boards 
without IRO configuration is just a compatible don't have real function 
but required by the binding. So if the compatible check is no longer 
needed, I think it can be dropped.

And also just adjusting the yaml is a good choice, but need to add 
fallback compatible for boards just like Apple.

> optional resources are defined.
> 


-- 
Best Regards
Jacobe