Messages in this thread | | | Date | Wed, 5 Aug 2020 17:57:50 -0700 | From | Prashant Malani <> | Subject | 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 +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
| |