Re: [PATCH v6 5/5] usb: dwc3: qcom: Modify interrupt handling for eUSB2 Phy targets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The driver performs a fragile resource lookup for the USB2 PHY by hardcoding index 0 instead of resolving it by its defined name.

Pre-existing issues:
- [High] The legacy wrapper driver (`dwc3-qcom-legacy.c`) completely misses the new eUSB2 remote wakeup interrupt logic, breaking remote wakeup on upstream eUSB2 platforms utilizing the legacy binding.
--

commit fb96885fe62bdb840bbac9d8be2fe275a3e945ae
Author: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>

usb: dwc3: qcom: Modify interrupt handling for eUSB2 Phy targets

This commit modifies the dwc3 qcom driver to handle wakeup interrupts
differently for eUSB2 PHY targets based on connected device speed. It checks
for an eUSB2 PHY and configures DP/DM interrupt line states accordingly.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index f43f73ac36ff1..9ba7ea31b524f 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> -static void dwc3_qcom_enable_port_interrupts(struct dwc3_qcom_port *port)
> +static void dwc3_qcom_enable_port_interrupts(struct dwc3_qcom *qcom, int port_index)
>  {
> +	struct dwc3_qcom_port *port = &qcom->ports[port_index];
> +
>  	dwc3_qcom_enable_wakeup_irq(port->qusb2_phy_irq, 0);
>  
>  	/*
>  	 * Configure DP/DM line interrupts based on the USB2 device attached to
> -	 * the root hub port. When HS/FS device is connected, configure the DP line
> -	 * as falling edge to detect both disconnect and remote wakeup scenarios. When
> -	 * LS device is connected, configure DM line as falling edge to detect both
> -	 * disconnect and remote wakeup. When no device is connected, configure both
> -	 * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
> +	 * the root hub port. For non-eUSB2 targets, when HS/FS device is connected,
> +	 * configure the DP line as falling edge to detect disconnect and remote
> +	 * wakeup. When LS device is connected, configure the DM line as falling edge
> +	 * for the same. For eUSB2 targets, the lines are swapped and rising edge is
> +	 * used: DP line for LS and DM line for HS/FS device detection. When no
> +	 * device is connected, configure both DP and DM lines as rising edge to
> +	 * detect HS/FS/LS device connect scenario.
>  	 */
>  
>  	if (port->usb2_speed == USB_SPEED_LOW) {
> -		dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
> -					    IRQ_TYPE_EDGE_FALLING);
> +		if (qcom->has_eusb2_phy)
> +			dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
> +						    IRQ_TYPE_EDGE_RISING);
> +		else
> +			dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
> +						    IRQ_TYPE_EDGE_FALLING);

[Severity: High]
This is a pre-existing issue, but does the legacy wrapper driver 
(drivers/usb/dwc3/dwc3-qcom-legacy.c) completely miss this new eUSB2 remote
wakeup interrupt logic?

The device tree for the Qualcomm SAR2130P SoC actively uses the legacy wrapper
binding (compatible = "qcom,sar2130p-dwc3", "qcom,dwc3") along with an eUSB2
PHY. When this SoC suspends the USB controller, dwc3_qcom_enable_port_interrupts
in the legacy driver configures the interrupt polarity incorrectly (as falling
edge instead of rising edge) because it lacks the eUSB2 checks introduced here.

Is an update required in the legacy wrapper so that upstream eUSB2 platforms
utilizing the legacy binding can correctly detect remote wakeup events?

[ ... ]
> @@ -683,6 +705,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (ignore_pipe_clk)
>  		dwc3_qcom_select_utmi_clk(qcom);
>  
> +	phy = devm_of_phy_get_by_index(dev, dev->of_node, 0);

[Severity: Medium]
Will this break if the USB3 PHY is placed first in the phys property array?

If index 0 retrieves the USB3 PHY instead, phy_get_type() will return
PHY_TYPE_USB3 rather than PHY_TYPE_EUSB2, falsely evaluating has_eusb2_phy
to false. The DT binding schema (snps,dwc3-common.yaml) does not enforce
the ordering of the phys array, but rather relies on the phy-names property
(enum: [usb2-phy, usb3-phy]).

Would it be more robust to use devm_phy_get(dev, "usb2-phy") to resolve the
PHY by its defined name instead of hardcoding index 0?

> +	if (IS_ERR(phy)) {
> +		if (PTR_ERR(phy) != -ENODEV) {
> +			ret = dev_err_probe(dev, PTR_ERR(phy),
> +					    "failed to get usb2 phy\n");
> +			goto clk_disable;
> +		}
> +		phy = NULL;
> +	}
> +	qcom->has_eusb2_phy = phy && (phy_get_type(phy) == PHY_TYPE_EUSB2);
> +
>  	qcom->mode = usb_get_dr_mode(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-eusb2-interrupt-v6-v6-0-ae2601cfa80c@oss.qualcomm.com?part=5