Re: [PATCH] Enable wifi on the BeagleV-Ahead

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing `keep-power-in-suspend` property breaks WoWLAN functionality.
- [Medium] Contradictory pull-up configuration on WiFi GPIOs causes power leakage.
- [Medium] Incorrect hardware-specific compatible string misrepresents the device.
- [Low] The `brcm,sd_head_align` and `brcm,sd_sgentry_align` properties are undocumented and ignored.
- [Low] The devicetree node name `brcmf_pwrseq` uses an underscore instead of a hyphen.
- [Low] Mixed space and tab indentation within the `wifi_pins` node.
--

commit 927889c06368228ec05bf0df849cea905cfb5d42
Author: Thomas Gerner <thomas.gerner@muenchen-mail.de>

Enable wifi on the BeagleV-Ahead

The BeagleV-Ahead board uses an AP6203BM WiFi chip from AMPAK Technology
Inc. connected to SDIO1. The chip is compatible to the broadcom wireless
driver.

> diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> index 91f3f9b987bc3..5e0dc47d263c9 100644
> --- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> +++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
[ ... ]
> @@ -86,6 +86,11 @@ hdmi_con_in: endpoint {
>  			};
>  		};
>  	};
> +
> +	brcmf_pwrseq: brcmf_pwrseq {

This isn't a bug, but does the node name violate the devicetree specification?
Section 2.2.1 and kernel conventions dictate using hyphens to separate words.
Using an underscore here will trigger a node_name_chars_strict warning from
the dtc compiler. Should it be renamed to brcmf-pwrseq or wifi-pwrseq?

> +		compatible = "mmc-pwrseq-simple";
> +		reset-gpios = <&gpio2 31 GPIO_ACTIVE_LOW>;	/* WL-REG-ON */
> +	};
>  };
[ ... ]
> @@ -239,6 +244,28 @@ rx-pins {
>  			slew-rate = <0>;
>  		};
>  	};
> +
> +	wifi_pins: wifi-pins {
> +	    host-wake-wl {

This isn't a bug, but are these lines indented with 4 spaces? The Linux
kernel coding style requires the exclusive use of tabs for indentation in
device tree sources.

> +			pins = "GPIO2_25";
> +			function = "gpio";
> +			bias-pull-up;

Since the host-wake interrupt is configured below as IRQ_TYPE_EDGE_RISING,
it implies the signal idles low. Does pulling up a signal that idles low
cause continuous power leakage? Should this be configured with bias-disable
or bias-pull-down instead?

> +			drive-strength = <1>;
> +			input-enable;
> +			input-schmitt-disable;
> +			slew-rate = <0>;
> +		};
> +
> +	    reg-on-wl {
> +		    pins = "GPIO2_31";
> +		    function = "gpio";
> +			bias-pull-up;

The mmc-pwrseq-simple configuration uses this pin as an active-low reset
and actively drives it low to power off the WiFi chip during sleep or
suspend. Could pulling up this pin while it is driven low cause continuous
current leakage to ground?

> +			drive-strength = <3>;
> +			input-disable;
> +			input-schmitt-disable;
> +			slew-rate = <0>;
> +	    };
> +	};
>  };
[ ... ]
> +&sdio1 {
> +	bus-width = <4>;
> +	max-frequency = <198000000>;
> +	status = "okay";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	non-removable;

Will Wake-on-WLAN (WoWLAN) fail to function without the
keep-power-in-suspend property here?

The wifi@1 node below configures an Out-Of-Band host-wake interrupt to
support WoWLAN. However, without keep-power-in-suspend on the sdio1 node,
the MMC core will not set the MMC_PM_KEEP_POWER flag and will assert the
reset GPIO to cut power to the WiFi chip during system suspend.

This would cause the WiFi module to lose its firmware state, rendering the
configured OOB interrupt useless.

> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&wifi_pins>;
> +	mmc-pwrseq = <&brcmf_pwrseq>;
> +
> +	wifi@1 {
> +		compatible = "cypress,cyw43012-fmac", "brcm,bcm4329-fmac";

Does this compatible string misrepresent the underlying hardware? 

The commit message states the board uses an AMPAK AP6203BM WiFi chip,
which is based on Broadcom BCM43436 silicon. However, this compatible
string corresponds to a dual-band Cypress chip architecture. Should it use
an appropriate BCM4343x compatible string (e.g., brcm,bcm43430-fmac)
instead?

> +		reg = <1>;
> +		brcm,sd_head_align = <4>;
> +		brcm,sd_sgentry_align = <512>;

Are these properties used by the kernel?

They do not appear to be defined in the brcm,bcm4329-fmac.yaml binding
schema, which strictly enforces unevaluatedProperties: false, and their
presence will cause make dtbs_check validation to fail.

A review of the brcmfmac driver also shows these properties are only used
for legacy C platform data structs and are never parsed from the device
tree.

> +		interrupt-parent = <&gpio2>;
> +		interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-names = "host-wake";
> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511141119.155369-1-thomas.gerner@muenchen-mail.de?part=1