Message-ID: <20160825115958.GE12117@kuha.fi.intel.com>
Date:   Thu, 25 Aug 2016 14:59:58 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Vincent Palatin <vpalatin@...omium.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Oliver Neukum <oneukum@...e.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Bin Gao <bin.gao@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCHv6 1/3] usb: USB Type-C connector class

Hi,

On Wed, Aug 24, 2016 at 04:08:23PM +0200, Vincent Palatin wrote:
> Sorry if I'm making redundant comments with previous discussions, I
> might have missed a few threads.
> 
> 
> On Mon, Aug 22, 2016 at 2:05 PM, Heikki Krogerus
> <heikki.krogerus@...ux.intel.com> wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  199 +++++
> >  Documentation/usb/typec.txt                 |  103 +++
> >  MAINTAINERS                                 |    9 +
> >  drivers/usb/Kconfig                         |    2 +
> >  drivers/usb/Makefile                        |    2 +
> >  drivers/usb/typec/Kconfig                   |    7 +
> >  drivers/usb/typec/Makefile                  |    1 +
> >  drivers/usb/typec/typec.c                   | 1090 +++++++++++++++++++++++++++
> >  include/linux/usb/typec.h                   |  260 +++++++
> >  9 files changed, 1673 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
> >  create mode 100644 Documentation/usb/typec.txt
> >  create mode 100644 drivers/usb/typec/Kconfig
> >  create mode 100644 drivers/usb/typec/Makefile
> >  create mode 100644 drivers/usb/typec/typec.c
> >  create mode 100644 include/linux/usb/typec.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > new file mode 100644
> > index 0000000..e6179d3
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -0,0 +1,199 @@
> > +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> > +
> > +What:          /sys/class/typec/<port>/current_data_role
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               The current USB data role the port is operating in. This
> > +               attribute can be used for requesting data role swapping on the
> > +               port.
> 
> role swapping is sometimes a long operation. maybe we need to say
> explicitly whether the 'write' is synchronous and returns when the
> swap has succeeded / failed or asynchronous (and requires polling
> current_data_role afterwards to know the result ?)

OK.

> > +
> > +               Valid values:
> > +               - host
> > +               - device
> 
> the USB workgroup has settled for DFP/UFP rather than host/device ?

(I don't think the workgroup has settled on anything.)

I already proposed DFP/UFP, but it did not fly. The naming really does
not need to reflect the spec exactly, especially since there is no
guarantee they will not change the names in future versions of the
spec. "host/device", "source/sink" are completely understandable,
unlike DRP/UFP.

> 
> > +
> > +What:          /sys/class/typec/<port>/current_power_role
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               The current power role of the port. This attribute can be used
> > +               to request power role swap on the port when the port supports
> 
> ditto
> 
> 
> > +               USB Power Delivery.
> > +
> > +               Valid values:
> > +               - source
> > +               - sink
> > +
> > +What:          /sys/class/typec/<port>/current_vconn_role
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows the current VCONN role of the port. This attribute can be
> > +               used to request VCONN role swap on the port when the port
> > +               supports USB Power Delivery.
> > +
> > +               Valid values are:
> > +               - source
> > +               - sink
> 
> 
> either we are currently sourcing vconn or not, but even if you are
> not, you are probably not a vconn sink either (ie only vconn-powered
> accessory are, your usual linux-powered laptop/phone is probably not)

It's not relevant to know whether the vconn is being actually used or
not here. I'm not sure what's your point?

And vconn does not only supply vonn-powered accessories, but
powered cables as well.

> > +
> > +What:          /sys/class/typec/<port>/power_operation_mode
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows the current power operational mode the port is in.
> > +
> > +               Valid values:
> > +               - USB - Normal power levels defined in USB specifications
> > +               - BC1.2 - Power levels defined in Battery Charging Specification
> > +                         v1.2
> > +               - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> > +                                   specification.
> > +               - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> > +                                   specification.
> > +                - USB Power Delivery - The voltages and currents defined in USB
> > +                                      Power Delivery specification
> > +
> > +What:          /sys/class/typec/<port>/preferred_role
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               The user space can notify the driver about the preferred role.
> > +               It should be handled as enabling of Try.SRC or Try.SNK, as
> > +               defined in USB Type-C specification, in the port drivers. By
> > +               default there is no preferred role.
> > +
> > +               Valid values:
> > +               - host
> > +               - device
> > +               - For example "none" to remove preference (anything else except
> > +                 "host" or "device")
> 
> 
> host/device are not really power roles, source/sink are (or SNK/SRC)

Try.SRC/SNK are primarily designed for systems that don't implement
USB PD, and therefore source = host and sink = device. But even when
USB PD is supported, the initial data role will be host in case of
source and device in case of sink.

And as said before, the naming does not need to match that of the spec
here.

> > +
> > +What:          /sys/class/typec/<port>/supported_accessory_modes
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Lists the Accessory Modes, defined in the USB Type-C
> > +               specification, the port supports.
> 
> 
> there aren't many modes defined in the type-C spec (most of them are
> in other specs or proprietary) overall I don't really what modes my
> port supports (and the list might be open-ended, e.g. user space
> implementations), I'm really interested in what the partner port
> supports.

I think you are mixing Accessory Modes and Alternate Modes (most
likely because of the vconn-powered accessory concept). Note that
vconn-powered accessory is actually just a sink that implements an
alternate mode.

You can have vendor specific alternate modes, but the accessory modes
are what the spec defines, so Audio and Debug (and Digital Audio in
the future).

> 
> > +
> > +What:          /sys/class/typec/<port>/supported_data_roles
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Lists the USB data roles the port is capable of supporting.
> > +
> > +               Valid values:
> > +               - device
> > +               - host
> > +               - device, host (DRD as defined in USB Type-C specification v1.2)
> > +
> > +What:          /sys/class/typec/<port>/supported_power_roles
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Lists the power roles the port is capable of supporting.
> > +
> > +               Valid values:
> > +               - source
> > +               - sink
> > +
> > +What:          /sys/class/typec/<port>/supports_usb_power_delivery
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows if the port supports USB Power Delivery.
> > +               - 1 if USB Power Delivery is supported
> > +               - 0 when it's not
> > +
> > +
> > +USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
> > +
> > +What:          /sys/class/typec/<port>-partner/accessory
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               The attribute is visible only when the partner's type is
> > +               "Accessory". The type can be read from its own attribute.
> > +
> > +               Shows the name of the Accessory Mode. The Accessory Modes are
> > +               defined in USB Type-C Specification.
> > +
> > +What:          /sys/class/typec/<port>-partner/type
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows the type of the partner. Can be one of the following:
> > +               - USB - When the partner is normal USB host/peripheral.
> > +               - Charger - When the partner has been identified as dedicated
> > +                           charger.
> > +               - Alternate Mode - When the partner supports Alternate Modes.
> > +               - Accessory - When the partner is one of the accessories with
> > +                             specific Accessory Mode defined in USB Type-C
> > +                             specification.
> 
> 
> where a dock would be classified ?

A dock is just USB PD capable device with a bunch of alternate modes
that is attached to the port. There is no specific identifier for a
"dock".

