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

Groups > linux.kernel > #1403714 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-19 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-05-19 17:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:00 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 15:50 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-21 08:00 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-21 08:50 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-22 18:00 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 07:40 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 15:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 16:10 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 16:50 +0200
                        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 18:00 +0200
                          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:00 +0200
                            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 12:10 +0200
                              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 12:30 +0200
                                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 13:10 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-19 20:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 12:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-20 19:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 11:30 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 16:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 12:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 13:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 11:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 11:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 15:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 15:50 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:40 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:20 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 21:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 14:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 16:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-25 16:30 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:10 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:30 +0200
    [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-25 20:40 +0200
      Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 10:00 +0200
        Re: [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-27 16:10 +0200
          Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 15:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-30 16:10 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-31 10:40 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1404335

On Thu, May 19, 2016 at 10:53:04AM -0700, Guenter Roeck wrote:
> Hello Heikki,
> 
> On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > The purpose of this class is to provide unified interface for user
> > space to get the status and basic information about USB Type-C
> > Connectors in the system, control data role swapping, and when USB PD
> > is available, also power role swapping and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/Kconfig         |   2 +
> >  drivers/usb/Makefile        |   2 +
> >  drivers/usb/type-c/Kconfig  |   7 +
> >  drivers/usb/type-c/Makefile |   1 +
> >  drivers/usb/type-c/typec.c  | 957 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/typec.h   | 230 +++++++++++
> >  6 files changed, 1199 insertions(+)
> >  create mode 100644 drivers/usb/type-c/Kconfig
> >  create mode 100644 drivers/usb/type-c/Makefile
> >  create mode 100644 drivers/usb/type-c/typec.c
> >  create mode 100644 include/linux/usb/typec.h
> > 
> > Hi,
> > 
> > Like I've told some of you guys, I'm trying to implement a bus for
> > the Alternate Modes, but I'm still nowhere near finished with that
> > one, so let's just get the class ready now. The altmode bus should in
> > any case not affect the userspace interface proposed in this patch.
> > 
> > As you can see, the Alternate Modes are handled completely differently
> > compared to the original proposal. Every Alternate Mode will have
> > their own device instance (which will be then later bound to an
> > Alternate Mode specific driver once we have the bus), but also every
> > partner, cable and cable plug will have their own device instances
> > representing them.
> > 
> > An other change is that the data role is now handled in two ways.
> > The current_data_role file will represent static mode of the port, and
> > it will use the names for the roles as they are defined in the spec:
> > DFP, UFP and DRP. This file should be used if the port needs to be
> > fixed to one specific role with DRP ports. So this approach will
> > replace the suggestions for "preferred" data role we had. The
> > current_usb_data_role will use values "host" and "device" and it will
> > be used for data role swapping when already connected.
> > 
> 
> What I am missing completely is a means to handle role and alternate mode
> changes triggered by the partner. The need for those should be obvious,
> unless I am really missing something (just consider two devices supporting
> this code connected to each other).

We are missing the notifications that are needed in these cases. But I
don't see much more we can do about those cases. We can not put any
policies in place at this level, because we have to be able to support
also things like USB PD and Type-C controllers that take care of all
that, leaving us to not be able to do anything else but to pass the
information forward. So the framework at this level has to be
"stupid", and if more infrastructure is needed, it has to be
introduced in an other layer.

> Also, I am not sure where the policy engine is supposed to reside.
> I understand that some policy changes (eg unsolicited requests to switch roles)
> can be triggered from user space. However, role change requests triggered from
> the partner need to be evaluated quickly (typically within 15 ms), so user
> space can not get involved. Maybe it would help to have some text describing
> where the policy engine is expected to reside and how it is involved
> in the decision making process. This includes the initial decision making
> process, when it needs to be decided if role changes should be requested
> or if one or multiple alternate modes should be entered after the initial
> connection has been established.

Well, yes we need to document these things, but you are now coupling
this framework with USB PD and we really should not do that.

The policy engine, and the whole USB PD stack, belongs inside the
kernel, and it will be completely separated from this framework. This
framework can not have any dependencies on the future USB PD stack.
This is not only because of the USB PD/Type-C controllers which handle
the policy engine on their own and only allow "unsolicited" requests
like "swap role" and "enter/exit mode", but also because this
framework must work smoothly on systems that don't want to use USB PD
and of course also with USB Type-C PHYs that simply don't include USB
PD transceiver.

The layer that joins these two parts together will be the port drivers
themselves, so the USB Type-C/PD PHYs and controllers, at least in the
beginning.

Any initial decisions about which role or which alternate mode to
select belongs to the stack. The userspace will need to be notified,
and the userspace can then attempt to request changes after that, but
if there is something that blocks the requests, the attempt has to
just fail. So we can't provide any knowledge for the userspace about
the requirements regarding the high level operations we allow the
userspace to request (so in practice swap role/power and enter/exit
mode). This information the userspace needs to get from somewhere else
just live without it. Nor do we expect the userspace to be aware of
the state of the system. So for example if the userspace attempts to
activate a mode, the framework will just pass it forward to the port
driver, which will then process it with the PD stack, and if for
example the state is not PE_SRC_Ready or PE_SNK_Ready or whatever, the
operation will just fail probable with -EBUSY in that case.

And the knowledge about dependencies related to the alternate modes
belong primarily to the alternate mode specific drivers in the end
(once we have the bus) like I said. We can't expect the USB PD stack
to be aware of those as they are alternate mode specific, and of
course we can not trust that the userspace will always do things the
right way. But the initial states after connection must be handle by
the policy engine of course.

> On top of that, I am concerned about synchronization problems with role
> changes triggered from user space. The driver is not told about the
> desired role, only that it shall perform a role change. If a role change
> triggered by the partner is ongoing at the time a role change request is
> made from user space, it may well happen that the dual role change results
> in a revert to the original role. Some synchronization primitives as well
> as an API change might resolve that, but essentially it would mean that
> drivers have to implement at least part of the policy engine. It might
> make more sense to have that code in the infrastructure.

Well, like I said above, this framework can not provide this
infrastructure. The framework at this level really has to be "stupid".
We simply can not do any decisions or have any expectations at this
level. If more infra is needed, it has to be provided in an other layer
on top of this bottom layer. So basically the driver have to implement
those things for now.

> On disconnect, a port reverts to the default role. However, on port
> registration, the port roles are left at 0, meaning they always
> default to device/sink independent of the port's capabilities. Is this
> on purpose ?

On disconnect, the port role is set according to the "fixed_role"? If
the port is DFP only, then the port will still be host/source after
disconnect. I don't see the problem here?

> current_data_role_store() lets user space set a fixed port role. However,
> the port role variables are not updated. How is this supposed to be handled ?
> The same is true for other role change attributes - I don't see any code
> to update the role variables. Presumably this would have to be done in the
> class code, since the port data structure is private.

This is a bug in the code indeed.

> Overall, I am quite concerned by the lack of synchronization primitives
> between the class code and port drivers, but also in the class code
> itself. For example, nothing prevents multiple user space processes
> from writing into the same (or different) attribute(s) repeatedly.

We clearly need consensus on what this class will be responsible of.
I've tried to explain how I see it above, hopefully with reasonable
explanations. So basically, USB PD for this class is just a external
feature that the alternate modes and power and vconn swapping depends
on, that the class can not take any responsibility of IMHO.

The UCSI spec defines an other layer on top of the USB PD stack that
basically describes what the userspace interface that I'm trying to
achieve with this class is. The "OS Policy".


Thanks,

-- 
heikki

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

#1404618

On Fri, May 20, 2016 at 01:47:03PM +0300, Heikki Krogerus wrote:
> On Thu, May 19, 2016 at 10:53:04AM -0700, Guenter Roeck wrote:
> > Hello Heikki,
> > 
> > On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > > The purpose of this class is to provide unified interface for user
> > > space to get the status and basic information about USB Type-C
> > > Connectors in the system, control data role swapping, and when USB PD
> > > is available, also power role swapping and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/Kconfig         |   2 +
> > >  drivers/usb/Makefile        |   2 +
> > >  drivers/usb/type-c/Kconfig  |   7 +
> > >  drivers/usb/type-c/Makefile |   1 +
> > >  drivers/usb/type-c/typec.c  | 957 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/usb/typec.h   | 230 +++++++++++
> > >  6 files changed, 1199 insertions(+)
> > >  create mode 100644 drivers/usb/type-c/Kconfig
> > >  create mode 100644 drivers/usb/type-c/Makefile
> > >  create mode 100644 drivers/usb/type-c/typec.c
> > >  create mode 100644 include/linux/usb/typec.h
> > > 
> > > Hi,
> > > 
> > > Like I've told some of you guys, I'm trying to implement a bus for
> > > the Alternate Modes, but I'm still nowhere near finished with that
> > > one, so let's just get the class ready now. The altmode bus should in
> > > any case not affect the userspace interface proposed in this patch.
> > > 
> > > As you can see, the Alternate Modes are handled completely differently
> > > compared to the original proposal. Every Alternate Mode will have
> > > their own device instance (which will be then later bound to an
> > > Alternate Mode specific driver once we have the bus), but also every
> > > partner, cable and cable plug will have their own device instances
> > > representing them.
> > > 
> > > An other change is that the data role is now handled in two ways.
> > > The current_data_role file will represent static mode of the port, and
> > > it will use the names for the roles as they are defined in the spec:
> > > DFP, UFP and DRP. This file should be used if the port needs to be
> > > fixed to one specific role with DRP ports. So this approach will
> > > replace the suggestions for "preferred" data role we had. The
> > > current_usb_data_role will use values "host" and "device" and it will
> > > be used for data role swapping when already connected.
> > > 
> > 
> > What I am missing completely is a means to handle role and alternate mode
> > changes triggered by the partner. The need for those should be obvious,
> > unless I am really missing something (just consider two devices supporting
> > this code connected to each other).
> 
> We are missing the notifications that are needed in these cases. But I
> don't see much more we can do about those cases. We can not put any
> policies in place at this level, because we have to be able to support
> also things like USB PD and Type-C controllers that take care of all
> that, leaving us to not be able to do anything else but to pass the
> information forward. So the framework at this level has to be
> "stupid", and if more infrastructure is needed, it has to be
> introduced in an other layer.
> 
Ok.

> > Also, I am not sure where the policy engine is supposed to reside.
> > I understand that some policy changes (eg unsolicited requests to switch roles)
> > can be triggered from user space. However, role change requests triggered from
> > the partner need to be evaluated quickly (typically within 15 ms), so user
> > space can not get involved. Maybe it would help to have some text describing
> > where the policy engine is expected to reside and how it is involved
> > in the decision making process. This includes the initial decision making
> > process, when it needs to be decided if role changes should be requested
> > or if one or multiple alternate modes should be entered after the initial
> > connection has been established.
> 
> Well, yes we need to document these things, but you are now coupling
> this framework with USB PD and we really should not do that.
> 
Not really. I was trying to understand where you would expect the policy engine
to reside, which you answered above.

> The policy engine, and the whole USB PD stack, belongs inside the
> kernel, and it will be completely separated from this framework. This
> framework can not have any dependencies on the future USB PD stack.
> This is not only because of the USB PD/Type-C controllers which handle
> the policy engine on their own and only allow "unsolicited" requests
> like "swap role" and "enter/exit mode", but also because this
> framework must work smoothly on systems that don't want to use USB PD
> and of course also with USB Type-C PHYs that simply don't include USB
> PD transceiver.
> 
> The layer that joins these two parts together will be the port drivers
> themselves, so the USB Type-C/PD PHYs and controllers, at least in the
> beginning.
> 
> Any initial decisions about which role or which alternate mode to
> select belongs to the stack. The userspace will need to be notified,
> and the userspace can then attempt to request changes after that, but
> if there is something that blocks the requests, the attempt has to
> just fail. So we can't provide any knowledge for the userspace about
> the requirements regarding the high level operations we allow the
> userspace to request (so in practice swap role/power and enter/exit
> mode). This information the userspace needs to get from somewhere else
> just live without it. Nor do we expect the userspace to be aware of
> the state of the system. So for example if the userspace attempts to
> activate a mode, the framework will just pass it forward to the port
> driver, which will then process it with the PD stack, and if for
> example the state is not PE_SRC_Ready or PE_SNK_Ready or whatever, the
> operation will just fail probable with -EBUSY in that case.
> 
Makes sense.

> And the knowledge about dependencies related to the alternate modes
> belong primarily to the alternate mode specific drivers in the end
> (once we have the bus) like I said. We can't expect the USB PD stack
> to be aware of those as they are alternate mode specific, and of
> course we can not trust that the userspace will always do things the
> right way. But the initial states after connection must be handle by
> the policy engine of course.
> 
> > On top of that, I am concerned about synchronization problems with role
> > changes triggered from user space. The driver is not told about the
> > desired role, only that it shall perform a role change. If a role change
> > triggered by the partner is ongoing at the time a role change request is
> > made from user space, it may well happen that the dual role change results
> > in a revert to the original role. Some synchronization primitives as well
> > as an API change might resolve that, but essentially it would mean that
> > drivers have to implement at least part of the policy engine. It might
> > make more sense to have that code in the infrastructure.
> 
> Well, like I said above, this framework can not provide this
> infrastructure. The framework at this level really has to be "stupid".

Ok.

> We simply can not do any decisions or have any expectations at this
> level. If more infra is needed, it has to be provided in an other layer
> on top of this bottom layer. So basically the driver have to implement
> those things for now.
> 
I still think that the lack of synchronization is inherently racy.
For example, on a power role swap request, port->pwr_role can change
(via the currently missing notification) after it was evaluated in
current_power_role_store(), but before port->cap->pr_swap() is called.
The lower level code has at this point no means to know which power role
change was requested, which may result in the power role being swapped
again even though it already is in the requested state.

> > On disconnect, a port reverts to the default role. However, on port
> > registration, the port roles are left at 0, meaning they always
> > default to device/sink independent of the port's capabilities. Is this
> > on purpose ?
> 
> On disconnect, the port role is set according to the "fixed_role"? If
> the port is DFP only, then the port will still be host/source after
> disconnect. I don't see the problem here?
> 
Roles are set differently on port registration vs. disconnect.
This means that roles (can) differ between "initial disconnect state"
and "disconnect state after connect". On port registration, usb_role,
pwr_role, and vconn_role are all set based on the current pwr_opmode.
During port registration, they are all initialized with 0.

> > current_data_role_store() lets user space set a fixed port role. However,
> > the port role variables are not updated. How is this supposed to be handled ?
> > The same is true for other role change attributes - I don't see any code
> > to update the role variables. Presumably this would have to be done in the
> > class code, since the port data structure is private.
> 
> This is a bug in the code indeed.
> 
> > Overall, I am quite concerned by the lack of synchronization primitives
> > between the class code and port drivers, but also in the class code
> > itself. For example, nothing prevents multiple user space processes
> > from writing into the same (or different) attribute(s) repeatedly.
> 
> We clearly need consensus on what this class will be responsible of.
> I've tried to explain how I see it above, hopefully with reasonable
> explanations. So basically, USB PD for this class is just a external
> feature that the alternate modes and power and vconn swapping depends
> on, that the class can not take any responsibility of IMHO.
> 
Sounds good to me, as long as the lower level code can inform the class
about state/role changes.

> The UCSI spec defines an other layer on top of the USB PD stack that
> basically describes what the userspace interface that I'm trying to
> achieve with this class is. The "OS Policy".
> 
Since you mention UCSI - in UCSI, the two API functions available to set
the power role (Set Power Direction Mode, Set Power Direction Mode)
both provide the desired role to the connector driver. How does this map
to the API in the class code, where a power swap is requested without
telling the low level code about the desired role ?

Personally I prefer the approach used in UCSI, or let's say what I perceive
the approach to be: Maybe the class code should just send a request to the
connector driver to change the role to X, independent of the current role.
This way, most if not all synchronization problems could be handled by the
lower level driver.

On a side note, I don't see how to request a vconn swap with UCSI.
Is that supported ?

Thanks,
Guenter

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

#1405190

On Fri, May 20, 2016 at 10:02:28AM -0700, Guenter Roeck wrote:
> On Fri, May 20, 2016 at 01:47:03PM +0300, Heikki Krogerus wrote:
> > On Thu, May 19, 2016 at 10:53:04AM -0700, Guenter Roeck wrote:
> > > Hello Heikki,
> > > 
> > > On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > > > The purpose of this class is to provide unified interface for user
> > > > space to get the status and basic information about USB Type-C
> > > > Connectors in the system, control data role swapping, and when USB PD
> > > > is available, also power role swapping and Alternate Modes.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > ---
> > > >  drivers/usb/Kconfig         |   2 +
> > > >  drivers/usb/Makefile        |   2 +
> > > >  drivers/usb/type-c/Kconfig  |   7 +
> > > >  drivers/usb/type-c/Makefile |   1 +
> > > >  drivers/usb/type-c/typec.c  | 957 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/usb/typec.h   | 230 +++++++++++
> > > >  6 files changed, 1199 insertions(+)
> > > >  create mode 100644 drivers/usb/type-c/Kconfig
> > > >  create mode 100644 drivers/usb/type-c/Makefile
> > > >  create mode 100644 drivers/usb/type-c/typec.c
> > > >  create mode 100644 include/linux/usb/typec.h
> > > > 
> > > > Hi,
> > > > 
> > > > Like I've told some of you guys, I'm trying to implement a bus for
> > > > the Alternate Modes, but I'm still nowhere near finished with that
> > > > one, so let's just get the class ready now. The altmode bus should in
> > > > any case not affect the userspace interface proposed in this patch.
> > > > 
> > > > As you can see, the Alternate Modes are handled completely differently
> > > > compared to the original proposal. Every Alternate Mode will have
> > > > their own device instance (which will be then later bound to an
> > > > Alternate Mode specific driver once we have the bus), but also every
> > > > partner, cable and cable plug will have their own device instances
> > > > representing them.
> > > > 
> > > > An other change is that the data role is now handled in two ways.
> > > > The current_data_role file will represent static mode of the port, and
> > > > it will use the names for the roles as they are defined in the spec:
> > > > DFP, UFP and DRP. This file should be used if the port needs to be
> > > > fixed to one specific role with DRP ports. So this approach will
> > > > replace the suggestions for "preferred" data role we had. The
> > > > current_usb_data_role will use values "host" and "device" and it will
> > > > be used for data role swapping when already connected.
> > > > 
> > > 
> > > What I am missing completely is a means to handle role and alternate mode
> > > changes triggered by the partner. The need for those should be obvious,
> > > unless I am really missing something (just consider two devices supporting
> > > this code connected to each other).
> > 
> > We are missing the notifications that are needed in these cases. But I
> > don't see much more we can do about those cases. We can not put any
> > policies in place at this level, because we have to be able to support
> > also things like USB PD and Type-C controllers that take care of all
> > that, leaving us to not be able to do anything else but to pass the
> > information forward. So the framework at this level has to be
> > "stupid", and if more infrastructure is needed, it has to be
> > introduced in an other layer.
> > 
> Ok.
> 
> > > Also, I am not sure where the policy engine is supposed to reside.
> > > I understand that some policy changes (eg unsolicited requests to switch roles)
> > > can be triggered from user space. However, role change requests triggered from
> > > the partner need to be evaluated quickly (typically within 15 ms), so user
> > > space can not get involved. Maybe it would help to have some text describing
> > > where the policy engine is expected to reside and how it is involved
> > > in the decision making process. This includes the initial decision making
> > > process, when it needs to be decided if role changes should be requested
> > > or if one or multiple alternate modes should be entered after the initial
> > > connection has been established.
> > 
> > Well, yes we need to document these things, but you are now coupling
> > this framework with USB PD and we really should not do that.
> > 
> Not really. I was trying to understand where you would expect the policy engine
> to reside, which you answered above.

Ah OK, got it. Sorry.

> > The policy engine, and the whole USB PD stack, belongs inside the
> > kernel, and it will be completely separated from this framework. This
> > framework can not have any dependencies on the future USB PD stack.
> > This is not only because of the USB PD/Type-C controllers which handle
> > the policy engine on their own and only allow "unsolicited" requests
> > like "swap role" and "enter/exit mode", but also because this
> > framework must work smoothly on systems that don't want to use USB PD
> > and of course also with USB Type-C PHYs that simply don't include USB
> > PD transceiver.
> > 
> > The layer that joins these two parts together will be the port drivers
> > themselves, so the USB Type-C/PD PHYs and controllers, at least in the
> > beginning.
> > 
> > Any initial decisions about which role or which alternate mode to
> > select belongs to the stack. The userspace will need to be notified,
> > and the userspace can then attempt to request changes after that, but
> > if there is something that blocks the requests, the attempt has to
> > just fail. So we can't provide any knowledge for the userspace about
> > the requirements regarding the high level operations we allow the
> > userspace to request (so in practice swap role/power and enter/exit
> > mode). This information the userspace needs to get from somewhere else
> > just live without it. Nor do we expect the userspace to be aware of
> > the state of the system. So for example if the userspace attempts to
> > activate a mode, the framework will just pass it forward to the port
> > driver, which will then process it with the PD stack, and if for
> > example the state is not PE_SRC_Ready or PE_SNK_Ready or whatever, the
> > operation will just fail probable with -EBUSY in that case.
> > 
> Makes sense.
> 
> > And the knowledge about dependencies related to the alternate modes
> > belong primarily to the alternate mode specific drivers in the end
> > (once we have the bus) like I said. We can't expect the USB PD stack
> > to be aware of those as they are alternate mode specific, and of
> > course we can not trust that the userspace will always do things the
> > right way. But the initial states after connection must be handle by
> > the policy engine of course.
> > 
> > > On top of that, I am concerned about synchronization problems with role
> > > changes triggered from user space. The driver is not told about the
> > > desired role, only that it shall perform a role change. If a role change
> > > triggered by the partner is ongoing at the time a role change request is
> > > made from user space, it may well happen that the dual role change results
> > > in a revert to the original role. Some synchronization primitives as well
> > > as an API change might resolve that, but essentially it would mean that
> > > drivers have to implement at least part of the policy engine. It might
> > > make more sense to have that code in the infrastructure.
> > 
> > Well, like I said above, this framework can not provide this
> > infrastructure. The framework at this level really has to be "stupid".
> 
> Ok.
> 
> > We simply can not do any decisions or have any expectations at this
> > level. If more infra is needed, it has to be provided in an other layer
> > on top of this bottom layer. So basically the driver have to implement
> > those things for now.
> > 
> I still think that the lack of synchronization is inherently racy.
> For example, on a power role swap request, port->pwr_role can change
> (via the currently missing notification) after it was evaluated in
> current_power_role_store(), but before port->cap->pr_swap() is called.
> The lower level code has at this point no means to know which power role
> change was requested, which may result in the power role being swapped
> again even though it already is in the requested state.

You are correct. We need to pass the requested role to the drivers.

> > > On disconnect, a port reverts to the default role. However, on port
> > > registration, the port roles are left at 0, meaning they always
> > > default to device/sink independent of the port's capabilities. Is this
> > > on purpose ?
> > 
> > On disconnect, the port role is set according to the "fixed_role"? If
> > the port is DFP only, then the port will still be host/source after
> > disconnect. I don't see the problem here?
> > 
> Roles are set differently on port registration vs. disconnect.
> This means that roles (can) differ between "initial disconnect state"
> and "disconnect state after connect". On port registration, usb_role,
> pwr_role, and vconn_role are all set based on the current pwr_opmode.
> During port registration, they are all initialized with 0.

Got it. I'll fix that.

> > > current_data_role_store() lets user space set a fixed port role. However,
> > > the port role variables are not updated. How is this supposed to be handled ?
> > > The same is true for other role change attributes - I don't see any code
> > > to update the role variables. Presumably this would have to be done in the
> > > class code, since the port data structure is private.
> > 
> > This is a bug in the code indeed.
> > 
> > > Overall, I am quite concerned by the lack of synchronization primitives
> > > between the class code and port drivers, but also in the class code
> > > itself. For example, nothing prevents multiple user space processes
> > > from writing into the same (or different) attribute(s) repeatedly.
> > 
> > We clearly need consensus on what this class will be responsible of.
> > I've tried to explain how I see it above, hopefully with reasonable
> > explanations. So basically, USB PD for this class is just a external
> > feature that the alternate modes and power and vconn swapping depends
> > on, that the class can not take any responsibility of IMHO.
> > 
> Sounds good to me, as long as the lower level code can inform the class
> about state/role changes.
> 
> > The UCSI spec defines an other layer on top of the USB PD stack that
> > basically describes what the userspace interface that I'm trying to
> > achieve with this class is. The "OS Policy".
> > 
> Since you mention UCSI - in UCSI, the two API functions available to set
> the power role (Set Power Direction Mode, Set Power Direction Mode)
> both provide the desired role to the connector driver. How does this map
> to the API in the class code, where a power swap is requested without
> telling the low level code about the desired role ?
> 
> Personally I prefer the approach used in UCSI, or let's say what I perceive
> the approach to be: Maybe the class code should just send a request to the
> connector driver to change the role to X, independent of the current role.
> This way, most if not all synchronization problems could be handled by the
> lower level driver.

That sounds good to me. And it fits to the plan that the class does
not make any decisions.

> On a side note, I don't see how to request a vconn swap with UCSI.
> Is that supported ?

No, with UCSI we can not support vconn swap. Originally I was not even
proposing an attribute for vconn swap in v1, but there was demand for
it.


Thanks,

-- 
heikki

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

#1404491

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> Like I've told some of you guys, I'm trying to implement a bus for
> the Alternate Modes, but I'm still nowhere near finished with that
> one, so let's just get the class ready now. The altmode bus should in
> any case not affect the userspace interface proposed in this patch.

Is this strictly divorced from USB PD?
How do you trigger a cable reset or a USB PD reset?

	Regards
		Oliver

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

#1405203

Hi Oliver,

On Fri, May 20, 2016 at 04:19:59PM +0200, Oliver Neukum wrote:
> On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > Like I've told some of you guys, I'm trying to implement a bus for
> > the Alternate Modes, but I'm still nowhere near finished with that
> > one, so let's just get the class ready now. The altmode bus should in
> > any case not affect the userspace interface proposed in this patch.
> 
> Is this strictly divorced from USB PD?

The bus can not be tied to the USB PD stack we will have in the
kernel completely, or there is no change of using it with things like
UCSI. It's going to be difficult to achieve that in any case as we
simply won't be able to send and rescieve the VDMs with things like
UCSI, but let's see.

> How do you trigger a cable reset or a USB PD reset?

There needs to be an API, but I'm sure that's not going to be a
problem. The bus and the altmode specific drivers will reside inside
kernel.

But I'm getting the sense that you are thinking about having some
responsibility of USB PD in userspace. Please correct me if I'm wrong.
I don't think it will be possible. I think the role of userspace can
only be the source for high level requests via this interface, like
enter/exit mode and swap role, and receiving the status and details of
the ports, but any knowledge about the requirements regarding those
steps belongs to the kernel. This includes also the knowledge about
stuff like mode dependencies, for example if cable plug has to be in a
certain mode in order for the partner to be able to enter some
specific mode, etc.


Thanks.

-- 
heikki

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

#1405273

On Mon, 2016-05-23 at 12:57 +0300, Heikki Krogerus wrote:
> Hi Oliver,
> 
> On Fri, May 20, 2016 at 04:19:59PM +0200, Oliver Neukum wrote:
> > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > Like I've told some of you guys, I'm trying to implement a bus for
> > > the Alternate Modes, but I'm still nowhere near finished with that
> > > one, so let's just get the class ready now. The altmode bus should in
> > > any case not affect the userspace interface proposed in this patch.
> > 
> > Is this strictly divorced from USB PD?
> 
> The bus can not be tied to the USB PD stack we will have in the
> kernel completely, or there is no change of using it with things like
> UCSI. It's going to be difficult to achieve that in any case as we
> simply won't be able to send and rescieve the VDMs with things like
> UCSI, but let's see.
> 
> > How do you trigger a cable reset or a USB PD reset?
> 
> There needs to be an API, but I'm sure that's not going to be a
> problem. The bus and the altmode specific drivers will reside inside
> kernel.

Absolutely. But at some point we need to settle on an API.
If I am to tell you whether your proposed API leaves out
something that needs to be covered, I need to know what
is to go into the other APIs.

A reset is a generic function, so it does not belong to specific
drivers.

> But I'm getting the sense that you are thinking about having some
> responsibility of USB PD in userspace. Please correct me if I'm wrong.

Gods help us all if we are ready to do that.
It would fail.
Yet I think the idea that PD and Alternate Modes can be cleanly
divorced is wrong. The selection of Alternate Modes is done by
USB PD messages. We can encapsulate that, but we cannot leave it out,
especially in the area of resets.

> I don't think it will be possible. I think the role of userspace can
> only be the source for high level requests via this interface, like
> enter/exit mode and swap role, and receiving the status and details of
> the ports, but any knowledge about the requirements regarding those
> steps belongs to the kernel. This includes also the knowledge about

Yes.

> stuff like mode dependencies, for example if cable plug has to be in a
> certain mode in order for the partner to be able to enter some
> specific mode, etc.

Yes.

So for Alternate Modes we need on a high level the following features

1. discovery of available Alternate Modes
2. selection of an Alternate Mode
3. notification about entering an Alternate Mode
4. triggering a reset
5. notification about resets

6. discovery about the current role
7. switching roles
8. setting preferred roles (Try.SRC and Try.SNK)

You covered 1. and 2.
3. can be covered by specific drivers
4. and 5. are not covered (and it makes no sense to tie it
to specific drivers)

6. and 7. is covered
8. is not

And 8. needs to be covered. It affects who selects the Alternate Mode.
You cannot tie it to USB and it doesn't fit with pure PD stuff.

I like your API as it is now. But it is incomplete.

	Regards
		Oliver

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

#1405522

On Mon, May 23, 2016 at 01:25:19PM +0200, Oliver Neukum wrote:
> On Mon, 2016-05-23 at 12:57 +0300, Heikki Krogerus wrote:
> > Hi Oliver,
> > 
> > On Fri, May 20, 2016 at 04:19:59PM +0200, Oliver Neukum wrote:
> > > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > > Like I've told some of you guys, I'm trying to implement a bus for
> > > > the Alternate Modes, but I'm still nowhere near finished with that
> > > > one, so let's just get the class ready now. The altmode bus should in
> > > > any case not affect the userspace interface proposed in this patch.
> > > 
> > > Is this strictly divorced from USB PD?
> > 
> > The bus can not be tied to the USB PD stack we will have in the
> > kernel completely, or there is no change of using it with things like
> > UCSI. It's going to be difficult to achieve that in any case as we
> > simply won't be able to send and rescieve the VDMs with things like
> > UCSI, but let's see.
> > 
> > > How do you trigger a cable reset or a USB PD reset?
> > 
> > There needs to be an API, but I'm sure that's not going to be a
> > problem. The bus and the altmode specific drivers will reside inside
> > kernel.
> 
> Absolutely. But at some point we need to settle on an API.
> If I am to tell you whether your proposed API leaves out
> something that needs to be covered, I need to know what
> is to go into the other APIs.
> 
> A reset is a generic function, so it does not belong to specific
> drivers.
> 
A would expect the driver to execute the reset.

Maybe the question should be phrased differently: Even USCI (which
doesn't provide for everything) has commands to reset the policy
manager and to reset the connector. The class should provide a means
to execute those commands.

> > But I'm getting the sense that you are thinking about having some
> > responsibility of USB PD in userspace. Please correct me if I'm wrong.
> 
> Gods help us all if we are ready to do that.
> It would fail.
> Yet I think the idea that PD and Alternate Modes can be cleanly
> divorced is wrong. The selection of Alternate Modes is done by
> USB PD messages. We can encapsulate that, but we cannot leave it out,
> especially in the area of resets.
> 
> > I don't think it will be possible. I think the role of userspace can
> > only be the source for high level requests via this interface, like
> > enter/exit mode and swap role, and receiving the status and details of
> > the ports, but any knowledge about the requirements regarding those
> > steps belongs to the kernel. This includes also the knowledge about
> 
> Yes.
> 
> > stuff like mode dependencies, for example if cable plug has to be in a
> > certain mode in order for the partner to be able to enter some
> > specific mode, etc.
> 
> Yes.
> 
> So for Alternate Modes we need on a high level the following features
> 
> 1. discovery of available Alternate Modes
> 2. selection of an Alternate Mode
> 3. notification about entering an Alternate Mode
> 4. triggering a reset
> 5. notification about resets
> 
> 6. discovery about the current role
> 7. switching roles
> 8. setting preferred roles (Try.SRC and Try.SNK)
> 

Isn't reset and role handling orthogonal to alternate mode functionality ?
Both will still be needed even if alternate mode support is not implemented
at all.

> You covered 1. and 2.
> 3. can be covered by specific drivers
> 4. and 5. are not covered (and it makes no sense to tie it
> to specific drivers)
> 
> 6. and 7. is covered
> 8. is not
> 
> And 8. needs to be covered. It affects who selects the Alternate Mode.

Doesn't the actual role determine that ? A device which prefers to be
a DFP might still end up as UFP.

> You cannot tie it to USB and it doesn't fit with pure PD stuff.
> 
> I like your API as it is now. But it is incomplete.
> 

Same here.

Thanks,
Guenter

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

#1405961

On Mon, 2016-05-23 at 10:09 -0700, Guenter Roeck wrote:
> On Mon, May 23, 2016 at 01:25:19PM +0200, Oliver Neukum wrote:
> > On Mon, 2016-05-23 at 12:57 +0300, Heikki Krogerus wrote:
> >
> > A reset is a generic function, so it does not belong to specific
> > drivers.
> > 
> A would expect the driver to execute the reset.
> 
> Maybe the question should be phrased differently: Even USCI (which
> doesn't provide for everything) has commands to reset the policy
> manager and to reset the connector. The class should provide a means
> to execute those commands.

Yes.

> > So for Alternate Modes we need on a high level the following features
> > 
> > 1. discovery of available Alternate Modes
> > 2. selection of an Alternate Mode
> > 3. notification about entering an Alternate Mode
> > 4. triggering a reset
> > 5. notification about resets
> > 
> > 6. discovery about the current role
> > 7. switching roles
> > 8. setting preferred roles (Try.SRC and Try.SNK)
> > 
> 
> Isn't reset and role handling orthogonal to alternate mode functionality ?
> Both will still be needed even if alternate mode support is not implemented
> at all.

In part. A reset can cause the Alternate Mode to be left unexpectedly
and unintentionally.
So how many APIs do we want?
Three:

- Alternate Modes
- USB PD
- type C for roles and reset

Or another number?

> > I like your API as it is now. But it is incomplete.
> > 
> 
> Same here.

So what is to be done?

	Regards
		Oliver

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

#1405999

On Mon, May 23, 2016 at 01:25:19PM +0200, Oliver Neukum wrote:
> So for Alternate Modes we need on a high level the following features
> 
> 1. discovery of available Alternate Modes
> 2. selection of an Alternate Mode
> 3. notification about entering an Alternate Mode
> 4. triggering a reset
> 5. notification about resets
> 
> 6. discovery about the current role
> 7. switching roles
> 8. setting preferred roles (Try.SRC and Try.SNK)
> 
> You covered 1. and 2.
> 3. can be covered by specific drivers
> 4. and 5. are not covered (and it makes no sense to tie it
> to specific drivers)
> 
> 6. and 7. is covered
> 8. is not
> 
> And 8. needs to be covered. It affects who selects the Alternate Mode.
> You cannot tie it to USB and it doesn't fit with pure PD stuff.
> 
> I like your API as it is now. But it is incomplete.

OK, Got it.


Thanks Oliver,

-- 
heikki

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

#1406128

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:

Hi,

as this discussion seems to go in circles, I am starting anew
at the top.

> Like I've told some of you guys, I'm trying to implement a bus for
> the Alternate Modes, but I'm still nowhere near finished with that
> one, so let's just get the class ready now. The altmode bus should in
> any case not affect the userspace interface proposed in this patch.
> 
> As you can see, the Alternate Modes are handled completely differently
> compared to the original proposal. Every Alternate Mode will have
> their own device instance (which will be then later bound to an
> Alternate Mode specific driver once we have the bus), but also every
> partner, cable and cable plug will have their own device instances
> representing them.

The API works for a DFP. I fail to see how the UFP learns about entering
an alternate mode.
Secondly, support to trigger a reset is missing

> An other change is that the data role is now handled in two ways.
> The current_data_role file will represent static mode of the port, and
> it will use the names for the roles as they are defined in the spec:
> DFP, UFP and DRP. This file should be used if the port needs to be

Good, but support for Try.SRC and Try.SNK is missing.
An additional problem with that is that it needs to work
without user space during boot. So I think module parameters
to set the default are necessary.

> fixed to one specific role with DRP ports. So this approach will
> replace the suggestions for "preferred" data role we had. The
> current_usb_data_role will use values "host" and "device" and it will
> be used for data role swapping when already connected.
> 
> The tree of devices that will be populated when the cable is active
> and when the cable has controller on both plug, will look as
> following:
> 
> usbc0
> |- usbc0-cable
> |   |- usbc0-plug0
> |   |   |- usbc0-plug.svid:xxx
> |   |   |   |-mode0
> |   |   |   |   |- vdo
> |   |   |   |   |- desc
> |   |   |   |   |- active
> ...
> |   |- usbc0-plug1
> |   |   |-usbc0-partner
> |   |   |   |- usbc0-partner.svid:xxxx
> |   |   |   |	|-mode0
> |   |   |   |	|   |- vdo
> |   |   |   |	|   |- desc
> |   |   |   |	|   |- active
> |   |   |   |	|-mode1
> ...
> |   |   |- usbc0-plug1.svid:xxx
> |   |   |   |-mode0
> |   |   |   |   |- vdo
> ...
> 
> If there is no active cable, the partner will be directly attached to
> the port, but symlink to the partner is now always added to the port
> folder in any case. I'm not sure about this approach. There is a
> question about it in the code. Please check it.

This approach looks workable.
An the whole the approach looks good, but needs to be extended.

	Regards
		Oliver

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

#1406848

Hi,

On Tue, May 24, 2016 at 02:51:40PM +0200, Oliver Neukum wrote:
> On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> 
> Hi,
> 
> as this discussion seems to go in circles, I am starting anew
> at the top.
> 
> > Like I've told some of you guys, I'm trying to implement a bus for
> > the Alternate Modes, but I'm still nowhere near finished with that
> > one, so let's just get the class ready now. The altmode bus should in
> > any case not affect the userspace interface proposed in this patch.
> > 
> > As you can see, the Alternate Modes are handled completely differently
> > compared to the original proposal. Every Alternate Mode will have
> > their own device instance (which will be then later bound to an
> > Alternate Mode specific driver once we have the bus), but also every
> > partner, cable and cable plug will have their own device instances
> > representing them.
> 
> The API works for a DFP. I fail to see how the UFP learns about entering
> an alternate mode.
> Secondly, support to trigger a reset is missing

I'm fine with adding an attribute for port and cable resets if it's
something that is needed. So do you want to be able to execute hard
reset on a port?

But could you please explain the case(s) where you need to tricker a
reset.

> > An other change is that the data role is now handled in two ways.
> > The current_data_role file will represent static mode of the port, and
> > it will use the names for the roles as they are defined in the spec:
> > DFP, UFP and DRP. This file should be used if the port needs to be
> 
> Good, but support for Try.SRC and Try.SNK is missing.

OK, but what is Try.SNK? It's not in the specs?

> An additional problem with that is that it needs to work
> without user space during boot. So I think module parameters
> to set the default are necessary.

I don't have a problem with that, but what does Greg say?

> > fixed to one specific role with DRP ports. So this approach will
> > replace the suggestions for "preferred" data role we had. The
> > current_usb_data_role will use values "host" and "device" and it will
> > be used for data role swapping when already connected.
> > 
> > The tree of devices that will be populated when the cable is active
> > and when the cable has controller on both plug, will look as
> > following:
> > 
> > usbc0
> > |- usbc0-cable
> > |   |- usbc0-plug0
> > |   |   |- usbc0-plug.svid:xxx
> > |   |   |   |-mode0
> > |   |   |   |   |- vdo
> > |   |   |   |   |- desc
> > |   |   |   |   |- active
> > ...
> > |   |- usbc0-plug1
> > |   |   |-usbc0-partner
> > |   |   |   |- usbc0-partner.svid:xxxx
> > |   |   |   |	|-mode0
> > |   |   |   |	|   |- vdo
> > |   |   |   |	|   |- desc
> > |   |   |   |	|   |- active
> > |   |   |   |	|-mode1
> > ...
> > |   |   |- usbc0-plug1.svid:xxx
> > |   |   |   |-mode0
> > |   |   |   |   |- vdo
> > ...
> > 
> > If there is no active cable, the partner will be directly attached to
> > the port, but symlink to the partner is now always added to the port
> > folder in any case. I'm not sure about this approach. There is a
> > question about it in the code. Please check it.
> 
> This approach looks workable.
> An the whole the approach looks good, but needs to be extended.


Thanks,

-- 
heikki

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

#1407006

On Wed, May 25, 2016 at 02:28:46PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, May 24, 2016 at 02:51:40PM +0200, Oliver Neukum wrote:
> > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > 
> > Hi,
> > 
> > as this discussion seems to go in circles, I am starting anew
> > at the top.
> > 
> > > Like I've told some of you guys, I'm trying to implement a bus for
> > > the Alternate Modes, but I'm still nowhere near finished with that
> > > one, so let's just get the class ready now. The altmode bus should in
> > > any case not affect the userspace interface proposed in this patch.
> > > 
> > > As you can see, the Alternate Modes are handled completely differently
> > > compared to the original proposal. Every Alternate Mode will have
> > > their own device instance (which will be then later bound to an
> > > Alternate Mode specific driver once we have the bus), but also every
> > > partner, cable and cable plug will have their own device instances
> > > representing them.
> > 
> > The API works for a DFP. I fail to see how the UFP learns about entering
> > an alternate mode.
> > Secondly, support to trigger a reset is missing
> 
> I'm fine with adding an attribute for port and cable resets if it's
> something that is needed. So do you want to be able to execute hard
> reset on a port?
> 
> But could you please explain the case(s) where you need to tricker a
> reset.
> 
> > > An other change is that the data role is now handled in two ways.
> > > The current_data_role file will represent static mode of the port, and
> > > it will use the names for the roles as they are defined in the spec:
> > > DFP, UFP and DRP. This file should be used if the port needs to be
> > 
> > Good, but support for Try.SRC and Try.SNK is missing.
> 
> OK, but what is Try.SNK? It's not in the specs?
> 

It is not in the USB PD specification, but in "USB Type-C Specification
Release 1.2 - Cable and Connector Specification".

Section 4.5.2.2.11, "Try.SNK State", says

"Note: if both Try.SRC and Try.SNK mechanisms are implemented, only one
 shall be enabled by the port at any given time. Deciding which of these
 two mechanisms is enabled is product design-specific."

... which is why I suggested earlier that there should be a platform
parameter to enable one or the other.

Guenter

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

#1407872

On Wed, May 25, 2016 at 08:19:47AM -0700, Guenter Roeck wrote:
> On Wed, May 25, 2016 at 02:28:46PM +0300, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Tue, May 24, 2016 at 02:51:40PM +0200, Oliver Neukum wrote:
> > > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > 
> > > Hi,
> > > 
> > > as this discussion seems to go in circles, I am starting anew
> > > at the top.
> > > 
> > > > Like I've told some of you guys, I'm trying to implement a bus for
> > > > the Alternate Modes, but I'm still nowhere near finished with that
> > > > one, so let's just get the class ready now. The altmode bus should in
> > > > any case not affect the userspace interface proposed in this patch.
> > > > 
> > > > As you can see, the Alternate Modes are handled completely differently
> > > > compared to the original proposal. Every Alternate Mode will have
> > > > their own device instance (which will be then later bound to an
> > > > Alternate Mode specific driver once we have the bus), but also every
> > > > partner, cable and cable plug will have their own device instances
> > > > representing them.
> > > 
> > > The API works for a DFP. I fail to see how the UFP learns about entering
> > > an alternate mode.
> > > Secondly, support to trigger a reset is missing
> > 
> > I'm fine with adding an attribute for port and cable resets if it's
> > something that is needed. So do you want to be able to execute hard
> > reset on a port?
> > 
> > But could you please explain the case(s) where you need to tricker a
> > reset.
> > 
> > > > An other change is that the data role is now handled in two ways.
> > > > The current_data_role file will represent static mode of the port, and
> > > > it will use the names for the roles as they are defined in the spec:
> > > > DFP, UFP and DRP. This file should be used if the port needs to be
> > > 
> > > Good, but support for Try.SRC and Try.SNK is missing.
> > 
> > OK, but what is Try.SNK? It's not in the specs?
> > 
> 
> It is not in the USB PD specification, but in "USB Type-C Specification
> Release 1.2 - Cable and Connector Specification".
> 
> Section 4.5.2.2.11, "Try.SNK State", says
> 
> "Note: if both Try.SRC and Try.SNK mechanisms are implemented, only one
>  shall be enabled by the port at any given time. Deciding which of these
>  two mechanisms is enabled is product design-specific."
> 
> ... which is why I suggested earlier that there should be a platform
> parameter to enable one or the other.

Heh, I'm still living the year 2015 :). I've missed the Type-C 1.2
spec.


Thanks guys,

-- 
heikki

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

#1406190

On 05/19/2016 05:44 AM, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface for user
> space to get the status and basic information about USB Type-C
> Connectors in the system, control data role swapping, and when USB PD
> is available, also power role swapping and Alternate Modes.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>   drivers/usb/Kconfig         |   2 +
>   drivers/usb/Makefile        |   2 +
>   drivers/usb/type-c/Kconfig  |   7 +
>   drivers/usb/type-c/Makefile |   1 +
>   drivers/usb/type-c/typec.c  | 957 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/usb/typec.h   | 230 +++++++++++
>   6 files changed, 1199 insertions(+)
>   create mode 100644 drivers/usb/type-c/Kconfig
>   create mode 100644 drivers/usb/type-c/Makefile
>   create mode 100644 drivers/usb/type-c/typec.c
>   create mode 100644 include/linux/usb/typec.h
>

Hi,

> +/*
> + * struct typec_capability - USB Type-C Port Capabilities
> + * @role: DFP (Host-only), UFP (Device-only) or DRP (Dual Role)
> + * @usb_pd: USB Power Delivery support
> + * @alt_modes: Alternate Modes the connector supports (null terminated)
> + * @audio_accessory: Audio Accessory Adapter Mode support
> + * @debug_accessory: Debug Accessory Mode support
> + * @fix_role: Set a fixed data role for DRP port
> + * @dr_swap: Data Role Swap support
> + * @pr_swap: Power Role Swap support
> + * @vconn_swap: VCONN Swap support
> + * @activate_mode: Enter/exit given Alternate Mode
> + *
> + * Static capabilities of a single USB Type-C port.
> + */
> +struct typec_capability {
> +	enum typec_data_role	role;
> +	unsigned int		usb_pd:1;
> +	struct typec_altmode	*alt_modes;
> +	unsigned int		audio_accessory:1;
> +	unsigned int		debug_accessory:1;
> +
> +	int			(*fix_role)(struct typec_port *,
> +					    enum typec_data_role);
> +
> +	int			(*dr_swap)(struct typec_port *);
> +	int			(*pr_swap)(struct typec_port *);
> +	int			(*vconn_swap)(struct typec_port *);
> +

The function parameter in those calls is all but useless to the caller.
It needs to store the typec_port returned from typec_register(), create a
list of ports, and then search through this list each time one of the
functions is called. This is quite expensive for no good reason.

Previously, with typec_port exported, the called code could use the stored
caps pointer to map to its internal data structures. This is no longer
possible.

I think it would be useful to provide a better means for the called function
to identify its context. Maybe provide a pointer to the private data in
the registration function and use it as parameter in the callback functions ?

Thanks,
Guenter

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

#1406852

Hi,

On Tue, May 24, 2016 at 06:42:09AM -0700, Guenter Roeck wrote:
> > +struct typec_capability {
> > +	enum typec_data_role	role;
> > +	unsigned int		usb_pd:1;
> > +	struct typec_altmode	*alt_modes;
> > +	unsigned int		audio_accessory:1;
> > +	unsigned int		debug_accessory:1;
> > +
> > +	int			(*fix_role)(struct typec_port *,
> > +					    enum typec_data_role);
> > +
> > +	int			(*dr_swap)(struct typec_port *);
> > +	int			(*pr_swap)(struct typec_port *);
> > +	int			(*vconn_swap)(struct typec_port *);
> > +
> 
> The function parameter in those calls is all but useless to the caller.
> It needs to store the typec_port returned from typec_register(), create a
> list of ports, and then search through this list each time one of the
> functions is called. This is quite expensive for no good reason.
> 
> Previously, with typec_port exported, the called code could use the stored
> caps pointer to map to its internal data structures. This is no longer
> possible.

True, the API now is in practice broken.

> I think it would be useful to provide a better means for the called function
> to identify its context. Maybe provide a pointer to the private data in
> the registration function and use it as parameter in the callback functions ?

Sounds reasonable.


Thanks,

-- 
heikki

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

#1406893

On 05/25/2016 04:30 AM, Heikki Krogerus wrote:
> Hi,
>
> On Tue, May 24, 2016 at 06:42:09AM -0700, Guenter Roeck wrote:
>>> +struct typec_capability {
>>> +	enum typec_data_role	role;
>>> +	unsigned int		usb_pd:1;
>>> +	struct typec_altmode	*alt_modes;
>>> +	unsigned int		audio_accessory:1;
>>> +	unsigned int		debug_accessory:1;
>>> +
>>> +	int			(*fix_role)(struct typec_port *,
>>> +					    enum typec_data_role);
>>> +
>>> +	int			(*dr_swap)(struct typec_port *);
>>> +	int			(*pr_swap)(struct typec_port *);
>>> +	int			(*vconn_swap)(struct typec_port *);
>>> +
>>
>> The function parameter in those calls is all but useless to the caller.
>> It needs to store the typec_port returned from typec_register(), create a
>> list of ports, and then search through this list each time one of the
>> functions is called. This is quite expensive for no good reason.
>>
>> Previously, with typec_port exported, the called code could use the stored
>> caps pointer to map to its internal data structures. This is no longer
>> possible.
>
> True, the API now is in practice broken.
>
>> I think it would be useful to provide a better means for the called function
>> to identify its context. Maybe provide a pointer to the private data in
>> the registration function and use it as parameter in the callback functions ?
>
> Sounds reasonable.
>

I'll send a follow-up patch hopefully later today which fixes all the problems
I have found so far.

Guenter

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

#1406407

On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface for user
> space to get the status and basic information about USB Type-C
> Connectors in the system, control data role swapping, and when USB PD
> is available, also power role swapping and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

[ ... ]

> +
> +static void typec_remove_partner(struct typec_port *port)
> +{
> +	sysfs_remove_link(&port->dev.kobj, "partner");
> +	typec_unregister_altmodes(port->partner->alt_modes);

This only unregisters alternate modes registered through typec_add_partner(),
but not alternate modes registered separately. Or is the calling code expected
to set port->partner->alt_modes when calling typec_register_altmodes()
directly ?

[ ... ]

> +
> +void typec_unregister_altmodes(struct typec_altmode *alt_modes)
> +{
> +	struct typec_altmode *alt;
> +
This will crash if alt_modes is NULL, which will happen if 
partner->alt_modes is NULL at connection time. Semantically
this is different to typec_register_altmodes(), which does
have a NULL check.

Guenter

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

#1406857

On Tue, May 24, 2016 at 12:28:26PM -0700, Guenter Roeck wrote:
> On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > The purpose of this class is to provide unified interface for user
> > space to get the status and basic information about USB Type-C
> > Connectors in the system, control data role swapping, and when USB PD
> > is available, also power role swapping and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> [ ... ]
> 
> > +
> > +static void typec_remove_partner(struct typec_port *port)
> > +{
> > +	sysfs_remove_link(&port->dev.kobj, "partner");
> > +	typec_unregister_altmodes(port->partner->alt_modes);
> 
> This only unregisters alternate modes registered through typec_add_partner(),
> but not alternate modes registered separately. Or is the calling code expected
> to set port->partner->alt_modes when calling typec_register_altmodes()
> directly ?

The altmodes for the partner are not meant to be registered
separately. With the partners and also cable plugs the class is in
control of registering and unregistering of the altmode devices after
typec_connect() is called.

The idea was that only the ports will register the alternate modes
they support separately, but I think we have to change that too. So I
don't think we'll export the typec_un/register_altmodes() at all.

We will have to prevent any drivers from being bound to the port
alternate mode devices when we add the alternate mode bus, and I had
some idea where by making the port drivers themselves in charge of
registering the port alternate modes, we could prevent it easily. But
it's probable easier to just handle those in the class driver as well.

> > +
> > +void typec_unregister_altmodes(struct typec_altmode *alt_modes)
> > +{
> > +	struct typec_altmode *alt;
> > +
> This will crash if alt_modes is NULL, which will happen if 
> partner->alt_modes is NULL at connection time. Semantically
> this is different to typec_register_altmodes(), which does
> have a NULL check.

Yes, need to fix that.


Thanks Guenter,

-- 
heikki

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

#1406896

On 05/25/2016 04:51 AM, Heikki Krogerus wrote:
> On Tue, May 24, 2016 at 12:28:26PM -0700, Guenter Roeck wrote:
>> On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
>>> The purpose of this class is to provide unified interface for user
>>> space to get the status and basic information about USB Type-C
>>> Connectors in the system, control data role swapping, and when USB PD
>>> is available, also power role swapping and Alternate Modes.
>>>
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> [ ... ]
>>
>>> +
>>> +static void typec_remove_partner(struct typec_port *port)
>>> +{
>>> +	sysfs_remove_link(&port->dev.kobj, "partner");
>>> +	typec_unregister_altmodes(port->partner->alt_modes);
>>
>> This only unregisters alternate modes registered through typec_add_partner(),
>> but not alternate modes registered separately. Or is the calling code expected
>> to set port->partner->alt_modes when calling typec_register_altmodes()
>> directly ?
>
> The altmodes for the partner are not meant to be registered
> separately. With the partners and also cable plugs the class is in
> control of registering and unregistering of the altmode devices after
> typec_connect() is called.
>
> The idea was that only the ports will register the alternate modes
> they support separately, but I think we have to change that too. So I
> don't think we'll export the typec_un/register_altmodes() at all.
>
> We will have to prevent any drivers from being bound to the port
> alternate mode devices when we add the alternate mode bus, and I had
> some idea where by making the port drivers themselves in charge of
> registering the port alternate modes, we could prevent it easily. But
> it's probable easier to just handle those in the class driver as well.
>

Alternate mode discovery is an orthogonal process to the connection
state machine, and may take a while to complete. Are you saying
that the call to typec_connect() should be delayed until after
alternate mode discovery completes or times out ?

So far I call typec_connect() in SRC.Ready and SNK.Ready, and
typec_register_altmodes() after mode discovery is complete.
It is also orthogonal, meaning it is only called if and when alternate
mode discovery completes, and the alternate mode discovery state machine
is separate to the port state machine.

No problem for me to change that, just making sure that the registration
delay is understood and accepted.

Thanks,
Guenter

>>> +
>>> +void typec_unregister_altmodes(struct typec_altmode *alt_modes)
>>> +{
>>> +	struct typec_altmode *alt;
>>> +
>> This will crash if alt_modes is NULL, which will happen if
>> partner->alt_modes is NULL at connection time. Semantically
>> this is different to typec_register_altmodes(), which does
>> have a NULL check.
>
> Yes, need to fix that.
>
>
> Thanks Guenter,
>

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

#1406928

On Wed, May 25, 2016 at 06:21:54AM -0700, Guenter Roeck wrote:
> On 05/25/2016 04:51 AM, Heikki Krogerus wrote:
> > On Tue, May 24, 2016 at 12:28:26PM -0700, Guenter Roeck wrote:
> > > On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > > > The purpose of this class is to provide unified interface for user
> > > > space to get the status and basic information about USB Type-C
> > > > Connectors in the system, control data role swapping, and when USB PD
> > > > is available, also power role swapping and Alternate Modes.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > [ ... ]
> > > 
> > > > +
> > > > +static void typec_remove_partner(struct typec_port *port)
> > > > +{
> > > > +	sysfs_remove_link(&port->dev.kobj, "partner");
> > > > +	typec_unregister_altmodes(port->partner->alt_modes);
> > > 
> > > This only unregisters alternate modes registered through typec_add_partner(),
> > > but not alternate modes registered separately. Or is the calling code expected
> > > to set port->partner->alt_modes when calling typec_register_altmodes()
> > > directly ?
> > 
> > The altmodes for the partner are not meant to be registered
> > separately. With the partners and also cable plugs the class is in
> > control of registering and unregistering of the altmode devices after
> > typec_connect() is called.
> > 
> > The idea was that only the ports will register the alternate modes
> > they support separately, but I think we have to change that too. So I
> > don't think we'll export the typec_un/register_altmodes() at all.
> > 
> > We will have to prevent any drivers from being bound to the port
> > alternate mode devices when we add the alternate mode bus, and I had
> > some idea where by making the port drivers themselves in charge of
> > registering the port alternate modes, we could prevent it easily. But
> > it's probable easier to just handle those in the class driver as well.
> > 
> 
> Alternate mode discovery is an orthogonal process to the connection
> state machine, and may take a while to complete. Are you saying
> that the call to typec_connect() should be delayed until after
> alternate mode discovery completes or times out ?
> 
> So far I call typec_connect() in SRC.Ready and SNK.Ready, and
> typec_register_altmodes() after mode discovery is complete.
> It is also orthogonal, meaning it is only called if and when alternate
> mode discovery completes, and the alternate mode discovery state machine
> is separate to the port state machine.
> 
> No problem for me to change that, just making sure that the registration
> delay is understood and accepted.

I'm not against leaving the responsibility of registering the alternate
modes to the drivers. I'm a little bit worried about relying then on
the drivers to also handle the unregistering accordingly, but I can
live with that. But we just shouldn't share the responsibility of
un/registering them between the class and the drivers, so the driver
should then handle the registration always.

Oliver, what do you think?


Thanks,

-- 
heikki

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web