[PATCH v2 0/2] TypeC Connector Class driver improvements

[PATCH v2 0/2] TypeC Connector Class driver improvements

[Azhar Shaikh]

Changes in v2:
* Patch 1: "platform/chrome: cros_ec_typec: Send enum values to
            usb_role_switch_set_role()"
  * Update the commit message to change 'USB_ROLE_HOST in case of
    UFP.'  to 'USB_ROLE_HOST in case of DFP.'

* Patch 2: "platform/chrome: cros_ec_typec: Avoid setting usb role twice
            during disconnect"
  * New patch added.

Azhar Shaikh (2):
  platform/chrome: cros_ec_typec: Send enum values to
    usb_role_switch_set_role()
  platform/chrome: cros_ec_typec: Avoid setting usb role twice during
    disconnect

 drivers/platform/chrome/cros_ec_typec.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Azhar Shaikh]

usb_role_switch_set_role() has the second argument as enum for usb_role.
Currently depending upon the data role i.e. UFP(0) or DFP(1) is sent.
This eventually translates to USB_ROLE_NONE in case of UFP and
USB_ROLE_DEVICE in case of DFP. Correct this by sending correct enum
values as USB_ROLE_DEVICE in case of UFP and USB_ROLE_HOST in case of
DFP.

Fixes: 7e7def15fa4b ("platform/chrome: cros_ec_typec: Add USB mux control")

Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
Cc: Prashant Malani <pmalani@chromium.org>
Reviewed-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 3eae01f4c9f7..eb4713b7ae14 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -590,7 +590,8 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 		dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
 
 	return usb_role_switch_set_role(typec->ports[port_num]->role_sw,
-					!!(resp.role & PD_CTRL_RESP_ROLE_DATA));
+				       resp.role & PD_CTRL_RESP_ROLE_DATA
+				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
 }
 
 static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
-- 
2.17.1

Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Prashant Malani]

Hi Azhar,

On Thu, Jul 30, 2020 at 03:56:08PM -0700, Azhar Shaikh wrote:
> usb_role_switch_set_role() has the second argument as enum for usb_role.
> Currently depending upon the data role i.e. UFP(0) or DFP(1) is sent.
> This eventually translates to USB_ROLE_NONE in case of UFP and
> USB_ROLE_DEVICE in case of DFP. Correct this by sending correct enum
> values as USB_ROLE_DEVICE in case of UFP and USB_ROLE_HOST in case of
> DFP.
> 
> Fixes: 7e7def15fa4b ("platform/chrome: cros_ec_typec: Add USB mux control")
> 
> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> Cc: Prashant Malani <pmalani@chromium.org>
> Reviewed-by: Prashant Malani <pmalani@chromium.org>
> ---

Please add the list of changes in each version after the "---" line.

>  drivers/platform/chrome/cros_ec_typec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 3eae01f4c9f7..eb4713b7ae14 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -590,7 +590,8 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  		dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
>  
>  	return usb_role_switch_set_role(typec->ports[port_num]->role_sw,
> -					!!(resp.role & PD_CTRL_RESP_ROLE_DATA));
> +				       resp.role & PD_CTRL_RESP_ROLE_DATA
> +				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
>  }
>  
>  static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
> -- 
> 2.17.1
> 

RE: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Shaikh, Azhar]

Hi Prashant,


> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Thursday, July 30, 2020 3:59 PM
> To: Shaikh, Azhar <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org;
> heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum
> values to usb_role_switch_set_role()
> 
> Hi Azhar,
> 
> On Thu, Jul 30, 2020 at 03:56:08PM -0700, Azhar Shaikh wrote:
> > usb_role_switch_set_role() has the second argument as enum for
> usb_role.
> > Currently depending upon the data role i.e. UFP(0) or DFP(1) is sent.
> > This eventually translates to USB_ROLE_NONE in case of UFP and
> > USB_ROLE_DEVICE in case of DFP. Correct this by sending correct enum
> > values as USB_ROLE_DEVICE in case of UFP and USB_ROLE_HOST in case of
> > DFP.
> >
> > Fixes: 7e7def15fa4b ("platform/chrome: cros_ec_typec: Add USB mux
> > control")
> >
> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> > Cc: Prashant Malani <pmalani@chromium.org>
> > Reviewed-by: Prashant Malani <pmalani@chromium.org>
> > ---
> 
> Please add the list of changes in each version after the "---" line.

I have added those in the cover letter.
> 
> >  drivers/platform/chrome/cros_ec_typec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index 3eae01f4c9f7..eb4713b7ae14 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -590,7 +590,8 @@ static int cros_typec_port_update(struct
> cros_typec_data *typec, int port_num)
> >  		dev_warn(typec->dev, "Configure muxes failed, err = %d\n",
> ret);
> >
> >  	return usb_role_switch_set_role(typec->ports[port_num]-
> >role_sw,
> > -					!!(resp.role &
> PD_CTRL_RESP_ROLE_DATA));
> > +				       resp.role & PD_CTRL_RESP_ROLE_DATA
> > +				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
> >  }
> >
> >  static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
> > --
> > 2.17.1
> >

Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Prashant Malani]

On Thu, Jul 30, 2020 at 11:02:28PM +0000, Shaikh, Azhar wrote:
> Hi Prashant,
> 
> > 
> > Please add the list of changes in each version after the "---" line.
> 
> I have added those in the cover letter.
> > 
It is good practice to add these to the individual change too, so that
the reader doesn't have to go back to the cover letter to determine
what has changed in each patch.

RE: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Shaikh, Azhar]

> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Thursday, July 30, 2020 4:05 PM
> To: Shaikh, Azhar <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org;
> heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum
> values to usb_role_switch_set_role()
> 
> 
> On Thu, Jul 30, 2020 at 11:02:28PM +0000, Shaikh, Azhar wrote:
> > Hi Prashant,
> >
> > >
> > > Please add the list of changes in each version after the "---" line.
> >
> > I have added those in the cover letter.
> > >
> It is good practice to add these to the individual change too, so that the
> reader doesn't have to go back to the cover letter to determine what has
> changed in each patch.

So are you suggesting to move it from cover letter to individual patches?
But then isn't the same thing what cover letter is for?

Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum values to usb_role_switch_set_role()

[Prashant Malani]

On Thu, Jul 30, 2020 at 11:14:39PM +0000, Shaikh, Azhar wrote:
> 
> 
> > -----Original Message-----
> > From: Prashant Malani <pmalani@chromium.org>
> > Sent: Thursday, July 30, 2020 4:05 PM
> > To: Shaikh, Azhar <azhar.shaikh@intel.com>
> > Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> > groeck@chromium.org; linux-kernel@vger.kernel.org;
> > heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> > <utkarsh.h.patel@intel.com>; Bowman, Casey G
> > <casey.g.bowman@intel.com>; Mani, Rajmohan
> > <rajmohan.mani@intel.com>
> > Subject: Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Send enum
> > values to usb_role_switch_set_role()
> > 
> > 
> > On Thu, Jul 30, 2020 at 11:02:28PM +0000, Shaikh, Azhar wrote:
> > > Hi Prashant,
> > >
> > > >
> > > > Please add the list of changes in each version after the "---" line.
> > >
> > > I have added those in the cover letter.
> > > >
> > It is good practice to add these to the individual change too, so that the
> > reader doesn't have to go back to the cover letter to determine what has
> > changed in each patch.
> 
> So are you suggesting to move it from cover letter to individual patches?
> But then isn't the same thing what cover letter is for?

No, I'm suggesting you keep it both places. Each patch should have a
change log documenting what has changed in the various versions.

[PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Azhar Shaikh]

On disconnect port partner is removed and usb role is set to NONE.
But then in cros_typec_port_update() the role is set again.
Avoid this by moving usb_role_switch_set_role() to
cros_typec_configure_mux().

Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
Suggested-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index eb4713b7ae14..df97431b2275 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -515,6 +515,12 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
 	if (ret)
 		return ret;
 
+	ret = usb_role_switch_set_role(typec->ports[port_num]->role_sw,
+				       pd_ctrl->role & PD_CTRL_RESP_ROLE_DATA
+				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
+	if (ret)
+		return ret;
+
 	if (mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) {
 		ret = cros_typec_enable_tbt(typec, port_num, pd_ctrl);
 	} else if (mux_flags & USB_PD_MUX_DP_ENABLED) {
@@ -589,9 +595,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 	if (ret)
 		dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
 
-	return usb_role_switch_set_role(typec->ports[port_num]->role_sw,
-				       resp.role & PD_CTRL_RESP_ROLE_DATA
-				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
+	return ret;
 }
 
 static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
-- 
2.17.1

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Prashant Malani]

Hi Azhar,

On Thu, Jul 30, 2020 at 03:56:09PM -0700, Azhar Shaikh wrote:
> On disconnect port partner is removed and usb role is set to NONE.
> But then in cros_typec_port_update() the role is set again.
> Avoid this by moving usb_role_switch_set_role() to
> cros_typec_configure_mux().
> 
> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> Suggested-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index eb4713b7ae14..df97431b2275 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -515,6 +515,12 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
>  	if (ret)
>  		return ret;
>  
> +	ret = usb_role_switch_set_role(typec->ports[port_num]->role_sw,
> +				       pd_ctrl->role & PD_CTRL_RESP_ROLE_DATA
> +				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
> +	if (ret)
> +		return ret;

Since this was the last switch being configured, please maintain the
same order and add this at the end of the function, after the if-else if
block.

Best regards,

-Prashant

RE: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Shaikh, Azhar]

Hi Prashant,


> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Thursday, July 30, 2020 4:03 PM
> To: Shaikh, Azhar <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org;
> heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting
> usb role during disconnect
> 
> Hi Azhar,
> 
> On Thu, Jul 30, 2020 at 03:56:09PM -0700, Azhar Shaikh wrote:
> > On disconnect port partner is removed and usb role is set to NONE.
> > But then in cros_typec_port_update() the role is set again.
> > Avoid this by moving usb_role_switch_set_role() to
> > cros_typec_configure_mux().
> >
> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> > Suggested-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index eb4713b7ae14..df97431b2275 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -515,6 +515,12 @@ static int cros_typec_configure_mux(struct
> cros_typec_data *typec, int port_num,
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = usb_role_switch_set_role(typec->ports[port_num]->role_sw,
> > +				       pd_ctrl->role &
> PD_CTRL_RESP_ROLE_DATA
> > +				       ? USB_ROLE_HOST : USB_ROLE_DEVICE);
> > +	if (ret)
> > +		return ret;
> 
> Since this was the last switch being configured, please maintain the same
> order and add this at the end of the function, after the if-else if block.
> 

Please correct if my understanding is not correct here:
Set the orientation , set the role, then configure the mux. Shouldn't this be the order?


> Best regards,
> 
> -Prashant

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Prashant Malani]

Hey Azhar,

On Thu, Jul 30, 2020 at 11:06:12PM +0000, Shaikh, Azhar wrote:
> Hi Prashant,
> > 
> > Since this was the last switch being configured, please maintain the same
> > order and add this at the end of the function, after the if-else if block.
> > 
> 
> Please correct if my understanding is not correct here:
> Set the orientation , set the role, then configure the mux. Shouldn't this be the order?

Is this documented anywhere? Kindly provide the links to that if so. I
wasn't aware of any ordering requirements (but I may be missing something).

Please keep in mind that each of these switches (orientation, data-role,
mode-switch, or what is referred to here as "mux") can theoretically be
different switches, controlled independently by distinct drivers and
hardware.

We should not change what ordering is already present unless there is a
requirement to do so. The existing ordering was orientation switch, "mux" or role switch,
then the data-role switch, so let us stick to that.

Best regards,

> 
> 
> > Best regards,
> > 
> > -Prashant

RE: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Shaikh, Azhar]

Hi Prashant,


> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Thursday, July 30, 2020 4:25 PM
> To: Shaikh, Azhar <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org;
> heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting
> usb role during disconnect
> 
> Hey Azhar,
> 
> On Thu, Jul 30, 2020 at 11:06:12PM +0000, Shaikh, Azhar wrote:
> > Hi Prashant,
> > >
> > > Since this was the last switch being configured, please maintain the
> > > same order and add this at the end of the function, after the if-else if
> block.
> > >
> >
> > Please correct if my understanding is not correct here:
> > Set the orientation , set the role, then configure the mux. Shouldn't this be
> the order?
> 
> Is this documented anywhere? Kindly provide the links to that if so. I wasn't
> aware of any ordering requirements (but I may be missing something).

The configuration of the connector should always happen in the order defined in the 
USB Type-C specification. Check ch. 2.3 (USB Type-C Spec R2.0). So that will basically give you:

1. orientation
2. role(s)
3. the rest.

Also I see in USB Type-C Port Manager driver the above mentioned sequence being followed
https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/tcpm/tcpm.c#L664


> 
> Please keep in mind that each of these switches (orientation, data-role,
> mode-switch, or what is referred to here as "mux") can theoretically be
> different switches, controlled independently by distinct drivers and
> hardware.
> 
> We should not change what ordering is already present unless there is a
> requirement to do so. The existing ordering was orientation switch, "mux" or
> role switch, then the data-role switch, so let us stick to that.
> 
> Best regards,
> 
> >
> >
> > > Best regards,
> > >
> > > -Prashant

Regards,
Azhar

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Prashant Malani]

Hi Azhar,


On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar <azhar.shaikh@intel.com> wrote:
>
> Hi Prashant,
>
> > Is this documented anywhere? Kindly provide the links to that if so. I wasn't
> > aware of any ordering requirements (but I may be missing something).
>
> The configuration of the connector should always happen in the order defined in the
> USB Type-C specification. Check ch. 2.3 (USB Type-C Spec R2.0). So that will basically give you:
>
> 1. orientation
> 2. role(s)
> 3. the rest.

Thanks for the link. Are you referring to Section 2.3 (Configuration
Process) ? I couldn't find anything there which
implied any required ordering (I'm reading Release 2.0, Aug 2019, so I
don't know if something has been added since).
Could you kindly point me to the appropriate subsection?

Additionally, I think any ordering requirements there are already
handled by the TCPM in the Chrome OS EC.

>
> Also I see in USB Type-C Port Manager driver the above mentioned sequence being followed
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/tcpm/tcpm.c#L664

In addition to the above, note that this is a TCPM implementation (on
Chrome OS the TCPM implementation is in the EC), so what is
done there doesn't necessarily apply to cros-ec-typec.

Best regards,

-Prashant

>
>
> > > > -Prashant
>
> Regards,
> Azhar

RE: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Shaikh, Azhar]

Hi Prashant,


> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Wednesday, August 5, 2020 12:37 PM
> To: Shaikh, Azhar <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org;
> heikki.krogerus@linux.intel.com; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting
> usb role during disconnect
> 
> Hi Azhar,
> 
> 
> On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar <azhar.shaikh@intel.com>
> wrote:
> >
> > Hi Prashant,
> >
> > > Is this documented anywhere? Kindly provide the links to that if so.
> > > I wasn't aware of any ordering requirements (but I may be missing
> something).
> >
> > The configuration of the connector should always happen in the order
> > defined in the USB Type-C specification. Check ch. 2.3 (USB Type-C Spec
> R2.0). So that will basically give you:
> >
> > 1. orientation
> > 2. role(s)
> > 3. the rest.
> 
> Thanks for the link. Are you referring to Section 2.3 (Configuration
> Process) ? I couldn't find anything there which implied any required ordering
> (I'm reading Release 2.0, Aug 2019, so I don't know if something has been
> added since).
> Could you kindly point me to the appropriate subsection?

That is the section I was referring to.

