linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rajaram R <rajaram.officemail@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Badhri Jagan Sridharan <badhri@google.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Oliver Neukum <oneukum@suse.com>,
	Mats Karrman <mats.dev.list@gmail.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Felipe Balbi <felipe.balbi@linux.intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	USB <linux-usb@vger.kernel.org>
Subject: Re: [PATCH v17 2/3] usb: USB Type-C connector class
Date: Sat, 22 Apr 2017 14:53:39 +0530	[thread overview]
Message-ID: <CAOiXhaKkrxaJCKpMw-VUteymAHWV3Tuyh351OUDFSe6gRWTM5g@mail.gmail.com> (raw)
In-Reply-To: <20170421164323.GA31344@roeck-us.net>

On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck <linux@roeck-us.net> 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
>> <badhri@google.com> 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/<port>/supported_power_roles
/sys/class/typec/<port>/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
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.
So we need to understand what non PD device will fit into this scenario ?

> Of course, that won't change anything if the partner does not support the
> desired role, but it is better than doing nothing. This is also comparable
> to requesting a role change from the partner if it does support PD.

All I am highlighting is that we can only TRY and there is no
guaranteed role swap with Type-C

> Do you have a better idea ?
>
If need a guaranteed role in a non-PD mode we need to set the required
role in supported_power_roles.
An understanding of scenario will help take better approach.

