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

[Guenter Roeck]

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,