| Message ID | 20180913021113.18150-1-badhri@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] typec: tcpm: Do not disconnect link for self powered devices | expand |
+Guenter On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote: > During HARD_RESET the data link is disconnected. > For self powered device, the spec is advising against doing that. > > >From USB_PD_R3_0 > 7.1.5 Response to Hard Resets > Device operation during and after a Hard Reset is defined as follows: > Self-powered devices Should Not disconnect from USB during a Hard Reset > (see Section 9.1.2). > Bus powered devices will disconnect from USB during a Hard Reset due to the > loss of their power source. > > Tackle this by letting TCPM know whether the device is self or bus powered. > > This overcomes unnecessary port disconnections from hard reset. > Also, speeds up the enumeration time when connected to Type-A ports. > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> > --- > drivers/usb/typec/tcpm.c | 6 +++++- > include/linux/usb/tcpm.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 4f1f4215f3d6..a4e0c027a2a9 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port) > memset(&port->pps_data, 0, sizeof(port->pps_data)); > tcpm_set_vconn(port, false); > tcpm_set_charge(port, false); > - tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > + > + if (port->tcpc->config->self_powered) Add a member for that flag to the struct tcpm_port, and check that here instead. I'll explain why below. > + tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); > + else > + tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > > /* > * VBUS may or may not toggle, depending on the adapter. > * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > index 7e7fbfb84e8e..50c74a77db55 100644 > --- a/include/linux/usb/tcpm.h > +++ b/include/linux/usb/tcpm.h > @@ -89,6 +89,7 @@ struct tcpc_config { > enum typec_port_data data; > enum typec_role default_role; > bool try_role_hw; /* try.{src,snk} implemented in hardware */ > + bool self_powered; /* port belongs to a self powered device */ I'm not sure we should add any more members to that structure, but maybe it's not a problem for now. We can't quite yet get rid of that structure. You do need to introduce a new device property already. Then read the value for that new member you added to struct tcpm_port in tcpm_fw_get_caps() and in tcpm_copy_caps(). Thanks,
On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote: > +Guenter > > On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote: > > During HARD_RESET the data link is disconnected. > > For self powered device, the spec is advising against doing that. > > > > >From USB_PD_R3_0 > > 7.1.5 Response to Hard Resets > > Device operation during and after a Hard Reset is defined as follows: > > Self-powered devices Should Not disconnect from USB during a Hard Reset > > (see Section 9.1.2). > > Bus powered devices will disconnect from USB during a Hard Reset due to the > > loss of their power source. > > > > Tackle this by letting TCPM know whether the device is self or bus powered. > > > > This overcomes unnecessary port disconnections from hard reset. > > Also, speeds up the enumeration time when connected to Type-A ports. > > > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> > > --- > > drivers/usb/typec/tcpm.c | 6 +++++- > > include/linux/usb/tcpm.h | 1 + > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > > index 4f1f4215f3d6..a4e0c027a2a9 100644 > > --- a/drivers/usb/typec/tcpm.c > > +++ b/drivers/usb/typec/tcpm.c > > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port) > > memset(&port->pps_data, 0, sizeof(port->pps_data)); > > tcpm_set_vconn(port, false); > > tcpm_set_charge(port, false); > > - tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > > + > > + if (port->tcpc->config->self_powered) > > Add a member for that flag to the struct tcpm_port, and check that > here instead. I'll explain why below. > > > + tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); > > + else > > + tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > > Maybe I am missing something, but tcpm_set_roles(port, port->self_powered, TYPEC_SINK, TYPEC_DEVICE); should accomplish the same (assuming the flag is moved to tcpm_port). Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed, or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only apply if the port is a sink, or also if is is source ? Guenter > > /* > > * VBUS may or may not toggle, depending on the adapter. > > * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > > index 7e7fbfb84e8e..50c74a77db55 100644 > > --- a/include/linux/usb/tcpm.h > > +++ b/include/linux/usb/tcpm.h > > @@ -89,6 +89,7 @@ struct tcpc_config { > > enum typec_port_data data; > > enum typec_role default_role; > > bool try_role_hw; /* try.{src,snk} implemented in hardware */ > > + bool self_powered; /* port belongs to a self powered device */ > > I'm not sure we should add any more members to that structure, but > maybe it's not a problem for now. We can't quite yet get rid of that > structure. > > You do need to introduce a new device property already. Then read the > value for that new member you added to struct tcpm_port in > tcpm_fw_get_caps() and in tcpm_copy_caps(). > > > Thanks, > > -- > heikki
On Thu, Sep 13, 2018 at 10:51 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote: > > +Guenter > > > > On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote: > > > During HARD_RESET the data link is disconnected. > > > For self powered device, the spec is advising against doing that. > > > > > > >From USB_PD_R3_0 > > > 7.1.5 Response to Hard Resets > > > Device operation during and after a Hard Reset is defined as follows: > > > Self-powered devices Should Not disconnect from USB during a Hard Reset > > > (see Section 9.1.2). > > > Bus powered devices will disconnect from USB during a Hard Reset due to the > > > loss of their power source. > > > > > > Tackle this by letting TCPM know whether the device is self or bus powered. > > > > > > This overcomes unnecessary port disconnections from hard reset. > > > Also, speeds up the enumeration time when connected to Type-A ports. > > > > > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> > > > --- > > > drivers/usb/typec/tcpm.c | 6 +++++- > > > include/linux/usb/tcpm.h | 1 + > > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > > > index 4f1f4215f3d6..a4e0c027a2a9 100644 > > > --- a/drivers/usb/typec/tcpm.c > > > +++ b/drivers/usb/typec/tcpm.c > > > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port) > > > memset(&port->pps_data, 0, sizeof(port->pps_data)); > > > tcpm_set_vconn(port, false); > > > tcpm_set_charge(port, false); > > > - tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > > > + > > > + if (port->tcpc->config->self_powered) > > > > Add a member for that flag to the struct tcpm_port, and check that > > here instead. I'll explain why below. > > > > > + tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); > > > + else > > > + tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); > > > > > Maybe I am missing something, but > tcpm_set_roles(port, port->self_powered, > TYPEC_SINK, TYPEC_DEVICE); > should accomplish the same (assuming the flag is moved to tcpm_port). Yeah Makes sense to me.. > > Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed, > or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only > apply if the port is a sink, or also if is is source ? Good question ! I dont see any direct guidance on the spec. All that I can see is the following: From 2.6.1 Source Operation: Restores the Port’s data role to DFP. From 7.1.5 Response to Hard Resets: "The Source Shall meet both tSafe5V and tSafe0V relative to the start of the voltage transition as shown in Figure 7-8." I am leaning towards to changing the link state from false to true for SRC_HARD_RESET_VBUS_OFF as well. This is to keep it symmetrical. What do you think ? > > Guenter > > > > /* > > > * VBUS may or may not toggle, depending on the adapter. > > > * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON > > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > > > index 7e7fbfb84e8e..50c74a77db55 100644 > > > --- a/include/linux/usb/tcpm.h > > > +++ b/include/linux/usb/tcpm.h > > > @@ -89,6 +89,7 @@ struct tcpc_config { > > > enum typec_port_data data; > > > enum typec_role default_role; > > > bool try_role_hw; /* try.{src,snk} implemented in hardware */ > > > + bool self_powered; /* port belongs to a self powered device */ > > > > I'm not sure we should add any more members to that structure, but > > maybe it's not a problem for now. We can't quite yet get rid of that > > structure. > > > > You do need to introduce a new device property already. Then read the This would be part of usb-connector binding. right ? > > value for that new member you added to struct tcpm_port in > > tcpm_fw_get_caps() and in tcpm_copy_caps(). > > > > > > Thanks, > > > > -- > > heikki
Hi, On Thu, Sep 13, 2018 at 04:16:08PM -0700, Badhri Jagan Sridharan wrote: > > > You do need to introduce a new device property already. Then read the > > This would be part of usb-connector binding. right ? Yes, that is probable the correct place for that. It's not tcpm/tcpci specific. But I don't think it's Type-C specific either. Perhaps it should go under "Optional Properties" instead of "Optional properties for usb-c-connector"? Thanks,
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index 4f1f4215f3d6..a4e0c027a2a9 100644 --- a/drivers/usb/typec/tcpm.c +++ b/drivers/usb/typec/tcpm.c @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port) memset(&port->pps_data, 0, sizeof(port->pps_data)); tcpm_set_vconn(port, false); tcpm_set_charge(port, false); - tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); + + if (port->tcpc->config->self_powered) + tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); + else + tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE); /* * VBUS may or may not toggle, depending on the adapter. * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index 7e7fbfb84e8e..50c74a77db55 100644 --- a/include/linux/usb/tcpm.h +++ b/include/linux/usb/tcpm.h @@ -89,6 +89,7 @@ struct tcpc_config { enum typec_port_data data; enum typec_role default_role; bool try_role_hw; /* try.{src,snk} implemented in hardware */ + bool self_powered; /* port belongs to a self powered device */ const struct typec_altmode_desc *alt_modes; };
During HARD_RESET the data link is disconnected. For self powered device, the spec is advising against doing that. From USB_PD_R3_0 7.1.5 Response to Hard Resets Device operation during and after a Hard Reset is defined as follows: Self-powered devices Should Not disconnect from USB during a Hard Reset (see Section 9.1.2). Bus powered devices will disconnect from USB during a Hard Reset due to the loss of their power source. Tackle this by letting TCPM know whether the device is self or bus powered. This overcomes unnecessary port disconnections from hard reset. Also, speeds up the enumeration time when connected to Type-A ports. Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> --- drivers/usb/typec/tcpm.c | 6 +++++- include/linux/usb/tcpm.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-)