Message-ID: <20250815005121.kaun5rsjgw3qlk7c@synopsys.com>
Date: Fri, 15 Aug 2025 00:51:23 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
        Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] usb: dwc3: qcom: Implement glue callbacks to
 facilitate runtime suspend

On Thu, Aug 14, 2025, Krishna Kurapati wrote:
> 
> 
> On 8/14/2025 5:46 AM, Thinh Nguyen wrote:
> > On Tue, Aug 12, 2025, 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.
> > > 
> > > Hence implement the following glue callbacks for flattened Qualcomm glue
> > > driver:
> > > 
> > > 1. set_role: To pass role switching information from drd layer to glue.
> > > This information is needed to identify NONE/DEVICE mode switch and modify
> > > QSCRATCH to generate connect-done event on device mode entry and disconnect
> > > event on cable removal in device mode.
> > > 
> > > 2. run_stop: When booting up in device mode, if autouspend is enabled and
> > > userspace doesn't write UDC on boot, controller enters autosuspend. After
> > > this, if the userspace writes to UDC in the future, run_stop notifier is
> > > required to enable UTMI_OTG_VBUS_VALID of QSCRATCH so that connect done
> > > event is generated after run_stop(1) is done to finish enumeration.
> > > 
> > > Signed-off-by: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
> > > ---
> > >   drivers/usb/dwc3/dwc3-qcom.c | 78 +++++++++++++++++++++++++++++++-----
> > >   1 file changed, 68 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index a7eaefaeec4d..d89fbb1d8994 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -83,6 +83,8 @@ struct dwc3_qcom {
> > >   	bool			pm_suspended;
> > >   	struct icc_path		*icc_path_ddr;
> > >   	struct icc_path		*icc_path_apps;
> > > +
> > > +	enum usb_role		current_role;
> > >   };
> > >   #define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
> > > @@ -111,10 +113,6 @@ static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
> > >   	readl(base + offset);
> > >   }
> > > -/*
> > > - * TODO: Make the in-core role switching code invoke dwc3_qcom_vbus_override_enable(),
> > > - * validate that the in-core extcon support is functional
> > > - */
> > >   static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
> > >   {
> > >   	if (enable) {
> > > @@ -560,6 +558,55 @@ static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *p
> > >   	return 0;
> > >   }
> > > +static void dwc3_qcom_set_role_notifier(struct dwc3 *dwc, enum usb_role next_role)
> > 
> > Minor nit: There's no notifier here, can we rename to
> > dwc3_qcom_pre_set_role() for clarity?
> > 
> > > +{
> > > +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> > > +
> > > +	if (qcom->current_role == next_role)
> > > +		return;
> > > +
> > > +	if (pm_runtime_resume_and_get(qcom->dev)) {
> > > +		dev_dbg(qcom->dev, "Failed to resume device\n");
> > 
> > Minor nit: should this be dev_err?
> > 
> > > +		return;
> > > +	}
> > > +
> > > +	if (qcom->current_role == USB_ROLE_DEVICE)
> > > +		dwc3_qcom_vbus_override_enable(qcom, false);
> > > +	else if (qcom->current_role != USB_ROLE_DEVICE)
> > > +		dwc3_qcom_vbus_override_enable(qcom, true);
> > > +
> > > +	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)
> > 
> > Minor nit: can we rename to dwc3_qcom_pre_run_stop()
> > 
> > > +{
> > > +	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;
> > > +	} else if (qcom->mode == USB_DR_MODE_PERIPHERAL) {
> > > +		qcom->current_role = USB_ROLE_DEVICE;
> > > +		dwc3_qcom_vbus_override_enable(qcom, true);
> > 
> > Shouldn't you determine if there's connection before setting vbus valid?
> > 
> > There are a couple things that seems at odd to me:
> > 1) Typically we use "vbus override" to describe the vbus control on host
> >     to turn on/off vbus. Where as here, the vbus override is used for
> >     vbus valid in device mode for vbus detection.
> 
> The register was named that way even in HW description we get internally. So
> it was kept as is.
> 
> > 
> > 2) It seems like you always enable vbus valid without checking for
> >     connection status. You also set vbus valid even if it's in host mode.
> >     Only when there's role-switch away from device that you'd clear the
> >     vbus valid.
> > 
> 
> In device mode, we do not get any role information (or cable connection) as
> role switch is not present and set_role notifier is not called. So if we
> keep the vbus_override set, it is still harmless because, it is used to
> generate a connection done event (which comes after bus reset) only when
> cable is connected to a host. If there is no cable connection, then keeping
> the bit set is a NOP and hence I set it by default when dr_mode is
> peripheral. I am setting this bit only is dr_mode is peripheral and when we
> are switching from any role to peripheral in case of a role switch.

Thanks for the info.

BR,
Thinh

> 
> > But I'm not familiar with your platform/design, so I can't judge much on
> > the above. Base on the description provided, I don't expect functional
> > issues from your implementation.
> > 
> > I think the comments above should not be blocking your change.
> > 
> > Whether or not you decide to make the updates for the minor nits, here's
> > my ack:
> > 
> > Acked-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
> > 
> 
> Thanks for the review.
> 
> Regards,
> Krishna,
> 
> > BR,
> > Thinh
> > 
> > > +	} 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);
> > > -- 
> > > 2.34.1
> > > 

Powered by blists - more mailing lists