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

[Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>]

On 6/10/2025 10:53 PM, Dmitry Baryshkov wrote:
> On Tue, Jun 10, 2025 at 10:06:24PM +0530, Krishna Kurapati wrote:
>>
>>
>> On 6/10/2025 4:28 PM, Dmitry Baryshkov wrote:
>>> On Tue, Jun 10, 2025 at 02:43:55PM +0530, 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@oss.qualcomm.com>
>>>> ---
>>>>    drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 73 insertions(+), 9 deletions(-)
>>>>
>>>> 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;
>>>>    };
>>>>    #define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
>>>> @@ -118,9 +124,9 @@ static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
>>>>    }
>>>>    /*
>>>> - * TODO: Make the in-core role switching code invoke dwc3_qcom_vbus_override_enable(),
>>>> - * validate that the in-core extcon support is functional, and drop extcon
>>>> - * handling from the glue
>>>> + * TODO: Validate that the in-core extcon support is functional, and drop
>>>> + * extcon handling from the glue. Make in-core extcon invoke
>>>> + * dwc3_qcom_vbus_override_enable()
>>>>     */
>>>>    static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
>>>>    {
>>>> @@ -641,6 +647,53 @@ 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)
>>>> +{
>>>> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>>>> +
>>>> +	if (qcom->current_role == next_role)
>>>> +		return;
>>>> +
>>>> +	if (pm_runtime_resume_and_get(qcom->dev) < 0) {
>>>> +		dev_dbg(qcom->dev, "Failed to resume device\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (qcom->current_role == USB_ROLE_DEVICE &&
>>>> +	    next_role != USB_ROLE_DEVICE)
>>>> +		dwc3_qcom_vbus_override_enable(qcom, false);
>>>> +	else if ((qcom->current_role != USB_ROLE_DEVICE) &&
>>>> +		 (next_role == USB_ROLE_DEVICE))
>>>> +		dwc3_qcom_vbus_override_enable(qcom, true);
>>>> +
>>>> +	pm_runtime_mark_last_busy(qcom->dev);
>>>> +	pm_runtime_put_sync(qcom->dev);
>>>> +
>>>> +	qcom->current_role = next_role;
>>>
>>> How is it protected by the mutex? Which mutex?
>>>
>>
>> I see a mutex lock in usb_role_switch_set_role() that invokes set role. I
>> think that should be sufficient here.
> 
> Please add a comment to the source code.
> 

Hi Dmitry,

  I added the comment at the declaration of variable in structure. Will 
move it to the function.

  Thank you for the review.

Regards,
Krishna,