Re: [PATCH 3/3] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Wesley Cheng <wcheng@codeaurora.org>]

On 8/10/2020 5:13 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Wesley Cheng <wcheng@codeaurora.org> writes:
>> @@ -190,6 +195,73 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
>>  	return 0;
>>  }
>>  
>> +static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw,
>> +					 enum usb_role role)
>> +{
>> +	struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw);
>> +	struct fwnode_handle *child;
>> +	bool enable = false;
>> +
>> +	if (!qcom->dwc3_drd_sw) {
>> +		child = device_get_next_child_node(qcom->dev, NULL);
>> +		if (child) {
>> +			qcom->dwc3_drd_sw = usb_role_switch_find_by_fwnode(child);
>> +			fwnode_handle_put(child);
>> +			if (IS_ERR(qcom->dwc3_drd_sw)) {
>> +				qcom->dwc3_drd_sw = NULL;
>> +				return 0;
>> +			}
>> +		}
>> +	}
>> +
>> +	usb_role_switch_set_role(qcom->dwc3_drd_sw, role);
> 
> why is this done at the glue layer instead of core.c?
> 
Hi Felipe,

Thanks for the feedback.  So the DWC3 DRD driver already registers a
role switch device for receiving external events.  However, the DWC3
glue (dwc3-qcom) needs to also know of the role changes, so that it can
set the override bits accordingly in the controller.  I've seen a few
implementations, ie using a notifier block to notify the glue of these
events, but that placed a dependency on the DWC3 core being available to
the DWC3 glue at probe time.  If the DWC3 core was not available at that
time, the dwc3-qcom driver will finish its probing routine, and since
the notifier was never registered, the role change events would not be
received.

By registering another role switch device in the DWC3 glue, this gives
us a place to attempt initializing a channel w/ the DWC3 core if it
wasn't ready during probe().  For example...

usb_conn_detect_cable(role=USB_ROLE_DEVICE)
-->usb_role_switch_set_role(sw=dwc3-qcom)
  -->dwc3_qcom_usb_role_switch_set()
    -- IF DWC3 core role switch available
	-->usb_role_switch_set_role(sw=drd)
    -- ELSE
	--> do nothing.

So basically, the goal is to just propagate the role change event down
to the DWC3 core, while breaking the dependency of it being available at
probe.
>> +	if (role == USB_ROLE_DEVICE)
>> +		enable = true;
>> +	else
>> +		enable = false;
>> +
>> +	qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
>> +					       USB_DR_MODE_PERIPHERAL;
>> +	dwc3_qcom_vbus_overrride_enable(qcom, enable);
> 
> could you add a patch fixing this typo?
> 
Sure, I'll submit a separate patch to remove that extra 'r'

Thanks
Wesley

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project