Re: [PATCH v2 13/14] arm64: dts: qcom: ipq5018: Enable USB Super-Speed PHY

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Power Management Regression: Missing `vdd-supply` on `usbphy0` causes USB 2.0 to lose power when `usbphy1` is suspended.
- [Medium] Missing Pin Control Configuration for the 5.0V Regulator GPIO.
- [Low] Misleading Devicetree Node Name for 5.0V Regulator.
--

--- Patch [13]: [PATCH v2 13/14] arm64: dts: qcom: ipq5018: Enable USB Super-Speed PHY ---
commit 0e34c498b874a88fdd6ad0b61268476b5d10f482
Author: George Moussalem <[email protected]>

arm64: dts: qcom: ipq5018: Enable USB Super-Speed PHY
    
Add the 5V supply regulator and enable USB Super-Speed PHY support.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> index fcd5a078a2b08..0b147c9529149 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> @@ -22,6 +22,14 @@ aliases {
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> +
> +	regulator_fixed_5p0: regulator-s0500 {

[Severity: Low]
This isn't a bug, but the node name regulator-s0500 implies a 500mV supply.

Since the regulator is explicitly configured for 5000000 microvolts, could 
this cause confusion? The fixed-regulator device tree binding schema typically 
prefers regulator-5v0 for a 5.0V regulator.

> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-name = "fixed_5p0";
> +		gpios = <&tlmm 24 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Does this rely on the bootloader to leave the pin multiplexed as a GPIO?

There are no corresponding pinctrl-0 states defined or assigned here to
guarantee that gpio24 is multiplexed as a general-purpose GPIO rather than
an alternate function.

If the bootloader leaves it in the wrong electrical state, could USB power 
initialization silently fail?

> +	};
>  };

[ ... ]

> @@ -117,6 +125,12 @@ &usbphy0 {
>  	status = "okay";
>  };
>  
> +&usbphy1 {
> +	vdd-supply = <&regulator_fixed_5p0>;

[Severity: High]
Will this cause USB 2.0 devices to lose power when usbphy1 is suspended?

The single 5V GPIO regulator provides power for the port used by both
usbphy0 (USB 2.0) and usbphy1 (USB 3.0 SS). 

If usbphy1 is suspended by runtime PM (or inactive when only a USB 2.0 
device is connected), the regulator core will disable regulator_fixed_5p0 
because its only known consumer no longer requires it. 

Should usbphy0 also explicitly declare vdd-supply = <&regulator_fixed_5p0>
so the regulator core keeps the power on for USB 2.0 devices?

> +
> +	status = "okay";
> +};
> +
>  &xo_board_clk {
>  	clock-div = <4>;
>  	clock-mult = <1>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy