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

[Rajaram R]

On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On Fri, Apr 21, 2017 at 07:57:52PM +0530, Rajaram R wrote:
>> On Fri, Apr 21, 2017 at 1:16 AM, Badhri Jagan Sridharan
>> <badhri@xxxxxxxxxx> 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 ?
>>
>
> If the partner is not PD capable, if a preferred role is specified,
> if the current cole does not match the preferred role, and if the request
> is to set the role to match the preferred role, I think it is reasonable
> to expect that re-establishing the connection would accomplish that if the
> partner supports it.
>
In this context I believe we have two different inputs as follows:

/sys/class/typec/<port>/supported_power_roles
/sys/class/typec/<port>/preferred_role

The need of preferred role is required when DRP is set in
supported_power_roles option.
Ideally a battery powered device will TRY to be SNK and a a/c plugged
device will TRY to be SRC

We need to understand which non-PD device will set to DRP? In the
current ecosystem all legacy devices
will sit behind adapters which either present an Rp or Rd.

If it is a power adapter in 5V range can either present Rp or DRP with
TRY.SRC and there is no role swap requirement.

If it is a laptop port or similar with non-PD (??) DRP  there is no
guaranteed role swap in a non-PD mode.
So we need to understand what non PD device will fit into this scenario ?

> Of course, that won't change anything if the partner does not support the
> desired role, but it is better than doing nothing. This is also comparable
> to requesting a role change from the partner if it does support PD.

All I am highlighting is that we can only TRY and there is no
guaranteed role swap with Type-C

> Do you have a better idea ?
>
If need a guaranteed role in a non-PD mode we need to set the required
role in supported_power_roles.
An understanding of scenario will help take better approach.

> Thanks,
> Guenter
>
>>
>> >
>> > TCPM code will issue hard reset in tcpm_dr_set and tcpm_pr_set if
>> > current_role is not same as the preferred_role.
>> >
>
> ... if the partner is not PD capable.
>
>> > 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
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> > the body of a message to majordomo@xxxxxxxxxxxxxxx
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html