usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Heikki Krogerus]

On Sat, Sep 29, 2018 at 10:26:20PM +0800, 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.
> 
> Signed-off-by: Yu Wang <yu1.wang@intel.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.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;
>  }

Thanks,

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Hans de Goede]

Hi All,

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.
> 
> Signed-off-by: Yu Wang <yu1.wang@intel.com>

As discussed before I've tested this on a Cherry Trail device to make
sure it does not cause regressions there. I'm happy to report that this
works fine there:

Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   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;
>   }
>

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Heikki Krogerus]

On Mon, Oct 01, 2018 at 04:23:31PM +0200, Hans de Goede wrote:
> 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?
> 
> 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?

So if I've understood correctly, there are certain conditions inside
both the xHCI and the DWC3 (which is here called xDCI) blocks that
have to be met before the internal mux state actually gets changed.

That means that there is no guarantee that the SW_IDPIN bit tells the
actual state of the mux.

> IOW what problem are you seeing without this patch and how does
> this fix it ?

If there is a change that we may in some situations read a wrong
state for the mux, then that is a problem. But it would be good to
explain the actual case that you are seeing in the commit message.

And this patch should probable also be marked as a fix for the
original commit that introduced the driver.

> 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).

I'm not sure if this is directly related to that, but Yu, can you
confirm that?

I'm guessing that this is indirectly related to that patch. DRD_CONFIG
bits apparently force the state of the mux, ignoring the state of xHCI
and DWC3, so by using them the problem of reading wrong state for
they mux could never happen.

In any case, the rationale for the patch sounds perfectly reasonable
to me. Even if we need to use the DRD_CONFIG bits in some cases, IMO
we still should rely on the CFG1 to get the real state of the mux.


Thanks,

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Hans de Goede]

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;
>>>   }
>>>

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Hans de Goede]

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?

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?

IOW what problem are you seeing without this patch and how does
this fix it ?

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).

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;
>   }
>

usb: roles: intel_xhci: Determine current role by DUAL_ROLE_CFG1

[Yu Wang]

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.

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;
 }