Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424933AbdD1Kwt (ORCPT ); Fri, 28 Apr 2017 06:52:49 -0400 Received: from mga03.intel.com ([134.134.136.65]:38009 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424264AbdD1Kwl (ORCPT ); Fri, 28 Apr 2017 06:52:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,387,1488873600"; d="scan'208";a="1162122897" Date: Fri, 28 Apr 2017 13:52:35 +0300 From: Heikki Krogerus To: Guenter Roeck , Rajaram R Cc: Badhri Jagan Sridharan , Oliver Neukum , Mats Karrman , Greg KH , Felipe Balbi , LKML , USB Subject: Re: [PATCH v17 2/3] usb: USB Type-C connector class Message-ID: <20170428105235.GG14071@kuha.fi.intel.com> References: <20170420122417.GB4769@kuha.fi.intel.com> <20170421164323.GA31344@roeck-us.net> <9485c5e8-6b79-0b93-d0e9-e7880aea9741@roeck-us.net> <20170427181055.GA28797@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170427181055.GA28797@roeck-us.net> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7475 Lines: 170 On Thu, Apr 27, 2017 at 11:10:55AM -0700, Guenter Roeck wrote: > On Thu, Apr 27, 2017 at 11:50:12AM +0530, Rajaram R wrote: > > On Tue, Apr 25, 2017 at 7:40 PM, Guenter Roeck wrote: > > > On 04/25/2017 01:26 AM, Rajaram R wrote: > > >> > > >> On Mon, Apr 24, 2017 at 11:20 PM, Badhri Jagan Sridharan > > >> wrote: > > >>> > > >>> On Sat, Apr 22, 2017 at 2:23 AM, Rajaram R > > >>> wrote: > > >>>> > > >>>> On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck > > >>>> wrote: > > >>>>> > > >>>>> On Fri, Apr 21, 2017 at 07:57:52PM +0530, Rajaram R wrote: > > >>>>>> > > >>>>>> On Fri, Apr 21, 2017 at 1:16 AM, Badhri Jagan Sridharan > > >>>>>> wrote: > > >>>>>>> > > >>>>>>> Thanks for the responses :) > > >>>>>>> > > >>>>>>> So seems like we have a plan. > > >>>>>>> > > >>>>>>> In Type-C connector class the checks for TYPEC_PWR_MODE_PD > > >>>>>>> and pd_revision for both the port and the partner will be removed in > > >>>>>>> power_role_store and the data_role_store and will be delegated > > >>>>>>> to the low level drivers. > > >>>>>> > > >>>>>> > > >>>>>> It is important to remember what USB Type-C provide is mechanisms for > > >>>>>> "TRYing" to become a particular role and not guaranteeing. > > >>>>>> > > >>>>>> With what device combination do you fore see we could get the desired > > >>>>>> role with this change ? > > >>>>>> > > >>>>> > > >>>>> If the partner is not PD capable, if a preferred role is specified, > > >>>>> if the current cole does not match the preferred role, and if the > > >>>>> request > > >>>>> is to set the role to match the preferred role, I think it is > > >>>>> reasonable > > >>>>> to expect that re-establishing the connection would accomplish that if > > >>>>> the > > >>>>> partner supports it. > > >>>>> > > >>>> In this context I believe we have two different inputs as follows: > > >>>> > > >>>> /sys/class/typec//supported_power_roles > > >>>> /sys/class/typec//preferred_role > > >>>> > > >>>> The need of preferred role is required when DRP is set in > > >>>> supported_power_roles option. > > >>>> Ideally a battery powered device will TRY to be SNK and a a/c plugged > > >>>> device will TRY to be SRC > > >>>> > > >>>> We need to understand which non-PD device will set to DRP? In the > > >>> > > >>> > > >>> Android Phones (actually it could be any phone which has a type-c port) > > >>> since it can act as usb gadget (when connected to PC) or Usb host > > >>> when connected to peripherals such as thumb drives, keyboard etc. > > >>> Phones with smaller form factors might be thermally limited to charge > > >>> above 15W, therefore supporting PD might be an overkill for them. > > >>> > > >>>> current ecosystem all legacy devices > > >>>> will sit behind adapters which either present an Rp or Rd. > > >>>> > > >>>> If it is a power adapter in 5V range can either present Rp or DRP with > > >>>> TRY.SRC and there is no role swap requirement. > > >>>> > > >>>> If it is a laptop port or similar with non-PD (??) DRP there is no > > >>>> guaranteed role swap in a non-PD mode. > > >>> > > >>> > > >>> This is true, but following a Try.SRC or Try.SNK state machine can > > >>> increase the chances of landing in the desired role/preferred role. > > >> > > >> > > >> Agree and as indicated it increases only chances. > > >> > > > > > > FWIW, this is pretty much the same as requesting a role change using PD. > > > Based on my experience with various devices, the chance for that to succeed > > > isn't really that high either. > > > > > > I am not really sure I understand your problem with using Try.{SRC,SNK} > > > to trigger (or attempt to trigger) a role change. Can we take a step back, > > > and can you explain ? > > > > The parameters required for a type-c connection is defined as follows > > and will have a default value. > > > > /sys/class/typec//supported_power_roles > > I don't see that attribute (it is implied in the power_role attribute). > It only existed in an earlier version of the driver. > > Also, the attribute (when it existed) listed the roles supported > by the port, not an administrative restriction of supported roles. > > > /sys/class/typec//preferred_role > > > > When two DRP devices are connected and for which we have > > preferred_role which provides input on the preference, In a DRP mode > > we have randomness in the mode of connection and hence we require role > > swap mechanisms. A Type-C only device cannot role swap as this is > > valid only in PD operation. > > > > The same is true for non-PD connections. Also, PD doesn't solve the problem > if both ends have the same source/sink preference. The result is still > randomness. To resolve that randomness, one of the connection partners > would have to give up its preference. That is quite independent of PD > support. > > > # Question was how to choose a particular role in non-PD mode. Only > > way to have a deterministic role in a non-PD mode is to set expected > > supported_role of choice rather than DRP. > > > > # As part of the solution suggested, checking of roles and triggering > > role swaps has to be done by the policy manager(PM) and delinked from > > Policy Engine. I guess the policy manager is at user space?. > > > > I do not have the complete git repo and may be i could be missing > > something. If this is in any public git please let me know > > > > Even if a device does support PD, there is no guarantee that a power role > swap is successful. That depends on the policy set by the partner, and > for all practical purposes it also depends on the quality and stability > of the PD protocol implementation on both ends. As mentioned above, if > both ends have the same preferred role, the result is still randomness > and requires manual intervention by the user if the "other" role is > desired. > > It seems to me that we are discussing if we should do as good as we can > or if we should only implement what can be guaranteed. I strongly favor > the first approach. I also think we should not preclude that approach in > the kernel implementation. It should be left to a specific policy manager > implementation to decide if a given device should be a DRP or source-only > or sink-only, independent of the question if PD is supported on either > end of a connection. > > If a given implementation wants to restrict available port modes, it > can do so by providing acceptable port capabilities (ie DRP or source > or sink) when registering the port with the infrastructure. > > I think you are suggesting that there should be a means to override port > capabilities (more specifically, permitted roles) after port registration. > That may be a matter for discussion, but it is a separate issue. That is how also I see it. > Also, I am not sure if it was already discussed. Does anyone remember ? Yes, I did propose an attribute that would allow fixing the role of a port to either sink or source at one point. I don't remember what was the reason I ended up dropping that proposal - I need to go through my archives - but I think it was seen as a feature that, if needed, can be added later by adding a new attribute file for it. So if that is the functionality you want, please start a new thread and propose the new attribute. At least don't try to modify the behaviour of the existing ones because of it. Thank you, -- heikki