Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 5 Aug 2020 17:57:50 -0700
From:   Prashant Malani <pmalani@chromium.org>
To:     "Shaikh, Azhar" <azhar.shaikh@intel.com>
Cc:     "bleung@chromium.org" <bleung@chromium.org>,
        "enric.balletbo@collabora.com" <enric.balletbo@collabora.com>,
        "groeck@chromium.org" <groeck@chromium.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "heikki.krogerus@linux.intel.com" <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
Message-ID: <20200806005750.GH3145664@google.com>
References: <20200730225609.7395-1-azhar.shaikh@intel.com>
 <20200730225609.7395-3-azhar.shaikh@intel.com>
 <20200730230238.GD3145664@google.com>
 <MWHPR11MB1518178C5B2335FC02CD36AE91710@MWHPR11MB1518.namprd11.prod.outlook.com>
 <20200730232504.GG3145664@google.com>
 <MWHPR11MB151867DF25664C80E99A326D914B0@MWHPR11MB1518.namprd11.prod.outlook.com>
 <CACeCKaf6WuW6XbFBQoVEW55w=OHfaVmmDn1xepiYYeRyMzZFrA@mail.gmail.com>
 <MWHPR11MB151843C494EAFDAD0DE6BA81914B0@MWHPR11MB1518.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MWHPR11MB151843C494EAFDAD0DE6BA81914B0@MWHPR11MB1518.namprd11.prod.outlook.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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