Re: [PATCH v5 3/3] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] arm64: dts: cix: usb4/usb5 aliases used to index syscon mode strap bits
- [Medium] usb: cdns3: cdnsp_sky1_probe() returns positive value on invalid ID
- [Low] arm64: dts: cix: invalid simple-bus node name usb-rcsu@91c0300
- [Low] usb: cdns3: cdnsp_sky1_probe() parses undocumented DT properties
- [Low] usb: cdns3: cdnsp_sky1_drd_init() disables clocks before acquiring them
- [Low] usb: cdns3: Kconfig help text for USB_CDNSP_SKY1 lists wrong compatible
--

commit f03c94102bbd5a8d24fc4ea8c5c765d7b2a4ab36
Author: Hongliang Yang (杨宏亮) <hongliang.yang@cixtech.com>

arm64: dts: cix: Add USB4/USB5 nodes for Sky1

This commit adds the USB4 and USB5 Cadence USBSSP controller nodes wrapped in
a shared RCSU simple-bus region for the CIX Sky1 SoC in sky1.dtsi, and
enables both controllers in host mode on the Radxa Orion O6 board.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 06a311ee14ef8..c10db0ff9af64 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -17,6 +17,11 @@ / {
>  	#address-cells = <2>;
>  	#size-cells = <2>;
>  
> +	aliases {
> +		usb4 = &usb4;
> +		usb5 = &usb5;
> +	};

[Severity: Medium]
Are these usb4 and usb5 aliases being used to index hardware mode strap bits
in the S5 syscon?

In drivers/usb/cdns3/cdnsp-sky1.c, cdnsp_sky1_probe() looks up the alias ID
or an undocumented id property and passes it to cdnsp_sky1_set_mode_by_id():

drivers/usb/cdns3/cdnsp-sky1.c:cdnsp_sky1_probe() {
	...
	ret = of_alias_get_id(dev->of_node, "usb");
	if (ret == -ENODEV) {
		if (device_property_read_u32(dev, "id", &ret))
			ret = -ENODEV;
	}
	if (ret < 0 || ret >=  SKY1_USB_S5_NUM) {
		dev_err(dev, "get alias failed.\n");
		return ret;
	}
	data->id = ret;
	...
}

drivers/usb/cdns3/cdnsp-sky1.c:cdnsp_sky1_set_mode_by_id() {
	struct cdnsp_sky1 *data = dev_get_drvdata(dev);

	return regmap_update_bits(data->usb_syscon,
				  strap_signals[data->id].offset,
				  GENMASK(strap_signals[data->id].bit + 1,
					  strap_signals[data->id].bit),
				  mode << strap_signals[data->id].bit);
}

Documentation/devicetree/bindings/writing-bindings.rst states that nodes
should not rely on custom aliases or instance ID properties to parameterize
how they act on a syscon, and should instead pass the register or bit offset
cells directly in the cix,usb-syscon phandle arguments.

With the current approach, if a board dts omits the alias (as in the example
in Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml, which also
does not document any alias or id requirement), cdnsp_sky1_probe() fails with
-ENODEV. Conversely, if a board renumbers its aliases (for example,
usb0 = &usb4), cdnsp_sky1_set_mode_by_id() will program the wrong
controller's strap bits in s5_syscon. Also, placing aliases in the SoC dtsi
exposes aliases to disabled nodes on boards that do not enable these
controllers.

Could the syscon register offset and bit position be passed as phandle
arguments in cix,usb-syscon instead of using usb aliases?

[Severity: Medium]
Related to the alias lookup in cdnsp_sky1_probe() shown above, what happens
if the alias ID or id property is greater than or equal to SKY1_USB_S5_NUM?

	if (ret < 0 || ret >=  SKY1_USB_S5_NUM) {
		dev_err(dev, "get alias failed.\n");
		return ret;
	}

When ret >= SKY1_USB_S5_NUM, does cdnsp_sky1_probe() return a positive
integer to really_probe() instead of a negative errno such as -EINVAL?

> +
>  	cpus {
>  		#address-cells = <2>;
>  		#size-cells = <0>;
> @@ -606,6 +611,96 @@ mbox_ap2sfh: mailbox@80a0000 {
>  			cix,mbox-dir = "tx";
>  		};
>  
> +		usbss_rcsu: usb-rcsu@91c0300 {
> +			compatible = "simple-bus";

[Severity: Low]
Does the node name usb-rcsu@91c0300 pass dtbs_check and dt_binding_check
when paired with compatible = "simple-bus"?

The dtschema simple-bus.yaml schema requires nodes with only
compatible = "simple-bus" to use a node name matching
^(bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$. Should this node (and the corresponding
node in the example in Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml)
be named bus@91c0300 instead?

> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges = <0x00 0x00000000 0x00 0x091c0300 0x00 0x150>,
> +				<0x00 0x091d0000 0x00 0x091d0000 0x00 0x20000>;
> +
> +			usb4: usb@14 {
> +				compatible = "cix,sky1-usbssp";

[Severity: Low]
In drivers/usb/cdns3/Kconfig, the help text for config USB_CDNSP_SKY1 says:

	  Glue driver for the Cadence USB dual-role controllers on CIX Sky1
	  (device tree compatible cix,sky1-usb3). It enables clocks and resets

Should that help text say cix,sky1-usbssp to match cdns_sky1_of_match,
cix,sky1-usbssp.yaml, and sky1.dtsi?

[Severity: Low]
When cdnsp_sky1_probe() binds to these cix,sky1-usbssp nodes, it also queries
several properties that are not documented in
Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml:

drivers/usb/cdns3/cdnsp-sky1.c:cdnsp_sky1_probe() {
	...
	if (ret == -ENODEV) {
		if (device_property_read_u32(dev, "id", &ret))
			ret = -ENODEV;
	}
	...
	data->u3_disable = device_property_read_bool(dev, "u3-port-disable");
	data->ssp_disable = device_property_read_bool(dev, "ssp-disable");
	...
	data->oc_gpio = devm_gpiod_get_optional(data->dev, "oc", GPIOD_IN);
	...
}

Because cix,sky1-usbssp.yaml sets unevaluatedProperties: false, any dts using
id, u3-port-disable, ssp-disable, or oc-gpios will fail schema validation, and
data->oc_gpio is never referenced again after cdnsp_sky1_probe(). Should
these undocumented property reads and the unused oc_gpio handling be removed
from cdnsp_sky1_probe(), or documented in the binding if needed?

[Severity: Low]
Also during probe of these nodes, cdnsp_sky1_probe() calls
cdnsp_sky1_drd_init(), which disables all clocks before enabling them:

drivers/usb/cdns3/cdnsp-sky1.c:cdnsp_sky1_drd_init() {
	int ret = 0;

	reset_control_assert(data->reset);
	reset_control_assert(data->preset);
	cdnsp_sky1_clk_disable_all(data->dev);
	ret = cdnsp_sky1_clk_enable_all(data->dev);
	...
}

Since data->cix_usb_clks[] is only populated with devm_clk_get() inside
cdnsp_sky1_clk_enable_all(), isn't cdnsp_sky1_clk_disable_all() just calling
clk_disable_unprepare(NULL) on all four uninitialized array entries here?
Should the cdnsp_sky1_clk_disable_all() call in cdnsp_sky1_drd_init() be
dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103074929.1971195-1-hongliang.yang@cixtech.com?part=3