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.w...@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;
}