> > +
> > +USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
> > +
> > +Note: Electronically Marked Cables will have a device also for one cable plug
> > +(eg. /sys/class/typec/usbc0-plug0). If the cable is active and has also SOP
> > +Double Prime controller (USB Power Deliver specification ch. 2.4) it will have
> > +second device also for the other plug. Both plugs may have their alternate modes
> > +as described in USB Type-C and USB Power Delivery specifications.
> > +
> > +What:          /sys/class/typec/<port>-cable/active
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows if the cable is active or passive.
> > +
> > +               Valid values:
> > +               - 0 when the cable is passive
> > +               - 1 when the cable is active
> > +
> > +What:          /sys/class/typec/<port>-cable/plug_type
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows type of the plug on the cable:
> > +               - Type-A - Standard A
> > +               - Type-B - Standard B
> > +               - Type-C - USB Type-C
> > +               - Captive - Non-standard
> > +
> > +
> > +Alternate Mode devices (For example,
> > +/sys/class/typec/usbc0-partner/usbc0-partner.svid:xxxx/). The ports, partners
> > +and cable plugs can have alternate modes.
> > +
> > +What:          /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/active
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows if the mode is active or not. The attribute can be used
> > +               for entering/exiting the mode with partners and cable plugs, and
> > +               with the port alternate modes it can be used for disabling
> > +               support for specific alternate modes.
> > +
> > +What:          /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows description of the mode. The description is optional for
> > +               the drivers, just like with the Billboard Devices.
> > +
> > +What:          /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/vdo
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows the VDO in hexadecimal returned from the Discover Modes
> > +               command.
> > +
> > +What:          /sys/class/typec/<port>/<port>.svid:<svid>/<mode>/supported_roles
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +               Shows the roles, source or sink, the mode is supported with.
> > +
> > +               This attribute is available for the devices describing the
> > +               alternate modes a port supports, and it will not be exposed with
> > +               the devices presenting the alternate modes the partners or cable
> > +               plugs support.
> > diff --git a/Documentation/usb/typec.txt b/Documentation/usb/typec.txt
> > new file mode 100644
> > index 0000000..dce5f07
> > --- /dev/null
> > +++ b/Documentation/usb/typec.txt
> > @@ -0,0 +1,103 @@
> > +USB Type-C connector class
> > +==========================
> > +
> > +Introduction
> > +------------
> > +The typec class is meant for describing the USB Type-C ports in a system to the
> > +user space in unified fashion. The class is designed to provide nothing else
> > +except the user space interface implementation in hope that it can be utilized
> > +on as many platforms as possible.
> > +
> > +The platforms are expected to register every USB Type-C port they have with the
> > +class. In a normal case the registration will be done by a USB Type-C or PD PHY
> > +driver, but it may be a driver for firmware interface such as UCSI, driver for
> > +USB PD controller or even driver for Thunderbolt3 controller. This document
> > +considers the component registering the USB Type-C ports with the class as "port
> > +driver".
> > +
> > +On top of showing the capabilities, the class also offer the user space control
> > +over the roles and alternate modes they support when the port driver is capable
> > +of supporting those features.
> > +
> > +The class provides an API for the port drivers described in this document. The
> > +attributes are described in Documentation/ABI/testing/sysfs-class-typec.
> > +
> > +
> > +Interface
> > +---------
> > +Every port will be presented as its own device under /sys/class/typec/. The
> > +first port will be named "usbc0", the second "usbc1" and so on.
> 
> 
> I would need a way to map /sys/bus/usb/ entries with /sys/class/typec/ entries.

This needs planning, however this should not effect the class driver
at this point. The linking needs to happen in the drivers registering
the ports at least in the beginning. I fear there isn't a single
method that could be used on all types of platforms.

With ACPI we can find the port with the companion ACPI device for
the port itself. I don't know is this documented anywhere, but the
ACPI device object of the port is provided as a child for the typec
devices on boards that I've seen so far. I have no idea how the
linking even could be done in DT.

> > +
> > +When connected, the partner will be presented also as its own device under
> > +/sys/class/typec/. The parent of the partner device will always be the port. The
> > +partner attached to port "usbc0" will be named "usbc0-partner". Full patch to
> 
> s/patch/path/

OK.

<snip>

> > +/*
> > + * typec_altmode_update_active - Notify about Enter/Exit mode
> > + * @alt: Handle to the Alternate Mode
> > + * @mode: Mode id
> > + * @active: True when the mode has been enterred
> > + */
> > +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> > +                                bool active)
> > +{
> > +       struct typec_mode *m = alt->modes + mode;
> > +       char dir[6];
> > +
> > +       m->active = active;
> > +       sprintf(dir, "mode%d", mode);
> 
> 
> maybe we need a safety check here and verify that `mode` is a single
> digit or clip properly the string ?

Sure.

> 
> > +       sysfs_notify(&alt->dev.kobj, dir, "active");
> > +}
> > +EXPORT_SYMBOL(typec_altmode_update_active);


Thanks for the review,

-- 
heikki

Powered by blists - more mailing lists