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

[Guenter Roeck <linux@roeck-us.net>]

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,
>