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 <[email protected]>
Suggested-by: Prashant Malani <[email protected]>
---
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
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 <[email protected]>
> Suggested-by: Prashant Malani <[email protected]>
> ---
> 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
Hi Prashant,
> -----Original Message-----
> From: Prashant Malani <[email protected]>
> Sent: Thursday, July 30, 2020 4:03 PM
> To: Shaikh, Azhar <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Patel, Utkarsh H
> <[email protected]>; Bowman, Casey G
> <[email protected]>; Mani, Rajmohan
> <[email protected]>
> 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 <[email protected]>
> > Suggested-by: Prashant Malani <[email protected]>
> > ---
> > 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
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
Hi Prashant,
> -----Original Message-----
> From: Prashant Malani <[email protected]>
> Sent: Thursday, July 30, 2020 4:25 PM
> To: Shaikh, Azhar <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Patel, Utkarsh H
> <[email protected]>; Bowman, Casey G
> <[email protected]>; Mani, Rajmohan
> <[email protected]>
> 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
Hi Azhar,
On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar <[email protected]> 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
Hi Prashant,
> -----Original Message-----
> From: Prashant Malani <[email protected]>
> Sent: Wednesday, August 5, 2020 12:37 PM
> To: Shaikh, Azhar <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Patel, Utkarsh H
> <[email protected]>; Bowman, Casey G
> <[email protected]>; Mani, Rajmohan
> <[email protected]>
> 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 <[email protected]>
> 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
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
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 <[email protected]> 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
Hi Heikki,
On Thu, Aug 6, 2020 at 4:39 AM Heikki Krogerus
<[email protected]> 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 <[email protected]> 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
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
Hi,
> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: Tuesday, August 11, 2020 6:07 AM
> To: Prashant Malani <[email protected]>; Shaikh, Azhar
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Patel, Utkarsh H
> <[email protected]>; Bowman, Casey G
> <[email protected]>; Mani, Rajmohan
> <[email protected]>
> 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