[PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

From the spec:
"7.1.5 Response to Hard Resets
Hard Reset Signaling indicates a communication failure has occurred and
the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
May reset during a Hard Reset since the VBUS voltage will be less than
vSafe5V for an extended period of time. After establishing the vSafe0V
voltage condition on VBUS, the Source Shall wait tSrcRecover before
re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
to the VCONN timing as specified in [USB Type-C 1.3]."

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 3ef37202ee37..e41c4e5d3c71 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
 		break;
 	case SRC_HARD_RESET_VBUS_OFF:
-		tcpm_set_vconn(port, true);
+		/*
+		 * 7.1.5 Response to Hard Resets
+		 * Hard Reset Signaling indicates a communication failure has occurred and the
+		 * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
+		 * drive VBUS to vSafe0V as shown in Figure 7-9.
+		 */
+		tcpm_set_vconn(port, false);
 		tcpm_set_vbus(port, false);
 		tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
 			       tcpm_data_role_for_source(port));
-		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
 		break;
 	case SRC_HARD_RESET_VBUS_ON:
+		tcpm_set_vconn(port, true);
 		tcpm_set_vbus(port, true);
 		port->tcpc->set_pd_rx(port->tcpc, true);
 		tcpm_set_attached_state(port, true);
@@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
 		tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
 		break;
 	case SRC_HARD_RESET_VBUS_OFF:
-		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
+		/*
+		 * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
+		 * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
+		 */
+		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
 		break;
 	case HARD_RESET_SEND:
 		break;
-- 
2.28.0.236.gb10cc79966-goog

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Guenter Roeck]

On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
>>From the spec:
> "7.1.5 Response to Hard Resets
> Hard Reset Signaling indicates a communication failure has occurred and
> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> May reset during a Hard Reset since the VBUS voltage will be less than
> vSafe5V for an extended period of time. After establishing the vSafe0V
> voltage condition on VBUS, the Source Shall wait tSrcRecover before
> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> to the VCONN timing as specified in [USB Type-C 1.3]."
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3ef37202ee37..e41c4e5d3c71 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
>  			tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
>  		break;
>  	case SRC_HARD_RESET_VBUS_OFF:
> -		tcpm_set_vconn(port, true);
> +		/*
> +		 * 7.1.5 Response to Hard Resets
> +		 * Hard Reset Signaling indicates a communication failure has occurred and the
> +		 * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> +		 * drive VBUS to vSafe0V as shown in Figure 7-9.
> +		 */
> +		tcpm_set_vconn(port, false);
>  		tcpm_set_vbus(port, false);
>  		tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
>  			       tcpm_data_role_for_source(port));
> -		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);

I am a bit concerned about this. If I understand correctly, it means that
we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
Is that correct ? What happens if that event is never received ?

Thanks,
Guenter

>  		break;
>  	case SRC_HARD_RESET_VBUS_ON:
> +		tcpm_set_vconn(port, true);
>  		tcpm_set_vbus(port, true);
>  		port->tcpc->set_pd_rx(port->tcpc, true);
>  		tcpm_set_attached_state(port, true);
> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
>  		tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
>  		break;
>  	case SRC_HARD_RESET_VBUS_OFF:
> -		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> +		/*
> +		 * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> +		 * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> +		 */
> +		tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>  		break;
>  	case HARD_RESET_SEND:
>  		break;
> 

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> >>From the spec:
> > "7.1.5 Response to Hard Resets
> > Hard Reset Signaling indicates a communication failure has occurred and
> > the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> > and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> > May reset during a Hard Reset since the VBUS voltage will be less than
> > vSafe5V for an extended period of time. After establishing the vSafe0V
> > voltage condition on VBUS, the Source Shall wait tSrcRecover before
> > re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> > to the VCONN timing as specified in [USB Type-C 1.3]."
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3ef37202ee37..e41c4e5d3c71 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> >                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> >               break;
> >       case SRC_HARD_RESET_VBUS_OFF:
> > -             tcpm_set_vconn(port, true);
> > +             /*
> > +              * 7.1.5 Response to Hard Resets
> > +              * Hard Reset Signaling indicates a communication failure has occurred and the
> > +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> > +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> > +              */
> > +             tcpm_set_vconn(port, false);
> >               tcpm_set_vbus(port, false);
> >               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> >                              tcpm_data_role_for_source(port));
> > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>
> I am a bit concerned about this. If I understand correctly, it means that
> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> Is that correct ? What happens if that event is never received ?
>
> Thanks,
> Guenter

The term PD event is a little ambiguous to me. Trying to summarize the workflow.
Lower level tcpc driver would have to call tcpm_vbus_change which
would in-turn trigger TCPM_VBUS_EVENT
and queries port->tcpc->get_vbus to get the vbus status. It is not
really a PD protocol driven event hence the
confusion.