> 
> Additionally, I think any ordering requirements there are already handled by
> the TCPM in the Chrome OS EC.
> 
> >
> > Also I see in USB Type-C Port Manager driver the above mentioned
> > sequence being followed
> > https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/tcpm/
> > tcpm.c#L664
> 
> In addition to the above, note that this is a TCPM implementation (on
> Chrome OS the TCPM implementation is in the EC), so what is done there
> doesn't necessarily apply to cros-ec-typec.
> 
> Best regards,
> 
> -Prashant
> 
> >
> >
> > > > > -Prashant
> >
> > Regards,
> > Azhar

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Prashant Malani]

Hi,

On Wed, Aug 05, 2020 at 08:28:22PM +0000, Shaikh, Azhar wrote:
> Hi Prashant,
> 
> > >
> > > Hi Prashant,
> > >
> > > > Is this documented anywhere? Kindly provide the links to that if so.
> > > > I wasn't aware of any ordering requirements (but I may be missing
> > something).
> > >
> > > The configuration of the connector should always happen in the order
> > > defined in the USB Type-C specification. Check ch. 2.3 (USB Type-C Spec
> > R2.0). So that will basically give you:
> > >
> > > 1. orientation
> > > 2. role(s)
> > > 3. the rest.
> > 
> > Thanks for the link. Are you referring to Section 2.3 (Configuration
> > Process) ? I couldn't find anything there which implied any required ordering
> > (I'm reading Release 2.0, Aug 2019, so I don't know if something has been
> > added since).
> > Could you kindly point me to the appropriate subsection?
> 
> That is the section I was referring to.

(Followed up with Azhar in a side-thread)
I don't see anything in that section that enforces an ordering on how
these switches should be configured. That said, I may be misinterpreting
it so I'm happy to follow up and withdraw my reservations to the change
given valid reasoning :)

As things stand, it looks like this is being reordered because it is
necessary for the particular switch driver (and architecture) in
question, i.e intel_pmc_mux.c to fix an edge use case
there (correcting the sequencing of PMC IPC messages when handling DP in warm
reboots).

I'd prefer the ordering be kept the way it was because:
1. We should preserve the existing ordering, and only fix the bug
   described in the commit message, i.e avoid setting usb_role switch to
   anything other than "NONE" during disconnect.

2. The CL should do 1 thing or call out why it's doing the re-ordering.
   Here it is not only fixing the double call to usb_role_switch_set_role (which is
   addressed in the commit message), but also re-ordering the call (which
   is not addressed at all).
   1.) and 2.) are sort-of related.

3. We should avoid fixing platform specific issues by changing the top
   level cros-ec-typec driver. Fixing this in the mux driver will make
   the driver more robust against any other sequencing issues that may
   crop up later.
   Also, if for example some other mux driver (driverB) requires a
   different ordering (e.g mode-switch before role-switch), changing
   that in cros-ec-typec will end up breaking the mux agent driver.
   driverB should handle the ordering issues internally, and so should
   intel_pmc_mux.c

BR,

-Prashant


> 
> > 
> > >
> > > Regards,
> > > Azhar

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Heikki Krogerus]

On Wed, Aug 05, 2020 at 12:37:14PM -0700, Prashant Malani wrote:
> Hi Azhar,
> 
> 
> On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar <azhar.shaikh@intel.com> wrote:
> >
> > Hi Prashant,
> >
> > > Is this documented anywhere? Kindly provide the links to that if so. I wasn't
> > > aware of any ordering requirements (but I may be missing something).
> >
> > The configuration of the connector should always happen in the order defined in the
> > USB Type-C specification. Check ch. 2.3 (USB Type-C Spec R2.0). So that will basically give you:
> >
> > 1. orientation
> > 2. role(s)
> > 3. the rest.
> 
> Thanks for the link. Are you referring to Section 2.3 (Configuration
> Process) ? I couldn't find anything there which
> implied any required ordering (I'm reading Release 2.0, Aug 2019, so I
> don't know if something has been added since).
> Could you kindly point me to the appropriate subsection?

Please check the section 4.5.1.2 (Connecting Sources and Sinks). Check
the typical flow. You can also check the Connection State Machine
Requirements. The order should be clear from those as well.

1. Source/sink detection
2. Orientation
3. Data role
4. VCONN
5. VBUS (USB Type-C currents)
6. The connector is now configured. We can start the PD communication
   that should lead into configuration of the mux if we enter a mode.

The data role, the thing that we are talking about here, really should
be set before the mux is configured.

> Additionally, I think any ordering requirements there are already
> handled by the TCPM in the Chrome OS EC.

The TCPM does not execute the steps that configure the port on this
platform. The OS is the part that actually executes the steps.

That is one reason (but not the only one) why it is important that
both parts follow the order that is proposed in the spec. Otherwise we
may endup negotiating things with the partner in one order but then
actually executing those steps in some other.


thanks,

-- 
heikki

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Prashant Malani]

Hi Heikki,

On Thu, Aug 6, 2020 at 4:39 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Aug 05, 2020 at 12:37:14PM -0700, Prashant Malani wrote:
> > Hi Azhar,
> >
> >
> > On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar <azhar.shaikh@intel.com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > > Is this documented anywhere? Kindly provide the links to that if so. I wasn't
> > > > aware of any ordering requirements (but I may be missing something).
> > >
> > > The configuration of the connector should always happen in the order defined in the
> > > USB Type-C specification. Check ch. 2.3 (USB Type-C Spec R2.0). So that will basically give you:
> > >
> > > 1. orientation
> > > 2. role(s)
> > > 3. the rest.
> >
> > Thanks for the link. Are you referring to Section 2.3 (Configuration
> > Process) ? I couldn't find anything there which
> > implied any required ordering (I'm reading Release 2.0, Aug 2019, so I
> > don't know if something has been added since).
> > Could you kindly point me to the appropriate subsection?
>
> Please check the section 4.5.1.2 (Connecting Sources and Sinks). Check
> the typical flow. You can also check the Connection State Machine
> Requirements. The order should be clear from those as well.

Thanks for sending the section info.

>
> 1. Source/sink detection
> 2. Orientation
> 3. Data role
> 4. VCONN
> 5. VBUS (USB Type-C currents)
> 6. The connector is now configured. We can start the PD communication
>    that should lead into configuration of the mux if we enter a mode.

The cros-ec-typec driver only receives a USB_PD_MUX_INFO [1] host
command after we've
already entered the mode as far as PD communication is concerned
(steps 1-5 and even PD communication
to enter the mode is already done by the time cros-ec-typec receives
PD_MUX_INFO).
There is no further PD communication to be done in this case (for a
particular mode), at least nothing that
is triggered by the AP.

>
> The data role, the thing that we are talking about here, really should
> be set before the mux is configured.

I apologize but I still didn't see anything there enforcing an
ordering for those on any AP switches. The state
machine you're referring to ((I assume you are referring to Figure
4-12 to Figure Figure 4-18)
is already implemented in the TCPM in the Chrome EC for the Chrome OS
Platform [2]

Perhaps we can take that discussion off-mailing list if necessary ?
(I'd like to avoid blasting the large mailing list
with more discussion email, but also happy to continue here if that's
the preference).

To be clear, all these comments are limited to the only Chrome OS platform.
>
> > Additionally, I think any ordering requirements there are already
> > handled by the TCPM in the Chrome OS EC.
>
> The TCPM does not execute the steps that configure the port on this
> platform. The OS is the part that actually executes the steps.

My response was w.r.t section 2.3 (the section which was originally
quoted) which deals
with things like:
"
- Source-to-Sink attach/detach detection
- Plug orientation/cable twist detection
- Detect if cable requires Vconn
"
etc.

All these things are performed by the Chrome OS EC (via TCPM or the TCPC).
For the items listed in Section 4.5.1.2, AFAIK those steps are
performed by the Chrome OS EC (via
a combination of the TCPM and TCPC).

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/tree/include/linux/platform_data/cros_ec_commands.h?h=for-kernelci#n5214
[2]: https://source.corp.google.com/chromeos_public/src/platform/ec/common/usbc/

