Re: [PATCH] arm64: dts: imx95-navq: add USB1/USB2 nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: imx95-navq: usb2 dr_mode = host breaks usb-role-switch
--

commit 7aa413a1299c4bee8b5a2613c0acf6256cd30c16
Author: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

arm64: dts: imx95-navq: add USB1/USB2 nodes

This commit adds and enables the device tree nodes for the USB1 and USB2
controllers and their Type-C port controllers on the imx95-navq board.

> diff --git a/arch/arm64/boot/dts/freescale/imx95-navq.dts b/arch/arm64/boot/dts/freescale/imx95-navq.dts
> index 7860c113e9e02..5fe245b623068 100644
> --- a/arch/arm64/boot/dts/freescale/imx95-navq.dts
> +++ b/arch/arm64/boot/dts/freescale/imx95-navq.dts

[ ... ]

> @@ -80,6 +82,72 @@ i2c3_gpio_expander_21: gpio-expander@21 {

[ ... ]

> +	ptn5110_usb2: tcpc@51 {
> +		compatible = "nxp,ptn5110", "tcpci";
> +		reg = <0x51>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-0 = <&pinctrl_typec>;
> +		pinctrl-names = "default";
> +
> +		/* no SINK circuit (EN_SINK1 tied to TP30) */
> +		connector {
> +			compatible = "usb-c-connector";
> +			label = "USB2 2.0 Type C";
> +			data-role = "dual";
> +			power-role = "source";
> +			self-powered;
> +			source-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)>;
> +
> +			port {
> +				typec2_con_hs: endpoint {
> +					remote-endpoint = <&usb2_data_hs>;
> +				};
> +			};
> +		};
> +	};
>  };

[ ... ]

> @@ -198,6 +270,55 @@ pinctrl_usdhc2: usdhc2grp {
>  	};
>  };
>  
> +&usb2 {
> +	disable-over-current;
> +	dr_mode = "host";
> +	usb-role-switch;
> +
> +	status = "okay";
> +
> +	port {
> +		usb2_data_hs: endpoint {
> +			remote-endpoint = <&typec2_con_hs>;
> +		};
> +	};
> +};

[Severity: Medium]
Should usb2 use dr_mode = "otg" here instead of "host" to match the
usb-role-switch property and the connector's data-role = "dual" in
ptn5110_usb2, or should ptn5110_usb2 use data-role = "host" without
usb-role-switch on usb2?

When dr_mode is set to "host", ci_hdrc_probe() in drivers/usb/chipidea/core.c
skips initializing the gadget role and OTG handling:

	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
		ret = ci_hdrc_gadget_init(ci);
		...
	}
	...
	if (ci->is_otg && ci->roles[CI_ROLE_GADGET]) {
		ret = ci_hdrc_otg_init(ci);
		...
	}

Because ci->roles[CI_ROLE_GADGET] remains NULL, ci_get_role() never enables
OTGSC_IDIE:

	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
		if (ci->is_otg) {
			role = ci_otg_role(ci);
			hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
		} else {

As a result, id_extcon.enabled and vbus_extcon.enabled remain false. Although
the Type-C port manager configures ptn5110_usb2 for dual-role data and accepts
USB PD data role swaps, calls along usb_role_switch_set_role() ->
ci_usb_role_switch_set() -> ci_irq() -> ci_irq_handler() will have OTGSC_IDIE
and OTGSC_BSVIE masked out by hw_read_otgsc(), leaving role switching
non-functional and the controller permanently in host mode.

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