Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant

From: Badhri Jagan Sridharan
Date: Sat Dec 10 2022 - 01:16:42 EST

Next message: Badhri Jagan Sridharan: "[PATCH v9 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant"
Previous message: Badhri Jagan Sridharan: "Re: [PATCH v7 2/3] usb: typec: tcpci: Add callback for evaluating contaminant presence"
In reply to: Badhri Jagan Sridharan: "Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Version 9 is the latest patchset.
Disregard earlier versions as I messed up versioning the stack.

Thanks,
Badhri

On Fri, Dec 9, 2022 at 10:06 PM Badhri Jagan Sridharan
<badhri@xxxxxxxxxx> wrote:
>
> path
>
> On Fri, Dec 9, 2022 at 5:44 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > On 12/8/22 18:39, Badhri Jagan Sridharan wrote:
> > > On Thu, Dec 8, 2022 at 3:56 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > >>
> > >> On 12/8/22 07:04, 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]
> > >>> ...
> > >>>
> > >>> TCPM invokes is_potential_contaminant callback to allow low level chip
> > >>> drivers to monitor TCPM state machine transitions and notify TCPM when
> > >>> the Type-C port needs to be checked for potential contaminant presence.
> > >>> 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.
> > >>>
> > >>> Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> > >>> ---
> > >>> Changes since v5:
> > >>> * Updated commit message. Removed change id.
> > >>> Changes since v4:
> > >>> * None
> > >>> Changes since v3:
> > >>> * None
> > >>> Changes since V2:
> > >>> * Offloaded tcpm from maintaining disconnect_while_debouncing logic
> > >>> * to lower level maxim tcpc driver based on feedback.
> > >>> ---
> > >>> drivers/usb/typec/tcpm/tcpm.c | 162 +++++++++-------------------------
> > >>> include/linux/usb/tcpm.h | 133 ++++++++++++++++++++++++++++
> > >>> 2 files changed, 177 insertions(+), 118 deletions(-)
> > >>>
> > >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > >>> index 904c7b4ce2f0..a138cea49612 100644
> > >>> --- a/drivers/usb/typec/tcpm/tcpm.c
> > >>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> > >>> @@ -33,119 +33,6 @@
> > >>>
> > >>> #include <uapi/linux/sched/types.h>
> > >>>
> > >>> -#define FOREACH_STATE(S) \
> > >>> - S(INVALID_STATE), \
> > >>> - S(TOGGLING), \
> > >>> - S(SRC_UNATTACHED), \
> > >>> - S(SRC_ATTACH_WAIT), \
> > >>> - S(SRC_ATTACHED), \
> > >>> - S(SRC_STARTUP), \
> > >>> - S(SRC_SEND_CAPABILITIES), \
> > >>> - S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> > >>> - S(SRC_NEGOTIATE_CAPABILITIES), \
> > >>> - S(SRC_TRANSITION_SUPPLY), \
> > >>> - S(SRC_READY), \
> > >>> - S(SRC_WAIT_NEW_CAPABILITIES), \
> > >>> - \
> > >>> - S(SNK_UNATTACHED), \
> > >>> - S(SNK_ATTACH_WAIT), \
> > >>> - S(SNK_DEBOUNCED), \
> > >>> - S(SNK_ATTACHED), \
> > >>> - S(SNK_STARTUP), \
> > >>> - S(SNK_DISCOVERY), \
> > >>> - S(SNK_DISCOVERY_DEBOUNCE), \
> > >>> - S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> > >>> - S(SNK_WAIT_CAPABILITIES), \
> > >>> - S(SNK_NEGOTIATE_CAPABILITIES), \
> > >>> - S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> > >>> - S(SNK_TRANSITION_SINK), \
> > >>> - S(SNK_TRANSITION_SINK_VBUS), \
> > >>> - S(SNK_READY), \
> > >>> - \
> > >>> - S(ACC_UNATTACHED), \
> > >>> - S(DEBUG_ACC_ATTACHED), \
> > >>> - S(AUDIO_ACC_ATTACHED), \
> > >>> - S(AUDIO_ACC_DEBOUNCE), \
> > >>> - \
> > >>> - S(HARD_RESET_SEND), \
> > >>> - S(HARD_RESET_START), \
> > >>> - S(SRC_HARD_RESET_VBUS_OFF), \
> > >>> - S(SRC_HARD_RESET_VBUS_ON), \
> > >>> - S(SNK_HARD_RESET_SINK_OFF), \
> > >>> - S(SNK_HARD_RESET_WAIT_VBUS), \
> > >>> - S(SNK_HARD_RESET_SINK_ON), \
> > >>> - \
> > >>> - S(SOFT_RESET), \
> > >>> - S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> > >>> - S(SNK_SOFT_RESET), \
> > >>> - S(SOFT_RESET_SEND), \
> > >>> - \
> > >>> - S(DR_SWAP_ACCEPT), \
> > >>> - S(DR_SWAP_SEND), \
> > >>> - S(DR_SWAP_SEND_TIMEOUT), \
> > >>> - S(DR_SWAP_CANCEL), \
> > >>> - S(DR_SWAP_CHANGE_DR), \
> > >>> - \
> > >>> - S(PR_SWAP_ACCEPT), \
> > >>> - S(PR_SWAP_SEND), \
> > >>> - S(PR_SWAP_SEND_TIMEOUT), \
> > >>> - S(PR_SWAP_CANCEL), \
> > >>> - S(PR_SWAP_START), \
> > >>> - S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> > >>> - S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> > >>> - S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> > >>> - S(PR_SWAP_SRC_SNK_SINK_ON), \
> > >>> - S(PR_SWAP_SNK_SRC_SINK_OFF), \
> > >>> - S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> > >>> - S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> > >>> - \
> > >>> - S(VCONN_SWAP_ACCEPT), \
> > >>> - S(VCONN_SWAP_SEND), \
> > >>> - S(VCONN_SWAP_SEND_TIMEOUT), \
> > >>> - S(VCONN_SWAP_CANCEL), \
> > >>> - S(VCONN_SWAP_START), \
> > >>> - S(VCONN_SWAP_WAIT_FOR_VCONN), \
> > >>> - S(VCONN_SWAP_TURN_ON_VCONN), \
> > >>> - S(VCONN_SWAP_TURN_OFF_VCONN), \
> > >>> - \
> > >>> - S(FR_SWAP_SEND), \
> > >>> - S(FR_SWAP_SEND_TIMEOUT), \
> > >>> - S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> > >>> - S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> > >>> - S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> > >>> - S(FR_SWAP_CANCEL), \
> > >>> - \
> > >>> - S(SNK_TRY), \
> > >>> - S(SNK_TRY_WAIT), \
> > >>> - S(SNK_TRY_WAIT_DEBOUNCE), \
> > >>> - S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> > >>> - S(SRC_TRYWAIT), \
> > >>> - S(SRC_TRYWAIT_DEBOUNCE), \
> > >>> - S(SRC_TRYWAIT_UNATTACHED), \
> > >>> - \
> > >>> - S(SRC_TRY), \
> > >>> - S(SRC_TRY_WAIT), \
> > >>> - S(SRC_TRY_DEBOUNCE), \
> > >>> - S(SNK_TRYWAIT), \
> > >>> - S(SNK_TRYWAIT_DEBOUNCE), \
> > >>> - S(SNK_TRYWAIT_VBUS), \
> > >>> - S(BIST_RX), \
> > >>> - \
> > >>> - S(GET_STATUS_SEND), \
> > >>> - S(GET_STATUS_SEND_TIMEOUT), \
> > >>> - S(GET_PPS_STATUS_SEND), \
> > >>> - S(GET_PPS_STATUS_SEND_TIMEOUT), \
> > >>> - \
> > >>> - S(GET_SINK_CAP), \
> > >>> - S(GET_SINK_CAP_TIMEOUT), \
> > >>> - \
> > >>> - S(ERROR_RECOVERY), \
> > >>> - S(PORT_RESET), \
> > >>> - S(PORT_RESET_WAIT_OFF), \
> > >>> - \
> > >>> - S(AMS_START), \
> > >>> - S(CHUNK_NOT_SUPP)
> > >>> -
> > >>> #define FOREACH_AMS(S) \
> > >>> S(NONE_AMS), \
> > >>> S(POWER_NEGOTIATION), \
> > >>> @@ -182,13 +69,8 @@
> > >>> S(COUNTRY_INFO), \
> > >>> S(COUNTRY_CODES)
> > >>>
> > >>> -#define GENERATE_ENUM(e) e
> > >>> #define GENERATE_STRING(s) #s
> > >>>
> > >>> -enum tcpm_state {
> > >>> - FOREACH_STATE(GENERATE_ENUM)
> > >>> -};
> > >>> -
> > >>> static const char * const tcpm_states[] = {
> > >>> FOREACH_STATE(GENERATE_STRING)
> > >>> };
> > >>> @@ -249,6 +131,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 +366,14 @@ struct tcpm_port {
> > >>> * SNK_READY for non-pd link.
> > >>> */
> > >>> bool slow_charger_loop;
> > >>> +
> > >>> + /*
> > >>> + * When true indicates that the lower level drivers indicate potential presence
> > >>> + * of contaminant in the connector pins based on the tcpm state machine
> > >>> + * transitions.
> > >>> + */
> > >>> + bool potential_contaminant;
> > >>> +
> > >>> #ifdef CONFIG_DEBUG_FS
> > >>> struct dentry *dentry;
> > >>> struct mutex logbuffer_lock; /* log buffer access lock */
> > >>> @@ -3904,15 +3795,26 @@ static void run_state_machine(struct tcpm_port *port)
> > >>> unsigned int msecs;
> > >>> enum tcpm_state upcoming_state;
> > >>>
> > >>> + if (port->tcpc->is_potential_contaminant)
> > >>> + port->potential_contaminant =
> > >>> + port->tcpc->is_potential_contaminant(port->tcpc, port->state);
> > >>> +
> > >>> port->enter_state = port->state;
> > >>> switch (port->state) {
> > >>> case TOGGLING:
> > >>> break;
> > >>> + case CHECK_CONTAMINANT:
> > >>> + port->tcpc->check_contaminant(port->tcpc);
> > >>> + break;
> > >>> /* SRC states */
> > >>> case SRC_UNATTACHED:
> > >>> if (!port->non_pd_role_swap)
> > >>> tcpm_swap_complete(port, -ENOTCONN);
> > >>> tcpm_src_detach(port);
> > >>> + if (port->potential_contaminant && 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;
> > >>> @@ -4150,6 +4052,10 @@ 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->potential_contaminant && 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;
> > >>> @@ -4926,6 +4832,9 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
> > >>> else if (tcpm_port_is_sink(port))
> > >>> tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
> > >>> break;
> > >>> + case CHECK_CONTAMINANT:
> > >>> + /* Wait for Toggling to be resumed */
> > >>> + break;
> > >>> case SRC_UNATTACHED:
> > >>> case ACC_UNATTACHED:
> > >>> if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
> > >>> @@ -5425,6 +5334,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);
> > >>> + }
> > >>
> > >> That sets the state to TOGGLING unconditionally, even if it not currently
> > >> CHECK_CONTAMINANT. Is that a potential problem ?
> > >
> > > Good catch ! Would make sense to check whether tcpm is in
> > > CHECK_CONTAMINANT state before setting the TOGGLING state.
> > >>
> > >>>
> > >>> spin_lock(&port->pd_event_lock);
> > >>> }
> > >>> @@ -5477,6 +5390,19 @@ void tcpm_sourcing_vbus(struct tcpm_port *port)
> > >>> }
> > >>> EXPORT_SYMBOL_GPL(tcpm_sourcing_vbus);
> > >>>
> > >>> +/*
> > >>> + * Low level tcpc drivers invoke this once the port is deemed clean to return
> > >>> + * the port to TOGGLING state.
> > >>> + */
> > >>> +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..9cf16372a6e4 100644
> > >>> --- a/include/linux/usb/tcpm.h
> > >>> +++ b/include/linux/usb/tcpm.h
> > >>> @@ -10,6 +10,126 @@
> > >>> #include <linux/usb/typec.h>
> > >>> #include "pd.h"
> > >>>
> > >>> +#define FOREACH_STATE(S) \
> > >>> + S(INVALID_STATE), \
> > >>> + S(TOGGLING), \
> > >>> + S(CHECK_CONTAMINANT), \
> > >>> + S(SRC_UNATTACHED), \
> > >>> + S(SRC_ATTACH_WAIT), \
> > >>> + S(SRC_ATTACHED), \
> > >>> + S(SRC_STARTUP), \
> > >>> + S(SRC_SEND_CAPABILITIES), \
> > >>> + S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> > >>> + S(SRC_NEGOTIATE_CAPABILITIES), \
> > >>> + S(SRC_TRANSITION_SUPPLY), \
> > >>> + S(SRC_READY), \
> > >>> + S(SRC_WAIT_NEW_CAPABILITIES), \
> > >>> + \
> > >>> + S(SNK_UNATTACHED), \
> > >>> + S(SNK_ATTACH_WAIT), \
> > >>> + S(SNK_DEBOUNCED), \
> > >>> + S(SNK_ATTACHED), \
> > >>> + S(SNK_STARTUP), \
> > >>> + S(SNK_DISCOVERY), \
> > >>> + S(SNK_DISCOVERY_DEBOUNCE), \
> > >>> + S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> > >>> + S(SNK_WAIT_CAPABILITIES), \
> > >>> + S(SNK_NEGOTIATE_CAPABILITIES), \
> > >>> + S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> > >>> + S(SNK_TRANSITION_SINK), \
> > >>> + S(SNK_TRANSITION_SINK_VBUS), \
> > >>> + S(SNK_READY), \
> > >>> + \
> > >>> + S(ACC_UNATTACHED), \
> > >>> + S(DEBUG_ACC_ATTACHED), \
> > >>> + S(AUDIO_ACC_ATTACHED), \
> > >>> + S(AUDIO_ACC_DEBOUNCE), \
> > >>> + \
> > >>> + S(HARD_RESET_SEND), \
> > >>> + S(HARD_RESET_START), \
> > >>> + S(SRC_HARD_RESET_VBUS_OFF), \
> > >>> + S(SRC_HARD_RESET_VBUS_ON), \
> > >>> + S(SNK_HARD_RESET_SINK_OFF), \
> > >>> + S(SNK_HARD_RESET_WAIT_VBUS), \
> > >>> + S(SNK_HARD_RESET_SINK_ON), \
> > >>> + \
> > >>> + S(SOFT_RESET), \
> > >>> + S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> > >>> + S(SNK_SOFT_RESET), \
> > >>> + S(SOFT_RESET_SEND), \
> > >>> + \
> > >>> + S(DR_SWAP_ACCEPT), \
> > >>> + S(DR_SWAP_SEND), \
> > >>> + S(DR_SWAP_SEND_TIMEOUT), \
> > >>> + S(DR_SWAP_CANCEL), \
> > >>> + S(DR_SWAP_CHANGE_DR), \
> > >>> + \
> > >>> + S(PR_SWAP_ACCEPT), \
> > >>> + S(PR_SWAP_SEND), \
> > >>> + S(PR_SWAP_SEND_TIMEOUT), \
> > >>> + S(PR_SWAP_CANCEL), \
> > >>> + S(PR_SWAP_START), \
> > >>> + S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> > >>> + S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> > >>> + S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> > >>> + S(PR_SWAP_SRC_SNK_SINK_ON), \
> > >>> + S(PR_SWAP_SNK_SRC_SINK_OFF), \
> > >>> + S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> > >>> + S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> > >>> + \
> > >>> + S(VCONN_SWAP_ACCEPT), \
> > >>> + S(VCONN_SWAP_SEND), \
> > >>> + S(VCONN_SWAP_SEND_TIMEOUT), \
> > >>> + S(VCONN_SWAP_CANCEL), \
> > >>> + S(VCONN_SWAP_START), \
> > >>> + S(VCONN_SWAP_WAIT_FOR_VCONN), \
> > >>> + S(VCONN_SWAP_TURN_ON_VCONN), \
> > >>> + S(VCONN_SWAP_TURN_OFF_VCONN), \
> > >>> + \
> > >>> + S(FR_SWAP_SEND), \
> > >>> + S(FR_SWAP_SEND_TIMEOUT), \
> > >>> + S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> > >>> + S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> > >>> + S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> > >>> + S(FR_SWAP_CANCEL), \
> > >>> + \
> > >>> + S(SNK_TRY), \
> > >>> + S(SNK_TRY_WAIT), \
> > >>> + S(SNK_TRY_WAIT_DEBOUNCE), \
> > >>> + S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> > >>> + S(SRC_TRYWAIT), \
> > >>> + S(SRC_TRYWAIT_DEBOUNCE), \
> > >>> + S(SRC_TRYWAIT_UNATTACHED), \
> > >>> + \
> > >>> + S(SRC_TRY), \
> > >>> + S(SRC_TRY_WAIT), \
> > >>> + S(SRC_TRY_DEBOUNCE), \
> > >>> + S(SNK_TRYWAIT), \
> > >>> + S(SNK_TRYWAIT_DEBOUNCE), \
> > >>> + S(SNK_TRYWAIT_VBUS), \
> > >>> + S(BIST_RX), \
> > >>> + \
> > >>> + S(GET_STATUS_SEND), \
> > >>> + S(GET_STATUS_SEND_TIMEOUT), \
> > >>> + S(GET_PPS_STATUS_SEND), \
> > >>> + S(GET_PPS_STATUS_SEND_TIMEOUT), \
> > >>> + \
> > >>> + S(GET_SINK_CAP), \
> > >>> + S(GET_SINK_CAP_TIMEOUT), \
> > >>> + \
> > >>> + S(ERROR_RECOVERY), \
> > >>> + S(PORT_RESET), \
> > >>> + S(PORT_RESET_WAIT_OFF), \
> > >>> + \
> > >>> + S(AMS_START), \
> > >>> + S(CHUNK_NOT_SUPP)
> > >>> +
> > >>> +#define GENERATE_ENUM(e) e
> > >>> +
> > >>> +enum tcpm_state {
> > >>> + FOREACH_STATE(GENERATE_ENUM)
> > >>> +};
> > >>> +
> > >>
> > >> Sorry for not bringing it up earlier; I have been struggling with it all along,
> > >> and I could not decide if I should bring it up or not.
> > >>
> > >> I really don't feel comfortable with exporting states outside tcpm.
> > >> Is this really necessary ? The only use seems to be
> > >>
> > >> + if ((tcpm_prev_state == SRC_ATTACH_WAIT && current_state == SRC_UNATTACHED) ||
> > >> + (tcpm_prev_state == SNK_ATTACH_WAIT && current_state == SNK_UNATTACHED))
> > >>
> > >> Plus, of course, the CHECK_CONTAMINANT state.
> > >>
> > >> Is there reason to believe that the relevant state transitions would be different
> > >> for other chips ? If not, would it possibly make sense to move this check
> > >> into the state machine ?
> > >
> > > In my understanding, this is definitely one of the transitions that I
> > > think will happen when the port has contaminant. It's quite possible
> > > that there are transitions that I am not aware of yet.
> > > I am OK moving it back to the tcpm state machine, however, then tcpm
> > > would have to keep track of the tcpm state machine transitions and
> > > that would look like V2 version of the patch
> > > (https://lore.kernel.org/lkml/20220831001555.285081-1-badhri@xxxxxxxxxx/)
> > > where disconnect_while_debouncing is used to track this. Would that be
> > > OK ?
> > >
> >
> > Kind of, except that I think it was way too invasive. The current code checks for
> > contaminant once when entering the state machine. If you pull that code into the
> > state machine, I don't see why it would have to be more than a single call.
> > One possibility might be to move the call into tcpm_set_state() to catch all
> > state transitions, or to use the existing enter_state variable for that
> > purpose.
>
> enter_state suited better ! tcpm_set_state's delayed state was making
> it tricky..
> So chose the enter_state path and have updated series (Version 8).
> I also added a small helper function to check whether the state machine
> is in toggling state.
>
> >
> > >>
> > >>> enum typec_cc_status {
> > >>> TYPEC_CC_OPEN,
> > >>> TYPEC_CC_RA,
> > >>> @@ -114,6 +234,16 @@ 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 invoked when chiplevel drivers indicated
> > >>> + * that the USB port needs to be checked for contaminant presence.
> > >>> + * Chip level drivers are expected to check for contaminant and call
> > >>> + * tcpm_clean_port when the port is clean to put the port back into
> > >>> + * toggling state.
> > >>> + * @is_potential_contaminant:
> > >>> + * Optional; TCPM invokes the callback for every TCPM state machine
> > >>> + * transition. Chiplevel drivers can monitor the state machine
> > >>> + * transitions to flag for potential contaminant presence.
> > >>
> > >> I kind of dislike the repeated checks for those callbacks in the state machine.
> > >> Does it make any sense to have one of those callbacks but not the other ?
> > >>
> > >> Because if not it might make sense to check in the registration function
> > >> if either both are NULL or both are set, and then drop most of the checks
> > >> in the state machine handler.
> > >
> > > I can definitely check for the registration of the callbacks but cant
> > > drop one of them unless we are OK to make tcpm transitions and invoke
> > > the check_contaminant callback directly.
> > >
> >
> > I didn't necessarily mean to drop one of the callbacks (unless one of them is
> > pulled into the tcpm driver). I envisioned a check such as
> > if ((!tcpc->check_contaminant && tcpc->is_potential_contaminant) ||
> > ((tcpc->check_contaminant && !tcpc->is_potential_contaminant))
> > return ERR_PTR(-EINVAL);
> > in tcpm_register_port().
> >
> > With that, it would no longer be necessary to keep checking for
> > port->tcpc->check_contaminant later on because potential_contaminant would only
> > ever be true if the check_contaminant callback exists.
>
> We now only have check_contaminant callback in v8 :)
>
> >
> > Another note: We currently don't log in SRC_UNATTACHED, SNK_UNATTACHED,
> > and TOGGLING states. Would it make sense to add CHECK_CONTAMINANT to
> > the checks in tcpm_log() ?
>
> Sure, made the change in v8.
>
> Thanks,
> Badhri
>
> >
> > Thanks,
> > Guenter
> >
> > > Thanks,
> > > Badhri
> > >
> > >
> > >>
> > >> Thanks,
> > >> Guenter
> > >>
> > >>> */
> > >>> struct tcpc_dev {
> > >>> struct fwnode_handle *fwnode;
> > >>> @@ -148,6 +278,8 @@ 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 (*is_potential_contaminant)(struct tcpc_dev *dev, enum tcpm_state current_state);
> > >>> };
> > >>>
> > >>> struct tcpm_port;
> > >>> @@ -165,5 +297,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 */
> > >>>
> > >>> base-commit: 1524ceb14dd5ebd6f724d993c5ec1a9a8d445d8e
> > >>
> >

Next message: Badhri Jagan Sridharan: "[PATCH v9 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant"
Previous message: Badhri Jagan Sridharan: "Re: [PATCH v7 2/3] usb: typec: tcpci: Add callback for evaluating contaminant presence"
In reply to: Badhri Jagan Sridharan: "Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]