Re: [PATCH v17 2/3] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

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.

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> >> >> +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