From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S945115AbdDTMYY (ORCPT <rfc822;w@1wt.eu>);
        Thu, 20 Apr 2017 08:24:24 -0400
Received: from mga07.intel.com ([134.134.136.100]:44458 "EHLO mga07.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S944402AbdDTMYW (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 20 Apr 2017 08:24:22 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.37,225,1488873600"; 
   d="scan'208";a="959146628"
Date: Thu, 20 Apr 2017 15:24:17 +0300
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Badhri Jagan Sridharan <badhri@google.com>
Cc: Guenter Roeck <linux@roeck-us.net>, 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
Message-ID: <20170420122417.GB4769@kuha.fi.intel.com>
References: <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>
 <CAPTae5LgoGrPaBdyLE5hHX7NAZwSqc99SCUOEuYHP77pznT=Bw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPTae5LgoGrPaBdyLE5hHX7NAZwSqc99SCUOEuYHP77pznT=Bw@mail.gmail.com>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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