[PATCH v10 0/5] Add AP6275P wireless support

[PATCH v10 0/5] 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 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 (5):
  dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d
  dt-bindings: net: wireless: brcm4329-fmac: add clock description for
    AP6275P
  arm64: dts: rockchip: Add AP6275P wireless support to Khadas Edge 2
  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 +++
 .../dts/rockchip/rk3588s-khadas-edge2.dts     | 16 ++++++
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +-
 .../broadcom/brcm80211/brcmfmac/common.c      |  3 +-
 .../wireless/broadcom/brcm80211/brcmfmac/of.c | 53 +++++++++++-------
 .../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 +
 10 files changed, 132 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH v10 1/5] 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

[PATCH v10 2/5] 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 v10 3/5] arm64: dts: rockchip: Add AP6275P wireless support to Khadas Edge 2

[Jacobe Zang]

Khadas Edge2 uses the PCI-e Ampak AP6275P 2T2R Wi-Fi 6 module. The
pcie@0 node can be used as Bridge1, so the wifi@0 node is used as a
device under the Bridge1.

Co-developed-by: Muhammed Efe Cetin <efectn@protonmail.com>
Signed-off-by: Muhammed Efe Cetin <efectn@protonmail.com>
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../boot/dts/rockchip/rk3588s-khadas-edge2.dts   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
index dbddfc3bb4641..a3361ca25867d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
@@ -283,6 +283,22 @@ &pcie2x1l2 {
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie_wl>;
 	status = "okay";
+
+	pcie@0,0 {
+		reg = <0x400000 0 0 0 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges;
+		device_type = "pci";
+		bus-range = <0x40 0x4f>;
+
+		wifi: wifi@0,0 {
+			compatible = "brcm,bcm4329-fmac", "pci14e4,449d";
+			reg = <0x410000 0 0 0 0>;
+			clocks = <&hym8563>;
+			clock-names = "lpo";
+		};
+	};
 };
 
 &pwm11 {
-- 
2.34.1

[PATCH v10 4/5] 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 | 53 +++++++++++--------
 .../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, 61 insertions(+), 36 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..f19dc7355e0e8 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);
 
-	if (bus_type != BRCMF_BUSTYPE_SDIO)
-		return;
+	if (bus_type == BRCMF_BUSTYPE_SDIO) {
+		if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
+			sdio->drive_strength = val;
 
-	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 0;
 
-	/* make sure there are interrupts defined in the node */
-	if (!of_property_present(np, "interrupts"))
-		return;
+		irq = irq_of_parse_and_map(np, 0);
+		if (!irq) {
+			brcmf_err("interrupt could not be mapped\n");
+			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;
+	}
 
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		brcmf_err("interrupt could not be mapped\n");
-		return;
+	clk = devm_clk_get_optional_enabled(dev, "lpo");
+	if (!IS_ERR_OR_NULL(clk)) {
+		brcmf_dbg(INFO, "enabling 32kHz clock\n");
+		return clk_set_rate(clk, 32768);
+	} else {
+		return PTR_ERR_OR_ZERO(clk);
 	}
-	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

[PATCH v10 5/5] 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 v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Arend van Spriel]

On 8/13/2024 10:20 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.

I was going to say it looks good to me, but....

> 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 | 53 +++++++++++--------
>   .../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, 61 insertions(+), 36 deletions(-)

[...]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> index e406e11481a62..f19dc7355e0e8 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c

[...]