"What happens if that event is never received ?"
Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
tcpm_vbus_change.
Do you suspect that existing tcpc would not have the capability to
monitor vbus status while sourcing and call tcpm_vbus_change?

Thanks,
Badhri


> >               break;
> >       case SRC_HARD_RESET_VBUS_ON:
> > +             tcpm_set_vconn(port, true);
> >               tcpm_set_vbus(port, true);
> >               port->tcpc->set_pd_rx(port->tcpc, true);
> >               tcpm_set_attached_state(port, true);
> > @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> >               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> >               break;
> >       case SRC_HARD_RESET_VBUS_OFF:
> > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> > +             /*
> > +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> > +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> > +              */
> > +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >               break;
> >       case HARD_RESET_SEND:
> >               break;
> >
>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Guenter Roeck]

On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> > >>From the spec:
> > > "7.1.5 Response to Hard Resets
> > > Hard Reset Signaling indicates a communication failure has occurred and
> > > the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> > > and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> > > May reset during a Hard Reset since the VBUS voltage will be less than
> > > vSafe5V for an extended period of time. After establishing the vSafe0V
> > > voltage condition on VBUS, the Source Shall wait tSrcRecover before
> > > re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> > > to the VCONN timing as specified in [USB Type-C 1.3]."
> > >
> > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > index 3ef37202ee37..e41c4e5d3c71 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> > >                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> > >               break;
> > >       case SRC_HARD_RESET_VBUS_OFF:
> > > -             tcpm_set_vconn(port, true);
> > > +             /*
> > > +              * 7.1.5 Response to Hard Resets
> > > +              * Hard Reset Signaling indicates a communication failure has occurred and the
> > > +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> > > +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> > > +              */
> > > +             tcpm_set_vconn(port, false);
> > >               tcpm_set_vbus(port, false);
> > >               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> > >                              tcpm_data_role_for_source(port));
> > > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >
> > I am a bit concerned about this. If I understand correctly, it means that
> > we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> > Is that correct ? What happens if that event is never received ?
> >
> > Thanks,
> > Guenter
> 
> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
> Lower level tcpc driver would have to call tcpm_vbus_change which
> would in-turn trigger TCPM_VBUS_EVENT
> and queries port->tcpc->get_vbus to get the vbus status. It is not
> really a PD protocol driven event hence the
> confusion.
> 
> "What happens if that event is never received ?"
> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
> tcpm_vbus_change.
> Do you suspect that existing tcpc would not have the capability to
> monitor vbus status while sourcing and call tcpm_vbus_change?
> 
That, or the driver might be buggy, or the hardware does't signal a status
update, or the update gets lost. I think we should have some backup,
to trigger if the event is not received in a reasonable amout of time.
I don't know if the specification has some kind of maximum limit. If
not, we should still have something.

Thanks,
Guenter

> Thanks,
> Badhri
> 
> 
> > >               break;
> > >       case SRC_HARD_RESET_VBUS_ON:
> > > +             tcpm_set_vconn(port, true);
> > >               tcpm_set_vbus(port, true);
> > >               port->tcpc->set_pd_rx(port->tcpc, true);
> > >               tcpm_set_attached_state(port, true);
> > > @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> > >               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> > >               break;
> > >       case SRC_HARD_RESET_VBUS_OFF:
> > > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> > > +             /*
> > > +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> > > +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> > > +              */
> > > +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> > >               break;
> > >       case HARD_RESET_SEND:
> > >               break;
> > >
> >

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
> > On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> > > >>From the spec:
> > > > "7.1.5 Response to Hard Resets
> > > > Hard Reset Signaling indicates a communication failure has occurred and
> > > > the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> > > > and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> > > > May reset during a Hard Reset since the VBUS voltage will be less than
> > > > vSafe5V for an extended period of time. After establishing the vSafe0V
> > > > voltage condition on VBUS, the Source Shall wait tSrcRecover before
> > > > re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> > > > to the VCONN timing as specified in [USB Type-C 1.3]."
> > > >
> > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > > index 3ef37202ee37..e41c4e5d3c71 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > > @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> > > >                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> > > >               break;
> > > >       case SRC_HARD_RESET_VBUS_OFF:
> > > > -             tcpm_set_vconn(port, true);
> > > > +             /*
> > > > +              * 7.1.5 Response to Hard Resets
> > > > +              * Hard Reset Signaling indicates a communication failure has occurred and the
> > > > +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> > > > +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> > > > +              */
> > > > +             tcpm_set_vconn(port, false);
> > > >               tcpm_set_vbus(port, false);
> > > >               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> > > >                              tcpm_data_role_for_source(port));
> > > > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> > >
> > > I am a bit concerned about this. If I understand correctly, it means that
> > > we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> > > Is that correct ? What happens if that event is never received ?
> > >
> > > Thanks,
> > > Guenter
> >
> > The term PD event is a little ambiguous to me. Trying to summarize the workflow.
> > Lower level tcpc driver would have to call tcpm_vbus_change which
> > would in-turn trigger TCPM_VBUS_EVENT
> > and queries port->tcpc->get_vbus to get the vbus status. It is not
> > really a PD protocol driven event hence the
> > confusion.
> >
> > "What happens if that event is never received ?"
> > Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
> > tcpm_vbus_change.
> > Do you suspect that existing tcpc would not have the capability to
> > monitor vbus status while sourcing and call tcpm_vbus_change?
> >
> That, or the driver might be buggy, or the hardware does't signal a status
> update, or the update gets lost. I think we should have some backup,
> to trigger if the event is not received in a reasonable amout of time.
> I don't know if the specification has some kind of maximum limit. If
> not, we should still have something
>
> Thanks,
> Guenter

Got it ! The specification actually has a bound for vbus off.
tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
So I will bound it to that.

From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
and then tells the Device Policy Manager to instruct the power supply
to perform a Hard Reset. The transition to vSafe0V Shall occur within
tSafe0V (t2).
5 After tSrcRecover the Source applies power to VBUS in an attempt to
re-establish communication with the Sink and resume USB Default
Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
(t4).

Thanks,
Badhri

>
> > Thanks,
> > Badhri
> >
> >
> > > >               break;
> > > >       case SRC_HARD_RESET_VBUS_ON:
> > > > +             tcpm_set_vconn(port, true);
> > > >               tcpm_set_vbus(port, true);
> > > >               port->tcpc->set_pd_rx(port->tcpc, true);
> > > >               tcpm_set_attached_state(port, true);
> > > > @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> > > >               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> > > >               break;
> > > >       case SRC_HARD_RESET_VBUS_OFF:
> > > > -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> > > > +             /*
> > > > +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> > > > +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> > > > +              */
> > > > +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> > > >               break;
> > > >       case HARD_RESET_SEND:
> > > >               break;
> > > >
> > >

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Guenter Roeck]

On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
> On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
>>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
>>>>> >From the spec:
>>>>> "7.1.5 Response to Hard Resets
>>>>> Hard Reset Signaling indicates a communication failure has occurred and
>>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
>>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
>>>>> May reset during a Hard Reset since the VBUS voltage will be less than
>>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
>>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
>>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
>>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
>>>>>
>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>>>> ---
>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
>>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>>> index 3ef37202ee37..e41c4e5d3c71 100644
>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
>>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
>>>>>               break;
>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>> -             tcpm_set_vconn(port, true);
>>>>> +             /*
>>>>> +              * 7.1.5 Response to Hard Resets
>>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
>>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
>>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
>>>>> +              */
>>>>> +             tcpm_set_vconn(port, false);
>>>>>               tcpm_set_vbus(port, false);
>>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
>>>>>                              tcpm_data_role_for_source(port));
>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>
>>>> I am a bit concerned about this. If I understand correctly, it means that
>>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
>>>> Is that correct ? What happens if that event is never received ?
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
>>> Lower level tcpc driver would have to call tcpm_vbus_change which
>>> would in-turn trigger TCPM_VBUS_EVENT
>>> and queries port->tcpc->get_vbus to get the vbus status. It is not
>>> really a PD protocol driven event hence the
>>> confusion.
>>>
>>> "What happens if that event is never received ?"
>>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
>>> tcpm_vbus_change.
>>> Do you suspect that existing tcpc would not have the capability to
>>> monitor vbus status while sourcing and call tcpm_vbus_change?
>>>
>> That, or the driver might be buggy, or the hardware does't signal a status
>> update, or the update gets lost. I think we should have some backup,
>> to trigger if the event is not received in a reasonable amout of time.
>> I don't know if the specification has some kind of maximum limit. If
>> not, we should still have something
>>
>> Thanks,
>> Guenter
> 
> Got it ! The specification actually has a bound for vbus off.
> tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
> So I will bound it to that.
> 

Excellent. Thanks a lot for looking into this!

Guenter

>>From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
> 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
> and then tells the Device Policy Manager to instruct the power supply
> to perform a Hard Reset. The transition to vSafe0V Shall occur within
> tSafe0V (t2).
> 5 After tSrcRecover the Source applies power to VBUS in an attempt to
> re-establish communication with the Sink and resume USB Default
> Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
> (t4).
> 
> Thanks,
> Badhri
> 
>>
>>> Thanks,
>>> Badhri
>>>
>>>
>>>>>               break;
>>>>>       case SRC_HARD_RESET_VBUS_ON:
>>>>> +             tcpm_set_vconn(port, true);
>>>>>               tcpm_set_vbus(port, true);
>>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
>>>>>               tcpm_set_attached_state(port, true);
>>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
>>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
>>>>>               break;
>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
>>>>> +             /*
>>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
>>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
>>>>> +              */
>>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>>               break;
>>>>>       case HARD_RESET_SEND:
>>>>>               break;
>>>>>
>>>>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

Thanks Guenter ! However I don't see a reviewed-by tag :)

On Tue, Aug 11, 2020 at 1:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
> > On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
> >>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> >>>>> >From the spec:
> >>>>> "7.1.5 Response to Hard Resets
> >>>>> Hard Reset Signaling indicates a communication failure has occurred and
> >>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> >>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> >>>>> May reset during a Hard Reset since the VBUS voltage will be less than
> >>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
> >>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
> >>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> >>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
> >>>>>
> >>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> >>>>> ---
> >>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> >>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>>>> index 3ef37202ee37..e41c4e5d3c71 100644
> >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
> >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> >>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> >>>>>               break;
> >>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>> -             tcpm_set_vconn(port, true);
> >>>>> +             /*
> >>>>> +              * 7.1.5 Response to Hard Resets
> >>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
> >>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> >>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> >>>>> +              */
> >>>>> +             tcpm_set_vconn(port, false);
> >>>>>               tcpm_set_vbus(port, false);
> >>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> >>>>>                              tcpm_data_role_for_source(port));
> >>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>
> >>>> I am a bit concerned about this. If I understand correctly, it means that
> >>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> >>>> Is that correct ? What happens if that event is never received ?
> >>>>
> >>>> Thanks,
> >>>> Guenter
> >>>
> >>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
> >>> Lower level tcpc driver would have to call tcpm_vbus_change which
> >>> would in-turn trigger TCPM_VBUS_EVENT
> >>> and queries port->tcpc->get_vbus to get the vbus status. It is not
> >>> really a PD protocol driven event hence the
> >>> confusion.
> >>>
> >>> "What happens if that event is never received ?"
> >>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
> >>> tcpm_vbus_change.
> >>> Do you suspect that existing tcpc would not have the capability to
> >>> monitor vbus status while sourcing and call tcpm_vbus_change?
> >>>
> >> That, or the driver might be buggy, or the hardware does't signal a status
> >> update, or the update gets lost. I think we should have some backup,
> >> to trigger if the event is not received in a reasonable amout of time.
> >> I don't know if the specification has some kind of maximum limit. If
> >> not, we should still have something
> >>
> >> Thanks,
> >> Guenter
> >
> > Got it ! The specification actually has a bound for vbus off.
> > tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
> > So I will bound it to that.
> >
>
> Excellent. Thanks a lot for looking into this!
>
> Guenter
>
> >>From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
> > 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
> > and then tells the Device Policy Manager to instruct the power supply
> > to perform a Hard Reset. The transition to vSafe0V Shall occur within
> > tSafe0V (t2).
> > 5 After tSrcRecover the Source applies power to VBUS in an attempt to
> > re-establish communication with the Sink and resume USB Default
> > Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
> > (t4).
> >
> > Thanks,
> > Badhri
> >
> >>
> >>> Thanks,
> >>> Badhri
> >>>
> >>>
> >>>>>               break;
> >>>>>       case SRC_HARD_RESET_VBUS_ON:
> >>>>> +             tcpm_set_vconn(port, true);
> >>>>>               tcpm_set_vbus(port, true);
> >>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
> >>>>>               tcpm_set_attached_state(port, true);
> >>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> >>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> >>>>>               break;
> >>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> >>>>> +             /*
> >>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> >>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> >>>>> +              */
> >>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>>               break;
> >>>>>       case HARD_RESET_SEND:
> >>>>>               break;
> >>>>>
> >>>>
>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Guenter Roeck]

On 8/11/20 4:21 PM, Badhri Jagan Sridharan wrote:
> Thanks Guenter ! However I don't see a reviewed-by tag :)
> 

Confused. Did you send a v2 with the changes we discussed ?
I didn't see that.

Thanks,
Guenter

> On Tue, Aug 11, 2020 at 1:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
>>> On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
>>>>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
>>>>>>> >From the spec:
>>>>>>> "7.1.5 Response to Hard Resets
>>>>>>> Hard Reset Signaling indicates a communication failure has occurred and
>>>>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
>>>>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
>>>>>>> May reset during a Hard Reset since the VBUS voltage will be less than
>>>>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
>>>>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
>>>>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
>>>>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
>>>>>>>
>>>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>>>>>> ---
>>>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
>>>>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>>>>> index 3ef37202ee37..e41c4e5d3c71 100644
>>>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
>>>>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
>>>>>>>               break;
>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>>>> -             tcpm_set_vconn(port, true);
>>>>>>> +             /*
>>>>>>> +              * 7.1.5 Response to Hard Resets
>>>>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
>>>>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
>>>>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
>>>>>>> +              */
>>>>>>> +             tcpm_set_vconn(port, false);
>>>>>>>               tcpm_set_vbus(port, false);
>>>>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
>>>>>>>                              tcpm_data_role_for_source(port));
>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>>>
>>>>>> I am a bit concerned about this. If I understand correctly, it means that
>>>>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
>>>>>> Is that correct ? What happens if that event is never received ?
>>>>>>
>>>>>> Thanks,
>>>>>> Guenter
>>>>>
>>>>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
>>>>> Lower level tcpc driver would have to call tcpm_vbus_change which
>>>>> would in-turn trigger TCPM_VBUS_EVENT
>>>>> and queries port->tcpc->get_vbus to get the vbus status. It is not
>>>>> really a PD protocol driven event hence the
>>>>> confusion.
>>>>>
>>>>> "What happens if that event is never received ?"
>>>>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
>>>>> tcpm_vbus_change.
>>>>> Do you suspect that existing tcpc would not have the capability to
>>>>> monitor vbus status while sourcing and call tcpm_vbus_change?
>>>>>
>>>> That, or the driver might be buggy, or the hardware does't signal a status
>>>> update, or the update gets lost. I think we should have some backup,
>>>> to trigger if the event is not received in a reasonable amout of time.
>>>> I don't know if the specification has some kind of maximum limit. If
>>>> not, we should still have something
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> Got it ! The specification actually has a bound for vbus off.
>>> tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
>>> So I will bound it to that.
>>>
>>
>> Excellent. Thanks a lot for looking into this!
>>
>> Guenter
>>
>>> >From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
>>> 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
>>> and then tells the Device Policy Manager to instruct the power supply
>>> to perform a Hard Reset. The transition to vSafe0V Shall occur within
>>> tSafe0V (t2).
>>> 5 After tSrcRecover the Source applies power to VBUS in an attempt to
>>> re-establish communication with the Sink and resume USB Default
>>> Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
>>> (t4).
>>>
>>> Thanks,
>>> Badhri
>>>
>>>>
>>>>> Thanks,
>>>>> Badhri
>>>>>
>>>>>
>>>>>>>               break;
>>>>>>>       case SRC_HARD_RESET_VBUS_ON:
>>>>>>> +             tcpm_set_vconn(port, true);
>>>>>>>               tcpm_set_vbus(port, true);
>>>>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
>>>>>>>               tcpm_set_attached_state(port, true);
>>>>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
>>>>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
>>>>>>>               break;
>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
>>>>>>> +             /*
>>>>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
>>>>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
>>>>>>> +              */
>>>>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>>>>               break;
>>>>>>>       case HARD_RESET_SEND:
>>>>>>>               break;
>>>>>>>
>>>>>>
>>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

Yes I did ! https://lkml.org/lkml/2020/8/11/788 !

For me gmail is grouping V1 and V2 patches in the same thread so
thought you saw the v2 patch :P

Thanks,
Badhri

On Tue, Aug 11, 2020 at 5:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/11/20 4:21 PM, Badhri Jagan Sridharan wrote:
> > Thanks Guenter ! However I don't see a reviewed-by tag :)
> >
>
> Confused. Did you send a v2 with the changes we discussed ?
> I didn't see that.
>
> Thanks,
> Guenter
>
> > On Tue, Aug 11, 2020 at 1:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
> >>> On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
> >>>>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>>>
> >>>>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> >>>>>>> >From the spec:
> >>>>>>> "7.1.5 Response to Hard Resets
> >>>>>>> Hard Reset Signaling indicates a communication failure has occurred and
> >>>>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> >>>>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> >>>>>>> May reset during a Hard Reset since the VBUS voltage will be less than
> >>>>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
> >>>>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
> >>>>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> >>>>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
> >>>>>>>
> >>>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> >>>>>>> ---
> >>>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> >>>>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>> index 3ef37202ee37..e41c4e5d3c71 100644
> >>>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> >>>>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> >>>>>>>               break;
> >>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>>>> -             tcpm_set_vconn(port, true);
> >>>>>>> +             /*
> >>>>>>> +              * 7.1.5 Response to Hard Resets
> >>>>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
> >>>>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> >>>>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> >>>>>>> +              */
> >>>>>>> +             tcpm_set_vconn(port, false);
> >>>>>>>               tcpm_set_vbus(port, false);
> >>>>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> >>>>>>>                              tcpm_data_role_for_source(port));
> >>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>>>
> >>>>>> I am a bit concerned about this. If I understand correctly, it means that
> >>>>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> >>>>>> Is that correct ? What happens if that event is never received ?
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Guenter
> >>>>>
> >>>>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
> >>>>> Lower level tcpc driver would have to call tcpm_vbus_change which
> >>>>> would in-turn trigger TCPM_VBUS_EVENT
> >>>>> and queries port->tcpc->get_vbus to get the vbus status. It is not
> >>>>> really a PD protocol driven event hence the
> >>>>> confusion.
> >>>>>
> >>>>> "What happens if that event is never received ?"
> >>>>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
> >>>>> tcpm_vbus_change.
> >>>>> Do you suspect that existing tcpc would not have the capability to
> >>>>> monitor vbus status while sourcing and call tcpm_vbus_change?
> >>>>>
> >>>> That, or the driver might be buggy, or the hardware does't signal a status
> >>>> update, or the update gets lost. I think we should have some backup,
> >>>> to trigger if the event is not received in a reasonable amout of time.
> >>>> I don't know if the specification has some kind of maximum limit. If
> >>>> not, we should still have something
> >>>>
> >>>> Thanks,
> >>>> Guenter
> >>>
> >>> Got it ! The specification actually has a bound for vbus off.
> >>> tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
> >>> So I will bound it to that.
> >>>
> >>
> >> Excellent. Thanks a lot for looking into this!
> >>
> >> Guenter
> >>
> >>> >From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
> >>> 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
> >>> and then tells the Device Policy Manager to instruct the power supply
> >>> to perform a Hard Reset. The transition to vSafe0V Shall occur within
> >>> tSafe0V (t2).
> >>> 5 After tSrcRecover the Source applies power to VBUS in an attempt to
> >>> re-establish communication with the Sink and resume USB Default
> >>> Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
> >>> (t4).
> >>>
> >>> Thanks,
> >>> Badhri
> >>>
> >>>>
> >>>>> Thanks,
> >>>>> Badhri
> >>>>>
> >>>>>
> >>>>>>>               break;
> >>>>>>>       case SRC_HARD_RESET_VBUS_ON:
> >>>>>>> +             tcpm_set_vconn(port, true);
> >>>>>>>               tcpm_set_vbus(port, true);
> >>>>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
> >>>>>>>               tcpm_set_attached_state(port, true);
> >>>>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> >>>>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> >>>>>>>               break;
> >>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> >>>>>>> +             /*
> >>>>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> >>>>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> >>>>>>> +              */
> >>>>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>>>>               break;
> >>>>>>>       case HARD_RESET_SEND:
> >>>>>>>               break;
> >>>>>>>
> >>>>>>
> >>
>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Guenter Roeck]

On 8/11/20 6:08 PM, Badhri Jagan Sridharan wrote:
> Yes I did ! https://lkml.org/lkml/2020/8/11/788 !
> 
> For me gmail is grouping V1 and V2 patches in the same thread so
> thought you saw the v2 patch :P
> 

You copied me on v1, but not on v2. I am not on any of the mailing lists
you sent v2 to, so I did not see it.

Guenter

> Thanks,
> Badhri
> 
> On Tue, Aug 11, 2020 at 5:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/11/20 4:21 PM, Badhri Jagan Sridharan wrote:
>>> Thanks Guenter ! However I don't see a reviewed-by tag :)
>>>
>>
>> Confused. Did you send a v2 with the changes we discussed ?
>> I didn't see that.
>>
>> Thanks,
>> Guenter
>>
>>> On Tue, Aug 11, 2020 at 1:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
>>>>> On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
>>>>>>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>>
>>>>>>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
>>>>>>>>> >From the spec:
>>>>>>>>> "7.1.5 Response to Hard Resets
>>>>>>>>> Hard Reset Signaling indicates a communication failure has occurred and
>>>>>>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
>>>>>>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
>>>>>>>>> May reset during a Hard Reset since the VBUS voltage will be less than
>>>>>>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
>>>>>>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
>>>>>>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
>>>>>>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
>>>>>>>>>
>>>>>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
>>>>>>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>>>>>>> index 3ef37202ee37..e41c4e5d3c71 100644
>>>>>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>>>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>>>>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
>>>>>>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
>>>>>>>>>               break;
>>>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>>>>>> -             tcpm_set_vconn(port, true);
>>>>>>>>> +             /*
>>>>>>>>> +              * 7.1.5 Response to Hard Resets
>>>>>>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
>>>>>>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
>>>>>>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
>>>>>>>>> +              */
>>>>>>>>> +             tcpm_set_vconn(port, false);
>>>>>>>>>               tcpm_set_vbus(port, false);
>>>>>>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
>>>>>>>>>                              tcpm_data_role_for_source(port));
>>>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>>>>>
>>>>>>>> I am a bit concerned about this. If I understand correctly, it means that
>>>>>>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
>>>>>>>> Is that correct ? What happens if that event is never received ?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Guenter
>>>>>>>
>>>>>>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
>>>>>>> Lower level tcpc driver would have to call tcpm_vbus_change which
>>>>>>> would in-turn trigger TCPM_VBUS_EVENT
>>>>>>> and queries port->tcpc->get_vbus to get the vbus status. It is not
>>>>>>> really a PD protocol driven event hence the
>>>>>>> confusion.
>>>>>>>
>>>>>>> "What happens if that event is never received ?"
>>>>>>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
>>>>>>> tcpm_vbus_change.
>>>>>>> Do you suspect that existing tcpc would not have the capability to
>>>>>>> monitor vbus status while sourcing and call tcpm_vbus_change?
>>>>>>>
>>>>>> That, or the driver might be buggy, or the hardware does't signal a status
>>>>>> update, or the update gets lost. I think we should have some backup,
>>>>>> to trigger if the event is not received in a reasonable amout of time.
>>>>>> I don't know if the specification has some kind of maximum limit. If
>>>>>> not, we should still have something
>>>>>>
>>>>>> Thanks,
>>>>>> Guenter
>>>>>
>>>>> Got it ! The specification actually has a bound for vbus off.
>>>>> tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
>>>>> So I will bound it to that.
>>>>>
>>>>
>>>> Excellent. Thanks a lot for looking into this!
>>>>
>>>> Guenter
>>>>
>>>>> >From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
>>>>> 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
>>>>> and then tells the Device Policy Manager to instruct the power supply
>>>>> to perform a Hard Reset. The transition to vSafe0V Shall occur within
>>>>> tSafe0V (t2).
>>>>> 5 After tSrcRecover the Source applies power to VBUS in an attempt to
>>>>> re-establish communication with the Sink and resume USB Default
>>>>> Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
>>>>> (t4).
>>>>>
>>>>> Thanks,
>>>>> Badhri
>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Badhri
>>>>>>>
>>>>>>>
>>>>>>>>>               break;
>>>>>>>>>       case SRC_HARD_RESET_VBUS_ON:
>>>>>>>>> +             tcpm_set_vconn(port, true);
>>>>>>>>>               tcpm_set_vbus(port, true);
>>>>>>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
>>>>>>>>>               tcpm_set_attached_state(port, true);
>>>>>>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
>>>>>>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
>>>>>>>>>               break;
>>>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
>>>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
>>>>>>>>> +             /*
>>>>>>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
>>>>>>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
>>>>>>>>> +              */
>>>>>>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
>>>>>>>>>               break;
>>>>>>>>>       case HARD_RESET_SEND:
>>>>>>>>>               break;
>>>>>>>>>
>>>>>>>>
>>>>
>>

Re: [PATCH v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures

[Badhri Jagan Sridharan]

On Tue, Aug 11, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/11/20 6:08 PM, Badhri Jagan Sridharan wrote:
> > Yes I did ! https://lkml.org/lkml/2020/8/11/788 !
> >
> > For me gmail is grouping V1 and V2 patches in the same thread so
> > thought you saw the v2 patch :P
> >
>
> You copied me on v1, but not on v2. I am not on any of the mailing lists
> you sent v2 to, so I did not see it.
>
> Guenter

My bad ! Apologies Guenter ! Screwed up while performing the git
send-email command.
Will make sure that I double check next time.

Thanks,
Badhri

>
> > Thanks,
> > Badhri
> >
> > On Tue, Aug 11, 2020 at 5:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/11/20 4:21 PM, Badhri Jagan Sridharan wrote:
> >>> Thanks Guenter ! However I don't see a reviewed-by tag :)
> >>>
> >>
> >> Confused. Did you send a v2 with the changes we discussed ?
> >> I didn't see that.
> >>
> >> Thanks,
> >> Guenter
> >>
> >>> On Tue, Aug 11, 2020 at 1:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On 8/11/20 12:39 PM, Badhri Jagan Sridharan wrote:
> >>>>> On Tue, Aug 11, 2020 at 11:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>>>
> >>>>>> On Tue, Aug 11, 2020 at 11:24:07AM -0700, Badhri Jagan Sridharan wrote:
> >>>>>>> On Mon, Aug 10, 2020 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>>>>>
> >>>>>>>> On 8/10/20 6:11 PM, Badhri Jagan Sridharan wrote:
> >>>>>>>>> >From the spec:
> >>>>>>>>> "7.1.5 Response to Hard Resets
> >>>>>>>>> Hard Reset Signaling indicates a communication failure has occurred and
> >>>>>>>>> the Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin
> >>>>>>>>> and Shall drive VBUS to vSafe0V as shown in Figure 7-9. The USB connection
> >>>>>>>>> May reset during a Hard Reset since the VBUS voltage will be less than
> >>>>>>>>> vSafe5V for an extended period of time. After establishing the vSafe0V
> >>>>>>>>> voltage condition on VBUS, the Source Shall wait tSrcRecover before
> >>>>>>>>> re-applying VCONN and restoring VBUS to vSafe5V. A Source Shall conform
> >>>>>>>>> to the VCONN timing as specified in [USB Type-C 1.3]."
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++++---
> >>>>>>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>>>> index 3ef37202ee37..e41c4e5d3c71 100644
> >>>>>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >>>>>>>>> @@ -3372,13 +3372,19 @@ static void run_state_machine(struct tcpm_port *port)
> >>>>>>>>>                       tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
> >>>>>>>>>               break;
> >>>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>>>>>> -             tcpm_set_vconn(port, true);
> >>>>>>>>> +             /*
> >>>>>>>>> +              * 7.1.5 Response to Hard Resets
> >>>>>>>>> +              * Hard Reset Signaling indicates a communication failure has occurred and the
> >>>>>>>>> +              * Source Shall stop driving VCONN, Shall remove Rp from the VCONN pin and Shall
> >>>>>>>>> +              * drive VBUS to vSafe0V as shown in Figure 7-9.
> >>>>>>>>> +              */
> >>>>>>>>> +             tcpm_set_vconn(port, false);
> >>>>>>>>>               tcpm_set_vbus(port, false);
> >>>>>>>>>               tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
> >>>>>>>>>                              tcpm_data_role_for_source(port));
> >>>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>>>>>
> >>>>>>>> I am a bit concerned about this. If I understand correctly, it means that
> >>>>>>>> we won't turn VBUS back on unless a SRC_HARD_RESET_VBUS_OFF PD event is received.
> >>>>>>>> Is that correct ? What happens if that event is never received ?
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Guenter
> >>>>>>>
> >>>>>>> The term PD event is a little ambiguous to me. Trying to summarize the workflow.
> >>>>>>> Lower level tcpc driver would have to call tcpm_vbus_change which
> >>>>>>> would in-turn trigger TCPM_VBUS_EVENT
> >>>>>>> and queries port->tcpc->get_vbus to get the vbus status. It is not
> >>>>>>> really a PD protocol driven event hence the
> >>>>>>> confusion.
> >>>>>>>
> >>>>>>> "What happens if that event is never received ?"
> >>>>>>> Yeah TCPM would be in SRC_HARD_RESET_VBUS_OFF till the tcpc calls the
> >>>>>>> tcpm_vbus_change.
> >>>>>>> Do you suspect that existing tcpc would not have the capability to
> >>>>>>> monitor vbus status while sourcing and call tcpm_vbus_change?
> >>>>>>>
> >>>>>> That, or the driver might be buggy, or the hardware does't signal a status
> >>>>>> update, or the update gets lost. I think we should have some backup,
> >>>>>> to trigger if the event is not received in a reasonable amout of time.
> >>>>>> I don't know if the specification has some kind of maximum limit. If
> >>>>>> not, we should still have something
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Guenter
> >>>>>
> >>>>> Got it ! The specification actually has a bound for vbus off.
> >>>>> tSafe0V  - Time to reach vSafe0V max - 650ms. (PD_T_SAFE_0V).
> >>>>> So I will bound it to that.
> >>>>>
> >>>>
> >>>> Excellent. Thanks a lot for looking into this!
> >>>>
> >>>> Guenter
> >>>>
> >>>>> >From Table 7-12 Sequence Description for a Source Initiated Hard Reset:
> >>>>> 4.Policy Engine waits tPSHardReset after sending Hard Reset Signaling
> >>>>> and then tells the Device Policy Manager to instruct the power supply
> >>>>> to perform a Hard Reset. The transition to vSafe0V Shall occur within
> >>>>> tSafe0V (t2).
> >>>>> 5 After tSrcRecover the Source applies power to VBUS in an attempt to
> >>>>> re-establish communication with the Sink and resume USB Default
> >>>>> Operation. The transition to vSafe5V Shall occur within tSrcTurnOn
> >>>>> (t4).
> >>>>>
> >>>>> Thanks,
> >>>>> Badhri
> >>>>>
> >>>>>>
> >>>>>>> Thanks,
> >>>>>>> Badhri
> >>>>>>>
> >>>>>>>
> >>>>>>>>>               break;
> >>>>>>>>>       case SRC_HARD_RESET_VBUS_ON:
> >>>>>>>>> +             tcpm_set_vconn(port, true);
> >>>>>>>>>               tcpm_set_vbus(port, true);
> >>>>>>>>>               port->tcpc->set_pd_rx(port->tcpc, true);
> >>>>>>>>>               tcpm_set_attached_state(port, true);
> >>>>>>>>> @@ -3944,7 +3950,11 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> >>>>>>>>>               tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> >>>>>>>>>               break;
> >>>>>>>>>       case SRC_HARD_RESET_VBUS_OFF:
> >>>>>>>>> -             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, 0);
> >>>>>>>>> +             /*
> >>>>>>>>> +              * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> >>>>>>>>> +              * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> >>>>>>>>> +              */
> >>>>>>>>> +             tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> >>>>>>>>>               break;
> >>>>>>>>>       case HARD_RESET_SEND:
> >>>>>>>>>               break;
> >>>>>>>>>
> >>>>>>>>
> >>>>
> >>
>