Message-ID: <CANVmJFm9m0u3ACw9HeOKSxC5TbVJt+vyAGhiCk2JH87wrfyHrg@mail.gmail.com>
Date:   Fri, 26 Aug 2016 15:16:16 +0200
From:   Vincent Palatin <vpalatin@...omium.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
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

On Thu, Aug 25, 2016 at 1:59 PM, Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
> 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.

OK.

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


My point was: saying we are a VCONN "sink" just because we are not
currently sourcing vconn is usually not true.


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

sure it does not, I just found it confusing since we are referring
only to the power role.

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

Indeed I was.

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

My remark was a bit too stern,
I meant a dock might be 'USB' 'Charger' 'Alternate Mode' , all at the
same time or alternately depending what you plug in.
I don't really see those types as mutually exclusive.


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