Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S968415AbdDSRWz (ORCPT <rfc822;w@1wt.eu>);
        Wed, 19 Apr 2017 13:22:55 -0400
Received: from mail-io0-f180.google.com ([209.85.223.180]:33452 "EHLO
        mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S968354AbdDSRWt (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 19 Apr 2017 13:22:49 -0400
MIME-Version: 1.0
In-Reply-To: <20170419151401.GA14036@roeck-us.net>
References: <20170303125940.GA6999@kuha.fi.intel.com> <6ddb2eac-03d5-127e-df1e-ad189968e6b2@gmail.com>
 <20170306131442.GC6999@kuha.fi.intel.com> <696552a7-c36a-1d73-9517-543907e9da39@gmail.com>
 <20170308135853.GH6999@kuha.fi.intel.com> <68817c44-d880-581a-e9f5-12845b9215eb@gmail.com>
 <ce0350a4-f7d9-3fdd-88ee-8cb827ecee2a@roeck-us.net> <CAPTae5+O83bJm6KL2zWNhYv-SiZ2E490CLt02BbK4hG1xKXraw@mail.gmail.com>
 <20170419112323.GD24062@kuha.fi.intel.com> <CAPTae5+C5xVqEbq2v+J+OWh8srsbS-fM612Sbu5Cq6MSgmRr7g@mail.gmail.com>
 <20170419151401.GA14036@roeck-us.net>
From: Badhri Jagan Sridharan <badhri@google.com>
Date: Wed, 19 Apr 2017 10:22:47 -0700
Message-ID: <CAPTae5LgoGrPaBdyLE5hHX7NAZwSqc99SCUOEuYHP77pznT=Bw@mail.gmail.com>
Subject: Re: [PATCH v17 2/3] usb: USB Type-C connector class
To: Guenter Roeck <linux@roeck-us.net>
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>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6182
Lines: 173

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.

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;
+}

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