[PATCH] usb: typec: tcpci: don't touch CC line which source Vconn

[PATCH] usb: typec: tcpci: don't touch CC line which source Vconn

[Xu Yang]

With the AMS and Collision Avoidance, tcpm often needs to change the CC's
termination. When one CC line is souring Vconn, if we still change its
termination, the voltage of the another CC line is likely to be fluctuant
and unstable.

Therefore, we should verify whether a CC line is soucing Vconn before
changing its termination. And only changing the termination that is
not a Vconn line. This can be done by reading the VCONN Present bit of
POWER_ STATUS register. To determinate the polarity, we can read the
Plug Orientation bit of TCPC_CONTROL register. Since only if Plug
Orientation is set, Vconn can be sourced.

Fixes: 0908c5aca31e ("usb: typec: tcpm: AMS and Collision Avoidance")
cc: <stable@vger.kernel.org>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/typec/tcpm/tcpci.c | 27 +++++++++++++++++++++++++++
 drivers/usb/typec/tcpm/tcpci.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 35a1307349a2..0bf4cbfaa21c 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -75,9 +75,26 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
 static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+	bool vconn_pres = false;
+	enum typec_cc_polarity polarity = TYPEC_POLARITY_CC1;
 	unsigned int reg;
 	int ret;
 
+	ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, &reg);
+	if (ret < 0)
+		return ret;
+
+	if (reg & TCPC_POWER_STATUS_VCONN_PRES) {
+		vconn_pres = true;
+
+		ret = regmap_read(tcpci->regmap, TCPC_TCPC_CTRL, &reg);
+		if (ret < 0)
+			return ret;
+
+		if (reg & TCPC_TCPC_CTRL_ORIENTATION)
+			polarity = TYPEC_POLARITY_CC2;
+	}
+
 	switch (cc) {
 	case TYPEC_CC_RA:
 		reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) |
@@ -112,6 +129,16 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 		break;
 	}
 
+	if (vconn_pres) {
+		if (polarity == TYPEC_POLARITY_CC2) {
+			reg &= ~(TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT);
+			reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT);
+		} else {
+			reg &= ~(TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT);
+			reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT);
+		}
+	}
+
 	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
index 2be7a77d400e..b2edd45f13c6 100644
--- a/drivers/usb/typec/tcpm/tcpci.h
+++ b/drivers/usb/typec/tcpm/tcpci.h
@@ -98,6 +98,7 @@
 #define TCPC_POWER_STATUS_SOURCING_VBUS	BIT(4)
 #define TCPC_POWER_STATUS_VBUS_DET	BIT(3)
 #define TCPC_POWER_STATUS_VBUS_PRES	BIT(2)
+#define TCPC_POWER_STATUS_VCONN_PRES	BIT(1)
 #define TCPC_POWER_STATUS_SINKING_VBUS	BIT(0)
 
 #define TCPC_FAULT_STATUS		0x1f
-- 
2.25.1

Re: [PATCH] usb: typec: tcpci: don't touch CC line which source Vconn

[Heikki Krogerus]

Hi,

I'm sorry, but I did not understand the subject line?

On Thu, Jan 06, 2022 at 04:53:25PM +0800, Xu Yang wrote:
> With the AMS and Collision Avoidance, tcpm often needs to change the CC's
> termination. When one CC line is souring Vconn, if we still change its
> termination, the voltage of the another CC line is likely to be fluctuant
> and unstable.
> 
> Therefore, we should verify whether a CC line is soucing Vconn before
> changing its termination. And only changing the termination that is
> not a Vconn line. This can be done by reading the VCONN Present bit of
> POWER_ STATUS register. To determinate the polarity, we can read the
> Plug Orientation bit of TCPC_CONTROL register. Since only if Plug
> Orientation is set, Vconn can be sourced.
> 
> Fixes: 0908c5aca31e ("usb: typec: tcpm: AMS and Collision Avoidance")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Okay, the commit message does explain what's this about, but could you
still change the subject to "..don't touch the CC line if it's VCONN
source" or something like that?

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 27 +++++++++++++++++++++++++++
>  drivers/usb/typec/tcpm/tcpci.h |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 35a1307349a2..0bf4cbfaa21c 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -75,9 +75,26 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> +	bool vconn_pres = false;
> +	enum typec_cc_polarity polarity = TYPEC_POLARITY_CC1;
>  	unsigned int reg;
>  	int ret;
>  
> +	ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg & TCPC_POWER_STATUS_VCONN_PRES) {

Isn't that bit optional? tcpm.c already knows the vconn status, right?
If it does, then maybe it would be safer to change the API so that you
pass also the information about the vconn status to the .set_cc
callback? So in this case:

static int tcpci_set_cc(struct tpcp_dev *tcpc, enum typec_cc_status cc, enum typec_role vconn)

That way the support would also be for all the other drivers too, so
not just for tcpci.c.

> +		vconn_pres = true;
> +
> +		ret = regmap_read(tcpci->regmap, TCPC_TCPC_CTRL, &reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (reg & TCPC_TCPC_CTRL_ORIENTATION)
> +			polarity = TYPEC_POLARITY_CC2;
> +	}
> +
>  	switch (cc) {
>  	case TYPEC_CC_RA:
>  		reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) |
> @@ -112,6 +129,16 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  		break;
>  	}
>  
> +	if (vconn_pres) {
> +		if (polarity == TYPEC_POLARITY_CC2) {
> +			reg &= ~(TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT);
> +			reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT);
> +		} else {
> +			reg &= ~(TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT);
> +			reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT);
> +		}
> +	}
> +
>  	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> index 2be7a77d400e..b2edd45f13c6 100644
> --- a/drivers/usb/typec/tcpm/tcpci.h
> +++ b/drivers/usb/typec/tcpm/tcpci.h
> @@ -98,6 +98,7 @@
>  #define TCPC_POWER_STATUS_SOURCING_VBUS	BIT(4)
>  #define TCPC_POWER_STATUS_VBUS_DET	BIT(3)
>  #define TCPC_POWER_STATUS_VBUS_PRES	BIT(2)
> +#define TCPC_POWER_STATUS_VCONN_PRES	BIT(1)
>  #define TCPC_POWER_STATUS_SINKING_VBUS	BIT(0)
>  
>  #define TCPC_FAULT_STATUS		0x1f
> -- 
> 2.25.1

