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

Groups > linux.kernel > #1411013 > unrolled thread

Re: [RFC PATCHv2] 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: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-01 11:30 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-02 01:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-02 08:40 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 10:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 12:20 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-02 18:20 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-03 15:30 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-03 16:00 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-03 17:20 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-03 20:50 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-06 15:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-06 15:40 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-07 10:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-07 19:00 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 10:10 +0200

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

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.

	Regards
		Oliver

[toc] | [next] | [standalone]

#1411672

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.

Any idea ?

Thanks,
Guenter

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

#1411892

On Wed, 2016-06-01 at 16:29 -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.

I was under the impression that this applies to the current role.

> 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.
> 
> Any idea ?

I would state the obvious that we need separate directories for that.
And how do we express captive cables?

	Regards
		Oliver

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

#1411995

On Thu, Jun 02, 2016 at 08:30:57AM +0200, Oliver Neukum wrote:
> On Wed, 2016-06-01 at 16:29 -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.
> 
> I was under the impression that this applies to the current role.
> 
> > 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.
> > 
> > Any idea ?
> 
> I would state the obvious that we need separate directories for that.
> And how do we express captive cables?

That means we can not present the alternate modes the ports support as
devices, as then we would have potentially two devices presenting the
same mode. Wish we could make those symlinks.. But perhaps we don't
need to present the port alternate modes as devices in any case.

But there is an other problem. The alternate modes a port can
support as host or device may be defined by the platform, but it can
also be dictated by the alternate mode spec itself. Knowing about the
platform is a problem for the port driver, but knowing about the
individual alternate modes is not. Those will be problems for the
alternate mode drivers in the end.

So the moment a port driver is expected to create the attributes (or
populate the devices) that will present the alternate modes it
supports, this thing will not always be clear to it.

I'm now wondering should we make it responsibility for the port
drivers to generate those attributes that present the alternate modes
they support after all?


Thanks,

-- 
heikki

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

#1412102

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.


Thanks,

-- 
heikki

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

#1412410

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.

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.

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

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() ?

Thanks,
Guenter

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

#1413234

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?

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


Thanks,

-- 
heikki

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

#1413263

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

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

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

Thanks,
Guenter

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

#1413296

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

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

#1413392

On Fri, Jun 03, 2016 at 06:17:46PM +0300, Heikki Krogerus wrote:
[ ... ]
> > > > 
> > > > 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.
> 
Any name preferences ?

> > 
> > 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.
> 
Ok, I'll look into it as I proceed with my implementation.

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

Thanks,
Guenter

---
From ab1f2d0671e3cda74b80c6d17d99cb3e386c0d08 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <groeck@chromium.org>
Date: Thu, 2 Jun 2016 10:09:50 -0700
Subject: [PATCH] usb: typec: Register supported alternate modes in
 typec_register_port()

By registering supported alternate modes when registering the port, we
automatically get the correct directory hierarchie in the class device.

