usb:typec:tcpm:support double Rp to Vbus cable as sink

Message ID 20230914003154.27977-1-michael@allwinnertech.com
State New
Headers
Series usb:typec:tcpm:support double Rp to Vbus cable as sink |

Commit Message

Michael Wu Sept. 14, 2023, 12:31 a.m. UTC
  The USB Type-C Cable and Connector Specification defines the wire
connections for the USB Type-C to USB 2.0 Standard-A cable assembly
(Release 2.2, Chapter 3.5.2).
The Notes says that Pin A5 (CC) of the USB Type-C plug shall be connected
to Vbus through a resister Rp.
However, there is a large amount of such double Rp connected to Vbus
non-standard cables which produced by UGREEN circulating on the market, and
it can affects the normal operations of the state machine easily,
especially to CC1 and CC2 be pulled up at the same time.
In fact, we can regard those cables as sink to avoid abnormal state.

Message as follow:
[   58.900212] VBUS on
[   59.265433] CC1: 0 -> 3, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
[   62.623308] CC1: 3 -> 0, CC2: 3 -> 0 [state TOGGLING, polarity 0, disconnected]
[   62.625006] VBUS off
[   62.625012] VBUS VSAFE0V

Signed-off-by: Michael Wu <michael@allwinnertech.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Heikki Krogerus Sept. 18, 2023, 10:31 a.m. UTC | #1
On Thu, Sep 14, 2023 at 08:31:54AM +0800, Michael Wu wrote:
> The USB Type-C Cable and Connector Specification defines the wire
> connections for the USB Type-C to USB 2.0 Standard-A cable assembly
> (Release 2.2, Chapter 3.5.2).
> The Notes says that Pin A5 (CC) of the USB Type-C plug shall be connected
> to Vbus through a resister Rp.
> However, there is a large amount of such double Rp connected to Vbus
> non-standard cables which produced by UGREEN circulating on the market, and
> it can affects the normal operations of the state machine easily,
> especially to CC1 and CC2 be pulled up at the same time.
> In fact, we can regard those cables as sink to avoid abnormal state.
> 
> Message as follow:
> [   58.900212] VBUS on
> [   59.265433] CC1: 0 -> 3, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> [   62.623308] CC1: 3 -> 0, CC2: 3 -> 0 [state TOGGLING, polarity 0, disconnected]
> [   62.625006] VBUS off
> [   62.625012] VBUS VSAFE0V
> 
> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d962f67c95ae6..beb7143128667 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -519,7 +519,8 @@ static const char * const pd_rev[] = {
>  
>  #define tcpm_port_is_sink(port) \
>  	((tcpm_cc_is_sink((port)->cc1) && !tcpm_cc_is_sink((port)->cc2)) || \
> -	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)))
> +	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)) || \
> +	 (tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2)))
>  
>  #define tcpm_cc_is_source(cc) ((cc) == TYPEC_CC_RD)
>  #define tcpm_cc_is_audio(cc) ((cc) == TYPEC_CC_RA)

This look OK to me, but I would still like to wait for comments from
Guenter - just in case.

thanks,
  
Guenter Roeck Sept. 18, 2023, 2:22 p.m. UTC | #2
On 9/18/23 03:31, Heikki Krogerus wrote:
> On Thu, Sep 14, 2023 at 08:31:54AM +0800, Michael Wu wrote:
>> The USB Type-C Cable and Connector Specification defines the wire
>> connections for the USB Type-C to USB 2.0 Standard-A cable assembly
>> (Release 2.2, Chapter 3.5.2).
>> The Notes says that Pin A5 (CC) of the USB Type-C plug shall be connected
>> to Vbus through a resister Rp.
>> However, there is a large amount of such double Rp connected to Vbus
>> non-standard cables which produced by UGREEN circulating on the market, and
>> it can affects the normal operations of the state machine easily,
>> especially to CC1 and CC2 be pulled up at the same time.
>> In fact, we can regard those cables as sink to avoid abnormal state.
>>
>> Message as follow:
>> [   58.900212] VBUS on
>> [   59.265433] CC1: 0 -> 3, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
>> [   62.623308] CC1: 3 -> 0, CC2: 3 -> 0 [state TOGGLING, polarity 0, disconnected]
>> [   62.625006] VBUS off
>> [   62.625012] VBUS VSAFE0V
>>
>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index d962f67c95ae6..beb7143128667 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -519,7 +519,8 @@ static const char * const pd_rev[] = {
>>   
>>   #define tcpm_port_is_sink(port) \
>>   	((tcpm_cc_is_sink((port)->cc1) && !tcpm_cc_is_sink((port)->cc2)) || \
>> -	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)))
>> +	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)) || \
>> +	 (tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2)))
>>   
>>   #define tcpm_cc_is_source(cc) ((cc) == TYPEC_CC_RD)
>>   #define tcpm_cc_is_audio(cc) ((cc) == TYPEC_CC_RA)
> 
> This look OK to me, but I would still like to wait for comments from
> Guenter - just in case.
> 

