RE: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect
Hi, > -Original Message- > From: Heikki Krogerus > Sent: Tuesday, August 11, 2020 6:07 AM > To: Prashant Malani ; Shaikh, Azhar > > Cc: ble...@chromium.org; enric.balle...@collabora.com; > gro...@chromium.org; linux-kernel@vger.kernel.org; Patel, Utkarsh H > ; Bowman, Casey G > ; Mani, Rajmohan > > 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 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? thanks, -- heikki
Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect
Hi Heikki, On Thu, Aug 6, 2020 at 4:39 AM Heikki Krogerus 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 > > 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
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 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
Hi, On Wed, Aug 05, 2020 at 08:28:22PM +, 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
Hi Prashant, > -Original Message- > From: Prashant Malani > Sent: Wednesday, August 5, 2020 12:37 PM > To: Shaikh, Azhar > Cc: ble...@chromium.org; enric.balle...@collabora.com; > gro...@chromium.org; linux-kernel@vger.kernel.org; > heikki.kroge...@linux.intel.com; Patel, Utkarsh H > ; Bowman, Casey G > ; Mani, Rajmohan > > 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 > 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
Hi Azhar, On Wed, Aug 5, 2020 at 12:22 PM Shaikh, Azhar 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
Hi Prashant, > -Original Message- > From: Prashant Malani > Sent: Thursday, July 30, 2020 4:25 PM > To: Shaikh, Azhar > Cc: ble...@chromium.org; enric.balle...@collabora.com; > gro...@chromium.org; linux-kernel@vger.kernel.org; > heikki.kroge...@linux.intel.com; Patel, Utkarsh H > ; Bowman, Casey G > ; Mani, Rajmohan > > 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 +, 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
Hey Azhar, On Thu, Jul 30, 2020 at 11:06:12PM +, 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
Hi Prashant, > -Original Message- > From: Prashant Malani > Sent: Thursday, July 30, 2020 4:03 PM > To: Shaikh, Azhar > Cc: ble...@chromium.org; enric.balle...@collabora.com; > gro...@chromium.org; linux-kernel@vger.kernel.org; > heikki.kroge...@linux.intel.com; Patel, Utkarsh H > ; Bowman, Casey G > ; Mani, Rajmohan > > 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 > > Suggested-by: Prashant Malani > > --- > > 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
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 > Suggested-by: Prashant Malani > --- > 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
[PATCH v2 2/2] platform/chrome: cros_ec_typec: Avoid setting usb role during disconnect
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
Suggested-by: Prashant Malani
---
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
