Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Fri, Jun 03, 2016 at 06:51:54AM -0700, Guenter Roeck wrote:
> On 06/03/2016 06:21 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Thu, Jun 02, 2016 at 09:12:19AM -0700, Guenter Roeck wrote:
> > > On Thu, Jun 02, 2016 at 01:18:53PM +0300, Heikki Krogerus wrote:
> > > > On Wed, Jun 01, 2016 at 04:29:26PM -0700, Guenter Roeck wrote:
> > > > > On Wed, Jun 01, 2016 at 11:26:09AM +0200, Oliver Neukum wrote:
> > > > > > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > > > > > Just noticed that the "active" file is for now read only, but it needs
> > > > > > > to be changed to writable. That file will of course provide means for
> > > > > > > the userspace to Exit and Enter modes. But please note that the
> > > > > > > responsibility of the dependencies between the modes, say, if a plug
> > > > > > > needs to be in one mode or the other in order for the partner to enter
> > > > > > > some specific mode, will fall on the Alternate Mode specific drivers
> > > > > > > once we have the altmode bus. I remember there were concerns about
> > > > > > > this in the original thread.
> > > > > > 
> > > > > > There's one thing we haven't touched upon yet. And I cannot really find
> > > > > > an answer in the spec.
> > > > > > 
> > > > > > What do we do if we return from S4 or S3? I think we need to restore
> > > > > > the ALternate Mode because our display may be running over that
> > > > > > Alternate Mode.
> > > > > > If we want to support USB persist we also need to restore data role
> > > > > > after S4.
> > > > > > 
> > > > > I don't have an answer ... but another interesting question.
> > > > > 
> > > > > How do we distinguish between alternate modes supported by a host vs.
> > > > > alternate modes supported by a sink ? typec_capability includes a pointer
> > > > > to alternate modes supportedf by the connector, but it is not clear if
> > > > > those are alternate modes supported as host, or alternate modes supported
> > > > > as device, or alternate modes supported by both.
> > > > > 
> > > > > This doesn't matter much if only a fixed role is supported, but it does matter
> > > > > for dual role ports. A laptop will typically only support DisplayPort as host,
> > > > > for example.
> > > > 
> > > > The DP alternate mode spec actually separates the display role from
> > > > Type-C role. A laptop most likely would only support the modes for
> > > > display host roles, but if the port was DRP port then it would still
> > > > do so in both Type-C roles.
> > > > 
> > > > So basically, even if the display was Type-C host, it would still work
> > > > as a display when attached to the laptop.
> > > > 
> > > > > Any idea ?
> > > > 
> > > > I'm actually not sure this is a problem.
> > > > 
> > > Yes, this was a bad example, since the DisplayPort mode vdo includes a flag
> > > indicating if the port supports source, sink, or both.
> > 
> > I meant that in case of DP alternate mode, there should not be a
> > problem.
> > 
> > > Let's use a different example:
> > > Google devices (such as power adapters) have mode '1' for firmware upgrades.
> > > Obviously hosts will support that, but what should the host advertise if it
> > > is configured as sink ?
> > > 
> > > Maybe this is just my personal confusion, and there is no real problem.
> > > It might as well be that the Google mode VDO _should_ include a flag
> > > indicating if the port supports updating the partner, and/or if it supports
> > > being updated. For now I'll just assume that this is the case.
> > 
> > Well, do you think we can rely on always being able to get this detail
> > from VDO?
> > 
> 
> Not really. Like in the Google case, one end will implement sending the firmware,
> the other end will implement receiving it and writing it to flash (or whatever).
> Which is which isn't currently made visible to user space. I suspect that
> other "intelligent" devices like Apple's multi-function adapter do the same,
> though obviously I don't really know what the two VDO modes do on the apple
> adapter.
> 
> I'll have to have some ABI to user space for the alternate mode, for example
> to send the firmware file to the kernel. I am not there yet, though, so I
> don't really know exactly how that will look like. Most likely it is going to be
> added sysfs attributes in the mode device (eg usbc0.svid:18d1/mode0/firmware).
> 
> > > Something else, which goes back into the symlink question. If I create the
> > > alternate mode devices before calling typec_register_port(), the devices won't
> > > have a parent and don't show up in the class directory. You previously solved
> > > that with the symlink. I am trying to solve it in my current code by calling
> > > typec_register_altmodes() from typec_register_port() - primarily because I
> > > don't really want to duplicate all the device creation code in my driver.
> > > 
> > > In my test case, this gives me
> > >      /sys/class/type-c/usbc0/
> > > 	usbc0.svid:18d1
> > > 	usbc0.svid:18d1/mode0
> > > 	usbc0.svid:18d1/mode0/vdo
> > > 	usbc0.svid:18d1/mode0/description
> > > 	usbc0.svid:18d1/mode0/active
> > > 	...
> > > 	usbc0.svid:ff01
> > > 	usbc0.svid:ff01/mode0/vdo
> > > 	usbc0.svid:ff01/mode0/description
> > > 	usbc0.svid:ff01/mode0/active
> 
> Side note: I didn't provide a description/name for the modes, because that
> would result in something like usbc0.DisplayPort/ instead of usbc0.svid:ff01/,
> and I prefer a consistent ABI. Since this _is_ part of the ABI, would it make
> sense to standardize on names for modes in sysfs ? For example, how should
> a "Display Port" mode directory be named ? It doesn't sound good if I
> use "usbc0.svid:ff01", someone else uses "usbc0.DisplayPort", and yet
> someone else uses "usbc0.displayport".

Yeah, let's make them standard.

> 
> Also, do we at some point need to standardize the ABI for the standard
> alternate modes such as DisplayPort (if there are any - again I am not
> there yet) ?

I don't have an answer to that.

> > > 
> > > in addition to
> > >      /sys/class/type-c/usbc0/
> > > 	usbc0-partner/usbc0-partner.svid:05ac
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode0
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode0/vdo
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode0/description
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode0/active
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode1
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode1/vdo
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode1/description
> > > 	usbc0-partner/usbc0-partner.svid:05ac/mode1/active
> > > 	...
> > > 	usbc0-partner/usbc0-partner.svid:ff01
> > > 	usbc0-partner/usbc0-partner.svid:ff01/mode0
> > > 	usbc0-partner/usbc0-partner.svid:ff01/mode0/vdo
> > > 	usbc0-partner/usbc0-partner.svid:ff01/mode0/description
> > > 	usbc0-partner/usbc0-partner.svid:ff01/mode0/active
> > > 
> > > (when connecting the Apple adapter), which is exactly what I would expect to see.
> > > 
> > > Is this sensible ? Do we have a reason for expecting the alternate mode
> > > _devices_ to be created (without parent) when calling typec_register_port() ?
> > 
> > So if you would prefer that the class code takes care of creating the
> > alternate modes when typec_register_port() is called, I'm fine with
> > that too. Let's make it so.
> > 
> 
> Sounds good to me. Many other subsystems do the same, ie create the subsystem
> device(s) during registration with the subsystem, so this is in line with other
> kernel code.
> 
> Should I send you a follow-up patch on top of yours ?

Sure. I'm a little bit stuck with an other tasks, so let's keep this
thing rolling.


Thanks,

-- 
heikki