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

[Guenter Roeck]

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