usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 01-10-18 16:53, Wang, Yu1 wrote:
> Hi Hans,
> 
> On 18-10-01 16:23:31, Hans de Goede wrote:
>> Hi,
>>
>> On 29-09-18 16:26, Yu Wang wrote:
>>> The USB PHY mux switch depend on both USB2/USB3 PHY state and xHCI/xDCI
>>> controller state. The role can't be switched if related states haven't
>>> satisfied. That is why we need to poll the DUAL_ROLE_CFG1 to check if
>>> the role switched successful or not.
>>>
>>> So the SW_IDPIN and SW_VBUS_VALID bits can't determine the current
>>> acting role.
>>>
>>> This patch changes the logic for getting role logic.
>>
>> I guess this matches up with the recent patches to change the
>> role-switching on non Cherry Trail devices to use the DRD_CONFIG bits
>> instead of the SW_IDPIN bit?
> Sorry, I just register this mailing list. Don't know the DRD_CONFIG
> history... Can you please help share the archives link? Somehow, the
> linux-usb archive link can't be opened successful in my environment, I
> am not sure if I am checking the correct archive address.

I'm talking about this patch:

https://patchwork.kernel.org/patch/10591729/

>> AFAIK under normal circumstances with our current code,
>> DUAL_ROLE_CFG1 will always follow SW_IDPIN, so I'm not entirly sure
>> what you are trying to fix here?
> I am developing on Intel ApolloLake platform. As my understanding, this
> silicon logic for PHY role switch should be same compared between APL
> and Cherry Trail.
> 
> As I mentioned in the commit message. The PHY switching is not only
> depend on the SW_IDPIN bits. The SW_IDPIN/SW_VBUS_VALID are only the
> "trigger" for PHY mux switch. But the silicon for DRD requires some
> specific states of xHCI/dwc3 and USB2/USB3 PHYs. If they are not
> satisfied. then the switch will be failed. That is why we poll the
> CFG1.bit29 with timeout mechanism.
> 
>>
>> IOW what problem are you seeing without this patch and how does
>> this fix it ?
> We met this issue with Intel ApolloLake platform, somehow the role
> switch get failed with timeout in intel_xhci_usb_set_role. That is mean
> the role set failed, but when trying to get the current acting role, it
> indicate it is under USB_ROLE_DEVICE.
> 
> About the issue why switch failed. We are still under investigation. But
> to report incorrect current acting device is not make sense. This patch
> is resolving this issue.
> 
>>
>> If this is related to the patch to use the DRD_CONFIG bits on
>> non Cherry Trail devices, then please first submit a new version
>> of the patch for that which leaves the functionality on CHT
>> devices as is (as discussed before).
> In my perspective, this patch should be compatible with all related
> Intel platforms. From current software logic, we set the role switch
> through SW_IDPIN/SW_VBUS_VALID bits, and we check if the current
> acting mode to determine if set role is successful or not, and report
> -ETIMEOUT if switch failed.
> 
>  From user space perspective, set the sysfs to switch to device mode, but
> get -ETIMEOUT which is means the device mode switch failed. And trying
> to get the current mode through this sysfs, it is showing under device
> mode... I think it is not make sense.

I understand with that explanation the proposed change seems
reasonable. I will test it on a Cherry Trail tablet (soonish I need
to make some time for this) and then give my acked-by.

Regards,

Hans



> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>>
>>> Signed-off-by: Yu Wang <yu1.wang@intel.com>
>>> ---
>>>   drivers/usb/roles/intel-xhci-usb-role-switch.c | 16 ++++++++++------
>>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
>>> index 1fb3dd0..c118c9a 100644
>>> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
>>> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
>>> @@ -110,14 +110,18 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
>>>   	pm_runtime_get_sync(dev);
>>>   	val = readl(data->base + DUAL_ROLE_CFG0);
>>> -	pm_runtime_put(dev);
>>> -	if (!(val & SW_IDPIN))
>>> -		role = USB_ROLE_HOST;
>>> -	else if (val & SW_VBUS_VALID)
>>> -		role = USB_ROLE_DEVICE;
>>> -	else
>>> +	if ((val & SW_IDPIN) && !(val & SW_VBUS_VALID))
>>>   		role = USB_ROLE_NONE;
>>> +	else {
>>> +		val = readl(data->base + DUAL_ROLE_CFG1);
>>> +		if (val & HOST_MODE)
>>> +			role = USB_ROLE_HOST;
>>> +		else
>>> +			role = USB_ROLE_DEVICE;
>>> +	}
>>> +
>>> +	pm_runtime_put(dev);
>>>   	return role;
>>>   }
>>>