RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-30 Thread Jun Li 


> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
> Sent: 2018年3月30日 6:49
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; a.ha...@samsung.com;
> shufan_...@richtek.com; Peter Chen <peter.c...@nxp.com>;
> devicet...@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>; de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
> 
> On Thu, Mar 29, 2018 at 12:06:15AM +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.
> >
> 
> Isn't CC set by the lower level driver in this case ? In other words, is it 
> ever
> necessary to call back into the low level driver to set CC again ? Doing that 
> in
> attached state seems a bit late.

In question case(drp without try.src or try.snk), you can see tcpm never call
tcpm_set_cc() from drp toggling to attached state, start_drp_toggling
set cc at the beginning in this case, but the value is the *start* value, may 
not
the right value to match its final role.

Per tcpci spec
Figure 4-16. DRP Initialization and Connection Detection
After debounce, tcpm should set cc(again, but may different value) and polarity.

ShuFan encountered this case as I understood on his setup like below:
- Tcpc start drp toggling with Rp/Rp,
- Connect to adapter with also Rp/Rp
- After connection resolved, the tcpc becomes a sink(HW present Rd), but
role control register value is still Rp/Rp.
- This should be corrected to set cc for keep the un-contacted cc line open.

> 
> It may make more sense to update port->cc_req when the state machine leaves
> DRP_TOGGLING state, ie in _tcpm_cc_change(), and to do it without callback 
> into
> the low level driver (it should not be necessary).

Currently there is no user of cc_req flag, so I can't catch the real intention
of this flag, as of now it's only set in tcpm_set_cc(), so I understood it's
for if tcpm already set tcpc a specific role.

Li Jun
> 
> Guenter
> 
> > Signed-off-by: Li Jun <jun...@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
> > 218c230..72d4232 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -2126,6 +2126,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 = 0;
> >  }
> >
> >  static void tcpm_detach(struct tcpm_port *port) @@ -2361,6 +2362,8 @@
> > static void run_state_machine(struct tcpm_port *port)
> > break;
> >
> > case SRC_ATTACHED:
> > +   if (!port->cc_req)
> > +   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); @@ -2531,6 
> > +2534,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)
> > +   tcpm_set_cc(port, TYPEC_CC_RD);
> > ret = tcpm_snk_attach(port);
> > if (ret < 0)
> > tcpm_set_state(port, SNK_UNATTACHED, 0);
> > --
> > 2.7.4
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-30 Thread Jun Li 
Hi
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年3月30日 5:19
> To: Jun Li <jun...@nxp.com>; robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net
> Cc: a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
> 
> Hi Li,
> 
> On 03/28/2018 06:06 PM, 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.
> >
> > Signed-off-by: Li Jun <jun...@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
> > 218c230..72d4232 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -2126,6 +2126,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 = 0;
> 
> I don't think it's OK to use "0" here. cc_req is an enum so why not use
> "|TYPEC_CC_OPEN"?|
> 

I will change to be TYPEC_CC_OPEN, also other place.

Li Jun
> >  }
> >
> >  static void tcpm_detach(struct tcpm_port *port) @@ -2361,6 +2362,8 @@
> > static void run_state_machine(struct tcpm_port *port)
> > break;
> >
> > case SRC_ATTACHED:
> > +   if (!port->cc_req)
> 
>           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); @@ -2531,6 
> > +2534,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)
> 
> Ditto.
> 
> > +   tcpm_set_cc(port, TYPEC_CC_RD);
> > ret = tcpm_snk_attach(port);
> > if (ret < 0)
> > tcpm_set_state(port, SNK_UNATTACHED, 0);
> 
> // Mats
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-29 Thread Guenter Roeck 
On Thu, Mar 29, 2018 at 12:06:15AM +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.
> 

Isn't CC set by the lower level driver in this case ? In other words, is it ever
necessary to call back into the low level driver to set CC again ? Doing that in
attached state seems a bit late.

It may make more sense to update port->cc_req when the state machine leaves
DRP_TOGGLING state, ie in _tcpm_cc_change(), and to do it without callback
into the low level driver (it should not be necessary).

Guenter

> Signed-off-by: Li Jun 
> ---
>  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 218c230..72d4232 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -2126,6 +2126,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 = 0;
>  }
>  
>  static void tcpm_detach(struct tcpm_port *port)
> @@ -2361,6 +2362,8 @@ static void run_state_machine(struct tcpm_port *port)
>   break;
>  
>   case SRC_ATTACHED:
> + if (!port->cc_req)
> + 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);
> @@ -2531,6 +2534,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)
> + tcpm_set_cc(port, TYPEC_CC_RD);
>   ret = tcpm_snk_attach(port);
>   if (ret < 0)
>   tcpm_set_state(port, SNK_UNATTACHED, 0);
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-29 Thread Mats Karrman 
Hi Li,

On 03/28/2018 06:06 PM, 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.
>
> Signed-off-by: Li Jun 
> ---
>  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 218c230..72d4232 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -2126,6 +2126,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 = 0;

I don't think it's OK to use "0" here. cc_req is an enum so why not use 
"|TYPEC_CC_OPEN"?|

>  }
>  
>  static void tcpm_detach(struct tcpm_port *port)
> @@ -2361,6 +2362,8 @@ static void run_state_machine(struct tcpm_port *port)
>   break;
>  
>   case SRC_ATTACHED:
> + if (!port->cc_req)

        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);
> @@ -2531,6 +2534,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)

Ditto.

> + tcpm_set_cc(port, TYPEC_CC_RD);
>   ret = tcpm_snk_attach(port);
>   if (ret < 0)
>   tcpm_set_state(port, SNK_UNATTACHED, 0);

// Mats


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel