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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, Aug 06, 2025, Krishna Kurapati wrote:
> 
> 
> On 8/6/2025 4:48 AM, Thinh Nguyen wrote:
> 
> [...]
> 
> > > > > > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > > > index ca7e1c02773a..d40b52e2ba01 100644
> > > > > > > > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > > > @@ -89,6 +89,12 @@ struct dwc3_qcom {
> > > > > > > > > > >       	bool			pm_suspended;
> > > > > > > > > > >       	struct icc_path		*icc_path_ddr;
> > > > > > > > > > >       	struct icc_path		*icc_path_apps;
> > > > > > > > > > > +
> > > > > > > > > > > +	/*
> > > > > > > > > > > +	 * Current role changes via usb_role_switch_set_role callback protected
> > > > > > > > > > > +	 * internally by mutex lock.
> > > > > > > > > > > +	 */
> > > > > > > > > > > +	enum usb_role		current_role;
> > > > > > > > > > 
> > > > > > > > > > Can we just track the current role through dwc3 core instead of an
> > > > > > > > > > addition field in the glue?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Core caches only mode. We need ROLE NONE to identify cable connect. So
> > > > > > > > > adding that in glue to keep track.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The controller is always either host or device, not somewhere in
> > > > > > > > between. You're using ROLE_NONE to indicate connection, which is a
> > > > > > > > separate state.
> > > > > > > > 
> > > > > > > 
> > > > > > > Yes, but there is no flag that indicates that in dwc structure today. Also
> > > > > > > since only dwc3-qcom needs it at the moment, I added that role info in glue
> > > > > > > layer.
> > > > > > 
> > > > > > How are you using ROLE NONE? Do you send a role-switch call to "none" to
> > > > > > indicate disconnect? Let's not do that. Currently the dwc3 driver would
> > > > > > switch back to the default mode if "none" is selected, but this is not
> > > > > > well defined and implementation specific. It can be no-op and would not
> > > > > > violate the interface.
> > > > > > 
> > > > > > The role-switch interface should only be used for role-switching and not
> > > > > > connection/disconnection.
> > > > > > 
> > > > > > > 
> > > > > > > > I feel this should be tracked separately for clarity. The dwc3 also
> > > > > > > > tracks the connection state, can we use that?
> > > > > > > > 
> > > > > > > 
> > > > > > > Are you referring to the "connected" flag in DWC structure ? I see that it
> > > > > > > indicates connection status only in gadget mode.
> > > > > > > 
> > > > > > 
> > > > > > Yes, that flag is only for gadget.
> > > > > > 
> > > > > > Can you provide more info of the setup? Is there a type-c controller or
> > > > > > phy that can detect attach/deattach? Or it only propagates to the usb
> > > > > > controller?
> > > > > 
> > > > > My response didn't show up on lore since the mail client I used before sent
> > > > > the message in HTML format. So resending my response again.
> > > > > 
> > > > > Hi Thinh,
> > > > > 
> > > > >    Yes this is for cases where role switching is present (either with a Type-C
> > > > > controller, USB-conn-gpio, or a glink based role-switch).
> > > > > 
> > > > >    Actually the requirement is as follows:
> > > > >    1. When in device mode, if we receive a cable disconnect, we need to clear
> > > > > OTG_VBUS_VALID bit of QSCRATCH register in glue address space.
> > > > >    2. When cable is connected in device mode, we need to set the
> > > > > OTG_VBUS_VALID bit of QSCRATCH register in glue address space.
> > > > >    3. When the runstop is set back after a suspend rotuine, then we need to
> > > > > set  OTG_VBUS_VALID bit of QSCRATCH register in glueaddress space.
> > > > > 
> > > > >    To take care of all the 3 scenarios above, the set_role and run_stop
> > > > > notifiers have been added.
> > > > > 
> > > > >    The role info propagates only from core to glue. It is not sent to the phy.
> > > > > 
> > > > 
> > > > When does ROLE NONE occur? Did you have the type-c driver set the role
> > > > switch to "none" indicate disconnect?
> > > > 
> > > > The vbus-valid is only for gadget, and you only care about the
> > > > OTG_VBUS_VALID right? Can we just use the dwc3->connected flag? Just
> > > > make sure that it's cleared on role-switch, which should be the case
> > > > because we always perform soft-disconnect on gadget unbind, and make
> > > > sure to set vbus-valid on pullup or gadget binding. Is there some
> > > > scenarios that dwc->connected does not cover?
> > > > 
> > > 
> > > Hi Thinh,
> > > 
> > >   In case of just a cable disconnect in device mode (and default dr mode is
> > > peripheral only), there would be no role switch. In that scenario, connected
> > > flag would stay "true" even after removing cable. In that case, we can
> > > generate disconnect interrupt only by clearing this VBUS_VALID bit and
> > > inturn the suspend will succeed. So wanted to use notification from set_role
> > > which would cover all cases:
> > > 1. cable disconnect/cable connect
> > > 2. Role switch from host->device and device->host
> > > 
> > 
> > Ok. Thanks for the explanation. How everything tied together seems
> > awkward: The connector does a role-switch to "none" to trigger a
> > role-switch in dwc3, which then triggers a callback to dwc3-qcom to
> > clear vbus_valid, which then allows the controller to see a disconnect
> > event in dwc3. Is that right?
> > 
> 
> Yes, that is right.
> 
> > The connector driver should have a separate interface for
> > attach/deattach in addition to role-switch. But I don't think type-c
> > controller, USB-conn-gpio, and glink share the same way to handle
> > connection/disconnection.
> > 
> 
> I think although ROLE_NONE is for selecting default mode, it still means the
> same in USB-conn-gpio / ucsi-glink and hd3ss3220 (not sure about
> typec-controller).
> 
> > Let's keep what you proposed, and keep in mind that ROLE NONE is for
> > selecting the default mode and not necessarily mean disconnection.
> > 
> 
> Sure. Will do the following in v3:
> 1. Instead of adding host notifiers, will add "pm_runtime_use_autosuspend"
> in xhci-plat.c
> 2. Fix up coding issues pointed by you in patch 1/4.
> 
> Let me know if this sounds fine.
> 

Yes. Sounds good.

Thanks,
Thinh