[PATCH v4 0/5] Add AP6275P wireless support

[PATCH v4 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 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 Wi-Fi device
  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 |  9 ++++
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 52 ++++++++++++++++---
 .../broadcom/brcm80211/include/brcm_hw_ids.h  |  2 +
 5 files changed, 80 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v4 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>
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 v4 2/5] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P Wi-Fi device

[Jacobe Zang]

Add clocks and clock-names for brcm4329-fmac.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
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 v4 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. As a PCIe device the wifi@0 can be probed
without compatible.

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 v4 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.

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>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index e406e11481a62..4db188a41fd52 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,6 +71,7 @@ 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;
@@ -113,6 +115,13 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 		of_node_put(root);
 	}
 
+	clk = devm_clk_get_optional_enabled(dev, "lpo");
+	if (IS_ERR(clk))
+	if (clk) {
+		brcmf_dbg(INFO, "enabling 32kHz clock\n");
+		clk_set_rate(clk, 32768);
+	}
+
 	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
 		return;
 
-- 
2.34.1

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

[Dragan Simic]

Hello Jacobe,

Please see one comment below.

On 2024-07-29 09:01, Jacobe Zang wrote:
> WiFi modules often require 32kHz clock to function. Add support to
> enable the clock to PCIe driver.
> 
> 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>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> index e406e11481a62..4db188a41fd52 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,6 +71,7 @@ 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;
> @@ -113,6 +115,13 @@ void brcmf_of_probe(struct device *dev, enum
> brcmf_bus_type bus_type,
>  		of_node_put(root);
>  	}
> 
> +	clk = devm_clk_get_optional_enabled(dev, "lpo");
> +	if (IS_ERR(clk))
> +	if (clk) {

These two lines looks really confusing.  Shouldn't it be just a single
"if (!IS_ERR(clk)) {" line instead?

> +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
> +		clk_set_rate(clk, 32768);
> +	}
> +
>  	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
>  		return;

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

[Jacobe Zang]

>> +     clk = devm_clk_get_optional_enabled(dev, "lpo");
>> +     if (IS_ERR(clk))
>> +     if (clk) {
>
> These two lines looks really confusing.  Shouldn't it be just a single
> "if (!IS_ERR(clk)) {" line instead?

Thanks for the correction... I forgot to organize this...

---
Best Regards
Jacobe

Re: [PATCH v4 2/5] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P Wi-Fi device

[Krzysztof Kozlowski]

On 29/07/2024 09:00, Jacobe Zang wrote:
> Add clocks and clock-names for brcm4329-fmac.

Why? Which devices have it? If only your newest addon, then squash the
patches and add appropriate allOf:if:then disallowing the clocks for
others. Or maybe all of them have it? Why commit msg does not explain
anything about the hardware?

> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> ---

Best regards,
Krzysztof

Re: [PATCH v4 5/5] wifi: brcmfmac: add flag for random seed during firmware download

[Arend van Spriel]

On 7/29/2024 9:01 AM, Jacobe Zang wrote:
> 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.

Not exactly what I meant by using my patch "as is". I would have kept 
the two patches separate, but it is not a deal breaker for me.

Regards,
Arend

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

Re: [PATCH v4 2/5] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P Wi-Fi device

[Jacobe Zang]

>> Add clocks and clock-names for brcm4329-fmac.
>
> Why? Which devices have it? If only your newest addon, then squash the
> patches and add appropriate allOf:if:then disallowing the clocks for
> others. Or maybe all of them have it? Why commit msg does not explain
> anything about the hardware?

Alright... Becuase of the datasheet said hardware has one LPO clock input. So
I will add allOf:if:then for this specific hardware

---
Best Regards
Jacobe

Re: [PATCH v4 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.
>
> Not exactly what I meant by using my patch "as is". I would have kept
> the two patches separate, but it is not a deal breaker for me.

Oh..so the correct approach should be for me to split this commit separately?

---
Best Regards
Jacobe

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

[Ondřej Jirman]

On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote:
> Hello Jacobe,
> 
> [...]
>
> > 
> > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
> > +	if (IS_ERR(clk))
> > +	if (clk) {
> 
> These two lines looks really confusing.  Shouldn't it be just a single
> "if (!IS_ERR(clk)) {" line instead?

It should be `!IS_ERR(clk) && clk` otherwise the debug message will be
incorrect.

Kind regards,
	o.

> > +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
> > +		clk_set_rate(clk, 32768);
> > +	}
> > +
> >  	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
> >  		return;

Re: [PATCH v4 2/5] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P Wi-Fi device

[Arend van Spriel]

On 7/29/2024 10:17 AM, Jacobe Zang wrote:
>>> Add clocks and clock-names for brcm4329-fmac.
>>
>> Why? Which devices have it? If only your newest addon, then squash the
>> patches and add appropriate allOf:if:then disallowing the clocks for
>> others. Or maybe all of them have it? Why commit msg does not explain
>> anything about the hardware?
> 
> Alright... Becuase of the datasheet said hardware has one LPO clock input. So
> I will add allOf:if:then for this specific hardware

Maybe this can help clarifying the commit message. All Broadcom wireless 
chips have this clock input, but often the chip end up on a wifi module 
that may or may not provide the clock. From chip perspective the clock 
input is always present, but it is optional as the chip can also use an 
internal clock.

As such I would not choose to make this specific to this AP6275P 
hardware (just my 2 cents).

Regards,
Arend

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

[Dragan Simic]

Hello Ondrej,

On 2024-07-29 10:44, Ondřej Jirman wrote:
> On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote:
>> Hello Jacobe,
>> 
>> [...]
>> 
>> >
>> > +	clk = devm_clk_get_optional_enabled(dev, "lpo");
>> > +	if (IS_ERR(clk))
>> > +	if (clk) {
>> 
>> These two lines looks really confusing.  Shouldn't it be just a single
>> "if (!IS_ERR(clk)) {" line instead?
> 
> It should be `!IS_ERR(clk) && clk` otherwise the debug message will be
> incorrect.

Ah, I see now, thanks.  There's also IS_ERR_OR_NULL, so the condition
can actually be "!IS_ERR_OR_NULL(clk)".

>> > +		brcmf_dbg(INFO, "enabling 32kHz clock\n");
>> > +		clk_set_rate(clk, 32768);
>> > +	}
>> > +
>> >  	if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
>> >  		return;

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

[Arend van Spriel]

On 7/29/2024 11:12 AM, Dragan Simic wrote:
> Hello Ondrej,
> 
> On 2024-07-29 10:44, Ondřej Jirman wrote:
>> On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote:
>>> Hello Jacobe,
>>>
>>> [...]
>>>
>>> >
>>> > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
>>> > +    if (IS_ERR(clk))
>>> > +    if (clk) {
>>>
>>> These two lines looks really confusing.  Shouldn't it be just a single
>>> "if (!IS_ERR(clk)) {" line instead?
>>
>> It should be `!IS_ERR(clk) && clk` otherwise the debug message will be
>> incorrect.
> 
> Ah, I see now, thanks.  There's also IS_ERR_OR_NULL, so the condition
> can actually be "!IS_ERR_OR_NULL(clk)".

++ best suggestion

>>> > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>> > +        clk_set_rate(clk, 32768);
>>> > +    }
>>> > +
>>> >      if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
>>> >          return;