> @@ -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);
>   
> -	if (bus_type != BRCMF_BUSTYPE_SDIO)
> -		return;
> +	if (bus_type == BRCMF_BUSTYPE_SDIO) {

Don't like the fact that this now has an extra indentation level and it 
offers no extra benefit. Just keep the original if-statement and return 
0. Consequently the LPO clock code should move just before the if-statement.

> +		if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
> +			sdio->drive_strength = val;
>   
> -	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 0;
>   
> -	/* make sure there are interrupts defined in the node */
> -	if (!of_property_present(np, "interrupts"))
> -		return;
> +		irq = irq_of_parse_and_map(np, 0);
> +		if (!irq) {
> +			brcmf_err("interrupt could not be mapped\n");
> +			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;
> +	}
>   
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (!irq) {
> -		brcmf_err("interrupt could not be mapped\n");
> -		return;
> +	clk = devm_clk_get_optional_enabled(dev, "lpo");
> +	if (!IS_ERR_OR_NULL(clk)) {
> +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
> +		return clk_set_rate(clk, 32768);
> +	} else {
> +		return PTR_ERR_OR_ZERO(clk);
>   	}

Change this to:

 > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
 > +	if (IS_ERR_OR_NULL(clk)) {
 > +		return PTR_ERR_OR_ZERO(clk);
 > +	}
 > +	brcmf_dbg(INFO, "enabling 32kHz clock\n");
 > +	clk_set_rate(clk, 32768);

As said above this should be moved before the if-statement:

 > -	if (bus_type != BRCMF_BUSTYPE_SDIO)
 > -		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;
>   }

[...]

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

[Arend Van Spriel]

On August 13, 2024 10:20:24 AM 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.

Hi Kalle,

There probably will be a v11, but wanted to know how this series will be 
handled as it involves device tree bindings, arm arch device tree spec, and 
brcmfmac driver code. Can it all go through wireless-next?

Regards,
Arend

> 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(+)

Re: [PATCH v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Arend van Spriel]

On 8/13/2024 1:57 PM, Arend van Spriel wrote:
> On 8/13/2024 10:20 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.
> 
> I was going to say it looks good to me, but....
> 
>> 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 | 53 +++++++++++--------
>>   .../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, 61 insertions(+), 36 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>> index e406e11481a62..f19dc7355e0e8 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> 
> [...]
> 
>> @@ -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);
>> -    if (bus_type != BRCMF_BUSTYPE_SDIO)
>> -        return;
>> +    if (bus_type == BRCMF_BUSTYPE_SDIO) {
> 
> Don't like the fact that this now has an extra indentation level and it 
> offers no extra benefit. Just keep the original if-statement and return 
> 0. Consequently the LPO clock code should move just before the 
> if-statement.
> 
>> +        if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
>> +            sdio->drive_strength = val;
>> -    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 0;
>> -    /* make sure there are interrupts defined in the node */
>> -    if (!of_property_present(np, "interrupts"))
>> -        return;
>> +        irq = irq_of_parse_and_map(np, 0);
>> +        if (!irq) {
>> +            brcmf_err("interrupt could not be mapped\n");
>> +            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;
>> +    }
>> -    irq = irq_of_parse_and_map(np, 0);
>> -    if (!irq) {
>> -        brcmf_err("interrupt could not be mapped\n");
>> -        return;
>> +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>> +    if (!IS_ERR_OR_NULL(clk)) {
>> +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
>> +        return clk_set_rate(clk, 32768);
>> +    } else {
>> +        return PTR_ERR_OR_ZERO(clk);
>>       }
> 
> Change this to:
> 
>  > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>  > +    if (IS_ERR_OR_NULL(clk)) {
>  > +        return PTR_ERR_OR_ZERO(clk);
>  > +    }
>  > +    brcmf_dbg(INFO, "enabling 32kHz clock\n");
>  > +    clk_set_rate(clk, 32768);

Do we really need to set the clock rate or could that be defined in the 
devicetree? Just wondering. I have looked at the clock.yaml schema, but 
not really an idea what is needed in the devicetree spec. Any pointers 
from DT devs would be appreciated or a hard no-do-not-do-that is also 
fine ;-)

Regards,
Arend

Re: [PATCH v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Alexey Charkov]

Hi Arend, Jacobe,

On Tuesday, August 13, 2024 2:57:28 PM GMT+3 Arend van Spriel wrote:
> On 8/13/2024 10:20 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.
> 
> I was going to say it looks good to me, but....
> 
> > 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 | 53 +++++++++++--------
> >   .../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, 61 insertions(+), 36 deletions(-)
> 
> [...]
> 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index
> > e406e11481a62..f19dc7355e0e8 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> 
> [...]
> 
> > @@ -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);
> > 
> > -	if (bus_type != BRCMF_BUSTYPE_SDIO)
> > -		return;
> > +	if (bus_type == BRCMF_BUSTYPE_SDIO) {
> 
> Don't like the fact that this now has an extra indentation level and it
> offers no extra benefit. Just keep the original if-statement and return
> 0. Consequently the LPO clock code should move just before the if-statement.
> > +		if (of_property_read_u32(np, "brcm,drive-strength", 
&val) == 0)
> > +			sdio->drive_strength = val;
> > 
> > -	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 0;
> > 
> > -	/* make sure there are interrupts defined in the node */
> > -	if (!of_property_present(np, "interrupts"))
> > -		return;
> > +		irq = irq_of_parse_and_map(np, 0);
> > +		if (!irq) {
> > +			brcmf_err("interrupt could not be 
mapped\n");
> > +			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;
> > +	}
> > 
> > -	irq = irq_of_parse_and_map(np, 0);
> > -	if (!irq) {
> > -		brcmf_err("interrupt could not be mapped\n");
> > -		return;
> > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
> > +	if (!IS_ERR_OR_NULL(clk)) {
> > +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
> > +		return clk_set_rate(clk, 32768);
> > +	} else {
> > +		return PTR_ERR_OR_ZERO(clk);
> > 
> >   	}
> 
> Change this to:
>  > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
>  > +	if (IS_ERR_OR_NULL(clk)) {
>  > +		return PTR_ERR_OR_ZERO(clk);

Perhaps in this case we should go for IS_ERR and PTR_ERR respectively. 
devm_clk_get_optional_enabled would return NULL when the optional clock is not 
found, so NULL is not an error state but serves as a dummy clock that can be 
used with clk_set_rate.

This way we won't skip over the interrupts initialization below in case the 
clock is absent.

>  > +	}
>  > +	brcmf_dbg(INFO, "enabling 32kHz clock\n");
>  > +	clk_set_rate(clk, 32768);
> 
> As said above this should be moved before the if-statement:
>  > -	if (bus_type != BRCMF_BUSTYPE_SDIO)
>  > -		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;
> > 
> >   }

Best regards,
Alexey

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

[Krzysztof Kozlowski]

On 13/08/2024 19:04, Arend Van Spriel wrote:
> On August 13, 2024 10:20:24 AM 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.
> 
> Hi Kalle,
> 
> There probably will be a v11, but wanted to know how this series will be 
> handled as it involves device tree bindings, arm arch device tree spec, and 
> brcmfmac driver code. Can it all go through wireless-next?

No, DTS must not go via wireless-next. Please split it from the series
and provide lore link in changelog for bindings.

Best regards,
Krzysztof

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

[Jacobe Zang]

On 2024/8/14 16:53, Krzysztof Kozlowski wrote:
> On 13/08/2024 19:04, Arend Van Spriel wrote:
>> On August 13, 2024 10:20:24 AM 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.
>>
>> Hi Kalle,
>>
>> There probably will be a v11, but wanted to know how this series will be
>> handled as it involves device tree bindings, arm arch device tree spec, and
>> brcmfmac driver code. Can it all go through wireless-next?
> 
> No, DTS must not go via wireless-next. Please split it from the series
> and provide lore link in changelog for bindings.
> 

I'm little confused that I should push bindings as a series, DTS as a 
series and driver as a series separately, so next time I should push 3 
series, right?

-- 
Best Regards
Jacobe

Re: [PATCH v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Jacobe Zang]

On 2024/8/14 16:47, Alexey Charkov wrote:
> Hi Arend, Jacobe,
> 
> On Tuesday, August 13, 2024 2:57:28 PM GMT+3 Arend van Spriel wrote:
>> On 8/13/2024 10:20 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.
>>
>> I was going to say it looks good to me, but....
>>
>>> 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 | 53 +++++++++++--------
>>>    .../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, 61 insertions(+), 36 deletions(-)
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index
>>> e406e11481a62..f19dc7355e0e8 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>
>> [...]
>>
>>> @@ -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);
>>>
>>> -	if (bus_type != BRCMF_BUSTYPE_SDIO)
>>> -		return;
>>> +	if (bus_type == BRCMF_BUSTYPE_SDIO) {
>>
>> Don't like the fact that this now has an extra indentation level and it
>> offers no extra benefit. Just keep the original if-statement and return
>> 0. Consequently the LPO clock code should move just before the if-statement.
>>> +		if (of_property_read_u32(np, "brcm,drive-strength",
> &val) == 0)
>>> +			sdio->drive_strength = val;
>>>
>>> -	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 0;
>>>
>>> -	/* make sure there are interrupts defined in the node */
>>> -	if (!of_property_present(np, "interrupts"))
>>> -		return;
>>> +		irq = irq_of_parse_and_map(np, 0);
>>> +		if (!irq) {
>>> +			brcmf_err("interrupt could not be
> mapped\n");
>>> +			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;
>>> +	}
>>>
>>> -	irq = irq_of_parse_and_map(np, 0);
>>> -	if (!irq) {
>>> -		brcmf_err("interrupt could not be mapped\n");
>>> -		return;
>>> +	clk = devm_clk_get_optional_enabled(dev, "lpo");
>>> +	if (!IS_ERR_OR_NULL(clk)) {
>>> +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>> +		return clk_set_rate(clk, 32768);
>>> +	} else {
>>> +		return PTR_ERR_OR_ZERO(clk);
>>>
>>>    	}
>>
>> Change this to:
>>   > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
>>   > +	if (IS_ERR_OR_NULL(clk)) {
>>   > +		return PTR_ERR_OR_ZERO(clk);
> 
> Perhaps in this case we should go for IS_ERR and PTR_ERR respectively.
> devm_clk_get_optional_enabled would return NULL when the optional clock is not
> found, so NULL is not an error state but serves as a dummy clock that can be> used with clk_set_rate.

I think we don't need to set clock rate for clock is NULL. So it should 
be changed to:

+	clk = devm_clk_get_optional_enabled(dev, "lpo");
+	if (IS_ERR(clk)) {
+		return PTR_ERR(clk);
+	} else if (clk) {
+		brcmf_dbg(INFO, "enabling 32kHz clock\n");
+		clk_set_rate(clk, 32768);
+	}

> 
> This way we won't skip over the interrupts initialization below in case the
> clock is absent.
> 
>>   > +	}
>>   > +	brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>   > +	clk_set_rate(clk, 32768);
>>
>> As said above this should be moved before the if-statement:
>>   > -	if (bus_type != BRCMF_BUSTYPE_SDIO)
>>   > -		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;
>>>
>>>    }
> 
> 

-- 
Best Regards
Jacobe

Re: [PATCH v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Alexey Charkov]

On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>
>
>
> On 2024/8/14 16:47, Alexey Charkov wrote:
> > Hi Arend, Jacobe,
> >
> > On Tuesday, August 13, 2024 2:57:28 PM GMT+3 Arend van Spriel wrote:
> >> On 8/13/2024 10:20 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.
> >>
> >> I was going to say it looks good to me, but....
> >>
> >>> 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 | 53 +++++++++++--------
> >>>    .../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, 61 insertions(+), 36 deletions(-)
> >>
> >> [...]
> >>
> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index
> >>> e406e11481a62..f19dc7355e0e8 100644
> >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>
> >> [...]
> >>
> >>> @@ -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);
> >>>
> >>> -   if (bus_type != BRCMF_BUSTYPE_SDIO)
> >>> -           return;
> >>> +   if (bus_type == BRCMF_BUSTYPE_SDIO) {
> >>
> >> Don't like the fact that this now has an extra indentation level and it
> >> offers no extra benefit. Just keep the original if-statement and return
> >> 0. Consequently the LPO clock code should move just before the if-statement.
> >>> +           if (of_property_read_u32(np, "brcm,drive-strength",
> > &val) == 0)
> >>> +                   sdio->drive_strength = val;
> >>>
> >>> -   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 0;
> >>>
> >>> -   /* make sure there are interrupts defined in the node */
> >>> -   if (!of_property_present(np, "interrupts"))
> >>> -           return;
> >>> +           irq = irq_of_parse_and_map(np, 0);
> >>> +           if (!irq) {
> >>> +                   brcmf_err("interrupt could not be
> > mapped\n");
> >>> +                   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;
> >>> +   }
> >>>
> >>> -   irq = irq_of_parse_and_map(np, 0);
> >>> -   if (!irq) {
> >>> -           brcmf_err("interrupt could not be mapped\n");
> >>> -           return;
> >>> +   clk = devm_clk_get_optional_enabled(dev, "lpo");
> >>> +   if (!IS_ERR_OR_NULL(clk)) {
> >>> +           brcmf_dbg(INFO, "enabling 32kHz clock\n");
> >>> +           return clk_set_rate(clk, 32768);
> >>> +   } else {
> >>> +           return PTR_ERR_OR_ZERO(clk);
> >>>
> >>>     }
> >>
> >> Change this to:
> >>   > +        clk = devm_clk_get_optional_enabled(dev, "lpo");
> >>   > +        if (IS_ERR_OR_NULL(clk)) {
> >>   > +                return PTR_ERR_OR_ZERO(clk);
> >
> > Perhaps in this case we should go for IS_ERR and PTR_ERR respectively.
> > devm_clk_get_optional_enabled would return NULL when the optional clock is not
> > found, so NULL is not an error state but serves as a dummy clock that can be> used with clk_set_rate.
>
> I think we don't need to set clock rate for clock is NULL. So it should
> be changed to:
>
> +       clk = devm_clk_get_optional_enabled(dev, "lpo");
> +       if (IS_ERR(clk)) {
> +               return PTR_ERR(clk);
> +       } else if (clk) {
> +               brcmf_dbg(INFO, "enabling 32kHz clock\n");
> +               clk_set_rate(clk, 32768);
> +       }

If clk is NULL then clk_set_rate returns immediately with status zero,
so there is little difference from whether you wrap it into another if
(clk) or not. You can probably drop the debug statement altogether and
call clk_set_rate unconditionally - this will look neater.

Best regards,
Alexey

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

[Arend van Spriel]

On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
> On 13/08/2024 19:04, Arend Van Spriel wrote:
>> On August 13, 2024 10:20:24 AM 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.
>>
>> Hi Kalle,
>>
>> There probably will be a v11, but wanted to know how this series will be
>> handled as it involves device tree bindings, arm arch device tree spec, and
>> brcmfmac driver code. Can it all go through wireless-next?
> 
> No, DTS must not go via wireless-next. Please split it from the series
> and provide lore link in changelog for bindings.

Hi Krzysztof,

Is it really important how the patches travel upstream to Linus. This 
binding is specific to Broadcom wifi devices so there are no 
dependencies(?). To clarify what you are asking I assume two separate 
series:

1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
	reference to: 
https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com

2) brcmfmac driver changes	  -> linux-wireless@vger.kernel.org

Regards,
Arend

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

[Krzysztof Kozlowski]

On 14/08/2024 11:12, Jacobe Zang wrote:
> 
> 
> On 2024/8/14 16:53, Krzysztof Kozlowski wrote:
>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>> On August 13, 2024 10:20:24 AM 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.
>>>
>>> Hi Kalle,
>>>
>>> There probably will be a v11, but wanted to know how this series will be
>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>> brcmfmac driver code. Can it all go through wireless-next?
>>
>> No, DTS must not go via wireless-next. Please split it from the series
>> and provide lore link in changelog for bindings.
>>
> 
> I'm little confused that I should push bindings as a series, DTS as a 
> series and driver as a series separately, so next time I should push 3 
> series, right?

No. I said only DTS.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 14/08/2024 12:08, Arend van Spriel wrote:
> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>> On August 13, 2024 10:20:24 AM 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.
>>>
>>> Hi Kalle,
>>>
>>> There probably will be a v11, but wanted to know how this series will be
>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>> brcmfmac driver code. Can it all go through wireless-next?
>>
>> No, DTS must not go via wireless-next. Please split it from the series
>> and provide lore link in changelog for bindings.
> 
> Hi Krzysztof,
> 
> Is it really important how the patches travel upstream to Linus. This 
> binding is specific to Broadcom wifi devices so there are no 
> dependencies(?). To clarify what you are asking I assume two separate 
> series:
> 
> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
> 	reference to: 
> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
> 
> 2) brcmfmac driver changes	  -> linux-wireless@vger.kernel.org

No. I said only DTS is separate. This was always the rule, since forever.

Documentation/devicetree/bindings/submitting-patches.rst

Best regards,
Krzysztof

Re: [PATCH v10 4/5] wifi: brcmfmac: Add optional lpo clock enable support

[Arend van Spriel]

On 8/14/2024 11:48 AM, Alexey Charkov wrote:
> On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>>
>>
>>
>> On 2024/8/14 16:47, Alexey Charkov wrote:
>>> Hi Arend, Jacobe,
>>>
>>> On Tuesday, August 13, 2024 2:57:28 PM GMT+3 Arend van Spriel wrote:
>>>> On 8/13/2024 10:20 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.
>>>>
>>>> I was going to say it looks good to me, but....
>>>>
>>>>> 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 | 53 +++++++++++--------
>>>>>     .../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, 61 insertions(+), 36 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index
>>>>> e406e11481a62..f19dc7355e0e8 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>>
>>>> [...]
>>>>
>>>>> @@ -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);
>>>>>
>>>>> -   if (bus_type != BRCMF_BUSTYPE_SDIO)
>>>>> -           return;
>>>>> +   if (bus_type == BRCMF_BUSTYPE_SDIO) {
>>>>
>>>> Don't like the fact that this now has an extra indentation level and it
>>>> offers no extra benefit. Just keep the original if-statement and return
>>>> 0. Consequently the LPO clock code should move just before the if-statement.
>>>>> +           if (of_property_read_u32(np, "brcm,drive-strength",
>>> &val) == 0)
>>>>> +                   sdio->drive_strength = val;
>>>>>
>>>>> -   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 0;
>>>>>
>>>>> -   /* make sure there are interrupts defined in the node */
>>>>> -   if (!of_property_present(np, "interrupts"))
>>>>> -           return;
>>>>> +           irq = irq_of_parse_and_map(np, 0);
>>>>> +           if (!irq) {
>>>>> +                   brcmf_err("interrupt could not be
>>> mapped\n");
>>>>> +                   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;
>>>>> +   }
>>>>>
>>>>> -   irq = irq_of_parse_and_map(np, 0);
>>>>> -   if (!irq) {
>>>>> -           brcmf_err("interrupt could not be mapped\n");
>>>>> -           return;
>>>>> +   clk = devm_clk_get_optional_enabled(dev, "lpo");
>>>>> +   if (!IS_ERR_OR_NULL(clk)) {
>>>>> +           brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>>>> +           return clk_set_rate(clk, 32768);
>>>>> +   } else {
>>>>> +           return PTR_ERR_OR_ZERO(clk);
>>>>>
>>>>>      }
>>>>
>>>> Change this to:
>>>>    > +        clk = devm_clk_get_optional_enabled(dev, "lpo");
>>>>    > +        if (IS_ERR_OR_NULL(clk)) {
>>>>    > +                return PTR_ERR_OR_ZERO(clk);
>>>
>>> Perhaps in this case we should go for IS_ERR and PTR_ERR respectively.
>>> devm_clk_get_optional_enabled would return NULL when the optional clock is not
>>> found, so NULL is not an error state but serves as a dummy clock that can be> used with clk_set_rate.
>>
>> I think we don't need to set clock rate for clock is NULL. So it should
>> be changed to:
>>
>> +       clk = devm_clk_get_optional_enabled(dev, "lpo");
>> +       if (IS_ERR(clk)) {
>> +               return PTR_ERR(clk);
>> +       } else if (clk) {
>> +               brcmf_dbg(INFO, "enabling 32kHz clock\n");
>> +               clk_set_rate(clk, 32768);
>> +       }
> 
> If clk is NULL then clk_set_rate returns immediately with status zero,
> so there is little difference from whether you wrap it into another if
> (clk) or not. You can probably drop the debug statement altogether and
> call clk_set_rate unconditionally - this will look neater.

The construct above is indeed only needed to get the debug statement 
correct given the behavior of clk_set_rate(). However, for debugging it 
is useful to know that the LPO clock is defined and used or not. Maybe 
do this:

        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);

Regards,
Arend

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

[Arend van Spriel]

On 8/14/2024 12:39 PM, Krzysztof Kozlowski wrote:
> On 14/08/2024 12:08, Arend van Spriel wrote:
>> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>>> On August 13, 2024 10:20:24 AM 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.
>>>>
>>>> Hi Kalle,
>>>>
>>>> There probably will be a v11, but wanted to know how this series will be
>>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>>> brcmfmac driver code. Can it all go through wireless-next?
>>>
>>> No, DTS must not go via wireless-next. Please split it from the series
>>> and provide lore link in changelog for bindings.
>>
>> Hi Krzysztof,
>>
>> Is it really important how the patches travel upstream to Linus. This
>> binding is specific to Broadcom wifi devices so there are no
>> dependencies(?). To clarify what you are asking I assume two separate
>> series:
>>
>> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
>> 	reference to:
>> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
>>
>> 2) brcmfmac driver changes	  -> linux-wireless@vger.kernel.org
> 
> No. I said only DTS is separate. This was always the rule, since forever.
> 
> Documentation/devicetree/bindings/submitting-patches.rst

I am going slightly mad (by Queen). That documents says:

   1) The Documentation/ and include/dt-bindings/ portion of the patch 
should
      be a separate patch.

and

   4) Submit the entire series to the devicetree mailinglist at

        devicetree@vger.kernel.org

Above I mentioned "series", not "patch". So 1) is a series of 3 patches 
(2 changes to the DT binding file and 1 patch for the Khadas Edge2 DTS. 
Is that correct?

Regards,
Arend

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

[Krzysztof Kozlowski]

On 14/08/2024 12:59, Arend van Spriel wrote:
> On 8/14/2024 12:39 PM, Krzysztof Kozlowski wrote:
>> On 14/08/2024 12:08, Arend van Spriel wrote:
>>> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>>>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>>>> On August 13, 2024 10:20:24 AM 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.
>>>>>
>>>>> Hi Kalle,
>>>>>
>>>>> There probably will be a v11, but wanted to know how this series will be
>>>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>>>> brcmfmac driver code. Can it all go through wireless-next?
>>>>
>>>> No, DTS must not go via wireless-next. Please split it from the series
>>>> and provide lore link in changelog for bindings.
>>>
>>> Hi Krzysztof,
>>>
>>> Is it really important how the patches travel upstream to Linus. This
>>> binding is specific to Broadcom wifi devices so there are no
>>> dependencies(?). To clarify what you are asking I assume two separate
>>> series:
>>>
>>> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
>>> 	reference to:
>>> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
>>>
>>> 2) brcmfmac driver changes	  -> linux-wireless@vger.kernel.org
>>
>> No. I said only DTS is separate. This was always the rule, since forever.
>>
>> Documentation/devicetree/bindings/submitting-patches.rst
> 
> I am going slightly mad (by Queen). That documents says:
> 
>    1) The Documentation/ and include/dt-bindings/ portion of the patch 
> should
>       be a separate patch.
> 
> and
> 
>    4) Submit the entire series to the devicetree mailinglist at
> 
>         devicetree@vger.kernel.org
> 
> Above I mentioned "series", not "patch". So 1) is a series of 3 patches 
> (2 changes to the DT binding file and 1 patch for the Khadas Edge2 DTS. 
> Is that correct?
> 

My bookmark to elixir.bootling does not work, so could not paste
specific line. Now it works, so:

https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79

The rule was/is:
1. Binding for typical devices always go via subsystem tree, with the
driver changes.
There can be exceptions from above, e.g. some subsystems do not pick up
bindings, so Rob does. But how patches are organized is not an exception
- it is completely normal workflow.

2. DTS *always* goes via SoC maintainer. DTS cannot go via any other
driver subsystem tree. There is no exception here. There cannot be an
exception, because it would mean the hardware depends on driver, which
is obviously false.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 14/08/2024 13:15, Krzysztof Kozlowski wrote:
> On 14/08/2024 12:59, Arend van Spriel wrote:
>> On 8/14/2024 12:39 PM, Krzysztof Kozlowski wrote:
>>> On 14/08/2024 12:08, Arend van Spriel wrote:
>>>> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>>>>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>>>>> On August 13, 2024 10:20:24 AM 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.
>>>>>>
>>>>>> Hi Kalle,
>>>>>>
>>>>>> There probably will be a v11, but wanted to know how this series will be
>>>>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>>>>> brcmfmac driver code. Can it all go through wireless-next?
>>>>>
>>>>> No, DTS must not go via wireless-next. Please split it from the series
>>>>> and provide lore link in changelog for bindings.
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> Is it really important how the patches travel upstream to Linus. This
>>>> binding is specific to Broadcom wifi devices so there are no
>>>> dependencies(?). To clarify what you are asking I assume two separate
>>>> series:
>>>>
>>>> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
>>>> 	reference to:
>>>> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
>>>>
>>>> 2) brcmfmac driver changes	  -> linux-wireless@vger.kernel.org
>>>
>>> No. I said only DTS is separate. This was always the rule, since forever.
>>>
>>> Documentation/devicetree/bindings/submitting-patches.rst
>>
>> I am going slightly mad (by Queen). That documents says:
>>
>>    1) The Documentation/ and include/dt-bindings/ portion of the patch 
>> should
>>       be a separate patch.
>>
>> and
>>
>>    4) Submit the entire series to the devicetree mailinglist at
>>
>>         devicetree@vger.kernel.org
>>
>> Above I mentioned "series", not "patch". So 1) is a series of 3 patches 
>> (2 changes to the DT binding file and 1 patch for the Khadas Edge2 DTS. 
>> Is that correct?
>>
> 
> My bookmark to elixir.bootling does not work, so could not paste
> specific line. Now it works, so:
> 
> https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79
> 
> The rule was/is:
> 1. Binding for typical devices always go via subsystem tree, with the
> driver changes.
> There can be exceptions from above, e.g. some subsystems do not pick up
> bindings, so Rob does. But how patches are organized is not an exception
> - it is completely normal workflow.
> 
> 2. DTS *always* goes via SoC maintainer. DTS cannot go via any other
> driver subsystem tree. There is no exception here. There cannot be an
> exception, because it would mean the hardware depends on driver, which
> is obviously false.

In case my message was not clear: we talk here about organizing
patchsets, not individual patches. If you ask about patches, then DTS,
bindings and driver are all separate patches. This set already is split
like that, so this was fine and I did not comment on it. Only through
whom the DTS patch goes - separate tree.

And just in case: this is neither specific to wireless nor to Broadcom.
This is for entire Linux kernel.

Best regards,
Krzysztof

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

[Arend Van Spriel]

On August 14, 2024 4:08:52 PM Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:

> On 14/08/2024 13:15, Krzysztof Kozlowski wrote:
>> On 14/08/2024 12:59, Arend van Spriel wrote:
>>> On 8/14/2024 12:39 PM, Krzysztof Kozlowski wrote:
>>>> On 14/08/2024 12:08, Arend van Spriel wrote:
>>>>> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>>>>>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>>>>>> On August 13, 2024 10:20:24 AM 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.
>>>>>>>
>>>>>>> Hi Kalle,
>>>>>>>
>>>>>>> There probably will be a v11, but wanted to know how this series will be
>>>>>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>>>>>> brcmfmac driver code. Can it all go through wireless-next?
>>>>>>
>>>>>> No, DTS must not go via wireless-next. Please split it from the series
>>>>>> and provide lore link in changelog for bindings.
>>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> Is it really important how the patches travel upstream to Linus. This
>>>>> binding is specific to Broadcom wifi devices so there are no
>>>>> dependencies(?). To clarify what you are asking I assume two separate
>>>>> series:
>>>>>
>>>>> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
>>>>> reference to:
>>>>> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
>>>>>
>>>>> 2) brcmfmac driver changes  -> linux-wireless@vger.kernel.org
>>>>
>>>> No. I said only DTS is separate. This was always the rule, since forever.
>>>>
>>>> Documentation/devicetree/bindings/submitting-patches.rst
>>>
>>> I am going slightly mad (by Queen). That documents says:
>>>
>>> 1) The Documentation/ and include/dt-bindings/ portion of the patch
>>> should
>>> be a separate patch.
>>>
>>> and
>>>
>>> 4) Submit the entire series to the devicetree mailinglist at
>>>
>>> devicetree@vger.kernel.org
>>>
>>> Above I mentioned "series", not "patch". So 1) is a series of 3 patches
>>> (2 changes to the DT binding file and 1 patch for the Khadas Edge2 DTS.
>>> Is that correct?
>>
>> My bookmark to elixir.bootling does not work, so could not paste
>> specific line. Now it works, so:
>>
>> https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79
>>
>> The rule was/is:
>> 1. Binding for typical devices always go via subsystem tree, with the
>> driver changes.
>> There can be exceptions from above, e.g. some subsystems do not pick up
>> bindings, so Rob does. But how patches are organized is not an exception
>> - it is completely normal workflow.
>>
>> 2. DTS *always* goes via SoC maintainer. DTS cannot go via any other
>> driver subsystem tree. There is no exception here. There cannot be an
>> exception, because it would mean the hardware depends on driver, which
>> is obviously false.
>
> In case my message was not clear: we talk here about organizing
> patchsets, not individual patches. If you ask about patches, then DTS,
> bindings and driver are all separate patches. This set already is split
> like that, so this was fine and I did not comment on it. Only through
> whom the DTS patch goes - separate tree.

I used the "series" which is my term for "patchset". Sorry for confusion. 
So "[PATCH 3/5] arm64: dts: rockchip: Add AP6275P wireless support to 
Khadas Edge 2" should be submitted to rockchip soc related tree and the 
rest can go through the wireless-next tree. Got it.

Regards,
Arend
---
$ ./scripts/get_maintainer.pl -f 
arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED 
DEVICE TREE BINDINGS)
Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND 
FLATTENED DEVICE TREE BINDINGS)
Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED 
DEVICE TREE BINDINGS)
Heiko Stuebner <heiko@sntech.de> (maintainer:ARM/Rockchip SoC 
support,commit_signer:11/11=100%,authored:1/11=9%,removed_lines:1/1=100%)
Muhammed Efe Cetin <efectn@protonmail.com> 
(commit_signer:10/11=91%,authored:10/11=91%,added_lines:685/685=100%)
Dragan Simic <dsimic@manjaro.org> (commit_signer:1/11=9%)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE 
TREE BINDINGS)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/Rockchip SoC support)
linux-rockchip@lists.infradead.org (open list:ARM/Rockchip SoC support)
linux-kernel@vger.kernel.org (open list)

>
> And just in case: this is neither specific to wireless nor to Broadcom.
> This is for entire Linux kernel.
>
> Best regards,
> Krzysztof

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

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On August 14, 2024 4:08:52 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>
>> On 14/08/2024 13:15, Krzysztof Kozlowski wrote:
>>> On 14/08/2024 12:59, Arend van Spriel wrote:
>>>> On 8/14/2024 12:39 PM, Krzysztof Kozlowski wrote:
>>>>> On 14/08/2024 12:08, Arend van Spriel wrote:
>>>>>> On 8/14/2024 10:53 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 13/08/2024 19:04, Arend Van Spriel wrote:
>>>>>>>> On August 13, 2024 10:20:24 AM 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.
>>>>>>>>
>>>>>>>> Hi Kalle,
>>>>>>>>
>>>>>>>> There probably will be a v11, but wanted to know how this series will be
>>>>>>>> handled as it involves device tree bindings, arm arch device tree spec, and
>>>>>>>> brcmfmac driver code. Can it all go through wireless-next?
>>>>>>>
>>>>>>> No, DTS must not go via wireless-next. Please split it from the series
>>>>>>> and provide lore link in changelog for bindings.
>>>>>>
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> Is it really important how the patches travel upstream to Linus. This
>>>>>> binding is specific to Broadcom wifi devices so there are no
>>>>>> dependencies(?). To clarify what you are asking I assume two separate
>>>>>> series:
>>>>>>
>>>>>> 1) DT binding + Khadas Edge2 DTS  -> devicetree@vger.kernel.org
>>>>>> reference to:
>>>>>> https://patch.msgid.link/20240813082007.2625841-1-jacobe.zang@wesion.com
>>>>>>
>>>>>> 2) brcmfmac driver changes  -> linux-wireless@vger.kernel.org
>>>>>
>>>>> No. I said only DTS is separate. This was always the rule, since forever.
>>>>>
>>>>> Documentation/devicetree/bindings/submitting-patches.rst
>>>>
>>>> I am going slightly mad (by Queen). That documents says:
>>>>
>>>> 1) The Documentation/ and include/dt-bindings/ portion of the patch
>>>> should
>>>> be a separate patch.
>>>>
>>>> and
>>>>
>>>> 4) Submit the entire series to the devicetree mailinglist at
>>>>
>>>> devicetree@vger.kernel.org
>>>>
>>>> Above I mentioned "series", not "patch". So 1) is a series of 3 patches
>>>> (2 changes to the DT binding file and 1 patch for the Khadas Edge2 DTS.
>>>> Is that correct?
>>>
>>> My bookmark to elixir.bootling does not work, so could not paste
>>> specific line. Now it works, so:
>>>
>>> https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79
>>>
>>> The rule was/is:
>>> 1. Binding for typical devices always go via subsystem tree, with the
>>> driver changes.
>>> There can be exceptions from above, e.g. some subsystems do not pick up
>>> bindings, so Rob does. But how patches are organized is not an exception
>>> - it is completely normal workflow.
>>>
>>> 2. DTS *always* goes via SoC maintainer. DTS cannot go via any other
>>> driver subsystem tree. There is no exception here. There cannot be an
>>> exception, because it would mean the hardware depends on driver, which
>>> is obviously false.
>>
>> In case my message was not clear: we talk here about organizing
>> patchsets, not individual patches. If you ask about patches, then DTS,
>> bindings and driver are all separate patches. This set already is split
>> like that, so this was fine and I did not comment on it. Only through
>> whom the DTS patch goes - separate tree.
>
> I used the "series" which is my term for "patchset". Sorry for
> confusion. So "[PATCH 3/5] arm64: dts: rockchip: Add AP6275P wireless
> support to Khadas Edge 2" should be submitted to rockchip soc related
> tree and the rest can go through the wireless-next tree. Got it.

Yes, this is how we have done before as well.

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

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