linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Badhri Jagan Sridharan <badhri@google.com>
Cc: 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: Wed, 19 Apr 2017 12:29:17 -0700	[thread overview]
Message-ID: <20170419192917.GA4950@roeck-us.net> (raw)
In-Reply-To: <CAPTae5LgoGrPaBdyLE5hHX7NAZwSqc99SCUOEuYHP77pznT=Bw@mail.gmail.com>

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.
> 
At least in the current implementation of tcpm, the port type is reported
from low level drivers and does not change if the partner is not PD
capable. Port capabilities don't change if the partner is not PD capable,
and both are reported separately to the typec infrastructure.

> > 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 ?
> 
That is what it would boil down to, but that is really up to user space
to decide. User space would check if the reported role is the desired role,
and request a role change if it is not.
 
> 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.
> 
> 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;
> + }
> +
Ah yes, you are right. Of course, you could request a data role change,
which doesn't have that restriction, at least not right now, but that
would not really be clean. Maybe it should be left to the low level
driver to do that check, or the check could be augmented to only
require pwr_opmode == TYPEC_PWR_MODE_PD if a preferred role is not set.

Thanks,
Guenter

> + 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;
> +}
> 
> Thanks,
> Badhri
> 
> >
> > My current code doesn't handle the !pd_capable state, so I'll need to do
> > something anyway.
> >
> > Thanks,
> > Guenter
> >
> >> >
> >> > The attribute will "enable" Try.SRC/SNK states, i.e. next time the
> >> > state machine is executed, those states need to be considered.
> >> > Changing the value of this attribute must not affect the current
> >> > connection.
> >> >
> >> >> 2. Wait till the subsequent connect for resolving port roles based on the
> >> >> new state machine.
> >> >
> >> > Yes.
> >> >
> >> >> For #1 to happen the policy_engine layer would have to reset the port
> >> >> to resolve the port roles based on the (Try.SRC /Try.SNK/ Default)
> >> >> new state machine preference.
> >> >>
> >> >> Say for example when two non-PD devices following none (default state
> >> >> machine) are connected, the port role resolution is going to be random.
> >> >> But, if the userspace in one of the devices later changes the
> >> >> preferred_role to source, then that device is most likely to become source
> >> >> if the Try.SRC state-machine is re-run.
> >> >>
> >> >> Does the above question fall under a policy decision ? If so, should there
> >> >> be another node to say if the port roles have to re-resolved  based on the
> >> >> new state machine right away ?
> >> >
> >> > I don't think we should even consider option #1, but just to be sure,
> >> > Oliver, what do you say?
> >>
> >> Can we at least consider exposing a port_reset field so that the userspace
> >> at least has an option to make the state machine to kick in right away with
> >> a hard reset ?
> >>
> >> Please do consider. We can't expect all low-end phones and devices with
> >> smaller form factors then phones to implement PD as it might be an overkill
> >> for them.
> >>
> >> >
> >> > I guess we need to say in the documentation explicitly that changing
> >> > the value will not affect the current connection.
> >> >
> >> >
> >> > Thanks,
> >> >
> >> > --
> >> > heikki

  reply	other threads:[~2017-04-19 19:29 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 [this message]
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
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=20170419192917.GA4950@roeck-us.net \
    --to=linux@roeck-us.net \
    --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=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).