Message-ID: <CAOiXhaK+Za7au=CQRx3ML+5Jj17pevYi-xUyyhOEdx5ieq9GKQ@mail.gmail.com>
Date:   Fri, 21 Apr 2017 19:57:52 +0530
From:   Rajaram R <rajaram.officemail@...il.com>
To:     Badhri Jagan Sridharan <badhri@...gle.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Oliver Neukum <oneukum@...e.com>,
        Mats Karrman <mats.dev.list@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v17 2/3] usb: USB Type-C connector class

On Fri, Apr 21, 2017 at 1:16 AM, Badhri Jagan Sridharan
<badhri@...gle.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 ?


>
> TCPM code will issue hard reset in tcpm_dr_set and tcpm_pr_set if
> current_role is not same as the preferred_role.
>
> 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@...ux.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@...ck-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@...ux.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@...ux.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists