| Message ID | 20200811011126.130297-1-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: typec: tcpm: Fix TDA 2.2.1.1 and TDA 2.2.1.2 failures | expand |
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; >
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; > > >
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; > > > > >
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; > > > > > > >
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; >>>>> >>>>
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; > >>>>> > >>>> >
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; >>>>>>> >>>>>> >>
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; > >>>>>>> > >>>>>> > >> >
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; >>>>>>>>> >>>>>>>> >>>> >>
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; > >>>>>>>>> > >>>>>>>> > >>>> > >> >
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;
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(-)