-- 
heikki

RE: [EXT] Re: [PATCH] usb: typec: tcpci: don't touch CC line which source Vconn

[Xu Yang]

Hi,

> 
> I'm sorry, but I did not understand the subject line?
> 
> On Thu, Jan 06, 2022 at 04:53:25PM +0800, Xu Yang wrote:
> > With the AMS and Collision Avoidance, tcpm often needs to change the
> CC's
> > termination. When one CC line is souring Vconn, if we still change its
> > termination, the voltage of the another CC line is likely to be fluctuant
> > and unstable.
> >
> > Therefore, we should verify whether a CC line is soucing Vconn before
> > changing its termination. And only changing the termination that is
> > not a Vconn line. This can be done by reading the VCONN Present bit of
> > POWER_ STATUS register. To determinate the polarity, we can read the
> > Plug Orientation bit of TCPC_CONTROL register. Since only if Plug
> > Orientation is set, Vconn can be sourced.
> >
> > Fixes: 0908c5aca31e ("usb: typec: tcpm: AMS and Collision Avoidance")
> > cc: <stable@vger.kernel.org>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> Okay, the commit message does explain what's this about, but could you
> still change the subject to "..don't touch the CC line if it's VCONN
> source" or something like that?

Sorry for the puzzling title, I will change it in the next formal patch.

