Re: [PATCH 10/10] ARM: dts: qcom: apq8074-dragonboard: enable the USB3 host

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `reset-gpios` and `vdd-supply` properties for the USB hubs and peripherals will be completely ignored because their compatible strings are not supported by the kernel's onboard USB dev driver.
- [High] `qcom_ipq806x_usb_hs_phy_init()` overwrites the entire `HSUSB_PHY_CTRL_REG`, destroying bootloader-configured frequency straps for MSM8974.
- [High] SuperSpeed USB devices (AX88179 and PL2775) are incorrectly nested under the HighSpeed hub (`hub_2_0`) instead of the SuperSpeed hub (`hub_3_0`).
- [High] The SuperSpeed hub (`hub_3_0`) is assigned an invalid port address (`reg = <2>`), breaking DT node mapping for both root hubs.
--

--- Patch [10]: [PATCH 10/10] ARM: dts: qcom: apq8074-dragonboard: enable the USB3 host ---
commit 316b8b1bbfd2cabe66fe0293f9ed61e3a48ec197
Author: Dmitry Baryshkov <[email protected]>

ARM: dts: qcom: apq8074-dragonboard: enable the USB3 host

The USB3 port of the dragonboard drives the onboard TUSB8040A hub with
the AX88179 GbE controller and the PL2775 USB-SATA bridge behind it.
This commit enables the DWC3 controller in host mode along with its PHYs
and describes the onboard chain as device nodes under the controller.

> diff --git a/arch/arm/boot/dts/qcom/qcom-apq8074-dragonboard.dts b/arch/arm/boot/dts/qcom/qcom-apq8074-dragonboard.dts
> --- a/arch/arm/boot/dts/qcom/qcom-apq8074-dragonboard.dts
> +++ b/arch/arm/boot/dts/qcom/qcom-apq8074-dragonboard.dts

[ ... ]

> +	/* TI TUSB8040A four port hub */
> +	hub_2_0: hub@1 {
> +		compatible = "usb451,8043";

[Severity: High]
Will the reset-gpios and vdd-supply properties be ignored here?

The new compatible strings (usb451,8043, usb451,8041, usb67b,2775,
usbb95,1790) do not appear to be supported by the onboard_usb_dev driver's
onboard_dev_match array. The generic USB core parses these nodes but does
not natively handle reset-gpios.

Could this cause the reset lines to float and power sequencing to be
unmanaged, preventing proper enumeration?

> +		reg = <1>;
> +		peer-hub = <&hub_3_0>;
> +		reset-gpios = <&pm8941_gpios 6 GPIO_ACTIVE_LOW>;
> +		vdd-supply = <&pwr_c_3p3>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* Prolific PL2775 USB-SATA bridge on the DN0 port */
> +		device@1 {
> +			compatible = "usb67b,2775";

[Severity: High]
Are the SuperSpeed USB devices (AX88179 and PL2775) nested under the
incorrect hub here?

Since these are SuperSpeed capable devices, they will enumerate on the
SuperSpeed bus. By placing them under the HighSpeed hub (hub_2_0) instead
of the SuperSpeed hub (hub_3_0), the USB core won't be able to match the
enumerated device to a DT child.

Will this cause device-specific DT properties like reset-gpios to be
silently ignored during USB enumeration?

> +			reg = <1>;
> +			reset-gpios = <&pm8941_gpios 10 GPIO_ACTIVE_LOW>;
> +			vdd-supply = <&pwr_c_3p3>;
> +		};
> +
> +		/* ASIX AX88179 USB-GbE controller on the DN2 port */
> +		ethernet@3 {
> +			compatible = "usbb95,1790";
> +			reg = <3>;
> +			reset-gpios = <&pm8941_gpios 9 GPIO_ACTIVE_LOW>;
> +			vdd-supply = <&pwr_c_3p3>;
> +		};
> +	};
> +
> +	hub_3_0: hub@2 {
> +		compatible = "usb451,8041";
> +		reg = <2>;

[Severity: High]
Is this port address valid for the SuperSpeed hub?

The APQ8074 DWC3 controller has a single physical port, meaning both root hubs
should expose exactly one port (port 1). Because this uses reg = <2>, the
USB 3.0 root hub will look for port 1 and instead find hub_2_0 (which has
reg = <1>).

Does this leave hub_3_0 orphaned and break DT node mapping?

> +		peer-hub = <&hub_2_0>;
> +		reset-gpios = <&pm8941_gpios 6 GPIO_ACTIVE_LOW>;
> +		vdd-supply = <&pwr_c_3p3>;
> +	};
> +};

A related issue appears to exist in the PHY initialization sequence for this
hardware:

drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c:qcom_ipq806x_usb_hs_phy_init() {
    ...
    writel(val, phy_dwc3->base + HSUSB_PHY_CTRL_REG);
    ...
}

[Severity: High]
Does qcom_ipq806x_usb_hs_phy_init() overwrite the entire HSUSB_PHY_CTRL_REG?

If the MSM8974 HS PHY probe reuses this function, calling writel() without a
prior readl() will destroy the bootloader-configured reference frequency
straps. This seems to contradict the requirement to preserve these strap
bits.

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

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