>
> That is one reason (but not the only one) why it is important that
> both parts follow the order that is proposed in the spec. Otherwise we
> may endup negotiating things with the partner in one order but then
> actually executing those steps in some other.

I agree with this, but since the role of TCPM is performed by the
Chrome EC, I'm not convinced this patch is
addressing any spec related ordering requirements.

As I mentioned above, the Chrome OS EC is following the state machine
as well as the section
of the spec you referred to.

I would suggest:
- Merging Patch 1 (role set correction) and Patch 2 (moving the
usb_role_switch_set_role() inside cros_typec_configure_mux()
*but* keep it at the end to preserve existing ordering) into 1 patch.
- Add another patch which re-orders the calls and which in the commit
message lists out all the reasons why this re-ordering
needs to be done.

Doing the above will help keep better track of why the changes were made.

BR,

-Prashant
>
>
> thanks,

>
> --
> heikki

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Heikki Krogerus]

Hi,

On Thu, Aug 06, 2020 at 11:39:08AM -0700, Prashant Malani wrote:
> I would suggest:
> - Merging Patch 1 (role set correction) and Patch 2 (moving the
> usb_role_switch_set_role() inside cros_typec_configure_mux()
> *but* keep it at the end to preserve existing ordering) into 1 patch.
> - Add another patch which re-orders the calls and which in the commit
> message lists out all the reasons why this re-ordering
> needs to be done.
> 
> Doing the above will help keep better track of why the changes were made.

So Azhar can you please prepare v3?


thanks,

-- 
heikki

RE: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect

[Shaikh, Azhar]

Hi,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Tuesday, August 11, 2020 6:07 AM
> To: Prashant Malani <pmalani@chromium.org>; Shaikh, Azhar
> <azhar.shaikh@intel.com>
> Cc: bleung@chromium.org; enric.balletbo@collabora.com;
> groeck@chromium.org; linux-kernel@vger.kernel.org; Patel, Utkarsh H
> <utkarsh.h.patel@intel.com>; Bowman, Casey G
> <casey.g.bowman@intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>
> Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting
> usb role during disconnect
> 
> Hi,
> 
> On Thu, Aug 06, 2020 at 11:39:08AM -0700, Prashant Malani wrote:
> > I would suggest:
> > - Merging Patch 1 (role set correction) and Patch 2 (moving the
> > usb_role_switch_set_role() inside cros_typec_configure_mux()
> > *but* keep it at the end to preserve existing ordering) into 1 patch.
> > - Add another patch which re-orders the calls and which in the commit
> > message lists out all the reasons why this re-ordering needs to be
> > done.
> >
> > Doing the above will help keep better track of why the changes were
> made.
> 
> So Azhar can you please prepare v3?
> 

Sure, sent v3.

> 
> thanks,
> 
> --
> Heikki

Regards,
Azhar

Re: [PATCH v2 0/2] TypeC Connector Class driver improvements

[Heikki Krogerus]

On Thu, Jul 30, 2020 at 03:56:07PM -0700, Azhar Shaikh wrote:
> Changes in v2:
> * Patch 1: "platform/chrome: cros_ec_typec: Send enum values to
>             usb_role_switch_set_role()"
>   * Update the commit message to change 'USB_ROLE_HOST in case of
>     UFP.'  to 'USB_ROLE_HOST in case of DFP.'
> 
> * Patch 2: "platform/chrome: cros_ec_typec: Avoid setting usb role twice
>             during disconnect"
>   * New patch added.
> 
> Azhar Shaikh (2):
>   platform/chrome: cros_ec_typec: Send enum values to
>     usb_role_switch_set_role()
>   platform/chrome: cros_ec_typec: Avoid setting usb role twice during
>     disconnect
> 
>  drivers/platform/chrome/cros_ec_typec.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

I've lost track of this topic. There are threads with subject "[PATCH
v2 0/2] TypeC Connector Class driver improvements" in my inbox? I
guess one of them was supposed to be v3?

Please send the v3 with proper change log. Let's start over.

thanks,

-- 
heikki