[PATCH 0/4] Fix some defects related Type-C TCPM

[PATCH 0/4] Fix some defects related Type-C TCPM

[Frank Wang]

This series fix some defects of TCPM like port reset error, SVIDs
discover error and etc.

Tested on Rockchip EVB board integrated with FUSB302 or HUSB311 Type-C
controller.

Frank Wang (4):
  usb: typec: tcpm: fix cc role at port reset
  usb: typec: tcpm: fix multiple times discover svids error
  usb: typec: tcpm: add get max power support
  usb: typec: tcpm: fix source caps may lost after soft reset

 drivers/usb/typec/tcpm/tcpm.c | 53 +++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Frank Wang]

In the current implementation, the tcpm set CC1/CC2 role to open when
it do port reset would cause the VBUS removed by the Type-C partner.

The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
in DRP initialization and connection detection.

So set CC1/CC2 to Rd to fix it.

Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index a0d943d785800..66de02a56f512 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4851,7 +4851,7 @@ static void run_state_machine(struct tcpm_port *port)
 		break;
 	case PORT_RESET:
 		tcpm_reset_port(port);
-		tcpm_set_cc(port, TYPEC_CC_OPEN);
+		tcpm_set_cc(port, TYPEC_CC_RD);
 		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
 			       PD_T_ERROR_RECOVERY);
 		break;
-- 
2.17.1

Re: [PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Heikki Krogerus]

On Mon, Mar 13, 2023 at 10:58:40AM +0800, Frank Wang wrote:
> In the current implementation, the tcpm set CC1/CC2 role to open when
> it do port reset would cause the VBUS removed by the Type-C partner.
> 
> The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
> role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
> in DRP initialization and connection detection.
> 
> So set CC1/CC2 to Rd to fix it.
> 
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index a0d943d785800..66de02a56f512 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4851,7 +4851,7 @@ static void run_state_machine(struct tcpm_port *port)
>  		break;
>  	case PORT_RESET:
>  		tcpm_reset_port(port);
> -		tcpm_set_cc(port, TYPEC_CC_OPEN);
> +		tcpm_set_cc(port, TYPEC_CC_RD);
>  		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>  			       PD_T_ERROR_RECOVERY);
>  		break;

Will this work if the port is for example source only?

thanks,

-- 
heikki

Re: [PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Frank Wang]

Hi Heikki,

On 2023/3/14 17:20, Heikki Krogerus wrote:
> On Mon, Mar 13, 2023 at 10:58:40AM +0800, Frank Wang wrote:
>> In the current implementation, the tcpm set CC1/CC2 role to open when
>> it do port reset would cause the VBUS removed by the Type-C partner.
>>
>> The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
>> role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
>> in DRP initialization and connection detection.
>>
>> So set CC1/CC2 to Rd to fix it.
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index a0d943d785800..66de02a56f512 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -4851,7 +4851,7 @@ static void run_state_machine(struct tcpm_port *port)
>>   		break;
>>   	case PORT_RESET:
>>   		tcpm_reset_port(port);
>> -		tcpm_set_cc(port, TYPEC_CC_OPEN);
>> +		tcpm_set_cc(port, TYPEC_CC_RD);
>>   		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>>   			       PD_T_ERROR_RECOVERY);
>>   		break;
> Will this work if the port is for example source only?

Yeah, this only set at port reset stage and CC value will be set again
(Rd for Sink, Rp_* for Source) when start toggling.

BR.
Frank

Re: [PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Heikki Krogerus]

