diff mbox series

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

Message ID 20230313025843.17162-5-frank.wang@rock-chips.com
State New
Headers show
Series Fix some defects related Type-C TCPM | expand

Commit Message

Frank Wang March 13, 2023, 2:58 a.m. UTC 
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(-)

Comments

Guenter Roeck March 17, 2023, 12:58 p.m. UTC | #1 
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
Frank Wang March 20, 2023, 2:38 a.m. UTC | #2 
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
>
Frank Wang March 20, 2023, 6:28 a.m. UTC | #3 
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
diff mbox series

Patch

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