| Message ID | 20220831001555.285081-1-badhri@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant | expand |
Hi Badhri, On Tue, Aug 30, 2022 at 05:15:53PM -0700, Badhri Jagan Sridharan wrote: > On some of the TCPC implementations, when the Type-C port is exposed > to contaminants, such as water, TCPC stops toggling while reporting OPEN > either by the time TCPM reads CC pin status or during CC debounce > window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made > to restart toggling, the behavior recurs causing redundant CPU wakeups > till the USB-C port is free of contaminant. > > [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > ... > > To mitigate redundant TCPM wakeups, TCPCs which do have the needed hardware > can implement the check_contaminant callback which is invoked by TCPM > to evaluate for presence of contaminant. Lower level TCPC driver can > restart toggling through TCPM_PORT_CLEAN event when the driver detects > that USB-C port is free of contaminant. check_contaminant callback also passes > the disconnect_while_debounce flag which when true denotes that the CC pins > transitioned to OPEN state during the CC debounce window. I'm a little bit concerned about the size of the state machine. I think this is a special case that at least in the beginning only the Maxim port controller can support, but it's still mixed into the "generic" state machine. How about if we just add "run_state_machine" callback for the port controller drivers so they can handle this kind of special cases on their own - they can then also add custom states? diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 904c7b4ce2f0c..91c22945ba258 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4858,9 +4858,11 @@ static void run_state_machine(struct tcpm_port *port) tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ? SRC_READY : SNK_READY, 0); break; default: - WARN(1, "Unexpected port state %d\n", port->state); break; } + + if (port->tcpc->run_state_machine) + port->tcpc->run_state_machine(port->tcpc); } static void tcpm_state_machine_work(struct kthread_work *work) thanks,
On 8/31/22 07:45, Heikki Krogerus wrote: > Hi Badhri, > > On Tue, Aug 30, 2022 at 05:15:53PM -0700, Badhri Jagan Sridharan wrote: >> On some of the TCPC implementations, when the Type-C port is exposed >> to contaminants, such as water, TCPC stops toggling while reporting OPEN >> either by the time TCPM reads CC pin status or during CC debounce >> window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made >> to restart toggling, the behavior recurs causing redundant CPU wakeups >> till the USB-C port is free of contaminant. >> >> [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] >> [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] >> [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] >> ... >> >> To mitigate redundant TCPM wakeups, TCPCs which do have the needed hardware >> can implement the check_contaminant callback which is invoked by TCPM >> to evaluate for presence of contaminant. Lower level TCPC driver can >> restart toggling through TCPM_PORT_CLEAN event when the driver detects >> that USB-C port is free of contaminant. check_contaminant callback also passes >> the disconnect_while_debounce flag which when true denotes that the CC pins >> transitioned to OPEN state during the CC debounce window. > > I'm a little bit concerned about the size of the state machine. I > think this is a special case that at least in the beginning only the > Maxim port controller can support, but it's still mixed into the > "generic" state machine. > > How about if we just add "run_state_machine" callback for the port > controller drivers so they can handle this kind of special cases on > their own - they can then also add custom states? > Same concern here. I would very much prefer an approach as suggested below, especially since the changes around the added disconnect_while_debounce flag are extensive and difficult to verify. Thanks, Guenter > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 904c7b4ce2f0c..91c22945ba258 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -4858,9 +4858,11 @@ static void run_state_machine(struct tcpm_port *port) > tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ? SRC_READY : SNK_READY, 0); > break; > default: > - WARN(1, "Unexpected port state %d\n", port->state); > break; > } > + > + if (port->tcpc->run_state_machine) > + port->tcpc->run_state_machine(port->tcpc); > } > > static void tcpm_state_machine_work(struct kthread_work *work) > > thanks, >
Thanks for taking the time out to review ! Circling back to address the feedback here in Version 3 of the patchset. I was able to move the logic around disconnect_while_debounce to the lower level maxim tcpc driver. However, I couldn't incorporate a callback as generic as run_state_machine. Instead I have added a callback, is_potential_contaminant, which calls the lower tcpc driver for every state transition in the TCPM state machine. This is somewhat similar to what Heikki had suggested but instead of calling it at the end, had to call the is_potential_contaminant at the beginning. is_potential_contaminant would allow the lower level tcpc driver to determine presence of potential contaminant. The CHECK_CONTAMINANT state in TCPM had to be retained as TCPM should not be reacting to changes in the system while the lower level tcpc driver is waiting for the port to be clean. Sending out the Version 3 of the patchset. Thanks, Badhri On Wed, Aug 31, 2022 at 10:51 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 8/31/22 07:45, Heikki Krogerus wrote: > > Hi Badhri, > > > > On Tue, Aug 30, 2022 at 05:15:53PM -0700, Badhri Jagan Sridharan wrote: > >> On some of the TCPC implementations, when the Type-C port is exposed > >> to contaminants, such as water, TCPC stops toggling while reporting OPEN > >> either by the time TCPM reads CC pin status or during CC debounce > >> window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made > >> to restart toggling, the behavior recurs causing redundant CPU wakeups > >> till the USB-C port is free of contaminant. > >> > >> [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > >> [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > >> [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] > >> ... > >> > >> To mitigate redundant TCPM wakeups, TCPCs which do have the needed hardware > >> can implement the check_contaminant callback which is invoked by TCPM > >> to evaluate for presence of contaminant. Lower level TCPC driver can > >> restart toggling through TCPM_PORT_CLEAN event when the driver detects > >> that USB-C port is free of contaminant. check_contaminant callback also passes > >> the disconnect_while_debounce flag which when true denotes that the CC pins > >> transitioned to OPEN state during the CC debounce window. > > > > I'm a little bit concerned about the size of the state machine. I > > think this is a special case that at least in the beginning only the > > Maxim port controller can support, but it's still mixed into the > > "generic" state machine. > > > > How about if we just add "run_state_machine" callback for the port > > controller drivers so they can handle this kind of special cases on > > their own - they can then also add custom states? > > > > Same concern here. I would very much prefer an approach as suggested below, > especially since the changes around the added disconnect_while_debounce flag > are extensive and difficult to verify. > > Thanks, > Guenter > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 904c7b4ce2f0c..91c22945ba258 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -4858,9 +4858,11 @@ static void run_state_machine(struct tcpm_port *port) > > tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ? SRC_READY : SNK_READY, 0); > > break; > > default: > > - WARN(1, "Unexpected port state %d\n", port->state); > > break; > > } > > + > > + if (port->tcpc->run_state_machine) > > + port->tcpc->run_state_machine(port->tcpc); > > } > > > > static void tcpm_state_machine_work(struct kthread_work *work) > > > > thanks, > > >
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index ea5a917c51b1..072c5a2817d0 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -36,6 +36,7 @@ #define FOREACH_STATE(S) \ S(INVALID_STATE), \ S(TOGGLING), \ + S(CHECK_CONTAMINANT), \ S(SRC_UNATTACHED), \ S(SRC_ATTACH_WAIT), \ S(SRC_ATTACHED), \ @@ -249,6 +250,7 @@ enum frs_typec_current { #define TCPM_RESET_EVENT BIT(2) #define TCPM_FRS_EVENT BIT(3) #define TCPM_SOURCING_VBUS BIT(4) +#define TCPM_PORT_CLEAN BIT(5) #define LOG_BUFFER_ENTRIES 1024 #define LOG_BUFFER_ENTRY_SIZE 128 @@ -483,6 +485,13 @@ struct tcpm_port { * SNK_READY for non-pd link. */ bool slow_charger_loop; + + /* + * When true indicates CC pins transitioned to OPEN state while + * debouncing(tCCDebounce). + */ + bool disconnect_while_debouncing; + #ifdef CONFIG_DEBUG_FS struct dentry *dentry; struct mutex logbuffer_lock; /* log buffer access lock */ @@ -3663,6 +3672,7 @@ static int tcpm_src_attach(struct tcpm_port *port) port->partner = NULL; port->attached = true; + port->disconnect_while_debouncing = false; port->send_discover = true; return 0; @@ -3797,6 +3807,7 @@ static int tcpm_snk_attach(struct tcpm_port *port) port->partner = NULL; port->attached = true; + port->disconnect_while_debouncing = false; port->send_discover = true; return 0; @@ -3824,6 +3835,7 @@ static int tcpm_acc_attach(struct tcpm_port *port) tcpm_typec_connect(port); port->attached = true; + port->disconnect_while_debouncing = false; return 0; } @@ -3908,11 +3920,22 @@ static void run_state_machine(struct tcpm_port *port) switch (port->state) { case TOGGLING: break; + case CHECK_CONTAMINANT: + port->tcpc->check_contaminant(port->tcpc, port->disconnect_while_debouncing); + port->disconnect_while_debouncing = false; + break; /* SRC states */ case SRC_UNATTACHED: if (!port->non_pd_role_swap) tcpm_swap_complete(port, -ENOTCONN); tcpm_src_detach(port); + if (port->disconnect_while_debouncing) { + /* Check for contaminant when port reports disconnected while debouncing */ + if (port->tcpc->check_contaminant) { + tcpm_set_state(port, CHECK_CONTAMINANT, 0); + break; + } + } if (tcpm_start_toggling(port, tcpm_rp_cc(port))) { tcpm_set_state(port, TOGGLING, 0); break; @@ -3922,6 +3945,7 @@ static void run_state_machine(struct tcpm_port *port) tcpm_set_state(port, SNK_UNATTACHED, PD_T_DRP_SNK); break; case SRC_ATTACH_WAIT: + port->disconnect_while_debouncing = true; if (tcpm_port_is_debug(port)) tcpm_set_state(port, DEBUG_ACC_ATTACHED, PD_T_CC_DEBOUNCE); @@ -3936,6 +3960,7 @@ static void run_state_machine(struct tcpm_port *port) break; case SNK_TRY: + port->disconnect_while_debouncing = false; port->try_snk_count++; /* * Requirements: @@ -4150,6 +4175,13 @@ static void run_state_machine(struct tcpm_port *port) tcpm_swap_complete(port, -ENOTCONN); tcpm_pps_complete(port, -ENOTCONN); tcpm_snk_detach(port); + if (port->disconnect_while_debouncing) { + port->disconnect_while_debouncing = false; + if (port->tcpc->check_contaminant) { + tcpm_set_state(port, CHECK_CONTAMINANT, 0); + break; + } + } if (tcpm_start_toggling(port, TYPEC_CC_RD)) { tcpm_set_state(port, TOGGLING, 0); break; @@ -4159,6 +4191,7 @@ static void run_state_machine(struct tcpm_port *port) tcpm_set_state(port, SRC_UNATTACHED, PD_T_DRP_SRC); break; case SNK_ATTACH_WAIT: + port->disconnect_while_debouncing = true; if ((port->cc1 == TYPEC_CC_OPEN && port->cc2 != TYPEC_CC_OPEN) || (port->cc1 != TYPEC_CC_OPEN && @@ -4170,14 +4203,16 @@ static void run_state_machine(struct tcpm_port *port) PD_T_PD_DEBOUNCE); break; case SNK_DEBOUNCED: - if (tcpm_port_is_disconnected(port)) + if (tcpm_port_is_disconnected(port)) { tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE); - else if (port->vbus_present) + } else if (port->vbus_present) { tcpm_set_state(port, tcpm_try_src(port) ? SRC_TRY : SNK_ATTACHED, 0); + port->disconnect_while_debouncing = false; + } break; case SRC_TRY: port->try_src_count++; @@ -4925,6 +4960,12 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1, tcpm_set_state(port, SRC_ATTACH_WAIT, 0); else if (tcpm_port_is_sink(port)) tcpm_set_state(port, SNK_ATTACH_WAIT, 0); + /* Check for contaminant when port reports disconnected while toggling */ + else if (tcpm_port_is_disconnected(port) && port->tcpc->check_contaminant) + tcpm_set_state(port, CHECK_CONTAMINANT, 0); + break; + case CHECK_CONTAMINANT: + /* Wait for Toggling to be resumed */ break; case SRC_UNATTACHED: case ACC_UNATTACHED: @@ -5225,6 +5266,7 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port) case SNK_ATTACH_WAIT: case SNK_DEBOUNCED: /* Do nothing, as TCPM is still waiting for vbus to reaach VSAFE5V to connect */ + port->disconnect_while_debouncing = false; break; case SNK_NEGOTIATE_CAPABILITIES: @@ -5425,6 +5467,10 @@ static void tcpm_pd_event_handler(struct kthread_work *work) port->vbus_source = true; _tcpm_pd_vbus_on(port); } + if (events & TCPM_PORT_CLEAN) { + tcpm_log(port, "port clean"); + tcpm_set_state(port, TOGGLING, 0); + } spin_lock(&port->pd_event_lock); } @@ -5477,6 +5523,15 @@ void tcpm_sourcing_vbus(struct tcpm_port *port) } EXPORT_SYMBOL_GPL(tcpm_sourcing_vbus); +void tcpm_port_clean(struct tcpm_port *port) +{ + spin_lock(&port->pd_event_lock); + port->pd_events |= TCPM_PORT_CLEAN; + spin_unlock(&port->pd_event_lock); + kthread_queue_work(port->wq, &port->event_work); +} +EXPORT_SYMBOL_GPL(tcpm_port_clean); + static void tcpm_enable_frs_work(struct kthread_work *work) { struct tcpm_port *port = container_of(work, struct tcpm_port, enable_frs); diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index bffc8d3e14ad..436563d91a49 100644 --- a/include/linux/usb/tcpm.h +++ b/include/linux/usb/tcpm.h @@ -114,6 +114,11 @@ enum tcpm_transmit_type { * Optional; The USB Communications Capable bit indicates if port * partner is capable of communication over the USB data lines * (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit. + * @check_contaminant: + * Optional; The callback is called when CC pins report open status + * at the end of the deboumce period or when the port is still + * toggling. Chip level drivers are expected to check for contaminant + * and call tcpm_clean_port when the port is clean. */ struct tcpc_dev { struct fwnode_handle *fwnode; @@ -148,6 +153,7 @@ struct tcpc_dev { bool pps_active, u32 requested_vbus_voltage); bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev); void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable); + void (*check_contaminant)(struct tcpc_dev *dev, bool disconnect_while_debouncing); }; struct tcpm_port; @@ -165,5 +171,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port, enum tcpm_transmit_status status); void tcpm_pd_hard_reset(struct tcpm_port *port); void tcpm_tcpc_reset(struct tcpm_port *port); +void tcpm_port_clean(struct tcpm_port *port); #endif /* __LINUX_USB_TCPM_H */
On some of the TCPC implementations, when the Type-C port is exposed to contaminants, such as water, TCPC stops toggling while reporting OPEN either by the time TCPM reads CC pin status or during CC debounce window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made to restart toggling, the behavior recurs causing redundant CPU wakeups till the USB-C port is free of contaminant. [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected] ... To mitigate redundant TCPM wakeups, TCPCs which do have the needed hardware can implement the check_contaminant callback which is invoked by TCPM to evaluate for presence of contaminant. Lower level TCPC driver can restart toggling through TCPM_PORT_CLEAN event when the driver detects that USB-C port is free of contaminant. check_contaminant callback also passes the disconnect_while_debounce flag which when true denotes that the CC pins transitioned to OPEN state during the CC debounce window. Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- drivers/usb/typec/tcpm/tcpm.c | 59 +++++++++++++++++++++++++++++++++-- include/linux/usb/tcpm.h | 7 +++++ 2 files changed, 64 insertions(+), 2 deletions(-)