On Wed, Mar 15, 2023 at 10:55:20AM +0800, Frank Wang wrote:
> Hi Heikki,
> 
> On 2023/3/14 17:20, Heikki Krogerus wrote:
> > On Mon, Mar 13, 2023 at 10:58:40AM +0800, Frank Wang wrote:
> > > In the current implementation, the tcpm set CC1/CC2 role to open when
> > > it do port reset would cause the VBUS removed by the Type-C partner.
> > > 
> > > The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
> > > role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
> > > in DRP initialization and connection detection.
> > > 
> > > So set CC1/CC2 to Rd to fix it.
> > > 
> > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> > > ---
> > >   drivers/usb/typec/tcpm/tcpm.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > index a0d943d785800..66de02a56f512 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -4851,7 +4851,7 @@ static void run_state_machine(struct tcpm_port *port)
> > >   		break;
> > >   	case PORT_RESET:
> > >   		tcpm_reset_port(port);
> > > -		tcpm_set_cc(port, TYPEC_CC_OPEN);
> > > +		tcpm_set_cc(port, TYPEC_CC_RD);
> > >   		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
> > >   			       PD_T_ERROR_RECOVERY);
> > >   		break;
> > Will this work if the port is for example source only?
> 
> Yeah, this only set at port reset stage and CC value will be set again
> (Rd for Sink, Rp_* for Source) when start toggling.

Okay. Let's wait for comments from Guenter.

thanks,

-- 
heikki

Re: [PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Guenter Roeck]

On 3/17/23 04:24, Heikki Krogerus wrote:
> On Wed, Mar 15, 2023 at 10:55:20AM +0800, Frank Wang wrote:
>> Hi Heikki,
>>
>> On 2023/3/14 17:20, Heikki Krogerus wrote:
>>> On Mon, Mar 13, 2023 at 10:58:40AM +0800, Frank Wang wrote:
>>>> In the current implementation, the tcpm set CC1/CC2 role to open when
>>>> it do port reset would cause the VBUS removed by the Type-C partner.
>>>>
>>>> The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
>>>> role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
>>>> in DRP initialization and connection detection.
>>>>
>>>> So set CC1/CC2 to Rd to fix it.
>>>>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> ---
>>>>    drivers/usb/typec/tcpm/tcpm.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>> index a0d943d785800..66de02a56f512 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>> @@ -4851,7 +4851,7 @@ static void run_state_machine(struct tcpm_port *port)
>>>>    		break;
>>>>    	case PORT_RESET:
>>>>    		tcpm_reset_port(port);
>>>> -		tcpm_set_cc(port, TYPEC_CC_OPEN);
>>>> +		tcpm_set_cc(port, TYPEC_CC_RD);
>>>>    		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>>>>    			       PD_T_ERROR_RECOVERY);
>>>>    		break;
>>> Will this work if the port is for example source only?
>>
>> Yeah, this only set at port reset stage and CC value will be set again
>> (Rd for Sink, Rp_* for Source) when start toggling.
> 
> Okay. Let's wait for comments from Guenter.
> 

Figure 4-20 is specifically for dual role ports. Also, start toggling would not
happen if the low level driver doesn't have a start_toggling callback. I think this
may require some tweaking based on the port type or, rather, tcpm_default_state().
Something like

	tcpm_set_cc(port, tcpm_default_state(port) == SNK_UNATTACHED ? TYPEC_CC_RD : tcpm_rp_cc(port));

Thanks,
Guenter

Re: [PATCH 1/4] usb: typec: tcpm: fix cc role at port reset

[Frank Wang]

Hi Guenter,

On 2023/3/17 20:47, Guenter Roeck wrote:
> On 3/17/23 04:24, Heikki Krogerus wrote:
>> On Wed, Mar 15, 2023 at 10:55:20AM +0800, Frank Wang wrote:
>>> Hi Heikki,
>>>
>>> On 2023/3/14 17:20, Heikki Krogerus wrote:
>>>> On Mon, Mar 13, 2023 at 10:58:40AM +0800, Frank Wang wrote:
>>>>> In the current implementation, the tcpm set CC1/CC2 role to open when
>>>>> it do port reset would cause the VBUS removed by the Type-C partner.
>>>>>
>>>>> The Figure 4-20 in the TCPCI 2.0 specification show that the CC1/CC2
>>>>> role should set to 01b (Rp) or 10b (Rd) at Power On or Reset stage
>>>>> in DRP initialization and connection detection.
>>>>>
>>>>> So set CC1/CC2 to Rd to fix it.
>>>>>
>>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>>> ---
>>>>>    drivers/usb/typec/tcpm/tcpm.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c 
>>>>> b/drivers/usb/typec/tcpm/tcpm.c
>>>>> index a0d943d785800..66de02a56f512 100644
>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>>> @@ -4851,7 +4851,7 @@ static void run_state_machine(struct 
>>>>> tcpm_port *port)
>>>>>            break;
>>>>>        case PORT_RESET:
>>>>>            tcpm_reset_port(port);
>>>>> -        tcpm_set_cc(port, TYPEC_CC_OPEN);
>>>>> +        tcpm_set_cc(port, TYPEC_CC_RD);
>>>>>            tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>>>>>                       PD_T_ERROR_RECOVERY);
>>>>>            break;
>>>> Will this work if the port is for example source only?
>>>
>>> Yeah, this only set at port reset stage and CC value will be set again
>>> (Rd for Sink, Rp_* for Source) when start toggling.
>>
>> Okay. Let's wait for comments from Guenter.
>>
>
> Figure 4-20 is specifically for dual role ports. Also, start toggling 
> would not
> happen if the low level driver doesn't have a start_toggling callback. 
> I think this
> may require some tweaking based on the port type or, rather, 
> tcpm_default_state().
> Something like
>
>     tcpm_set_cc(port, tcpm_default_state(port) == SNK_UNATTACHED ? 
> TYPEC_CC_RD : tcpm_rp_cc(port));

To amend likes above make sense, I shall fix it later.


BR.
Frank

>
> Thanks,
> Guenter
>

[PATCH 2/4] usb: typec: tcpm: fix multiple times discover svids error

[Frank Wang]

PD3.0 Spec 6.4.4.3.2 say that only Responder supports 12 or more SVIDs,
the Discover SVIDs Command Shall be executed multiple times until a
Discover SVIDs VDO is returned ending either with a SVID value of
0x0000 in the last part of the last VDO or with a VDO containing two
SVIDs with values of 0x0000.

In the current implementation, if the last VDO does not find that the
Discover SVIDs Command would be executed multiple times even if the
Responder SVIDs are less than 12, and we found some odd dockers just
meet this case. So fix it.

Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 66de02a56f512..2962f7c261976 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1515,7 +1515,21 @@ static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
 		pmdata->svids[pmdata->nsvids++] = svid;
 		tcpm_log(port, "SVID %d: 0x%x", pmdata->nsvids, svid);
 	}
-	return true;
+
+	/*
+	 * PD3.0 Spec 6.4.4.3.2: The SVIDs are returned 2 per VDO (see Table
+	 * 6-43), and can be returned maximum 6 VDOs per response (see Figure
+	 * 6-19). If the Respondersupports 12 or more SVID then the Discover
+	 * SVIDs Command Shall be executed multiple times until a Discover
+	 * SVIDs VDO is returned ending either with a SVID value of 0x0000 in
+	 * the last part of the last VDO or with a VDO containing two SVIDs
+	 * with values of 0x0000.
+	 *
+	 * However, some odd dockers support SVIDs less than 12 but without
+	 * 0x0000 in the last VDO, so we need to break the Discover SVIDs
+	 * request and return false here.
+	 */
+	return cnt == 7 ? true : false;
 abort:
 	tcpm_log(port, "SVID_DISCOVERY_MAX(%d) too low!", SVID_DISCOVERY_MAX);
 	return false;
-- 
2.17.1

Re: [PATCH 2/4] usb: typec: tcpm: fix multiple times discover svids error

[Heikki Krogerus]

