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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>>>> ---
>>>>> 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
>
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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
> > > ---
> > > ? 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
> >
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 <[email protected]>
>>>> ---
>>>> 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：[email protected]
********************************************************************