> Thanks,
> Guenter
>
>>
>> >
>> > TCPM code will issue hard reset in tcpm_dr_set and tcpm_pr_set if
>> > current_role is not same as the preferred_role.
>> >
>
> ... if the partner is not PD capable.
>
>> > I am going to make changes in my local kernel code base to start
>> > making the corresponding changes in userspace.
>> > Should I post-back the local kernel changes or Heikki and Geunter
>> > you are planning to upload them ?
>> >
>> > Thanks for the support !!
>> > Badhri.
>> >
>> > On Thu, Apr 20, 2017 at 5:24 AM, Heikki Krogerus
>> > <heikki.krogerus@linux.intel.com> wrote:
>> >> On Wed, Apr 19, 2017 at 10:22:47AM -0700, Badhri Jagan Sridharan wrote:
>> >>> On Wed, Apr 19, 2017 at 8:14 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >>> > On Wed, Apr 19, 2017 at 07:45:00AM -0700, Badhri Jagan Sridharan wrote:
>> >>> >> On Wed, Apr 19, 2017 at 4:23 AM, Heikki Krogerus
>> >>> >> <heikki.krogerus@linux.intel.com> wrote:
>> >>> >> > Hi,
>> >>> >> >
>> >>> >> > On Tue, Apr 18, 2017 at 11:52:33AM -0700, Badhri Jagan Sridharan wrote:
>> >>> >> >> Hi Heikki,
>> >>> >> >>
>> >>> >> >> I have a question regarding the preferred_role node.
>> >>> >> >>
>> >>> >> >> +What:          /sys/class/typec/<port>/preferred_role
>> >>> >> >> +Date:          March 2017
>> >>> >> >> +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> >>> >> >> +Description:
>> >>> >> >> +               The user space can notify the driver about the preferred role.
>> >>> >> >> +               It should be handled as enabling of Try.SRC or Try.SNK, as
>> >>> >> >> +               defined in USB Type-C specification, in the port drivers. By
>> >>> >> >> +               default the preferred role should come from the platform.
>> >>> >> >> +
>> >>> >> >> +               Valid values: source, sink, none (to remove preference)
>> >>> >> >>
>> >>> >> >> What is the expected behavior when the userspace changes the
>> >>> >> >> preferred_role node when the port is in connected state ?
>> >>> >> >>
>> >>> >> >> 1.  the state machine re-resolves the port roles right away based on
>> >>> >> >> the new state machine in place ? (or)
>> >>> >> >
>> >>> >> > No! There are separate attributes for sending role swap requests.
>> >>> >>
>> >>> >> Right. But, that might not be helpful in cases when PD is not implemented.
>> >>> >> and Implementing PD is not mandatory according the spec :/
>> >>> >>
>> >>> >> FYI quoting from the Type-C specification release(page 24),
>> >>> >> role swaps are not limited to devices that only support PD.
>> >>> >>
>> >>> >> "Two independent set of mechanisms are defined to allow a USB Type-C
>> >>> >> DRP to functionally swap power and data roles. When USB PD is
>> >>> >> supported, power and data role swapping is performed as a subsequent
>> >>> >> step following the initial connection process. For non-PD implementations,
>> >>> >> power/data role swapping can optionally be dealt with as part of the initial
>> >>> >> connection process."
>> >>> >>
>> >>> >> But, the current interface definition actually prevents current/data role
>> >>> >> swaps for non-pd devices.
>> >>> >>
>> >>>
>> >>> > This is correct for the attribute definition, but it is not implemented
>> >>> > that way. Writing the attribute is only read-only for non-DRP ports.
>> >>>
>> >>> i.e. tcpm_dr_set/tcpm_pr_set at tcpm.c would return EINVAL when type
>> >>> is not TYPEC_PORT_DRP, is that what you are referring to ?
>> >>>
>> >>>         if (port->typec_caps.type != TYPEC_PORT_DRP) {
>> >>>                 ret = -EINVAL;
>> >>>                 goto port_unlock;
>> >>>         }
>> >>>
>> >>> I do agree that this is actually correct. I am referring to the case
>> >>> where port is
>> >>> dual-role-power and dual-role-data but NOT PD capable.
>> >>>
>> >>> > Given the standard, I would consider that to be intentional; it might
>> >>> > make sense to update the description accordingly.
>> >>> >
>> >>> > How about implementing a mechanism in the dr_set and pr_set code in tcpm
>> >>> > which would handle that situation ? Something along the line of
>> >>> >
>> >>> >         if (!port->pd_capable && connected && current role != desired role) {
>> >>> >                 reset_port();
>> >>> >                 goto done;
>> >>> >         }
>> >>>
>> >>> By "desired role" you are referring to preferred_role right ?
>> >>>
>> >>> If so yes, That's a good idea as well and it might work as long as
>> >>> type-c connector
>> >>> class allows the call to reach tcpm code :) But the current connector
>> >>> class code does
>> >>> not allow that because the power_role and data_role nodes are defined that way.
>> >>
>> >> Well, the data_role does not limit the requests from reaching the low
>> >> level drivers, but..
>> >>
>> >>> port->cap->pd_revision and the port->pwr_opmode check in the below code
>> >>> stub have to removed/refactored to make current_role/data_role writes to
>> >>> reach the tcpm code.
>> >>>
>> >>> +static ssize_t power_role_store(struct device *dev,
>> >>> + struct device_attribute *attr,
>> >>> + const char *buf, size_t size)
>> >>> +{
>> >>> + struct typec_port *port = to_typec_port(dev);
>> >>> + int ret = size;
>> >>> +
>> >>> + if (!port->cap->pd_revision) {
>> >>> + dev_dbg(dev, "USB Power Delivery not supported\n");
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>> +
>> >>> + if (!port->cap->pr_set) {
>> >>> + dev_dbg(dev, "power role swapping not supported\n");
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>> +
>> >>> + if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
>> >>> + dev_dbg(dev, "partner unable to swap power role\n");
>> >>> + return -EIO;
>> >>> + }
>> >>> +
>> >>> + ret = sysfs_match_string(typec_roles, buf);
>> >>> + if (ret < 0)
>> >>> + return ret;
>> >>> +
>> >>> + ret = port->cap->pr_set(port->cap, ret);
>> >>> + if (ret)
>> >>> + return ret;
>> >>> +
>> >>> + return size;
>> >>> +}
>> >>
>> >> .. yes. The power_role_store() does indeed need to be refactored. The
>> >> PD requirement should only be applied to Type-C spec versions < 1.2,
>> >> or removed completely. I would be happy to leave the checks to the low
>> >> level drivers.
>> >>
>> >>
>> >> Thanks,
>> >>
>> >> --
>> >> heikki
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2017-04-22  9:23 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-21 14:24 [PATCH v17 0/3] USB Type-C Connector class Heikki Krogerus
2017-02-21 14:24 ` [PATCH v17 1/3] lib/string: add sysfs_match_string helper Heikki Krogerus
2017-02-21 14:24 ` [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus
2017-03-02 15:22   ` Mats Karrman
2017-03-03  3:13     ` Guenter Roeck
2017-03-03  7:29       ` Mats Karrman
2017-03-03  9:48         ` Enric Balletbo Serra
2017-03-03 12:59         ` Heikki Krogerus
2017-03-03 14:49           ` Guenter Roeck
2017-03-03 19:27           ` Mats Karrman
2017-03-06  9:37             ` Oliver Neukum
2017-03-06 13:14             ` Heikki Krogerus
2017-03-07 22:30               ` Mats Karrman
2017-03-08  1:38                 ` Guenter Roeck
2017-04-08 23:09                   ` USB Type-C Port Manager API concern Mats Karrman
2017-04-09 15:16                     ` Guenter Roeck
2017-04-09 21:05                       ` Mats Karrman
2017-04-14  2:57                         ` Guenter Roeck
2017-04-14  8:30                           ` Mats Karrman
2017-03-08 13:58                 ` [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus
2017-03-10 22:22                   ` Mats Karrman
2017-03-10 23:41                     ` Guenter Roeck
2017-04-18 18:52                       ` Badhri Jagan Sridharan
2017-04-19 11:23                         ` Heikki Krogerus
2017-04-19 14:45                           ` Badhri Jagan Sridharan
2017-04-19 15:14                             ` Guenter Roeck
2017-04-19 17:22                               ` Badhri Jagan Sridharan
2017-04-19 19:29                                 ` Guenter Roeck
2017-04-20 12:24                                 ` Heikki Krogerus
2017-04-20 19:46                                   ` Badhri Jagan Sridharan
2017-04-21 12:12                                     ` Heikki Krogerus
2017-04-21 13:14                                       ` Guenter Roeck
2017-04-21 14:27                                     ` Rajaram R
2017-04-21 16:43                                       ` Guenter Roeck
2017-04-22  9:23                                         ` Rajaram R [this message]
2017-04-24 17:50                                           ` Badhri Jagan Sridharan
2017-04-25  8:26                                             ` Rajaram R
2017-04-25 14:10                                               ` Guenter Roeck
2017-04-27  6:20                                                 ` Rajaram R
2017-04-27 18:10                                                   ` Guenter Roeck
2017-04-28 10:52                                                     ` Heikki Krogerus
2017-04-20 11:55                             ` Heikki Krogerus
2017-03-03 14:41         ` Guenter Roeck
2017-03-03  3:35   ` Peter Chen
2017-03-03  4:29     ` Guenter Roeck
2017-03-03  4:52       ` Peter Chen
2017-03-03 14:36         ` Guenter Roeck
2017-03-06  1:24           ` Peter Chen
2017-03-03 14:31     ` Heikki Krogerus
2017-03-06  1:15       ` Peter Chen
2017-03-06 13:16         ` Heikki Krogerus
2017-03-07  1:36           ` Peter Chen
2017-03-07  8:57             ` Heikki Krogerus
2017-03-08  1:53               ` Guenter Roeck
2017-03-08  6:50                 ` Peter Chen
2017-03-08 14:44                   ` Guenter Roeck
2017-03-09  2:00                     ` Peter Chen
2017-02-21 14:24 ` [PATCH v17 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Heikki Krogerus
2017-02-21 15:42 ` [PATCH v17 0/3] USB Type-C Connector class Felipe Balbi
2017-03-21 11:14   ` Heikki Krogerus
2017-03-21 10:23 ` Greg KH
2017-03-21 10:37   ` Heikki Krogerus
2017-03-22 21:15     ` Mats Karrman
2017-03-23  8:16       ` Heikki Krogerus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOiXhaKkrxaJCKpMw-VUteymAHWV3Tuyh351OUDFSe6gRWTM5g@mail.gmail.com \
    --to=rajaram.officemail@gmail.com \
    --cc=badhri@google.com \
    --cc=felipe.balbi@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mats.dev.list@gmail.com \
    --cc=oneukum@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).