On Mon, Mar 13, 2023 at 10:58:41AM +0800, Frank Wang wrote:
> PD3.0 Spec 6.4.4.3.2 say that only Responder supports 12 or more SVIDs,
> the Discover SVIDs Command Shall be executed multiple times until a
> Discover SVIDs VDO is returned ending either with a SVID value of
> 0x0000 in the last part of the last VDO or with a VDO containing two
> SVIDs with values of 0x0000.
> 
> In the current implementation, if the last VDO does not find that the
> Discover SVIDs Command would be executed multiple times even if the
> Responder SVIDs are less than 12, and we found some odd dockers just
> meet this case. So fix it.
> 
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 66de02a56f512..2962f7c261976 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1515,7 +1515,21 @@ static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
>  		pmdata->svids[pmdata->nsvids++] = svid;
>  		tcpm_log(port, "SVID %d: 0x%x", pmdata->nsvids, svid);
>  	}
> -	return true;
> +
> +	/*
> +	 * PD3.0 Spec 6.4.4.3.2: The SVIDs are returned 2 per VDO (see Table
> +	 * 6-43), and can be returned maximum 6 VDOs per response (see Figure
> +	 * 6-19). If the Respondersupports 12 or more SVID then the Discover
> +	 * SVIDs Command Shall be executed multiple times until a Discover
> +	 * SVIDs VDO is returned ending either with a SVID value of 0x0000 in
> +	 * the last part of the last VDO or with a VDO containing two SVIDs
> +	 * with values of 0x0000.
> +	 *
> +	 * However, some odd dockers support SVIDs less than 12 but without
> +	 * 0x0000 in the last VDO, so we need to break the Discover SVIDs
> +	 * request and return false here.
> +	 */
> +	return cnt == 7 ? true : false;

        return cnt == 7


thanks,

-- 
heikki

Re: [PATCH 2/4] usb: typec: tcpm: fix multiple times discover svids error

[Frank Wang]

Hi Heikki,

On 2023/3/14 17:22, Heikki Krogerus wrote:
> On Mon, Mar 13, 2023 at 10:58:41AM +0800, Frank Wang wrote:
>> PD3.0 Spec 6.4.4.3.2 say that only Responder supports 12 or more SVIDs,
>> the Discover SVIDs Command Shall be executed multiple times until a
>> Discover SVIDs VDO is returned ending either with a SVID value of
>> 0x0000 in the last part of the last VDO or with a VDO containing two
>> SVIDs with values of 0x0000.
>>
>> In the current implementation, if the last VDO does not find that the
>> Discover SVIDs Command would be executed multiple times even if the
>> Responder SVIDs are less than 12, and we found some odd dockers just
>> meet this case. So fix it.
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 66de02a56f512..2962f7c261976 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -1515,7 +1515,21 @@ static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
>>   		pmdata->svids[pmdata->nsvids++] = svid;
>>   		tcpm_log(port, "SVID %d: 0x%x", pmdata->nsvids, svid);
>>   	}
>> -	return true;
>> +
>> +	/*
>> +	 * PD3.0 Spec 6.4.4.3.2: The SVIDs are returned 2 per VDO (see Table
>> +	 * 6-43), and can be returned maximum 6 VDOs per response (see Figure
>> +	 * 6-19). If the Respondersupports 12 or more SVID then the Discover
>> +	 * SVIDs Command Shall be executed multiple times until a Discover
>> +	 * SVIDs VDO is returned ending either with a SVID value of 0x0000 in
>> +	 * the last part of the last VDO or with a VDO containing two SVIDs
>> +	 * with values of 0x0000.
>> +	 *
>> +	 * However, some odd dockers support SVIDs less than 12 but without
>> +	 * 0x0000 in the last VDO, so we need to break the Discover SVIDs
>> +	 * request and return false here.
>> +	 */
>> +	return cnt == 7 ? true : false;
>          return cnt == 7

Okay, next to fix it.

BR.
Frank

[PATCH 3/4] usb: typec: tcpm: add get max power support

[Frank Wang]

Traverse fixed pdos to calculate the maximum power that the charger
can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT
property.

Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 2962f7c261976..9e583060e64fc 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -6319,6 +6319,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port,
 	return 0;
 }
 
