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

[Guenter Roeck]

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.

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.

Do you have a better idea ?

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