Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625547 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v17 2/3] usb: USB Type-C connector class Badhri Jagan Sridharan <badhri@google.com> - 2017-04-18 21:00 +0200
    Re: [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-04-19 13:30 +0200
      Re: [PATCH v17 2/3] usb: USB Type-C connector class Badhri Jagan Sridharan <badhri@google.com> - 2017-04-19 16:50 +0200
        Re: [PATCH v17 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-04-19 17:20 +0200
          Re: [PATCH v17 2/3] usb: USB Type-C connector class Badhri Jagan Sridharan <badhri@google.com> - 2017-04-19 19:30 +0200
            Re: [PATCH v17 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-04-19 21:30 +0200
            Re: [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-04-20 14:30 +0200
              Re: [PATCH v17 2/3] usb: USB Type-C connector class Badhri Jagan Sridharan <badhri@google.com> - 2017-04-20 21:50 +0200
                Re: [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-04-21 14:20 +0200
                  Re: [PATCH v17 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-04-21 15:20 +0200
                Re: [PATCH v17 2/3] usb: USB Type-C connector class Rajaram R <rajaram.officemail@gmail.com> - 2017-04-21 16:30 +0200
                  Re: [PATCH v17 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-04-21 19:10 +0200
                    Re: [PATCH v17 2/3] usb: USB Type-C connector class Rajaram R <rajaram.officemail@gmail.com> - 2017-04-22 11:30 +0200
                      Re: [PATCH v17 2/3] usb: USB Type-C connector class Badhri Jagan Sridharan <badhri@google.com> - 2017-04-24 20:00 +0200
        Re: [PATCH v17 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-04-20 14:00 +0200

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

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@linux.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)
2. Wait till the subsequent connect for resolving port roles based on the
new state machine.

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 ?

Thanks !!

On Fri, Mar 10, 2017 at 3:41 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/10/2017 02:22 PM, Mats Karrman wrote:
>>
>> On 2017-03-08 14:58, Heikki Krogerus wrote:
>>
>>> On Tue, Mar 07, 2017 at 11:30:54PM +0100, Mats Karrman wrote:
>>>>
>>>> If I read Heikki's original suggestion I understand it like the DP
>>>> driver would be
>>>> responsible for AM specific USB PD/VDM communication. But wouldn't that
>>>> lead
>>>> to a lot of code duplication since the AM protocol is the same for all
>>>> drivers of
>>>> a kind?
>>>
>>> No that's not what I mean. I'm still mixing your PD controller with
>>> something else above, sorry about that. Your PD controller driver
>>> should not ideally even need to be aware of Type-C connector, right?
>>> It definitely does not need to do any USB PD communication.
>>
>>
>> Right.
>>
>>> I would imagine you have on top of the DP controller, a mux (which
>>> could be a DP/USB3 PHY like on Rockchip RK3399, discrete mux like
>>> Pericom PI3USB30532, or something else), and a USB Type-C PHY or USB
>>> PD controller. The bus would be tying the mux to the Type-C port (PHY
>>> or PD controller) and its partner (note that it does not tie the mux
>>> to the DP controller). Please correct me if I'm wrong about your
>>> hardware.
>>
>>
>> No, you're correct, a discrete mux and a fusb302.
>>
>>> Assuming that is how your board roughly looks like, the driver for the
>>> mux would be the driver for the DP altmode devices. That driver would
>>> be the one converting things like the Attention messages notifying
>>> about HPD into toggling of GPIOs, or what ever is needed on your
>>> board, etc.
>>
>>
>> OK.
>>
>>> The actual PD communication with VDMs should be considered as just the
>>> protocol, so we probable should have "protocol drivers". For example
>>> DP alternate mode VDMs and communication will always be the same
>>> despite of the hardware. The DP alternate mode "protocol driver" would
>>> then be tied to the alternate mode device for the partner, and that
>>> driver could have its own hooks for what ever is needed, like HPD
>>> signal handling, configuration changes, whatever. In any case,
>>> hopefully making things easy and straightforward for the "mux driver",
>>> _so that it does not need to care about the actual PD communication_.
>>
>>
>> I'm digesting your and Guenter's replies and patches.
>> I will try getting something up and running too soon and hopefully the
>> foggy parts will
>> dissolve. As for now I find it a lot easier to grok Guenter's drivers than
>> to see the
>> advantages and/or disadvantages of an altmode bus :-)
>>
>> @Guenter: There _is_ interest for your fusb302 driver, thank you
>>
>
> Ok, I'll see what I need to do to publish it.
>
> Guenter
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [next] | [standalone]

#1626032

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@linux.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.

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?

I guess we need to say in the documentation explicitly that changing
the value will not affect the current connection.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1626199

On Wed, Apr 19, 2017 at 4:23 AM, Heikki Krogerus
<heikki.krogerus@linux.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@linux.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.

>
> 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

[toc] | [prev] | [next] | [standalone]

#1626325

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@linux.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@linux.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.
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;
	}

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

[toc] | [prev] | [next] | [standalone]

#1626627

On Wed, Apr 19, 2017 at 8:14 AM, Guenter Roeck <linux@roeck-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@linux.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@linux.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.

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

[toc] | [prev] | [next] | [standalone]

#1626723

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@roeck-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@linux.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@linux.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.
> 
At least in the current implementation of tcpm, the port type is reported
from low level drivers and does not change if the partner is not PD
capable. Port capabilities don't change if the partner is not PD capable,
and both are reported separately to the typec infrastructure.

> > 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 ?
> 
That is what it would boil down to, but that is really up to user space
to decide. User space would check if the reported role is the desired role,
and request a role change if it is not.
 
> 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;
> + }
> +
Ah yes, you are right. Of course, you could request a data role change,
which doesn't have that restriction, at least not right now, but that
would not really be clean. Maybe it should be left to the low level
driver to do that check, or the check could be augmented to only
require pwr_opmode == TYPEC_PWR_MODE_PD if a preferred role is not set.

Thanks,
Guenter

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

[toc] | [prev] | [next] | [standalone]

#1627409

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@roeck-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@linux.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@linux.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

[toc] | [prev] | [next] | [standalone]

#1627767

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@linux.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@roeck-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@linux.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@linux.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

[toc] | [prev] | [next] | [standalone]

#1628195

Hi,

On Thu, Apr 20, 2017 at 12:46:59PM -0700, Badhri Jagan Sridharan 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.
> 
> 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 ?

Please send a patch for the class at least. I think the changes to the
tcpm can just be amended still at this point. That is of course up to
Guenter.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1628241

On 04/21/2017 05:12 AM, Heikki Krogerus wrote:
> Hi,
>
> On Thu, Apr 20, 2017 at 12:46:59PM -0700, Badhri Jagan Sridharan 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.
>>
>> 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 ?
>
> Please send a patch for the class at least. I think the changes to the
> tcpm can just be amended still at this point. That is of course up to
> Guenter.
>

I'll try to make the changes and post a new version today.

Guenter

[toc] | [prev] | [next] | [standalone]

#1628336

On Fri, Apr 21, 2017 at 1:16 AM, Badhri Jagan Sridharan
<badhri@google.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@linux.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@roeck-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@linux.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@linux.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1628384

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@google.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 ?
> 

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@linux.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@roeck-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@linux.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@linux.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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1628812

On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck <linux@roeck-us.net> 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@google.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 ?
>>
>
> 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@linux.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@roeck-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@linux.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@linux.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@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1629833

On Sat, Apr 22, 2017 at 2:23 AM, Rajaram R <rajaram.officemail@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck <linux@roeck-us.net> 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@google.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 ?
>>>
>>
>> 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

Android Phones (actually it could be any phone which has a type-c port)
since it can act as usb gadget (when connected to PC) or Usb host
when connected to peripherals such as thumb drives, keyboard etc.
Phones with smaller form factors might be thermally limited to charge
above 15W, therefore supporting PD might be an overkill for them.

> 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.

This is true, but following a Try.SRC or Try.SNK state machine can
increase the chances of landing in the desired role/preferred role.

> So we need to understand what non PD device will fit into this scenario ?
Answered above.

>
>> 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.

The current Type-c connector class interface defines the support_*_roles as
read-only nodes. Leaving that apart, I think what you are trying to say is that
instead of running through the state machine again by switching to
Try.SRC or Try.SNK, you are suggesting that switch from DRP to source/host
(or) sink/device to make sure that CC is either pulled up through Rp or
grounded through Rd so that it increases the chances of settling in the desired
role. I do agree this, but, there is a pitfall here. Say when a DRP is
connected to
a pure sink/device, when the DRP switches to being a pure sink as well, then
the port roles would not resolve at all as both would be asserting Rd on CC and
therefore it might not be possible to detect a disconnect unless we have
a VCONN powered cable. Following Try.SRC, Try.SNK state machine actually
takes care of this for you. When in Try.SRC or Try.SNK state, CC would either
be pulled up or down for a specific amount of time (tCCDebounce) to check if the
port partner is capable of switching to another role. If no port
resolution happens
within the timer expiry, the state machine forces the port into the
other role and
port resolution would eventually happen. IMHO So in short it is more safer to
switch to between Try.SRC and Try.SNK state machine to land in a preferred role
rather than switching a DRP to source or sink.

>
>> 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@linux.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@roeck-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@linux.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@linux.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@vger.kernel.org
>>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1627395

Hi,

On Wed, Apr 19, 2017 at 07:45:00AM -0700, Badhri Jagan Sridharan wrote:
> >> 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.

No it doesn't. When USB PD is not supported, you can still use
data_role to swap the role. Of course, the lower level driver may
not support any role swapping in that case.


Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web