+static int tcpm_psy_get_input_power_limit(struct tcpm_port *port,
+					  union power_supply_propval *val)
+{
+	unsigned int src_mv, src_ma, max_src_mw = 0;
+	unsigned int i, tmp;
+
+	for (i = 0; i < port->nr_source_caps; i++) {
+		u32 pdo = port->source_caps[i];
+
+		if (pdo_type(pdo) == PDO_TYPE_FIXED) {
+			src_mv = pdo_fixed_voltage(pdo);
+			src_ma = pdo_max_current(pdo);
+			tmp = src_mv * src_ma / 1000;
+			max_src_mw = tmp > max_src_mw ? tmp : max_src_mw;
+		}
+	}
+
+	val->intval = max_src_mw;
+	return 0;
+}
+
 static int tcpm_psy_get_prop(struct power_supply *psy,
 			     enum power_supply_property psp,
 			     union power_supply_propval *val)
@@ -6348,6 +6369,9 @@ static int tcpm_psy_get_prop(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		ret = tcpm_psy_get_current_now(port, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_POWER_LIMIT:
+		tcpm_psy_get_input_power_limit(port, val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
-- 
2.17.1

[PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

[Frank Wang]

Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
set_pd_rx() before sending Soft Reset in case Source caps may be flushed
at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES state.

Without this patch, in PD charger stress test, the FUSB302 driver may
occur the following exceptions in power negotiation stage.

[ ...]
[ 4.512252] fusb302_irq_intn
[ 4.512260] AMS SOFT_RESET_AMS finished
[ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[ 4.514511] pd := on
[ 4.514516] pending state change SNK_WAIT_CAPABILITIES ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
[ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
[ 4.515431] IRQ: BC_LVL, handler pending
[ 4.515435] IRQ: PD sent good CRC
[ 4.516434] PD message header: 0
[ 4.516437] PD message len: 0
[ 4.516444] PD RX, header: 0x0 [1]

Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 9e583060e64fc..ba6bf71838eed 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4321,10 +4321,12 @@ static void run_state_machine(struct tcpm_port *port)
 		tcpm_set_state(port, unattached_state(port), 0);
 		break;
 	case SNK_WAIT_CAPABILITIES:
-		ret = port->tcpc->set_pd_rx(port->tcpc, true);
-		if (ret < 0) {
-			tcpm_set_state(port, SNK_READY, 0);
-			break;
+		if (port->prev_state != SOFT_RESET_SEND) {
+			ret = port->tcpc->set_pd_rx(port->tcpc, true);
+			if (ret < 0) {
+				tcpm_set_state(port, SNK_READY, 0);
+				break;
+			}
 		}
 		/*
 		 * If VBUS has never been low, and we time out waiting
@@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port *port)
 	case SOFT_RESET_SEND:
 		port->message_id = 0;
 		port->rx_msgid = -1;
+		port->tcpc->set_pd_rx(port->tcpc, true);
 		if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
 			tcpm_set_state_cond(port, hard_reset_state(port), 0);
 		else
-- 
2.17.1

Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

[Guenter Roeck]

On 3/12/23 19:58, Frank Wang wrote:
> Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
> set_pd_rx() before sending Soft Reset in case Source caps may be flushed
> at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES state.
> 

Isn't that a problem of the fusb302 driver that it flushes its buffers
unconditionally when its set_pd_rx() callback is called ?

Guenter

> Without this patch, in PD charger stress test, the FUSB302 driver may
> occur the following exceptions in power negotiation stage.
> 
> [ ...]
> [ 4.512252] fusb302_irq_intn
> [ 4.512260] AMS SOFT_RESET_AMS finished
> [ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> [ 4.514511] pd := on
> [ 4.514516] pending state change SNK_WAIT_CAPABILITIES ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
> [ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
> [ 4.515431] IRQ: BC_LVL, handler pending
> [ 4.515435] IRQ: PD sent good CRC
> [ 4.516434] PD message header: 0
> [ 4.516437] PD message len: 0
> [ 4.516444] PD RX, header: 0x0 [1]
> 
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 9e583060e64fc..ba6bf71838eed 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4321,10 +4321,12 @@ static void run_state_machine(struct tcpm_port *port)
>   		tcpm_set_state(port, unattached_state(port), 0);
>   		break;
>   	case SNK_WAIT_CAPABILITIES:
> -		ret = port->tcpc->set_pd_rx(port->tcpc, true);
> -		if (ret < 0) {
> -			tcpm_set_state(port, SNK_READY, 0);
> -			break;
> +		if (port->prev_state != SOFT_RESET_SEND) {
> +			ret = port->tcpc->set_pd_rx(port->tcpc, true);
> +			if (ret < 0) {
> +				tcpm_set_state(port, SNK_READY, 0);
> +				break;
> +			}
>   		}
>   		/*
>   		 * If VBUS has never been low, and we time out waiting
> @@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port *port)
>   	case SOFT_RESET_SEND:
>   		port->message_id = 0;
>   		port->rx_msgid = -1;
> +		port->tcpc->set_pd_rx(port->tcpc, true);
>   		if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
>   			tcpm_set_state_cond(port, hard_reset_state(port), 0);
>   		else

Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

[Frank Wang]

Hi Guenter,

On 2023/3/17 20:58, Guenter Roeck wrote:
> On 3/12/23 19:58, Frank Wang wrote:
>> Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
>> set_pd_rx() before sending Soft Reset in case Source caps may be flushed
>> at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES 
>> state.
>>
>
> Isn't that a problem of the fusb302 driver that it flushes its buffers
> unconditionally when its set_pd_rx() callback is called ?
>
> Guenter
>

The story goes like this,  the fusb302 notified SOFT_RESET completion to 
TCPM and began to receive the Source Caps automatically,
TCPM got completion from fusb302 and changed state to 
SNK_WAIT_CAPABILITIES and invoked set_pd_rx() callback. However, the
fusb302 or TCPM's worker may be not scheduled in time, set_pd_rx() would 
be performed after the Source Caps packets had received
into fusb302's FIFO, even after the GoodCRC (Source Caps) had be replied.

So make forward set_pd_rx() process before PD_CTRL_SOFT_RESET sent at 
SOFT_RESET_SEND state can cleanup the context in our side
and ensure the right PD commucation. I am not sure whether it is sensible?

BR.
Frank

>> Without this patch, in PD charger stress test, the FUSB302 driver may
>> occur the following exceptions in power negotiation stage.
>>
>> [ ...]
>> [ 4.512252] fusb302_irq_intn
>> [ 4.512260] AMS SOFT_RESET_AMS finished
>> [ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES 
>> [rev3 NONE_AMS]
>> [ 4.514511] pd := on
>> [ 4.514516] pending state change SNK_WAIT_CAPABILITIES 
>> ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
>> [ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
>> [ 4.515431] IRQ: BC_LVL, handler pending
>> [ 4.515435] IRQ: PD sent good CRC
>> [ 4.516434] PD message header: 0
>> [ 4.516437] PD message len: 0
>> [ 4.516444] PD RX, header: 0x0 [1]
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c 
>> b/drivers/usb/typec/tcpm/tcpm.c
>> index 9e583060e64fc..ba6bf71838eed 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -4321,10 +4321,12 @@ static void run_state_machine(struct 
>> tcpm_port *port)
>>           tcpm_set_state(port, unattached_state(port), 0);
>>           break;
>>       case SNK_WAIT_CAPABILITIES:
>> -        ret = port->tcpc->set_pd_rx(port->tcpc, true);
>> -        if (ret < 0) {
>> -            tcpm_set_state(port, SNK_READY, 0);
>> -            break;
>> +        if (port->prev_state != SOFT_RESET_SEND) {
>> +            ret = port->tcpc->set_pd_rx(port->tcpc, true);
>> +            if (ret < 0) {
>> +                tcpm_set_state(port, SNK_READY, 0);
>> +                break;
>> +            }
>>           }
>>           /*
>>            * If VBUS has never been low, and we time out waiting
>> @@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port 
>> *port)
>>       case SOFT_RESET_SEND:
>>           port->message_id = 0;
>>           port->rx_msgid = -1;
>> +        port->tcpc->set_pd_rx(port->tcpc, true);
>>           if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
>>               tcpm_set_state_cond(port, hard_reset_state(port), 0);
>>           else
>

Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

[Guenter Roeck]

On Mon, Mar 20, 2023 at 10:38:35AM +0800, Frank Wang wrote:
> Hi Guenter,
> 
> On 2023/3/17 20:58, Guenter Roeck wrote:
> > On 3/12/23 19:58, Frank Wang wrote:
> > > Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
> > > set_pd_rx() before sending Soft Reset in case Source caps may be flushed
> > > at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES
> > > state.
> > > 
> > 
> > Isn't that a problem of the fusb302 driver that it flushes its buffers
> > unconditionally when its set_pd_rx() callback is called ?
> > 
> > Guenter
> > 
> 
> The story goes like this,  the fusb302 notified SOFT_RESET completion to
> TCPM and began to receive the Source Caps automatically,
> TCPM got completion from fusb302 and changed state to SNK_WAIT_CAPABILITIES
> and invoked set_pd_rx() callback. However, the
> fusb302 or TCPM's worker may be not scheduled in time, set_pd_rx() would be
> performed after the Source Caps packets had received
> into fusb302's FIFO, even after the GoodCRC (Source Caps) had be replied.
> 

Yes, but the fusb302 driver clears its fifo in the set_pd_rx() callback.
I see that as a problem in the fusb302 driver( it could clear its fifo when
it notifies the TCPM that soft reset is complete, for example), and I am
hesitant to work around that problem in the tcpm code.

Guenter

> So make forward set_pd_rx() process before PD_CTRL_SOFT_RESET sent at
> SOFT_RESET_SEND state can cleanup the context in our side
> and ensure the right PD commucation. I am not sure whether it is sensible?
> 
> BR.
> Frank
> 
> > > Without this patch, in PD charger stress test, the FUSB302 driver may
> > > occur the following exceptions in power negotiation stage.
> > > 
> > > [ ...]
> > > [ 4.512252] fusb302_irq_intn
> > > [ 4.512260] AMS SOFT_RESET_AMS finished
> > > [ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES
> > > [rev3 NONE_AMS]
> > > [ 4.514511] pd := on
> > > [ 4.514516] pending state change SNK_WAIT_CAPABILITIES
> > > ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
> > > [ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
> > > [ 4.515431] IRQ: BC_LVL, handler pending
> > > [ 4.515435] IRQ: PD sent good CRC
> > > [ 4.516434] PD message header: 0
> > > [ 4.516437] PD message len: 0
> > > [ 4.516444] PD RX, header: 0x0 [1]
> > > 
> > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> > > ---
> > >   drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > > b/drivers/usb/typec/tcpm/tcpm.c
> > > index 9e583060e64fc..ba6bf71838eed 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -4321,10 +4321,12 @@ static void run_state_machine(struct
> > > tcpm_port *port)
> > >           tcpm_set_state(port, unattached_state(port), 0);
> > >           break;
> > >       case SNK_WAIT_CAPABILITIES:
> > > -        ret = port->tcpc->set_pd_rx(port->tcpc, true);
> > > -        if (ret < 0) {
> > > -            tcpm_set_state(port, SNK_READY, 0);
> > > -            break;
> > > +        if (port->prev_state != SOFT_RESET_SEND) {
> > > +            ret = port->tcpc->set_pd_rx(port->tcpc, true);
> > > +            if (ret < 0) {
> > > +                tcpm_set_state(port, SNK_READY, 0);
> > > +                break;
> > > +            }
> > >           }
> > >           /*
> > >            * If VBUS has never been low, and we time out waiting
> > > @@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port
> > > *port)
> > >       case SOFT_RESET_SEND:
> > >           port->message_id = 0;
> > >           port->rx_msgid = -1;
> > > +        port->tcpc->set_pd_rx(port->tcpc, true);
> > >           if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
> > >               tcpm_set_state_cond(port, hard_reset_state(port), 0);
> > >           else
> > 

Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

[Frank Wang]

Hi Guenter,

On 2023/3/20 11:35, Guenter Roeck wrote:
> On Mon, Mar 20, 2023 at 10:38:35AM +0800, Frank Wang wrote:
>> Hi Guenter,
>>
>> On 2023/3/17 20:58, Guenter Roeck wrote:
>>> On 3/12/23 19:58, Frank Wang wrote:
>>>> Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
>>>> set_pd_rx() before sending Soft Reset in case Source caps may be flushed
>>>> at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES
>>>> state.
>>>>
>>> Isn't that a problem of the fusb302 driver that it flushes its buffers
>>> unconditionally when its set_pd_rx() callback is called ?
>>>
>>> Guenter
>>>
>> The story goes like this,  the fusb302 notified SOFT_RESET completion to
>> TCPM and began to receive the Source Caps automatically,
>> TCPM got completion from fusb302 and changed state to SNK_WAIT_CAPABILITIES
>> and invoked set_pd_rx() callback. However, the
>> fusb302 or TCPM's worker may be not scheduled in time, set_pd_rx() would be
>> performed after the Source Caps packets had received
>> into fusb302's FIFO, even after the GoodCRC (Source Caps) had be replied.
>>
> Yes, but the fusb302 driver clears its fifo in the set_pd_rx() callback.
> I see that as a problem in the fusb302 driver( it could clear its fifo when
> it notifies the TCPM that soft reset is complete, for example), and I am
> hesitant to work around that problem in the tcpm code.
>
> Guenter

Okay, I shall abandon this from the series.

BR.
Frank

>> So make forward set_pd_rx() process before PD_CTRL_SOFT_RESET sent at
>> SOFT_RESET_SEND state can cleanup the context in our side
>> and ensure the right PD commucation. I am not sure whether it is sensible?
>>
>> BR.
>> Frank
>>
>>>> Without this patch, in PD charger stress test, the FUSB302 driver may
>>>> occur the following exceptions in power negotiation stage.
>>>>
>>>> [ ...]
>>>> [ 4.512252] fusb302_irq_intn
>>>> [ 4.512260] AMS SOFT_RESET_AMS finished
>>>> [ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES
>>>> [rev3 NONE_AMS]
>>>> [ 4.514511] pd := on
>>>> [ 4.514516] pending state change SNK_WAIT_CAPABILITIES
>>>> ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
>>>> [ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
>>>> [ 4.515431] IRQ: BC_LVL, handler pending
>>>> [ 4.515435] IRQ: PD sent good CRC
>>>> [ 4.516434] PD message header: 0
>>>> [ 4.516437] PD message len: 0
>>>> [ 4.516444] PD RX, header: 0x0 [1]
>>>>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> ---
>>>>    drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c
>>>> b/drivers/usb/typec/tcpm/tcpm.c
>>>> index 9e583060e64fc..ba6bf71838eed 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>> @@ -4321,10 +4321,12 @@ static void run_state_machine(struct
>>>> tcpm_port *port)
>>>>            tcpm_set_state(port, unattached_state(port), 0);
>>>>            break;
>>>>        case SNK_WAIT_CAPABILITIES:
>>>> -        ret = port->tcpc->set_pd_rx(port->tcpc, true);
>>>> -        if (ret < 0) {
>>>> -            tcpm_set_state(port, SNK_READY, 0);
>>>> -            break;
>>>> +        if (port->prev_state != SOFT_RESET_SEND) {
>>>> +            ret = port->tcpc->set_pd_rx(port->tcpc, true);
>>>> +            if (ret < 0) {
>>>> +                tcpm_set_state(port, SNK_READY, 0);
>>>> +                break;
>>>> +            }
>>>>            }
>>>>            /*
>>>>             * If VBUS has never been low, and we time out waiting
>>>> @@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port
>>>> *port)
>>>>        case SOFT_RESET_SEND:
>>>>            port->message_id = 0;
>>>>            port->rx_msgid = -1;
>>>> +        port->tcpc->set_pd_rx(port->tcpc, true);
>>>>            if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
>>>>                tcpm_set_state_cond(port, hard_reset_state(port), 0);
>>>>            else

-- 
底层平台部 王明成（Frank Wang）
******************************************
公司：瑞芯微电子股份有限公司
地址：福建省福州市铜盘路软件大道89号软件园A区21号楼
邮编：350003
Tel：0591-83991906转8960（分机号）
E-mail：frank.wang@rock-chips.com
********************************************************************