Re: [PATCH v2 3/4] arm64: dts: nuvoton: ma35d1: add USB controllers and dual-port PHY node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The PHY driver fails to probe unconditionally because the new device tree node lacks required properties.
- [High] The PHY driver does not support multi-PHY indexing, causing the second USB port to remain uninitialized.
- [Low] The `sys` node lacks a `ranges` property, breaking standard device tree address translation for its children.
--

commit 9dc588096b739f5b70f171c1eb14764b230dfb57
Author: Joey Lu <a0987203069@gmail.com>

arm64: dts: nuvoton: ma35d1: add USB controllers and dual-port PHY node

This commit adds device tree nodes for the MA35D1 USB subsystem, including
the simple-mfd sys node, the usb-phy child node, and the DWC2, EHCI0/1, 
and OHCI0/1 controllers. It also enables them and sets up pinctrl in the 
board files.

> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> index e51b98f5bdce4..a6a354f283117 100644
> --- a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> @@ -83,9 +83,18 @@ soc {
>  		ranges;
>  
>  		sys: system-management@40460000 {
> -			compatible = "nuvoton,ma35d1-reset", "syscon";
> +			compatible = "nuvoton,ma35d1-reset", "syscon", "simple-mfd";
>  			reg = <0x0 0x40460000 0x0 0x200>;
>  			#reset-cells = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;

[Severity: Low]
Since this sys node is acting as a bus for usb_phy and now defines 
#address-cells and #size-cells, does it also need a "ranges;" property?

Standard device tree specification requires a ranges property in the
parent to map the child's address space. Omitting it here breaks standard
device tree address translation and can trigger dtbs_check warnings.

> +
> +			usb_phy: usb-phy@60 {
> +				compatible = "nuvoton,ma35d1-usb2-phy";
> +				reg = <0x60 0x14>;
> +				#phy-cells = <1>;
> +				status = "disabled";
> +			};

[Severity: High]
Will this PHY node fail to probe since it lacks the "nuvoton,sys" and
"clocks" properties?

Looking at ma35_usb_phy_probe() in drivers/phy/nuvoton/phy-ma35d1-usb2.c,
it unconditionally requests both of these:

    p_phy->sysreg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
                                                    "nuvoton,sys");
    if (IS_ERR(p_phy->sysreg))
        return dev_err_probe(&pdev->dev, PTR_ERR(p_phy->sysreg),
                             "Failed to get SYS registers\n");

Because they are omitted from this node, won't the lookup return -ENODEV
and cause all dependent USB controllers (DWC2, EHCI0/1, OHCI0/1) to defer 
indefinitely?

>  		};
>  
>  		clk: clock-controller@40460200 {

[ ... ]

> +		ehci1: usb@401c0000 {
> +			compatible = "generic-ehci";
> +			reg = <0x0 0x401c0000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clk HUSBH1_GATE>;
> +			phys = <&usb_phy 1>;

[Severity: High]
Does the current driver actually support indexing the PHY like this?

The device tree maps controllers to <&usb_phy 0> and <&usb_phy 1>, but 
looking at ma35_usb_phy_probe(), the driver registers with a simple xlate
that ignores the index:

    provider = devm_of_phy_provider_register(&pdev->dev,
                                             of_phy_simple_xlate);

Since of_phy_simple_xlate returns the first PHY object for all requests, 
and the power routines like ma35_usb_phy_power_on() are hardcoded to only 
manipulate PHY0 bits:

    regmap_update_bits(p_phy->sysreg, MA35_SYS_REG_USBPMISCR, 0x7,
                       (PHY0POR | PHY0SUSPEND));

Will the hardware for PHY1 (the dedicated host port) remain completely 
unpowered and fail to initialize when ehci1 and ohci1 attempt to use it?

> +			phy-names = "usb";
> +			companion = <&ohci1>;
> +			status = "disabled";
> +		};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625023958.569299-1-a0987203069@gmail.com?part=3