Re: [PATCH v4 2/3] usb: dwc3: qcom: Implement glue callbacks to facilitate runtime suspend

[Krishna Kurapati]

On 8/12/2025 2:39 PM, Konrad Dybcio wrote:
> On 8/12/25 7:55 AM, Krishna Kurapati wrote:
> > On Qualcomm DWC3 dual-role controllers, the conndone/disconnect events in
> > device mode are generated by controller when software writes to QSCRATCH
> > registers in Qualcomm Glue layer rather than the vbus line being routed to
> > dwc3 core IP for it to recognize and generate these events.
> >
> > UTMI_OTG_VBUS_VALID  bit of QSCRATCH_HS_PHY_CTRL register needs to be set
> > to generate a connection done event and to be cleared for the controller to
> > generate a disconnect event during cable removal. When the disconnect is
> > not generated upon cable removal, the "connected" flag of dwc3 is left
> > marked as "true" and it blocks suspend routines and for that to happen upon
> > cable removal, the cable disconnect notification coming in via set_role
> > call need to be provided to the Qualcomm glue layer as well.
> >
> > Currently, the way DWC3 core and Qualcomm legacy glue driver are designed,
> > there is no mechanism through which the DWC3 core can notify the Qualcomm
> > glue layer of any role changes which it receives via role switch. To
> > register these glue callbacks at probe time, for enabling core to notify
> > glue layer, the legacy Qualcomm driver has no way to find out when the
> > child driver probe was successful since it does not check for the same
> > during of_platform_populate.
>
> [...]
>
> > +	if (qcom->current_role == USB_ROLE_DEVICE)
>
> I think it makes more sense to check for next_role here (we know
> it'll be the opposite of current_role, but this is confusing to read)
>
> > +		dwc3_qcom_vbus_override_enable(qcom, false);
> > +	else if (qcom->current_role != USB_ROLE_DEVICE)
> > +		dwc3_qcom_vbus_override_enable(qcom, true);
>
> I think the formerly proposed inline comparison grew on me..
> or at least drop the 'else' condition, '==' only goes two ways

ACK.

> > +
> > +	pm_runtime_mark_last_busy(qcom->dev);
> > +	pm_runtime_put_sync(qcom->dev);
> > +
> > +	/*
> > +	 * Current role changes via usb_role_switch_set_role callback protected
> > +	 * internally by mutex lock.
> > +	 */
> > +	qcom->current_role = next_role;
> > +}
> > +
> > +static void dwc3_qcom_run_stop_notifier(struct dwc3 *dwc, bool is_on)
> > +{
> > +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> > +
> > +	/*
> > +	 * When autosuspend is enabled and controller goes to suspend
> > +	 * after removing UDC from userspace, the next UDC write needs
> > +	 * setting of QSCRATCH VBUS_VALID to "1" to generate a connect
> > +	 * done event.
> > +	 */
> > +	if (!is_on)
> > +		return;
> > +
> > +	dwc3_qcom_vbus_override_enable(qcom, true);
> > +	pm_runtime_mark_last_busy(qcom->dev);
> > +}
> > +
> > +struct dwc3_glue_ops dwc3_qcom_glue_ops = {
> > +	.pre_set_role	= dwc3_qcom_set_role_notifier,
> > +	.pre_run_stop	= dwc3_qcom_run_stop_notifier,
> > +};
> > +
> >   static int dwc3_qcom_probe(struct platform_device *pdev)
> >   {
> >   	struct dwc3_probe_data	probe_data = {};
> > @@ -636,6 +683,23 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >   	if (ignore_pipe_clk)
> >   		dwc3_qcom_select_utmi_clk(qcom);
> >   
> > +	qcom->mode = usb_get_dr_mode(dev);
> > +
> > +	if (qcom->mode == USB_DR_MODE_HOST) {
> > +		qcom->current_role = USB_ROLE_HOST;
>
> Should we explicitly clear the VBUS override in this and the last branch?

Not necessary. Even if its kept set (somehow carried on from 
bootloader), its still a NOP if dr mode is host or default mode is host.

Regards,
Krishna,

> Konrad
>
> > +	} else if (qcom->mode == USB_DR_MODE_PERIPHERAL) {
> > +		qcom->current_role = USB_ROLE_DEVICE;
> > +		dwc3_qcom_vbus_override_enable(qcom, true);
> > +	} else {
> > +		if ((device_property_read_bool(dev, "usb-role-switch")) &&
> > +		    (usb_get_role_switch_default_mode(dev) == USB_DR_MODE_HOST))
> > +			qcom->current_role = USB_ROLE_HOST;
> > +		else
> > +			qcom->current_role = USB_ROLE_DEVICE;
> > +	}
> > +
> > +	qcom->dwc.glue_ops = &dwc3_qcom_glue_ops;
> > +
> >   	qcom->dwc.dev = dev;
> >   	probe_data.dwc = &qcom->dwc;
> >   	probe_data.res = &res;
> > @@ -650,12 +714,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		goto remove_core;
> >   
> > -	qcom->mode = usb_get_dr_mode(dev);
> > -
> > -	/* enable vbus override for device mode */
> > -	if (qcom->mode != USB_DR_MODE_HOST)
> > -		dwc3_qcom_vbus_override_enable(qcom, true);
> > -
> >   	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> >   	device_init_wakeup(&pdev->dev, wakeup_source);
> >