Change-Id: I543da5f4ce922ded0532e6b0a0fdb8bc55cb5a80
Signed-off-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/usb/type-c/typec.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
index 41ad955..22ee7eb 100644
--- a/drivers/usb/type-c/typec.c
+++ b/drivers/usb/type-c/typec.c
@@ -892,14 +892,22 @@ struct typec_port *typec_register_port(struct device *dev,
 	typec_init_roles(port);
 
 	ret = device_register(&port->dev);
-	if (ret) {
-		ida_simple_remove(&typec_index_ida, id);
-		put_device(&port->dev);
-		kfree(port);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto reg_err;
+
+	ret = typec_register_altmodes(&port->dev, cap->alt_modes);
+	if (ret)
+		goto alt_err;
 
 	return port;
+
+alt_err:
+	device_unregister(&port->dev);
+reg_err:
+	ida_simple_remove(&typec_index_ida, id);
+	put_device(&port->dev);
+	kfree(port);
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(typec_register_port);
 
@@ -908,6 +916,7 @@ void typec_unregister_port(struct typec_port *port)
 	if (port->connected)
 		typec_disconnect(port);
 
+	typec_unregister_altmodes(port->cap->alt_modes);
 	device_unregister(&port->dev);
 }
 EXPORT_SYMBOL_GPL(typec_unregister_port);
-- 
2.1.2

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

#1415034

On Fri, Jun 03, 2016 at 11:39:27AM -0700, Guenter Roeck wrote:
> On Fri, Jun 03, 2016 at 06:17:46PM +0300, Heikki Krogerus wrote:
> [ ... ]
> > > > > 
> > > > > 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.
> > 
> Any name preferences ?

I would prefer lower case letters. I don't know the SIDs there are at
them moment, other then Display Port. Do you know them?

I don't think we can ever guarantee that in every case we will be able
to provide a human readable name for the alternate modes if they are
vendor defined. We will then potentially still have the names in two
different forms: "usbc0.displayport" and "usbc0.svid:xxxx". Is that
something acceptable to everybody?

I guess if it's not, then to only way to go forward would be to always
just use the svid in the name.

> > > 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.
> > 
> Ok, I'll look into it as I proceed with my implementation.
> 
> > > 
> > > 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.
> > 
> See below.

Thanks. I pushed it to my work branch for this class:

https://github.com/krohei/linux/commits/typec_class

There are still a few open questions, but I'm in any case going to
start polishing the driver. I think the other attributes are more or
less decided, excluding the alternate modes.


Cheers,

-- 
heikki

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

#1415042

On Mon, 2016-06-06 at 16:28 +0300, Heikki Krogerus wrote:

> I would prefer lower case letters. I don't know the SIDs there are at
> them moment, other then Display Port. Do you know them?
> 
> I don't think we can ever guarantee that in every case we will be able
> to provide a human readable name for the alternate modes if they are
> vendor defined. We will then potentially still have the names in two
> different forms: "usbc0.displayport" and "usbc0.svid:xxxx". Is that
> something acceptable to everybody?
> 
> I guess if it's not, then to only way to go forward would be to always
> just use the svid in the name.

Yes, that is better. It makes more sense to use the number.
We'd get into versioning problems if we use names.

	Regards
		Oliver

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

#1415849

On Mon, Jun 06, 2016 at 03:35:35PM +0200, Oliver Neukum wrote:
> On Mon, 2016-06-06 at 16:28 +0300, Heikki Krogerus wrote:
> 
> > I would prefer lower case letters. I don't know the SIDs there are at
> > them moment, other then Display Port. Do you know them?
> > 
> > I don't think we can ever guarantee that in every case we will be able
> > to provide a human readable name for the alternate modes if they are
> > vendor defined. We will then potentially still have the names in two
> > different forms: "usbc0.displayport" and "usbc0.svid:xxxx". Is that
> > something acceptable to everybody?
> > 
> > I guess if it's not, then to only way to go forward would be to always
> > just use the svid in the name.
> 
> Yes, that is better. It makes more sense to use the number.
> We'd get into versioning problems if we use names.

OK, so we'll always use the svid in the name.

Thanks,

-- 
heikki

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

#1416422

On Mon, Jun 06, 2016 at 04:28:08PM +0300, Heikki Krogerus wrote:
[ ... ]
> 
> I would prefer lower case letters. I don't know the SIDs there are at
> them moment, other then Display Port. Do you know them?
> 
SIDs:
	0xff00 - power delivery
	0xff01 - displayport
	0xff02 - MHL
	0xff03 - Thunderbolt 3 (if I recall correctly)

VIDs: Can obviously be anything. I have seen
	0x05ac - Apple
	0x18d1 - Google

> I don't think we can ever guarantee that in every case we will be able
> to provide a human readable name for the alternate modes if they are
> vendor defined. We will then potentially still have the names in two
> different forms: "usbc0.displayport" and "usbc0.svid:xxxx". Is that
> something acceptable to everybody?
> 
> I guess if it's not, then to only way to go forward would be to always
> just use the svid in the name.
> 
I think we should just use the svid.

> > > > 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.
> > > 
> > Ok, I'll look into it as I proceed with my implementation.
> > 
> > > > 
> > > > 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.
> > > 
> > See below.
> 
> Thanks. I pushed it to my work branch for this class:
> 
> https://github.com/krohei/linux/commits/typec_class
> 
> There are still a few open questions, but I'm in any case going to
> start polishing the driver. I think the other attributes are more or
> less decided, excluding the alternate modes.
> 
Sounds good.

Thanks,
Guenter

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

#1411974

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.

It would make sense.. What are all the things we need to consider with
that? An example, how do we know if the attached device has changed,
etc.

I'll try to put somekind of suspend and resume hooks to the API, but
this needs some thinking.

Does this have an effect on the interface, meaning would this be
something we could potentially add later? We need to document the
expected behaviour from the beginning, i.e. the roles and the modes
should be restored when resuming, in any case I think.


Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web