Re: [PATCH v2] 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: missing OTG capability disables on usb2 node
--

commit 1ff89e7196fb33692fe862b6274ad535b519c7f7
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 i.MX95 NavQ board.

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

[ ... ]

> @@ -198,6 +270,55 @@ pinctrl_usdhc2: usdhc2grp {
>  	};
>  };
>  
> +&usb2 {
> +	disable-over-current;
> +	dr_mode = "otg";
> +	usb-role-switch;
> +
> +	status = "okay";

[Severity: Medium]
Should the &usb2 node also include adp-disable, hnp-disable, and
srp-disable, matching &usb3_dwc3 below?

In drivers/usb/chipidea/core.c:ci_get_platdata(), when dr_mode is set to
"otg", the ChipIdea driver enables hnp_support and srp_support by default
before calling of_usb_update_otg_caps():

	if (platdata->dr_mode == USB_DR_MODE_OTG) {
		/* We can support HNP and SRP of OTG 2.0 */
		platdata->ci_otg_caps.otg_rev = 0x0200;
		platdata->ci_otg_caps.hnp_support = true;
		platdata->ci_otg_caps.srp_support = true;

		/* Update otg capabilities by DT properties */
		ret = of_usb_update_otg_caps(dev->of_node,
					&platdata->ci_otg_caps);
		if (ret)
			return ret;
	}

Without those disable properties in the device tree, hnp_support and
srp_support remain true. In drivers/usb/chipidea/udc.c:udc_start(), this
sets ci->gadget.is_otg = 1 on a Type-C port, and when CONFIG_USB_OTG_FSM is
enabled, drivers/usb/chipidea/ci.h:ci_otg_is_fsm_mode() evaluates to true:

	return ci->is_otg && ci->roles[CI_ROLE_HOST] &&
		ci->roles[CI_ROLE_GADGET] && (otg_caps->srp_support ||
		otg_caps->hnp_support || otg_caps->adp_support);

Entering OTG FSM mode routes role changes to ci_otg_fsm_irq() instead of
ci_handle_id_switch() and ci_handle_vbus_change(). When TCPM calls
ci_usb_role_switch_set(), both id_extcon.changed (OTGSC_IDIS) and
vbus_extcon.changed (OTGSC_BSVIS) can be set together before invoking
ci_irq(), but drivers/usb/chipidea/otg_fsm.c:ci_otg_fsm_irq() handles them
in a mutually exclusive if / else if chain:

	if (otg_int_src) {
		if (otg_int_src & OTGSC_DPIS) {
			hw_write_otgsc(ci, OTGSC_DPIS, OTGSC_DPIS);
			fsm->a_srp_det = 1;
			fsm->a_bus_drop = 0;
		} else if (otg_int_src & OTGSC_IDIS) {
			hw_write_otgsc(ci, OTGSC_IDIS, OTGSC_IDIS);
			if (fsm->id == 0) {
				fsm->a_bus_drop = 0;
				fsm->a_bus_req = 1;
				ci->id_event = true;
			}
		} else if (otg_int_src & OTGSC_BSVIS) {

Could this cause OTGSC_BSVIS to be dropped on role transitions and prevent
software-driven host/device role switching from working properly on &usb2?

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