RE: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach

[Jun Li <jun.li@nxp.com>]

Hi
> -----Original Message-----
> From: Jun Li
> Sent: 2018年6月13日 19:07
> To: Guenter Roeck <linux@roeck-us.net>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; shufan_lee@richtek.com
> Cc: robh+dt@kernel.org; gregkh@linuxfoundation.org; cw00.choi@samsung.com;
> a.hajda@samsung.com; Peter Chen <peter.chen@nxp.com>;
> garsilva@embeddedor.com; gsomlo@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach
> 
> Hi,
> > -----Original Message-----
> > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: 2018年6月11日 21:35
> > To: Heikki Krogerus <heikki.krogerus@linux.intel.com>; Jun Li
> > <jun.li@nxp.com>
> > Cc: robh+dt@kernel.org; gregkh@linuxfoundation.org;
> > cw00.choi@samsung.com; a.hajda@samsung.com; shufan_lee@richtek.com;
> > Peter Chen <peter.chen@nxp.com>; garsilva@embeddedor.com;
> > gsomlo@gmail.com; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp
> > toggling attach
> >
> > On 06/11/2018 05:29 AM, Heikki Krogerus wrote:
> > > On Mon, May 28, 2018 at 10:52:44AM +0800, Li Jun wrote:
> > >> In case of drp toggling, we may need set correct cc value for role
> > >> control after attach as it may never been set.
> > >
> > > Is this something that should be considered as a fix?
> > >
> >
> > The problem with this patch is that it hides a problem. CC should have
> > been set by the time a port reaches the attached state. The patch
> > means that there is a state machine path where this does not happen.
> > I'd rather understand that path and fix the problem where it happens.
> 
> Here is my previous explanation about this state machine path[1]
> 
> [1]https://www.spinics.net/lists/linux-usb/msg167467.html
> 
> The physical CC line state is correct, just the Role Control register value may
> mismatch on some(e.g. shufan's) HW, if the common change in tcpm is not the
> right way, I have to ask shufan to try resolve this in his custom rt1711h driver.
> 
> I understand the concern of setting cc at attached state is late, should be done
> before that, how about I move this in cc_change like below:
> 
> @@ -3492,10 +3492,13 @@ static void _tcpm_cc_change(struct tcpm_port *port,
> enum typec_cc_status cc1,
>         switch (port->state) {
>         case DRP_TOGGLING:
>                 if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
> -                   tcpm_port_is_source(port))
> +                   tcpm_port_is_source(port)) {
> +                       tcpm_set_cc(port, tcpm_rp_cc(port));
>                         tcpm_set_state(port, SRC_ATTACH_WAIT, 0);
> -               else if (tcpm_port_is_sink(port))
> +               } else if (tcpm_port_is_sink(port)) {
> +                       if (port->cc_req == TYPEC_CC_OPEN)

Sorry, here should be:

+               } else if (tcpm_port_is_sink(port)) {
+                       tcpm_set_cc(port, TYPEC_CC_RD);
                        tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
+               }
>                         tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
> +               }
>                 break;
>         case SRC_UNATTACHED:
>         case ACC_UNATTACHED:
> 
> thanks
> Li Jun
> 
> >
> > Guenter
> >
> > >> Signed-off-by: Li Jun <jun.li@nxp.com>
> > >> ---
> > >>   drivers/usb/typec/tcpm.c | 5 +++++
> > >>   1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > >> index d885bff..98ea916 100644
> > >> --- a/drivers/usb/typec/tcpm.c
> > >> +++ b/drivers/usb/typec/tcpm.c
> > >> @@ -2599,6 +2599,7 @@ static void tcpm_reset_port(struct tcpm_port
> *port)
> > >>   	tcpm_set_attached_state(port, false);
> > >>   	port->try_src_count = 0;
> > >>   	port->try_snk_count = 0;
> > >> +	port->cc_req = TYPEC_CC_OPEN;
> > >>   	port->supply_voltage = 0;
> > >>   	port->current_limit = 0;
> > >>   	port->usb_type = POWER_SUPPLY_USB_TYPE_C; @@ -2831,6 +2832,8
> > @@
> > >> static void run_state_machine(struct tcpm_port *port)
> > >>   		break;
> > >>
> > >>   	case SRC_ATTACHED:
> > >> +		if (port->cc_req == TYPEC_CC_OPEN)
> > >> +			tcpm_set_cc(port, tcpm_rp_cc(port));
> > >>   		ret = tcpm_src_attach(port);
> > >>   		tcpm_set_state(port, SRC_UNATTACHED,
> > >>   			       ret < 0 ? 0 : PD_T_PS_SOURCE_ON); @@ -3004,6
> > +3007,8 @@
> > >> static void run_state_machine(struct tcpm_port *port)
> > >>   		tcpm_set_state(port, SNK_UNATTACHED,
> > PD_T_PD_DEBOUNCE);
> > >>   		break;
> > >>   	case SNK_ATTACHED:
> > >> +		if (port->cc_req == TYPEC_CC_OPEN)
> > >> +			tcpm_set_cc(port, TYPEC_CC_RD);
> > >>   		ret = tcpm_snk_attach(port);
> > >>   		if (ret < 0)
> > >>   			tcpm_set_state(port, SNK_UNATTACHED, 0);
> > >