Look at the conditions. Reordered, we end up with
	(tcpm_cc_is_sink((port)->cc1) && !tcpm_cc_is_sink((port)->cc2)) ||
	(tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2))
which simplifies to
	tcpm_cc_is_sink((port)->cc1)
making the complete expression
	tcpm_cc_is_sink((port)->cc1) ||
	(tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1))
which simplifies further to
	tcpm_cc_is_sink((port)->cc1) || tcpm_cc_is_sink((port)->cc2)

The simplified expression doesn't conflict with other detections, so I am
ok with it. It might be worthwhile adding a comment to the code, though,
explaining the reason.

Guenter

> thanks,
>
  
Michael Wu Sept. 20, 2023, 10:45 a.m. UTC | #3
On 2023/9/18 22:22, Guenter Roeck wrote:
> On 9/18/23 03:31, Heikki Krogerus wrote:
>> On Thu, Sep 14, 2023 at 08:31:54AM +0800, Michael Wu wrote:
>>> The USB Type-C Cable and Connector Specification defines the wire
>>> connections for the USB Type-C to USB 2.0 Standard-A cable assembly
>>> (Release 2.2, Chapter 3.5.2).
>>> The Notes says that Pin A5 (CC) of the USB Type-C plug shall be 
>>> connected
>>> to Vbus through a resister Rp.
>>> However, there is a large amount of such double Rp connected to Vbus
>>> non-standard cables which produced by UGREEN circulating on the 
>>> market, and
>>> it can affects the normal operations of the state machine easily,
>>> especially to CC1 and CC2 be pulled up at the same time.
>>> In fact, we can regard those cables as sink to avoid abnormal state.
>>>
>>> Message as follow:
>>> [   58.900212] VBUS on
>>> [   59.265433] CC1: 0 -> 3, CC2: 0 -> 3 [state TOGGLING, polarity 0, 
>>> connected]
>>> [   62.623308] CC1: 3 -> 0, CC2: 3 -> 0 [state TOGGLING, polarity 0, 
>>> disconnected]
>>> [   62.625006] VBUS off
>>> [   62.625012] VBUS VSAFE0V
>>>
>>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>>> ---
>>>   drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c 
>>> b/drivers/usb/typec/tcpm/tcpm.c
>>> index d962f67c95ae6..beb7143128667 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -519,7 +519,8 @@ static const char * const pd_rev[] = {
>>>   #define tcpm_port_is_sink(port) \
>>>       ((tcpm_cc_is_sink((port)->cc1) && 
>>> !tcpm_cc_is_sink((port)->cc2)) || \
>>> -     (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)))
>>> +     (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)) 
>>> || \
>>> +     (tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2)))
>>>   #define tcpm_cc_is_source(cc) ((cc) == TYPEC_CC_RD)
>>>   #define tcpm_cc_is_audio(cc) ((cc) == TYPEC_CC_RA)
>>
>> This look OK to me, but I would still like to wait for comments from
>> Guenter - just in case.
>>
> 
> Look at the conditions. Reordered, we end up with
>      (tcpm_cc_is_sink((port)->cc1) && !tcpm_cc_is_sink((port)->cc2)) ||
>      (tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2))
> which simplifies to
>      tcpm_cc_is_sink((port)->cc1)
> making the complete expression
>      tcpm_cc_is_sink((port)->cc1) ||
>      (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1))
> which simplifies further to
>      tcpm_cc_is_sink((port)->cc1) || tcpm_cc_is_sink((port)->cc2)
> 
> The simplified expression doesn't conflict with other detections, so I am
> ok with it. It might be worthwhile adding a comment to the code, though,
> explaining the reason
> Guenter
> 
>> thanks,
>>
Dear Guenter,

I have modified it according to your opinion, and resend it as patch 
v2[1]. Please review.

[1] 
https://lore.kernel.org/all/20230920063030.66312-1-michael@allwinnertech.com/
  

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d962f67c95ae6..beb7143128667 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -519,7 +519,8 @@  static const char * const pd_rev[] = {
 
 #define tcpm_port_is_sink(port) \
 	((tcpm_cc_is_sink((port)->cc1) && !tcpm_cc_is_sink((port)->cc2)) || \
-	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)))
+	 (tcpm_cc_is_sink((port)->cc2) && !tcpm_cc_is_sink((port)->cc1)) || \
+	 (tcpm_cc_is_sink((port)->cc1) && tcpm_cc_is_sink((port)->cc2)))
 
 #define tcpm_cc_is_source(cc) ((cc) == TYPEC_CC_RD)
 #define tcpm_cc_is_audio(cc) ((cc) == TYPEC_CC_RA)