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

[Badhri Jagan Sridharan]

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.

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;
+}

Thanks,
Badhri

>
> My current code doesn't handle the !pd_capable state, so I'll need to do
> something anyway.
>
> Thanks,
> Guenter
>
>> >
>> > The attribute will "enable" Try.SRC/SNK states, i.e. next time the
>> > state machine is executed, those states need to be considered.
>> > Changing the value of this attribute must not affect the current
>> > connection.
>> >
>> >> 2. Wait till the subsequent connect for resolving port roles based on the
>> >> new state machine.
>> >
>> > Yes.
>> >
>> >> For #1 to happen the policy_engine layer would have to reset the port
>> >> to resolve the port roles based on the (Try.SRC /Try.SNK/ Default)
>> >> new state machine preference.
>> >>
>> >> Say for example when two non-PD devices following none (default state
>> >> machine) are connected, the port role resolution is going to be random.
>> >> But, if the userspace in one of the devices later changes the
>> >> preferred_role to source, then that device is most likely to become source
>> >> if the Try.SRC state-machine is re-run.
>> >>
>> >> Does the above question fall under a policy decision ? If so, should there
>> >> be another node to say if the port roles have to re-resolved  based on the
>> >> new state machine right away ?
>> >
>> > I don't think we should even consider option #1, but just to be sure,
>> > Oliver, what do you say?
>>
>> Can we at least consider exposing a port_reset field so that the userspace
>> at least has an option to make the state machine to kick in right away with
>> a hard reset ?
>>
>> Please do consider. We can't expect all low-end phones and devices with
>> smaller form factors then phones to implement PD as it might be an overkill
>> for them.
>>
>> >
>> > I guess we need to say in the documentation explicitly that changing
>> > the value will not affect the current connection.
>> >
>> >
>> > Thanks,
>> >
>> > --
>> > heikki