[PATCH v6 0/5] Add AP6275P wireless support

[PATCH v6 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 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

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 ++++++
 .../wireless/broadcom/brcm80211/brcmfmac/of.c | 12 ++++-
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 52 ++++++++++++++++---
 .../broadcom/brcm80211/include/brcm_hw_ids.h  |  2 +
 5 files changed, 82 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v6 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 v6 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 v6 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>
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..b80a552dad883 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 = "pci14e4,449d";
+			reg = <0x410000 0 0 0 0>;
+			clocks = <&hym8563>;
+			clock-names = "lpo";
+		};
+	};
 };
 
 &pwm11 {
-- 
2.34.1

[PATCH v6 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

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index e406e11481a62..7e0a2ad5c7c8a 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"
@@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 {
 	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;
+
 	/* Apple ARM64 platforms have their own idea of board type, passed in
 	 * via the device tree. They also have an antenna SKU parameter
 	 */
@@ -113,8 +118,13 @@ 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"))
+	clk = devm_clk_get_optional_enabled(dev, "lpo");
+	if (!IS_ERR_OR_NULL(clk)) {
+		brcmf_dbg(INFO, "enabling 32kHz clock\n");
+		clk_set_rate(clk, 32768);
+	} else {
 		return;
+	}
 
 	err = brcmf_of_get_country_codes(dev, settings);
 	if (err)
-- 
2.34.1

[PATCH v6 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 06698a714b523..938632daf30a9 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[];
 
@@ -2477,9 +2511,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);
@@ -2665,14 +2700,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[] = {
@@ -2700,9 +2735,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 v6 3/5] arm64: dts: rockchip: Add AP6275P wireless support to Khadas Edge 2

[Arend Van Spriel]

On July 31, 2024 8:11:58 AM Jacobe Zang <jacobe.zang@wesion.com> wrote:

> 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>
> 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..b80a552dad883 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 = "pci14e4,449d";

Probably need to add "brcm,bcm4329-fmac" as well here.

Regards,
Arend

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

[Alexey Charkov]

Hi Jacobe,


On 31/07/2024 9:11 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
 >
 > Co-developed-by: Ondrej Jirman <megi@xff.cz>
 > Signed-off-by: Ondrej Jirman <megi@xff.cz>
 > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
 > ---
 >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
 >  1 file changed, 11 insertions(+), 1 deletion(-)
 >
 > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
 > index e406e11481a62..7e0a2ad5c7c8a 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"
 > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum 
brcmf_bus_type bus_type,
 >  {
 >      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;

Did you test this? The DTS patch you sent as part of this series doesn't 
list "brcm,bcm4329-fmac" in the compatible, so this will probably return 
right here, skipping over the rest of your patch.

Please test before resending, both with and without the driver for the 
Bluetooth part of the chip (since it also touches clocks).

You are also changing the behavior for other systems by putting this 
check further up the probe path, which might break things for no reason. 
Better put your clk-related addition below where this check was 
originally, rather than reorder stuff you don't have to reorder.

 > +
 >      /* Apple ARM64 platforms have their own idea of board type, 
passed in
 >       * via the device tree. They also have an antenna SKU parameter
 >       */
 > @@ -113,8 +118,13 @@ 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"))
 > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
 > +    if (!IS_ERR_OR_NULL(clk)) {
 > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
 > +        clk_set_rate(clk, 32768);
 > +    } else {
 >          return;

Why return here? If the clock is optional, a lot of systems will not 
have it - that shouldn't prevent the driver from probing. And you are 
still not handling the -EPROBE_DEFER case which was mentioned on your 
previous submission.

Best regards,
Alexey

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

[Arend van Spriel]

On 7/31/2024 12:16 PM, Alexey Charkov wrote:
> Hi Jacobe,
> 
> 
> On 31/07/2024 9:11 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
>  >
>  > Co-developed-by: Ondrej Jirman <megi@xff.cz>
>  > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>  > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>  > ---
>  >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
>  >  1 file changed, 11 insertions(+), 1 deletion(-)
>  >
>  > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>  > index e406e11481a62..7e0a2ad5c7c8a 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"
>  > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum 
> brcmf_bus_type bus_type,
>  >  {
>  >      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;
> 
> Did you test this? The DTS patch you sent as part of this series doesn't 
> list "brcm,bcm4329-fmac" in the compatible, so this will probably return 
> right here, skipping over the rest of your patch.
> 
> Please test before resending, both with and without the driver for the 
> Bluetooth part of the chip (since it also touches clocks).
> 
> You are also changing the behavior for other systems by putting this 
> check further up the probe path, which might break things for no reason. 
> Better put your clk-related addition below where this check was 
> originally, rather than reorder stuff you don't have to reorder.

That was upon my suggestion. That check was originally at the top of the 
function, but people added stuff before that. I agree that makes the 
compatible "brcm,brcm4329-fmac" required which is what the textual 
binding stated before the switch to YAML was made:

"""
Broadcom BCM43xx Fullmac wireless SDIO devices

This node provides properties for controlling the Broadcom wireless 
device. The
node is expected to be specified as a child node to the SDIO controller that
connects the device to the system.

Required properties:

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

Not sure whether this is still true for YAML version (poor YAML reading 
skills ;-) ), but it should as the switch from textual to YAML should 
not have changed the bindings specification.

>  > +
>  >      /* Apple ARM64 platforms have their own idea of board type, 
> passed in
>  >       * via the device tree. They also have an antenna SKU parameter
>  >       */
>  > @@ -113,8 +118,13 @@ 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"))
>  > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>  > +    if (!IS_ERR_OR_NULL(clk)) {
>  > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
>  > +        clk_set_rate(clk, 32768);
>  > +    } else {
>  >          return;
> 
> Why return here? If the clock is optional, a lot of systems will not 
> have it - that shouldn't prevent the driver from probing. And you are 
> still not handling the -EPROBE_DEFER case which was mentioned on your 
> previous submission.

Right. The else statement above could/should be:

} else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
         return PTR_ERR(clk);
}

Regards,
Arend

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

[Alexey Charkov]

On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
> > Hi Jacobe,
> >
> >
> > On 31/07/2024 9:11 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
> >  >
> >  > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> >  > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> >  > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> >  > ---
> >  >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
> >  >  1 file changed, 11 insertions(+), 1 deletion(-)
> >  >
> >  > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >  > index e406e11481a62..7e0a2ad5c7c8a 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"
> >  > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
> > brcmf_bus_type bus_type,
> >  >  {
> >  >      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;
> >
> > Did you test this? The DTS patch you sent as part of this series doesn't
> > list "brcm,bcm4329-fmac" in the compatible, so this will probably return
> > right here, skipping over the rest of your patch.
> >
> > Please test before resending, both with and without the driver for the
> > Bluetooth part of the chip (since it also touches clocks).
> >
> > You are also changing the behavior for other systems by putting this
> > check further up the probe path, which might break things for no reason.
> > Better put your clk-related addition below where this check was
> > originally, rather than reorder stuff you don't have to reorder.
>
> That was upon my suggestion. That check was originally at the top of the
> function, but people added stuff before that. I agree that makes the
> compatible "brcm,brcm4329-fmac" required which is what the textual
> binding stated before the switch to YAML was made:
>
> """
> Broadcom BCM43xx Fullmac wireless SDIO devices
>
> This node provides properties for controlling the Broadcom wireless
> device. The
> node is expected to be specified as a child node to the SDIO controller that
> connects the device to the system.
>
> Required properties:
>
>   - compatible : Should be "brcm,bcm4329-fmac".
> """
>
> Not sure whether this is still true for YAML version (poor YAML reading
> skills ;-) ), but it should as the switch from textual to YAML should
> not have changed the bindings specification.
>
> >  > +
> >  >      /* Apple ARM64 platforms have their own idea of board type,
> > passed in
> >  >       * via the device tree. They also have an antenna SKU parameter
> >  >       */
> >  > @@ -113,8 +118,13 @@ 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"))
> >  > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
> >  > +    if (!IS_ERR_OR_NULL(clk)) {
> >  > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
> >  > +        clk_set_rate(clk, 32768);
> >  > +    } else {
> >  >          return;
> >
> > Why return here? If the clock is optional, a lot of systems will not
> > have it - that shouldn't prevent the driver from probing. And you are
> > still not handling the -EPROBE_DEFER case which was mentioned on your
> > previous submission.
>
> Right. The else statement above could/should be:
>
> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
>          return PTR_ERR(clk);
> }

... plus change the function prototype to return int and propagate
that error code through brcmf_get_module_param to brcmf_pcie_probe's
return value. I guess checking clk for NULL is also redundant in this
case?

Best,
Alexey

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

[Arend van Spriel]

On 7/31/2024 2:01 PM, Alexey Charkov wrote:
> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
>>> Hi Jacobe,
>>>
>>>
>>> On 31/07/2024 9:11 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
>>>   >
>>>   > Co-developed-by: Ondrej Jirman <megi@xff.cz>
>>>   > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>>   > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>   > ---
>>>   >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
>>>   >  1 file changed, 11 insertions(+), 1 deletion(-)
>>>   >
>>>   > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>   > index e406e11481a62..7e0a2ad5c7c8a 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"
>>>   > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
>>> brcmf_bus_type bus_type,
>>>   >  {
>>>   >      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;
>>>
>>> Did you test this? The DTS patch you sent as part of this series doesn't
>>> list "brcm,bcm4329-fmac" in the compatible, so this will probably return
>>> right here, skipping over the rest of your patch.
>>>
>>> Please test before resending, both with and without the driver for the
>>> Bluetooth part of the chip (since it also touches clocks).
>>>
>>> You are also changing the behavior for other systems by putting this
>>> check further up the probe path, which might break things for no reason.
>>> Better put your clk-related addition below where this check was
>>> originally, rather than reorder stuff you don't have to reorder.
>>
>> That was upon my suggestion. That check was originally at the top of the
>> function, but people added stuff before that. I agree that makes the
>> compatible "brcm,brcm4329-fmac" required which is what the textual
>> binding stated before the switch to YAML was made:
>>
>> """
>> Broadcom BCM43xx Fullmac wireless SDIO devices
>>
>> This node provides properties for controlling the Broadcom wireless
>> device. The
>> node is expected to be specified as a child node to the SDIO controller that
>> connects the device to the system.
>>
>> Required properties:
>>
>>    - compatible : Should be "brcm,bcm4329-fmac".
>> """
>>
>> Not sure whether this is still true for YAML version (poor YAML reading
>> skills ;-) ), but it should as the switch from textual to YAML should
>> not have changed the bindings specification.
>>
>>>   > +
>>>   >      /* Apple ARM64 platforms have their own idea of board type,
>>> passed in
>>>   >       * via the device tree. They also have an antenna SKU parameter
>>>   >       */
>>>   > @@ -113,8 +118,13 @@ 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"))
>>>   > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>>>   > +    if (!IS_ERR_OR_NULL(clk)) {
>>>   > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>>   > +        clk_set_rate(clk, 32768);
>>>   > +    } else {
>>>   >          return;
>>>
>>> Why return here? If the clock is optional, a lot of systems will not
>>> have it - that shouldn't prevent the driver from probing. And you are
>>> still not handling the -EPROBE_DEFER case which was mentioned on your
>>> previous submission.
>>
>> Right. The else statement above could/should be:
>>
>> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
>>           return PTR_ERR(clk);
>> }
> 
> ... plus change the function prototype to return int and propagate
> that error code through brcmf_get_module_param to brcmf_pcie_probe's
> return value. I guess checking clk for NULL is also redundant in this
> case?

Only wanted to give the suggestion to get started. Propagating the 
return value seemed obvious to me, but you are absolutely right. 
PTR_ERR(NULL) will probably be something else than -EPROBE_DEFER but it 
seems odd to me. Maybe PTR_ERR_OR_ZERO(clk) is a better option here.

Regards,
Arend

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

[Alexey Charkov]

On Wed, Jul 31, 2024 at 3:32 PM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 7/31/2024 2:01 PM, Alexey Charkov wrote:
> > On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
> > <arend.vanspriel@broadcom.com> wrote:
> >>
> >> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
> >>> Hi Jacobe,
> >>>
> >>>
> >>> On 31/07/2024 9:11 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
> >>>   >
> >>>   > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> >>>   > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> >>>   > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> >>>   > ---
> >>>   >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
> >>>   >  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>   >
> >>>   > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>>   > index e406e11481a62..7e0a2ad5c7c8a 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"
> >>>   > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
> >>> brcmf_bus_type bus_type,
> >>>   >  {
> >>>   >      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;
> >>>
> >>> Did you test this? The DTS patch you sent as part of this series doesn't
> >>> list "brcm,bcm4329-fmac" in the compatible, so this will probably return
> >>> right here, skipping over the rest of your patch.
> >>>
> >>> Please test before resending, both with and without the driver for the
> >>> Bluetooth part of the chip (since it also touches clocks).
> >>>
> >>> You are also changing the behavior for other systems by putting this
> >>> check further up the probe path, which might break things for no reason.
> >>> Better put your clk-related addition below where this check was
> >>> originally, rather than reorder stuff you don't have to reorder.
> >>
> >> That was upon my suggestion. That check was originally at the top of the
> >> function, but people added stuff before that. I agree that makes the
> >> compatible "brcm,brcm4329-fmac" required which is what the textual
> >> binding stated before the switch to YAML was made:
> >>
> >> """
> >> Broadcom BCM43xx Fullmac wireless SDIO devices
> >>
> >> This node provides properties for controlling the Broadcom wireless
> >> device. The
> >> node is expected to be specified as a child node to the SDIO controller that
> >> connects the device to the system.
> >>
> >> Required properties:
> >>
> >>    - compatible : Should be "brcm,bcm4329-fmac".
> >> """
> >>
> >> Not sure whether this is still true for YAML version (poor YAML reading
> >> skills ;-) ), but it should as the switch from textual to YAML should
> >> not have changed the bindings specification.
> >>
> >>>   > +
> >>>   >      /* Apple ARM64 platforms have their own idea of board type,
> >>> passed in
> >>>   >       * via the device tree. They also have an antenna SKU parameter
> >>>   >       */
> >>>   > @@ -113,8 +118,13 @@ 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"))
> >>>   > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
> >>>   > +    if (!IS_ERR_OR_NULL(clk)) {
> >>>   > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
> >>>   > +        clk_set_rate(clk, 32768);
> >>>   > +    } else {
> >>>   >          return;
> >>>
> >>> Why return here? If the clock is optional, a lot of systems will not
> >>> have it - that shouldn't prevent the driver from probing. And you are
> >>> still not handling the -EPROBE_DEFER case which was mentioned on your
> >>> previous submission.
> >>
> >> Right. The else statement above could/should be:
> >>
> >> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
> >>           return PTR_ERR(clk);
> >> }
> >
> > ... plus change the function prototype to return int and propagate
> > that error code through brcmf_get_module_param to brcmf_pcie_probe's
> > return value. I guess checking clk for NULL is also redundant in this
> > case?
>
> Only wanted to give the suggestion to get started. Propagating the
> return value seemed obvious to me, but you are absolutely right.
> PTR_ERR(NULL) will probably be something else than -EPROBE_DEFER but it
> seems odd to me. Maybe PTR_ERR_OR_ZERO(clk) is a better option here.

Indeed. Perhaps something along the lines of:

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

... which should then go at the very end of brcmf_of_probe. And all of
the existing void returns should get appropriate errno's. And the
functions prototypes should be updated along the call chain. And then
it would still only work after pwrseq is added to ensure that power
and wake signals are applied correctly along with this clock, as
Sebastian pointed out in the other thread :)

Which really prompts a question: should this clock be added to the
PCIe driver and the respective DT binding in the first place, or
should it instead be claimed by pwrseq, leaving brcmfmac alone?

Best regards,
Alexey

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

[Jacobe Zang]

>>On 7/31/2024 2:01 PM, Alexey Charkov wrote:
>>> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>>
>>>> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
>>>>> Hi Jacobe,
>>>>>
>>>>>
>>>>> On 31/07/2024 9:11 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
>>>>>   >
>>>>>   > Co-developed-by: Ondrej Jirman <megi@xff.cz>
>>>>>   > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>>>>   > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>>>   > ---
>>>>>   >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
>>>>>   >  1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>   >
>>>>>   > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>>>>   > index e406e11481a62..7e0a2ad5c7c8a 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"
>>>>>   > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
>>>>> brcmf_bus_type bus_type,
>>>>>   >  {
>>>>>   >      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;
>>>>>
>>>>> Did you test this? The DTS patch you sent as part of this series doesn't
>>>>> list "brcm,bcm4329-fmac" in the compatible, so this will probably return
>>>>> right here, skipping over the rest of your patch.
>>>>>
>>>>> Please test before resending, both with and without the driver for the
>>>>> Bluetooth part of the chip (since it also touches clocks).
>>>>>
>>>>> You are also changing the behavior for other systems by putting this
>>>>> check further up the probe path, which might break things for no reason.
>>>>> Better put your clk-related addition below where this check was
>>>>> originally, rather than reorder stuff you don't have to reorder.
>>>>
>>>> That was upon my suggestion. That check was originally at the top of the
>>>> function, but people added stuff before that. I agree that makes the
>>>> compatible "brcm,brcm4329-fmac" required which is what the textual
>>>> binding stated before the switch to YAML was made:
>>>>
>>>> """
>>>> Broadcom BCM43xx Fullmac wireless SDIO devices
>>>>
>>>> This node provides properties for controlling the Broadcom wireless
>>>> device. The
>>>> node is expected to be specified as a child node to the SDIO controller that
>>>> connects the device to the system.
>>>>
>>>> Required properties:
>>>>
>>>>    - compatible : Should be "brcm,bcm4329-fmac".
>>>> """
>>>>
>>>> Not sure whether this is still true for YAML version (poor YAML reading
>>>> skills ;-) ), but it should as the switch from textual to YAML should
>>>> not have changed the bindings specification.
>>>>
>>>>>   > +
>>>>>   >      /* Apple ARM64 platforms have their own idea of board type,
>>>>> passed in
>>>>>   >       * via the device tree. They also have an antenna SKU parameter
>>>>>   >       */
>>>>>   > @@ -113,8 +118,13 @@ 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"))
>>>>>   > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>>>>>   > +    if (!IS_ERR_OR_NULL(clk)) {
>>>>>   > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>>>>   > +        clk_set_rate(clk, 32768);
>>>>>   > +    } else {
>>>>>   >          return;
>>>>>
>>>>> Why return here? If the clock is optional, a lot of systems will not
>>>>> have it - that shouldn't prevent the driver from probing. And you are
>>>>> still not handling the -EPROBE_DEFER case which was mentioned on your
>>>>> previous submission.
>>>>
>>>> Right. The else statement above could/should be:
>>>>
>>>> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
>>>>           return PTR_ERR(clk);
>>>> }
>>>
>>> ... plus change the function prototype to return int and propagate
>>> that error code through brcmf_get_module_param to brcmf_pcie_probe's
>>> return value. I guess checking clk for NULL is also redundant in this
>>> case?
>>
>>Only wanted to give the suggestion to get started. Propagating the
>>return value seemed obvious to me, but you are absolutely right.
>>PTR_ERR(NULL) will probably be something else than -EPROBE_DEFER but it
>>seems odd to me. Maybe PTR_ERR_OR_ZERO(clk) is a better option here.
> 
> Indeed. Perhaps something along the lines of:
> 
>        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);
>        }
> 
> ... which should then go at the very end of brcmf_of_probe. And all of

But before end of brcmf_of_probe is to set interrupt configuration which
wifi chip connect via sdio. Like this:
```
	if (bus_type != BRCMF_BUSTYPE_SDIO)
		return;

	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;

	irq = irq_of_parse_and_map(np, 0);
	if (!irq) {
		brcmf_err("interrupt could not be mapped\n");
		return;
	}
	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;
```
So I think the interrupt should be set in the if statement while
bus_type==BRCMF_BUSTYPE_SDIO, and add else statement
to enable clock(or simply put it at the end as Alexey said). And
can also use else-if statement to deal with
bus_type == BRCMF_BUSTYPE_USB or PCIE in the future.

> the existing void returns should get appropriate errno's. And the
> functions prototypes should be updated along the call chain. And then
> it would still only work after pwrseq is added to ensure that power
> and wake signals are applied correctly along with this clock, as
> Sebastian pointed out in the other thread :)
> 
> Which really prompts a question: should this clock be added to the
> PCIe driver and the respective DT binding in the first place, or
> should it instead be claimed by pwrseq, leaving brcmfmac alone?

---
Best Regards
Jacobe

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

[Alexey Charkov]

On Thu, Aug 1, 2024 at 6:53 AM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>
> >>On 7/31/2024 2:01 PM, Alexey Charkov wrote:
> >>> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
> >>> <arend.vanspriel@broadcom.com> wrote:
> >>>>
> >>>> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
> >>>>> Hi Jacobe,
> >>>>>
> >>>>>
> >>>>> On 31/07/2024 9:11 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
> >>>>>   >
> >>>>>   > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> >>>>>   > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> >>>>>   > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> >>>>>   > ---
> >>>>>   >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
> >>>>>   >  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>>>   >
> >>>>>   > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >>>>>   > index e406e11481a62..7e0a2ad5c7c8a 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"
> >>>>>   > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
> >>>>> brcmf_bus_type bus_type,
> >>>>>   >  {
> >>>>>   >      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;
> >>>>>
> >>>>> Did you test this? The DTS patch you sent as part of this series doesn't
> >>>>> list "brcm,bcm4329-fmac" in the compatible, so this will probably return
> >>>>> right here, skipping over the rest of your patch.
> >>>>>
> >>>>> Please test before resending, both with and without the driver for the
> >>>>> Bluetooth part of the chip (since it also touches clocks).
> >>>>>
> >>>>> You are also changing the behavior for other systems by putting this
> >>>>> check further up the probe path, which might break things for no reason.
> >>>>> Better put your clk-related addition below where this check was
> >>>>> originally, rather than reorder stuff you don't have to reorder.
> >>>>
> >>>> That was upon my suggestion. That check was originally at the top of the
> >>>> function, but people added stuff before that. I agree that makes the
> >>>> compatible "brcm,brcm4329-fmac" required which is what the textual
> >>>> binding stated before the switch to YAML was made:
> >>>>
> >>>> """
> >>>> Broadcom BCM43xx Fullmac wireless SDIO devices
> >>>>
> >>>> This node provides properties for controlling the Broadcom wireless
> >>>> device. The
> >>>> node is expected to be specified as a child node to the SDIO controller that
> >>>> connects the device to the system.
> >>>>
> >>>> Required properties:
> >>>>
> >>>>    - compatible : Should be "brcm,bcm4329-fmac".
> >>>> """
> >>>>
> >>>> Not sure whether this is still true for YAML version (poor YAML reading
> >>>> skills ;-) ), but it should as the switch from textual to YAML should
> >>>> not have changed the bindings specification.
> >>>>
> >>>>>   > +
> >>>>>   >      /* Apple ARM64 platforms have their own idea of board type,
> >>>>> passed in
> >>>>>   >       * via the device tree. They also have an antenna SKU parameter
> >>>>>   >       */
> >>>>>   > @@ -113,8 +118,13 @@ 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"))
> >>>>>   > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
> >>>>>   > +    if (!IS_ERR_OR_NULL(clk)) {
> >>>>>   > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
> >>>>>   > +        clk_set_rate(clk, 32768);
> >>>>>   > +    } else {
> >>>>>   >          return;
> >>>>>
> >>>>> Why return here? If the clock is optional, a lot of systems will not
> >>>>> have it - that shouldn't prevent the driver from probing. And you are
> >>>>> still not handling the -EPROBE_DEFER case which was mentioned on your
> >>>>> previous submission.
> >>>>
> >>>> Right. The else statement above could/should be:
> >>>>
> >>>> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
> >>>>           return PTR_ERR(clk);
> >>>> }
> >>>
> >>> ... plus change the function prototype to return int and propagate
> >>> that error code through brcmf_get_module_param to brcmf_pcie_probe's
> >>> return value. I guess checking clk for NULL is also redundant in this
> >>> case?
> >>
> >>Only wanted to give the suggestion to get started. Propagating the
> >>return value seemed obvious to me, but you are absolutely right.
> >>PTR_ERR(NULL) will probably be something else than -EPROBE_DEFER but it
> >>seems odd to me. Maybe PTR_ERR_OR_ZERO(clk) is a better option here.
> >
> > Indeed. Perhaps something along the lines of:
> >
> >        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);
> >        }
> >
> > ... which should then go at the very end of brcmf_of_probe. And all of
>
> But before end of brcmf_of_probe is to set interrupt configuration which
> wifi chip connect via sdio. Like this:
> ```
>         if (bus_type != BRCMF_BUSTYPE_SDIO)
>                 return;
>
>         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;
>
>         irq = irq_of_parse_and_map(np, 0);
>         if (!irq) {
>                 brcmf_err("interrupt could not be mapped\n");
>                 return;
>         }
>         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;
> ```
> So I think the interrupt should be set in the if statement while
> bus_type==BRCMF_BUSTYPE_SDIO, and add else statement
> to enable clock(or simply put it at the end as Alexey said). And
> can also use else-if statement to deal with
> bus_type == BRCMF_BUSTYPE_USB or PCIE in the future.

SDIO devices might also want to enable a clock, so I think wrapping
the drive strength and interrupts handling into an if statement and
putting the clock-related stuff right after it (but not in the else
block) is better.

Best regards,
Alexey