> 
> > ---
> >  drivers/usb/typec/tcpm/tcpci.c | 27 +++++++++++++++++++++++++++
> >  drivers/usb/typec/tcpm/tcpci.h |  1 +
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c
> b/drivers/usb/typec/tcpm/tcpci.c
> > index 35a1307349a2..0bf4cbfaa21c 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -75,9 +75,26 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned
> int reg, u16 val)
> >  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> >  {
> >       struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > +     bool vconn_pres = false;
> > +     enum typec_cc_polarity polarity = TYPEC_POLARITY_CC1;
> >       unsigned int reg;
> >       int ret;
> >
> > +     ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, &reg);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (reg & TCPC_POWER_STATUS_VCONN_PRES) {
> 
> Isn't that bit optional? tcpm.c already knows the vconn status, right?
> If it does, then maybe it would be safer to change the API so that you
> pass also the information about the vconn status to the .set_cc
> callback? So in this case:
> 
> static int tcpci_set_cc(struct tpcp_dev *tcpc, enum typec_cc_status cc, enum
> typec_role vconn)
> 
> That way the support would also be for all the other drivers too, so
> not just for tcpci.c.

Yeah, it's better to change the API in the tcpm.c. But from to my observation,
other drivers already have their own implementation to set CC's termination. 

For fusb302: 
It use chip->cc_polarity to choose which CC line to be changed.

For wcove:
It only changes one CC's termination at one time.

So, there is no such a problem for them with the AMS and Collision Avoidance.  
In tcpci, this problem appears because two CC's termination are changed at the 
same time even though Vconn enabled.

Therefore, I'm not sure the API in tcpm should be changed or only tcpci driver
should be changed at this case. Any suggestion for this?

> 
> > +             vconn_pres = true;
> > +
> > +             ret = regmap_read(tcpci->regmap, TCPC_TCPC_CTRL, &reg);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             if (reg & TCPC_TCPC_CTRL_ORIENTATION)
> > +                     polarity = TYPEC_POLARITY_CC2;
> > +     }
> > +
> >       switch (cc) {
> >       case TYPEC_CC_RA:
> >               reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) |
> > @@ -112,6 +129,16 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum
> typec_cc_status cc)
> >               break;
> >       }
> >
> > +     if (vconn_pres) {
> > +             if (polarity == TYPEC_POLARITY_CC2) {
> > +                     reg &= ~(TCPC_ROLE_CTRL_CC1_MASK <<
> TCPC_ROLE_CTRL_CC1_SHIFT);
> > +                     reg |= (TCPC_ROLE_CTRL_CC_OPEN <<
> TCPC_ROLE_CTRL_CC1_SHIFT);
> > +             } else {
> > +                     reg &= ~(TCPC_ROLE_CTRL_CC2_MASK <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> > +                     reg |= (TCPC_ROLE_CTRL_CC_OPEN <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> > +             }
> > +     }
> > +
> >       ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> >       if (ret < 0)
> >               return ret;
> > diff --git a/drivers/usb/typec/tcpm/tcpci.h
> b/drivers/usb/typec/tcpm/tcpci.h
> > index 2be7a77d400e..b2edd45f13c6 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.h
> > +++ b/drivers/usb/typec/tcpm/tcpci.h
> > @@ -98,6 +98,7 @@
> >  #define TCPC_POWER_STATUS_SOURCING_VBUS      BIT(4)
> >  #define TCPC_POWER_STATUS_VBUS_DET   BIT(3)
> >  #define TCPC_POWER_STATUS_VBUS_PRES  BIT(2)
> > +#define TCPC_POWER_STATUS_VCONN_PRES BIT(1)
> >  #define TCPC_POWER_STATUS_SINKING_VBUS       BIT(0)
> >
> >  #define TCPC_FAULT_STATUS            0x1f
> > --
> > 2.25.1
> 
> --
> Heikki

Xu Yang

Re: [EXT] Re: [PATCH] usb: typec: tcpci: don't touch CC line which source Vconn

[Heikki Krogerus]

On Tue, Jan 11, 2022 at 04:43:42AM +0000, Xu Yang wrote:
> Hi,
> 
> > 
> > I'm sorry, but I did not understand the subject line?
> > 
> > On Thu, Jan 06, 2022 at 04:53:25PM +0800, Xu Yang wrote:
> > > With the AMS and Collision Avoidance, tcpm often needs to change the
> > CC's
> > > termination. When one CC line is souring Vconn, if we still change its
> > > termination, the voltage of the another CC line is likely to be fluctuant
> > > and unstable.
> > >
> > > Therefore, we should verify whether a CC line is soucing Vconn before
> > > changing its termination. And only changing the termination that is
> > > not a Vconn line. This can be done by reading the VCONN Present bit of
> > > POWER_ STATUS register. To determinate the polarity, we can read the
> > > Plug Orientation bit of TCPC_CONTROL register. Since only if Plug
> > > Orientation is set, Vconn can be sourced.
> > >
> > > Fixes: 0908c5aca31e ("usb: typec: tcpm: AMS and Collision Avoidance")
> > > cc: <stable@vger.kernel.org>
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> > Okay, the commit message does explain what's this about, but could you
> > still change the subject to "..don't touch the CC line if it's VCONN
> > source" or something like that?
> 
> Sorry for the puzzling title, I will change it in the next formal patch.
> 
> > 
> > > ---
> > >  drivers/usb/typec/tcpm/tcpci.c | 27 +++++++++++++++++++++++++++
> > >  drivers/usb/typec/tcpm/tcpci.h |  1 +
> > >  2 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci.c
> > b/drivers/usb/typec/tcpm/tcpci.c
> > > index 35a1307349a2..0bf4cbfaa21c 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > > @@ -75,9 +75,26 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned
> > int reg, u16 val)
> > >  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> > >  {
> > >       struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > > +     bool vconn_pres = false;
> > > +     enum typec_cc_polarity polarity = TYPEC_POLARITY_CC1;
> > >       unsigned int reg;
> > >       int ret;
> > >
> > > +     ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, &reg);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     if (reg & TCPC_POWER_STATUS_VCONN_PRES) {
> > 
> > Isn't that bit optional? tcpm.c already knows the vconn status, right?
> > If it does, then maybe it would be safer to change the API so that you
> > pass also the information about the vconn status to the .set_cc
> > callback? So in this case:
> > 
> > static int tcpci_set_cc(struct tpcp_dev *tcpc, enum typec_cc_status cc, enum
> > typec_role vconn)
> > 
> > That way the support would also be for all the other drivers too, so
> > not just for tcpci.c.
> 
> Yeah, it's better to change the API in the tcpm.c. But from to my observation,
> other drivers already have their own implementation to set CC's termination. 
> 
> For fusb302: 
> It use chip->cc_polarity to choose which CC line to be changed.
> 
> For wcove:
> It only changes one CC's termination at one time.
> 
> So, there is no such a problem for them with the AMS and Collision Avoidance.  
> In tcpci, this problem appears because two CC's termination are changed at the 
> same time even though Vconn enabled.
> 
> Therefore, I'm not sure the API in tcpm should be changed or only tcpci driver
> should be changed at this case. Any suggestion for this?

Well, if this is only a problem for tcpci.c, then I guess API change
would not make much sense. Maybe we can just go with this for now.

thanks,

-- 
heikki