[PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

Hi,

The OS, or more precisely the user space, needs to be able to control
a few things regarding USB Type-C ports. The first thing that must be
allowed to be controlled is the data role. USB Type-C ports will
select the data role randomly with DRP ports. When USB PD is
supported, also independent (from data role) power role swapping can
be supported together with Alternate Mode control.

I'm proposing with this set a Class for the Type-C connectors that
gives the user space control over those things on top of getting basic
details about the USB Type-C connectors and also partners. The details
include the capabilities of the port, the supported data and power
roles, supported accessories (audio and debug), supported Alternate
Modes, USB PD support and of course the type of the partner (USB, Alt
Mode, Accessory or Charger), and more or less the same details about
the partner.

I'm not considering cables with this Class, and I have deliberately
left out some more technical details, like cable orientation, firstly
because I did not see much use for the user space from knowing that
an secondly because that kind of details are not always available for
example with UCSI.

So the interface to the user space is kept as simple as I dared to
make it.

NOTE: In case there is somebody wondering, this is not adding USB PD
support to Linux kernel. This is just about USB Type-C.


Heikki Krogerus (3):
  usb: USB Type-C Connector Class
  usb: type-c: USB Type-C Connector System Software Interface
  usb: type-c: UCSI ACPI driver

 drivers/usb/Kconfig            |   2 +
 drivers/usb/Makefile           |   2 +
 drivers/usb/type-c/Kconfig     |  25 +++
 drivers/usb/type-c/Makefile    |   3 +
 drivers/usb/type-c/typec.c     | 446 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/type-c/ucsi.c      | 450 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/type-c/ucsi.h      | 219 ++++++++++++++++++++
 drivers/usb/type-c/ucsi_acpi.c | 133 ++++++++++++
 include/linux/usb/typec.h      | 114 +++++++++++
 9 files changed, 1394 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 drivers/usb/type-c/ucsi.c
 create mode 100644 drivers/usb/type-c/ucsi.h
 create mode 100644 drivers/usb/type-c/ucsi_acpi.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.7.0

[PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

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

The class will export the following interfaces for every
USB Type-C Connector in the system to sysfs:

1. connected - Connection status of the connector
2. alternate_mode - The current Alternate Mode
3. alternate_modes - Lists all Alternate Modes the connector supports
4. partner_alt_modes - Lists partner's Alternate Modes when connected
5. partner_type - Can be USB, Charger, Alt Mode or Accessory
6. data_role - The current data role, host or device
7. data_roles - Data roles supported by the connector
8. power_role - Connector's current power role, source or sink
9. power_roles - Power roles supported by the connector
10. power_operation_mode - The current power level in use
11. usb_pd - yes if the connector supports USB PD.
12. audio_accessory - yes if the connector supports Audio Accessory
13. debug_accessory - yes if the connector supports Debug Accessory

The data_role, power_role and alternate_mode are also
writable and can be used for executing role swapping and
entering modes. When USB PD is not supported by the
connector or partner, power_role will reflect the value of
the data_role, and is not swappable independently.

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  | 446 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h   | 114 +++++++++++
 6 files changed, 572 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

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8ed451d..0c45547 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -151,6 +151,8 @@ source "drivers/usb/phy/Kconfig"
 
 source "drivers/usb/gadget/Kconfig"
 
+source "drivers/usb/type-c/Kconfig"
+
 config USB_LED_TRIG
 	bool "USB LED Triggers"
 	depends on LEDS_CLASS && USB_COMMON && LEDS_TRIGGERS
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index d5c57f1..4d712ee 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -61,3 +61,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
 obj-$(CONFIG_USB_COMMON)	+= common/
 
 obj-$(CONFIG_USBIP_CORE)	+= usbip/
+
+obj-$(CONFIG_TYPEC)		+= type-c/
diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
new file mode 100644
index 0000000..b229fb9
--- /dev/null
+++ b/drivers/usb/type-c/Kconfig
@@ -0,0 +1,7 @@
+
+menu "USB PD and Type-C drivers"
+
+config TYPEC
+	tristate
+
+endmenu
diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
new file mode 100644
index 0000000..1012a8b
--- /dev/null
+++ b/drivers/usb/type-c/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TYPEC)		+= typec.o
diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
new file mode 100644
index 0000000..e425955
--- /dev/null
+++ b/drivers/usb/type-c/typec.c
@@ -0,0 +1,446 @@
+/*
+ * USB Type-C class
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb/typec.h>
+
+#define to_typec_port(p) container_of(p, struct typec_port, dev)
+
+static DEFINE_IDA(typec_index_ida);
+
+/* -------------------------------- */
+
+int typec_connect(struct typec_port *port)
+{
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_connect);
+
+void typec_disconnect(struct typec_port *port)
+{
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+}
+EXPORT_SYMBOL_GPL(typec_disconnect);
+
+/* -------------------------------- */
+
+static ssize_t alternate_mode_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode alt_mode;
+	int ret;
+
+	if (!port->cap->set_alt_mode) {
+		dev_warn(dev, "entering Alternate Modes not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (sscanf(buf, "0x%hx,%u", &alt_mode.svid, &alt_mode.mid) != 2)
+		return -EINVAL;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->set_alt_mode(port, &alt_mode);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t alternate_mode_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (!port->cur_alt_mode)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	return sprintf(buf, "0x%hx,%u\n", port->cur_alt_mode->svid,
+		       port->cur_alt_mode->mid);
+}
+static DEVICE_ATTR_RW(alternate_mode);
+
+static ssize_t alternate_modes_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode *alt_mode;
+	int len = 0;
+
+	if (!port->cap->alt_modes)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	for (alt_mode = port->cap->alt_modes; alt_mode->svid; alt_mode++)
+		len += sprintf(buf + len, "0x%hx,%u\n", alt_mode->svid,
+			       alt_mode->mid);
+
+	buf[len - 1] = '\0';
+	return len;
+}
+static DEVICE_ATTR_RO(alternate_modes);
+
+static ssize_t partner_alt_modes_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode *alt_mode;
+	int len = 0;
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (!port->partner_alt_modes)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	for (alt_mode = port->partner_alt_modes; alt_mode->svid; alt_mode++)
+		len += sprintf(buf + len, "0x%hx,%u\n", alt_mode->svid,
+			       alt_mode->mid);
+
+	buf[len - 1] = '\0';
+	return len;
+}
+static DEVICE_ATTR_RO(partner_alt_modes);
+
+static const char * const typec_partner_types[] = {
+	[TYPEC_PARTNER_NONE] = "unknown",
+	[TYPEC_PARTNER_USB] = "USB",
+	[TYPEC_PARTNER_CHARGER] = "Charger",
+	[TYPEC_PARTNER_ALTMODE] = "Alternate Mode",
+	[TYPEC_PARTNER_AUDIO] = "Audio Accessory",
+	[TYPEC_PARTNER_DEBUG] = "Debug Accessroy",
+};
+
+static ssize_t partner_type_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (!port->connected)
+		return -ENXIO;
+
+	return sprintf(buf, "%s\n", typec_partner_types[port->partner_type]);
+}
+static DEVICE_ATTR_RO(partner_type);
+
+static ssize_t data_role_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	enum typec_data_role role;
+	int ret;
+
+	if (port->cap->type != TYPEC_PORT_DRP) {
+		dev_dbg(dev, "data role swap only supported with DRP ports\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->cap->dr_swap) {
+		dev_warn(dev, "data role swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (!strncmp(buf, "host", 4))
+		role = TYPEC_HOST;
+	else if (!strncmp(buf, "device", 6))
+		role = TYPEC_DEVICE;
+	else
+		return -EINVAL;
+
+	if (port->data_role == role)
+		goto out;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->dr_swap(port);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+out:
+	return size;
+}
+
+static ssize_t data_role_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	switch (port->cap->type) {
+	case TYPEC_PORT_DFP:
+		return sprintf(buf, "host\n");
+	case TYPEC_PORT_UFP:
+		return sprintf(buf, "device\n");
+	case TYPEC_PORT_DRP:
+		return sprintf(buf, "%s\n", port->data_role == TYPEC_HOST ?
+			       "host" : "device");
+	default:
+		return sprintf(buf, "unknown\n");
+	};
+}
+static DEVICE_ATTR_RW(data_role);
+
+static ssize_t data_roles_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (port->cap->type == TYPEC_PORT_DRP)
+		return sprintf(buf, "host, device\n");
+
+	return data_role_show(dev, attr, buf);
+}
+static DEVICE_ATTR_RO(data_roles);
+
+static ssize_t power_role_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	enum typec_pwr_role role;
+	int ret;
+
+	if (!port->cap->usb_pd) {
+		dev_dbg(dev, "power role swap only supported with USB PD\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->cap->pr_swap) {
+		dev_warn(dev, "power role swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
+		dev_dbg(dev, "partner unable to swap power role\n");
+		return -EIO;
+	}
+
+	if (!strncmp(buf, "source", 6))
+		role = TYPEC_PWR_SOURCE;
+	else if (!strncmp(buf, "sink", 4))
+		role = TYPEC_PWR_SINK;
+	else
+		return -EINVAL;
+
+	if (port->pwr_role == role)
+		return size;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->pr_swap(port);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t power_role_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	switch (port->pwr_role) {
+	case TYPEC_PWR_SOURCE:
+		return sprintf(buf, "source\n");
+	case TYPEC_PWR_SINK:
+		return sprintf(buf, "sink\n");
+	default:
+		return sprintf(buf, "unknown\n");
+	};
+}
+static DEVICE_ATTR_RW(power_role);
+
+static ssize_t power_roles_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (port->cap->usb_pd || port->cap->type == TYPEC_PORT_DRP)
+		return sprintf(buf, "source, sink\n");
+
+	return power_role_show(dev, attr, buf);
+}
+static DEVICE_ATTR_RO(power_roles);
+
+static const char * const typec_pwr_opmodes[] = {
+	[TYPEC_PWR_MODE_USB] = "USB",
+	[TYPEC_PWR_MODE_BC1_2] = "BC1.2",
+	[TYPEC_PWR_MODE_1_5A] = "USB Type-C 1.5A",
+	[TYPEC_PWR_MODE_3_0A] = "USB Type-C 3.0A",
+	[TYPEC_PWR_MODE_PD] = "USB PD",
+};
+
+static ssize_t power_operation_mode_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", typec_pwr_opmodes[port->pwr_opmode]);
+}
+static DEVICE_ATTR_RO(power_operation_mode);
+
+static ssize_t connected_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", port->connected ? "yes" : "no");
+}
+static DEVICE_ATTR_RO(connected);
+
+static ssize_t usb_pd_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->usb_pd ? "" : "not ");
+}
+static DEVICE_ATTR_RO(usb_pd);
+
+static ssize_t audio_accessory_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->audio_accessory ?
+		       "" : "not ");
+}
+static DEVICE_ATTR_RO(audio_accessory);
+
+static ssize_t debug_accessory_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->debug_accessory ?
+		       "" : "not ");
+}
+static DEVICE_ATTR_RO(debug_accessory);
+
+/* REVISIT: Consider creating the partner dependent sysfs files at runtime. */
+static struct attribute *typec_attrs[] = {
+	&dev_attr_alternate_mode.attr,
+	&dev_attr_alternate_modes.attr,
+	&dev_attr_partner_alt_modes.attr,
+	&dev_attr_partner_type.attr,
+	&dev_attr_data_role.attr,
+	&dev_attr_data_roles.attr,
+	&dev_attr_power_role.attr,
+	&dev_attr_power_roles.attr,
+	&dev_attr_power_operation_mode.attr,
+	&dev_attr_connected.attr,
+	&dev_attr_usb_pd.attr,
+	&dev_attr_audio_accessory.attr,
+	&dev_attr_debug_accessory.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(typec);
+
+static int typec_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	int ret;
+
+	ret = add_uevent_var(env, "TYPEC_PORT=%s", dev_name(dev));
+	if (ret)
+		dev_err(dev, "failed to add uevent TYPEC_PORT\n");
+
+	return ret;
+}
+
+static void typec_release(struct device *dev)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	ida_simple_remove(&typec_index_ida, port->id);
+	kfree(port);
+}
+
+static struct class typec_class = {
+	.name = "type-c",
+	.dev_uevent = typec_uevent,
+	.dev_groups = typec_groups,
+	.dev_release = typec_release,
+};
+
+struct typec_port *typec_register_port(struct device *dev,
+				       struct typec_capability *cap)
+{
+	struct typec_port *port;
+	int ret;
+	int id;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return ERR_PTR(-ENOMEM);
+
+	id = ida_simple_get(&typec_index_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		kfree(port);
+		return ERR_PTR(id);
+	}
+
+	port->id = id;
+	port->cap = cap;
+	port->dev.class = &typec_class;
+	port->dev.parent = dev;
+	dev_set_name(&port->dev, "usbc%d", id);
+	mutex_init(&port->lock);
+
+	ret = device_register(&port->dev);
+	if (ret) {
+		ida_simple_remove(&typec_index_ida, id);
+		put_device(&port->dev);
+		kfree(port);
+		return ERR_PTR(ret);
+	}
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(typec_register_port);
+
+void typec_unregister_port(struct typec_port *port)
+{
+	device_unregister(&port->dev);
+}
+EXPORT_SYMBOL_GPL(typec_unregister_port);
+
+static int __init typec_init(void)
+{
+	return class_register(&typec_class);
+}
+subsys_initcall(typec_init);
+
+static void __exit typec_exit(void)
+{
+	return class_unregister(&typec_class);
+}
+module_exit(typec_exit);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("USB Type-C Connector Class");
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
new file mode 100644
index 0000000..d6e562c
--- /dev/null
+++ b/include/linux/usb/typec.h
@@ -0,0 +1,114 @@
+
+#ifndef __LINUX_USB_TYPEC_H
+#define __LINUX_USB_TYPEC_H
+
+#include <linux/types.h>
+
+enum typec_port_type {
+	TYPEC_PORT_DFP,
+	TYPEC_PORT_UFP,
+	TYPEC_PORT_DRP,
+};
+
+enum typec_data_role {
+	TYPEC_DEVICE,
+	TYPEC_HOST,
+};
+
+enum typec_pwr_role {
+	TYPEC_PWR_SINK,
+	TYPEC_PWR_SOURCE,
+};
+
+enum typec_pwr_opmode {
+	TYPEC_PWR_MODE_USB,
+	TYPEC_PWR_MODE_BC1_2,
+	TYPEC_PWR_MODE_1_5A,
+	TYPEC_PWR_MODE_3_0A,
+	TYPEC_PWR_MODE_PD,
+};
+
+enum typec_partner_type {
+	TYPEC_PARTNER_NONE,
+	TYPEC_PARTNER_USB,
+	TYPEC_PARTNER_CHARGER,
+	TYPEC_PARTNER_ALTMODE,
+	TYPEC_PARTNER_AUDIO,
+	TYPEC_PARTNER_DEBUG,
+};
+
+struct typec_alt_mode {
+	u16 svid;
+	u32 mid;
+};
+
+struct typec_port;
+
+/*
+ * struct typec_capability - USB Type-C Port Capabilities
+ * @type: 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
+ * @dr_swap: Data Role Swap support
+ * @pr_swap: Power Role Swap support
+ * @set_alt_mode: Enter given Alternate Mode
+ *
+ * Static capabilities of a single USB Type-C port.
+ */
+struct typec_capability {
+	enum typec_port_type	type;
+	unsigned int		usb_pd:1;
+	struct typec_alt_mode	*alt_modes;
+	unsigned int		audio_accessory:1;
+	unsigned int		debug_accessory:1;
+
+	int			(*dr_swap)(struct typec_port *);
+	int			(*pr_swap)(struct typec_port *);
+	int			(*set_alt_mode)(struct typec_port *,
+						struct typec_alt_mode *);
+};
+
+/*
+ * struct typec_port - USB Type-C Port
+ * @id: port index
+ * @dev: struct device instance
+ * @lock: Lock to protect concurrent access
+ * @data_role: Current USB role - Host or Device
+ * @pwr_role: Current Power role - Source or Sink
+ * @pwr_opmode: The power level in use at the moment
+ * @cur_alt_mode: The Alternate Mode currently in use
+ * @connected: Connection status
+ * @partner_type: Port type of the partner
+ * @partner_alt_modes: Alternate Modes the partner supports (null terminated)
+ * @cap: Port Capabilities
+ *
+ * Current status of a USB Type-C port and relevant partner details when
+ * connected.
+ */
+struct typec_port {
+	unsigned int		id;
+	struct device		dev;
+	struct mutex		lock;
+
+	enum typec_data_role	data_role;
+	enum typec_pwr_role	pwr_role;
+	enum typec_pwr_opmode	pwr_opmode;
+	struct typec_alt_mode	*cur_alt_mode;
+
+	unsigned char		connected;
+	enum typec_partner_type	partner_type;
+	struct typec_alt_mode	*partner_alt_modes;
+
+	const struct typec_capability *cap;
+};
+
+struct typec_port *typec_register_port(struct device *dev,
+				       struct typec_capability *cap);
+void typec_unregister_port(struct typec_port *port);
+
+int typec_connect(struct typec_port *port);
+void typec_disconnect(struct typec_port *port);
+
+#endif /* __LINUX_USB_TYPEC_H */
-- 
2.7.0

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Greg KH]

On Tue, Feb 09, 2016 at 07:01:21PM +0200, 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 Altenate Modes.
> 
> The class will export the following interfaces for every
> USB Type-C Connector in the system to sysfs:
> 
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports
> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> 6. data_role - The current data role, host or device
> 7. data_roles - Data roles supported by the connector
> 8. power_role - Connector's current power role, source or sink
> 9. power_roles - Power roles supported by the connector
> 10. power_operation_mode - The current power level in use
> 11. usb_pd - yes if the connector supports USB PD.
> 12. audio_accessory - yes if the connector supports Audio Accessory
> 13. debug_accessory - yes if the connector supports Debug Accessory

You forgot to document these sysfs files in Documenataion/ABI :(

And what is userspace going to do with these files?  Why does it care?

> The data_role, power_role and alternate_mode are also
> writable and can be used for executing role swapping and
> entering modes. When USB PD is not supported by the
> connector or partner, power_role will reflect the value of
> the data_role, and is not swappable independently.

How does this implementation differ from those in other drivers that we
have seen, but not submitted for merging?  I'm referring to the code
from Fairchild for their USB Type C driver:
	https://github.com/gregkh/fusb30x
and the driver that is in the latest Nexus 6 Android release (don't have
the link to the android kernel tree at the moment sorry, but it's public
and I think Linaro is working on cleaning it up...)

thanks,

greg k-h

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Tue, Feb 09, 2016 at 10:20:45AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:21PM +0200, 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 Altenate Modes.
> > 
> > The class will export the following interfaces for every
> > USB Type-C Connector in the system to sysfs:
> > 
> > 1. connected - Connection status of the connector
> > 2. alternate_mode - The current Alternate Mode
> > 3. alternate_modes - Lists all Alternate Modes the connector supports
> > 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> > 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> > 6. data_role - The current data role, host or device
> > 7. data_roles - Data roles supported by the connector
> > 8. power_role - Connector's current power role, source or sink
> > 9. power_roles - Power roles supported by the connector
> > 10. power_operation_mode - The current power level in use
> > 11. usb_pd - yes if the connector supports USB PD.
> > 12. audio_accessory - yes if the connector supports Audio Accessory
> > 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> You forgot to document these sysfs files in Documenataion/ABI :(

Man, it's almost like I enjoy making these stupid mistakes :(

> And what is userspace going to do with these files?  Why does it care?

The OS policy regarding USB Type-C ports needs to come from user
space. The user must be allowed to select the USB data role, and when
USB PD is supported, also the power role, and at the same time we need
to export all the relevant information about the USB Type-C ports to
the user space, like connection status, the type of partner etc. And
all that from a single interface.

I'm pretty sure that this is exactly what distributions like Ubuntu,
RedHat etc. want, an I know for fact that Chrome OS and Android will
expect the user to be in control over the roles and get that
information about the ports one way or the other.

> > The data_role, power_role and alternate_mode are also
> > writable and can be used for executing role swapping and
> > entering modes. When USB PD is not supported by the
> > connector or partner, power_role will reflect the value of
> > the data_role, and is not swappable independently.
> 
> How does this implementation differ from those in other drivers that we
> have seen, but not submitted for merging?  I'm referring to the code
> from Fairchild for their USB Type C driver:
> 	https://github.com/gregkh/fusb30x
> and the driver that is in the latest Nexus 6 Android release (don't have
> the link to the android kernel tree at the moment sorry, but it's public
> and I think Linaro is working on cleaning it up...)

That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
It's the second USB PD stack I've seen (and sadly also second driver
for fusb30x), but I just know there are more.

My class is just about exporting control of USB Type-C ports to the
user space, and note, USB Type-C *not* USB PD. I don't thing that my
little class and the USB PD stack, where ever it ends up coming from,
conflict with each other.

The only difference is that I'm clearly separating USB Type-C from USB
PD (and actually everything else), which is the correct thing to do.
USB Type-C is not the same thing as USB PD. USB Type-C does not always
come with USB PD.

I did not go through that code, but I'm guessing the guys have for
example exported similar role swapping controls to user space from
some part of their stack. So those guys would just need to register
their fusb30x with this class, let the class take care of exporting
those controls and probable continue using their USB PD stack exactly
like they have done before.

I hope I was able to explain myself.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Greg KH]

On Wed, Feb 10, 2016 at 12:38:40PM +0200, Heikki Krogerus wrote:
> > And what is userspace going to do with these files?  Why does it care?
> 
> The OS policy regarding USB Type-C ports needs to come from user
> space.

What drives this "need"?

> The user must be allowed to select the USB data role, and when
> USB PD is supported, also the power role, and at the same time we need
> to export all the relevant information about the USB Type-C ports to
> the user space, like connection status, the type of partner etc. And
> all that from a single interface.

Again, what drives this "need" to be in a "single interface"?

> I'm pretty sure that this is exactly what distributions like Ubuntu,
> RedHat etc. want, an I know for fact that Chrome OS and Android will
> expect the user to be in control over the roles and get that
> information about the ports one way or the other.

Given that ChromeOS and Android already do this type of thing today, why
not work with those developers to ensure that this really is the
interface they want / expect?

> > > The data_role, power_role and alternate_mode are also
> > > writable and can be used for executing role swapping and
> > > entering modes. When USB PD is not supported by the
> > > connector or partner, power_role will reflect the value of
> > > the data_role, and is not swappable independently.
> > 
> > How does this implementation differ from those in other drivers that we
> > have seen, but not submitted for merging?  I'm referring to the code
> > from Fairchild for their USB Type C driver:
> > 	https://github.com/gregkh/fusb30x
> > and the driver that is in the latest Nexus 6 Android release (don't have
> > the link to the android kernel tree at the moment sorry, but it's public
> > and I think Linaro is working on cleaning it up...)
> 
> That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
> It's the second USB PD stack I've seen (and sadly also second driver
> for fusb30x), but I just know there are more.

Oh, there's more than just 2 drivers for that fusb30x chip floating
around.  My repo is not the latest version and it's a truly horrid piece
of code, never to be run on any hardware you actually care about power
as it doesn't care.

> My class is just about exporting control of USB Type-C ports to the
> user space, and note, USB Type-C *not* USB PD. I don't thing that my
> little class and the USB PD stack, where ever it ends up coming from,
> conflict with each other.

But we need to ensure that it doesn't conflict, and given that you are
already using the same directory those stacks are using, perhaps you can
look at them to see that you aren't duplicating any work?

> The only difference is that I'm clearly separating USB Type-C from USB
> PD (and actually everything else), which is the correct thing to do.
> USB Type-C is not the same thing as USB PD. USB Type-C does not always
> come with USB PD.

I agree, keeping them separate seems good, but I worry when you have to
do both how that is going to look.

> I did not go through that code, but I'm guessing the guys have for
> example exported similar role swapping controls to user space from
> some part of their stack. So those guys would just need to register
> their fusb30x with this class, let the class take care of exporting
> those controls and probable continue using their USB PD stack exactly
> like they have done before.

the fusb30x code does it all within kernel space, no userspace
interactions needed due to timing requirements (so they say).  I'm not
saying that this is a good idea / design, just that I'm getting
conflicting requirements from different camps at the moment and it's
really hard to sort it all out :(

thanks,

greg k-h

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 09:26:19AM -0800, Greg KH wrote:
> On Wed, Feb 10, 2016 at 12:38:40PM +0200, Heikki Krogerus wrote:
> > > And what is userspace going to do with these files?  Why does it care?
> > 
> > The OS policy regarding USB Type-C ports needs to come from user
> > space.
> 
> What drives this "need"?
> 
> > The user must be allowed to select the USB data role, and when
> > USB PD is supported, also the power role, and at the same time we need
> > to export all the relevant information about the USB Type-C ports to
> > the user space, like connection status, the type of partner etc. And
> > all that from a single interface.
> 
> Again, what drives this "need" to be in a "single interface"?

Because the alternative is that every platform will have a custom way
of doing these things.. With the single interface I mean the class.
If we don't present USB Type-C ports to the userspace from a unified
interface, how can anybody create userspace software to support
control of USB Type-C ports? Or can we accept that we will always need
platform specific user space software if we want to control the USB
Type-C ports?

In case you are still skeptical about why userspace would need any
interaction with the USB Type-C ports, think about the following use
case: You plug what you think is something that will charge your
phone, nothing is printed on the screen - we don't now get any details
about the parter - but we must be charging, right? After a while you
realize that you are not charging, instead the thing you plugged in is
actually draining you phone of power? This will happen if the partner
happens to be for example..
1) Accessory
2) Alternate Mode that we enter successfully, so no Billboard Device.
3) Both our device and the partner are DRP and we end up being the
   host initially.

Nothing bad has happened in this example, but it's horrible user
experience.

> > I'm pretty sure that this is exactly what distributions like Ubuntu,
> > RedHat etc. want, an I know for fact that Chrome OS and Android will
> > expect the user to be in control over the roles and get that
> > information about the ports one way or the other.
> 
> Given that ChromeOS and Android already do this type of thing today, why
> not work with those developers to ensure that this really is the
> interface they want / expect?

I'm in contact with some Android developers, but Android is a bit
frustrating. I know the absolute minimum about the world of Android,
but the way it looks to me is that everybody is doing their own thing.
The expectations about things also vary from company to company.
Maybe there is a forum somewhere that I'm not aware of?

I'm pretty sure ChromeOS is going to be different. I'll try to make
some contacts there.

> > > > The data_role, power_role and alternate_mode are also
> > > > writable and can be used for executing role swapping and
> > > > entering modes. When USB PD is not supported by the
> > > > connector or partner, power_role will reflect the value of
> > > > the data_role, and is not swappable independently.
> > > 
> > > How does this implementation differ from those in other drivers that we
> > > have seen, but not submitted for merging?  I'm referring to the code
> > > from Fairchild for their USB Type C driver:
> > > 	https://github.com/gregkh/fusb30x
> > > and the driver that is in the latest Nexus 6 Android release (don't have
> > > the link to the android kernel tree at the moment sorry, but it's public
> > > and I think Linaro is working on cleaning it up...)
> > 
> > That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
> > It's the second USB PD stack I've seen (and sadly also second driver
> > for fusb30x), but I just know there are more.
> 
> Oh, there's more than just 2 drivers for that fusb30x chip floating
> around.  My repo is not the latest version and it's a truly horrid piece
> of code, never to be run on any hardware you actually care about power
> as it doesn't care.
> 
> > My class is just about exporting control of USB Type-C ports to the
> > user space, and note, USB Type-C *not* USB PD. I don't thing that my
> > little class and the USB PD stack, where ever it ends up coming from,
> > conflict with each other.
> 
> But we need to ensure that it doesn't conflict, and given that you are
> already using the same directory those stacks are using, perhaps you can
> look at them to see that you aren't duplicating any work?

Well, in case of that fusb30x driver, there definitely is no conflict
if there really is no interaction with the userspace. They would just
never register with this class.

But there really should never be any conflict between USB Type-C and
USB PD. For USB Type-C connectors USB PD is just an optional "library"
that can have the following functions and nothing else:

1) DR_Swap - Data Role swap
2) PR_Swap - Power Role swap
2) VCONN_Swap - VCONN Source swap
3) Discover SVIDs - Alternate Modes
3) Discover Modes - The actual modes under SVID
4) Enter Mode
4) Exit Mode

So those functions will always be the same. From Type-C point of view
it really makes no difference how they have actually been implemented
(in software, PD controller, firmware, etc).

> > The only difference is that I'm clearly separating USB Type-C from USB
> > PD (and actually everything else), which is the correct thing to do.
> > USB Type-C is not the same thing as USB PD. USB Type-C does not always
> > come with USB PD.
> 
> I agree, keeping them separate seems good, but I worry when you have to
> do both how that is going to look.

I'm fairly certain that we can make the Type-C part good. It's so
straight forward. I'm more worried about USB PD.

I'm a bit skeptical about whether it will even be possible to create a
stack that works for every type of USB PD thingy out there. For cases
where there is just the PHY like fusb30x, the PD stack will be made
completely in software. That we can (maybe) handle. Also cases where
we have a complex USB PD controller that handles all layers starting
from protocol, we will be able to handle, as with them the stack is not
needed at all. Even the simple USB PD controllers that take care of
protocol layer (completely) but leave the policy layers to the
software, we will be able to handle. The problem comes from the USB PD
controller and PHYs that fall somewhere in-between. The ones that
implement "partial" protocol layer and "partial" policy engine, or
consist of several components, or..

..I have to stop there. I just vomited in my mouth a little. I fear
USB PD can end up being a little bit like USB Charging (Felipe can
tell more about that) except many times worse! Maybe we need to start
thinking about the ground rules when somebody adds the PD stack, like
you can use only complete parts of stack and never try to mix your
oddities into it, otherwise you have to implement everything from
scratch for your particular platform, etc.

Perhaps things are not as bad as I fear, but let's just make sure
Type-C is always separate from USB PD!

> > I did not go through that code, but I'm guessing the guys have for
> > example exported similar role swapping controls to user space from
> > some part of their stack. So those guys would just need to register
> > their fusb30x with this class, let the class take care of exporting
> > those controls and probable continue using their USB PD stack exactly
> > like they have done before.
> 
> the fusb30x code does it all within kernel space, no userspace
> interactions needed due to timing requirements (so they say).  I'm not
> saying that this is a good idea / design, just that I'm getting
> conflicting requirements from different camps at the moment and it's
> really hard to sort it all out :(

I think there must have been some misunderstanding. We really have to
interact with the userspace with USB Type-C ports.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
> 
> The class will export the following interfaces for every
> USB Type-C Connector in the system to sysfs:
> 
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports

These names are a bit problematic, as they are too similar.
How about

current_alternate_mode
potential_alternate_modes

> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> 6. data_role - The current data role, host or device
> 7. data_roles - Data roles supported by the connector
> 8. power_role - Connector's current power role, source or sink
> 9. power_roles - Power roles supported by the connector
> 10. power_operation_mode - The current power level in use
> 11. usb_pd - yes if the connector supports USB PD.
> 12. audio_accessory - yes if the connector supports Audio Accessory
> 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> The data_role, power_role and alternate_mode are also
> writable and can be used for executing role swapping and
> entering modes. When USB PD is not supported by the
> connector or partner, power_role will reflect the value of
> the data_role, and is not swappable independently.
> 
> 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  | 446 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h   | 114 +++++++++++
>  6 files changed, 572 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
> 
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8ed451d..0c45547 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -151,6 +151,8 @@ source "drivers/usb/phy/Kconfig"
>  
>  source "drivers/usb/gadget/Kconfig"
>  
> +source "drivers/usb/type-c/Kconfig"
> +
>  config USB_LED_TRIG
>  	bool "USB LED Triggers"
>  	depends on LEDS_CLASS && USB_COMMON && LEDS_TRIGGERS
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index d5c57f1..4d712ee 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -61,3 +61,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
>  obj-$(CONFIG_USB_COMMON)	+= common/
>  
>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> +
> +obj-$(CONFIG_TYPEC)		+= type-c/
> diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
> new file mode 100644
> index 0000000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/type-c/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"
> +
> +config TYPEC
> +	tristate
> +
> +endmenu
> diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
> new file mode 100644
> index 0000000..1012a8b
> --- /dev/null
> +++ b/drivers/usb/type-c/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TYPEC)		+= typec.o
> diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
> new file mode 100644
> index 0000000..e425955
> --- /dev/null
> +++ b/drivers/usb/type-c/typec.c
> @@ -0,0 +1,446 @@
> +/*
> + * USB Type-C class
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +
> +#define to_typec_port(p) container_of(p, struct typec_port, dev)
> +
> +static DEFINE_IDA(typec_index_ida);
> +
> +/* -------------------------------- */
> +
> +int typec_connect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_connect);
> +
> +void typec_disconnect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +}
> +EXPORT_SYMBOL_GPL(typec_disconnect);
> +
> +/* -------------------------------- */
> +
> +static ssize_t alternate_mode_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	struct typec_alt_mode alt_mode;
> +	int ret;
> +
> +	if (!port->cap->set_alt_mode) {
> +		dev_warn(dev, "entering Alternate Modes not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->connected)
> +		return -ENXIO;

Doesn't this need locking?
And why wouldn't user space want to preselect a mode?

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
>>
>> The class will export the following interfaces for every
>> USB Type-C Connector in the system to sysfs:
>>
>> 1. connected - Connection status of the connector
>> 2. alternate_mode - The current Alternate Mode
>> 3. alternate_modes - Lists all Alternate Modes the connector supports
>
> These names are a bit problematic, as they are too similar.
> How about
>
> current_alternate_mode
> potential_alternate_modes

I would vote for supported_*

>> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
>> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
>> 6. data_role - The current data role, host or device
>> 7. data_roles - Data roles supported by the connector
>> 8. power_role - Connector's current power role, source or sink
>> 9. power_roles - Power roles supported by the connector
>> 10. power_operation_mode - The current power level in use
>> 11. usb_pd - yes if the connector supports USB PD.
>> 12. audio_accessory - yes if the connector supports Audio Accessory
>> 13. debug_accessory - yes if the connector supports Debug Accessory

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 01:05:27PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
> >>
> >> The class will export the following interfaces for every
> >> USB Type-C Connector in the system to sysfs:
> >>
> >> 1. connected - Connection status of the connector
> >> 2. alternate_mode - The current Alternate Mode
> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
> >
> > These names are a bit problematic, as they are too similar.
> > How about
> >
> > current_alternate_mode

That works for me.

> > potential_alternate_modes
> 
> I would vote for supported_*

How about connector_alternate_modes?


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 1:11 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Feb 10, 2016 at 01:05:27PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> > On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
>> >>
>> >> The class will export the following interfaces for every
>> >> USB Type-C Connector in the system to sysfs:
>> >>
>> >> 1. connected - Connection status of the connector
>> >> 2. alternate_mode - The current Alternate Mode
>> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
>> >
>> > These names are a bit problematic, as they are too similar.
>> > How about
>> >
>> > current_alternate_mode
>
> That works for me.
>
>> > potential_alternate_modes
>>
>> I would vote for supported_*
>
> How about connector_alternate_modes?

Would be fine as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi Oliver,

> > +static ssize_t alternate_mode_store(struct device *dev,
> > +				    struct device_attribute *attr,
> > +				    const char *buf, size_t size)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	struct typec_alt_mode alt_mode;
> > +	int ret;
> > +
> > +	if (!port->cap->set_alt_mode) {
> > +		dev_warn(dev, "entering Alternate Modes not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!port->connected)
> > +		return -ENXIO;
> 
> Doesn't this need locking?

Yes, I need to fix the locking.

> And why wouldn't user space want to preselect a mode?

That is tricky, as we would need to keep a list of the preselected
modes and for all SVIDs the connector supports. I don't think it would
be practical to do from this file as we would then use it differently
when connected and not connected, so the preselected modes would
probable be better to give from a separate file.

That is certainly doable, but is it really useful? I not really
against adding that support, but I would like to keep this interface
as simple as possible.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Wed, 2016-02-10 at 13:23 +0200, Heikki Krogerus wrote:

> That is tricky, as we would need to keep a list of the preselected
> modes and for all SVIDs the connector supports. I don't think it would
> be practical to do from this file as we would then use it differently
> when connected and not connected, so the preselected modes would
> probable be better to give from a separate file.
> 
> That is certainly doable, but is it really useful? I not really
> against adding that support, but I would like to keep this interface
> as simple as possible.

Just for the record. Heikki is right. Preselecting an alternate mode
is stupid.

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

Oliver Neukum <oneukum@suse.com> writes:
> On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
>> 
>> The class will export the following interfaces for every
>> USB Type-C Connector in the system to sysfs:
>> 
>> 1. connected - Connection status of the connector
>> 2. alternate_mode - The current Alternate Mode
>> 3. alternate_modes - Lists all Alternate Modes the connector supports
>
> These names are a bit problematic, as they are too similar.
> How about
>
> current_alternate_mode
> potential_alternate_modes

available_ ?

>> 4. partner_alt_modes - Lists partner's Alternate Modes when connected

partner_alternate_modes ? (it's a file name, we can spell it out)

>> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
>> 6. data_role - The current data role, host or device
>> 7. data_roles - Data roles supported by the connector

current_data_role
available_data_roles

>> 8. power_role - Connector's current power role, source or sink
>> 9. power_roles - Power roles supported by the connector

ditto...

>> 10. power_operation_mode - The current power level in use
>> 11. usb_pd - yes if the connector supports USB PD.

supports_usb_power_delivery ?

>> 12. audio_accessory - yes if the connector supports Audio Accessory

supports_audio_accessory

>> 13. debug_accessory - yes if the connector supports Debug Accessory

supports_debug_accessory

> Doesn't this need locking?
> And why wouldn't user space want to preselect a mode?

isn't USB always the default mode ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-02-11 at 10:55 +0200, Felipe Balbi wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> > On Tue, 2016-02-09 at 19:01 +0200, 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 Altenate Modes.
> >> 
> >> The class will export the following interfaces for every
> >> USB Type-C Connector in the system to sysfs:
> >> 
> >> 1. connected - Connection status of the connector
> >> 2. alternate_mode - The current Alternate Mode
> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
> >
> > These names are a bit problematic, as they are too similar.
> > How about
> >
> > current_alternate_mode
> > potential_alternate_modes
> 
> available_ ?

Also good. Frankly I think it boils down to taste.
I just want to make sure that they not be mistaken for each other.

> >> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> partner_alternate_modes ? (it's a file name, we can spell it out)

True

[..]

> > Doesn't this need locking?
> > And why wouldn't user space want to preselect a mode?
> 
> isn't USB always the default mode ?

True for mode. But the question also applies to roles.
I would think that user space should be able to preselect
that we always want to be upstream or downstream or flexible.
And it should be able to preemptively disallow power delivery
even if no cable is present.

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Thu, Feb 11, 2016 at 10:08:14AM +0100, Oliver Neukum wrote:
> I would think that user space should be able to preselect
> that we always want to be upstream or downstream or flexible.

Agreed.

> And it should be able to preemptively disallow power delivery
> even if no cable is present.

By power delivery you mean power sourcing, not USB PD, right? So
basically being able to preselect also power_role?

That is OK by me, but we need to remember that it's something that is
only possible when both our connector and also the partner support USB
PD. Otherwise the power role will be based on the data role. So when
USB PD is not supported, host is always the source and the device
always the sink.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi Felipe,

> >> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> partner_alternate_modes ? (it's a file name, we can spell it out)
> 
> >> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> >> 6. data_role - The current data role, host or device
> >> 7. data_roles - Data roles supported by the connector
> 
> current_data_role
> available_data_roles
> 
> >> 8. power_role - Connector's current power role, source or sink
> >> 9. power_roles - Power roles supported by the connector
> 
> ditto...
> 
> >> 10. power_operation_mode - The current power level in use
> >> 11. usb_pd - yes if the connector supports USB PD.
> 
> supports_usb_power_delivery ?
> 
> >> 12. audio_accessory - yes if the connector supports Audio Accessory
> 
> supports_audio_accessory
> 
> >> 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> supports_debug_accessory

Those all make sense.

> > Doesn't this need locking?
> > And why wouldn't user space want to preselect a mode?
> 
> isn't USB always the default mode ?

Just in case there are no misunderstandings here, Oliver was
commenting on alternate_mode_store function, and USB of course is not
Alternate Mode.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-02-11 at 16:36 +0200, Heikki Krogerus wrote:
> Just in case there are no misunderstandings here, Oliver was
> commenting on alternate_mode_store function, and USB of course is not
> Alternate Mode.

Yes I was because that is the hardest case. In hindsight it is also the
most useless. If you see a billboard device you can still react.
But the point also applies to all "switchable" features,
most importantly PD.
I usually want to charge my phone if it is connected, but not
e.g. if my battery is at less than 60%. Such things belong
to user space and should ideally be setable before anything is
plugged in.

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports
> 4. partner_alt_modes - Lists partner's Alternate Modes when connected

Now that I think about it, there's a gap.
Which SVIDs do we expose if we are UFP (slave)?

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Hi Oliver,

On Wed, Feb 17, 2016 at 03:07:27PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > 1. connected - Connection status of the connector
> > 2. alternate_mode - The current Alternate Mode
> > 3. alternate_modes - Lists all Alternate Modes the connector supports
> > 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> Now that I think about it, there's a gap.
> Which SVIDs do we expose if we are UFP (slave)?

In the alternate_modes listing the connectors alt modes, we can not
have modes that the hardware can not support of course, and it is the
responsibility of the drivers registering the type-c ports with this
clss to make sure they are not part of the list.

In partner alternate modes, we will list all alternate modes the
partner supports, even the ones our connector doesn't.

The modes that can actually be selected have to be supported by both
the connector and the partner, and this is where I'm putting the ball
on the userspace at the moment. I'm not offering a list of
"possible_alternate_modes" where I list the combination, but instead
expect the userspace to be figure out that on it's own.

Do you think we should add "possible_alternate_modes" file?


P.S. That reminds me, here's my current draft for the
Documentation/ABI/. Could you take a look?


Thanks,

-- 
heikki

[-- Attachment #2: sysfs-class-type-c --]
[-- Type: text/plain, Size: 8523 bytes --]

What:		/sys/class/type-c/
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		USB Type-C Connectors can be used for other purposes than just
		connecting USB peripherals to USB host. Devices with USB Type-C
		connector can be of one of the following types:

		1. USB - In normal USB use
		2. Accessory Mode - Audio or Debug. Special purpose modes for
				    the connector which are defined in the USB
				    Type-C specification. When an Accessory is
				    connected to the connector, it can not be
				    used for anything besides what the Accessory
				    Mode defines, including USB.
		3. Alternate Mode - Modes where the connector can be used also
				    for other purposes then just connecting USB
				    peripherals to USB hosts, and which are not
				    defined in USB Type-C Specification. The
				    Alternate Modes depend on USB Power Delivery
				    support as they are controlled with protocol
				    defined in USB Power Delivery specification.

		There is also only one type of USB Type-C Connector, so
		the connector will be the same for both host and peripheral. If
		two systems that are both dual-role capable (can act as both USB
		Host and USB Device) are connected together, the roles are
		selected randomly.

		This class gives the userspace the control over the USB role and
		the Alternate Mode with USB Type-C connectors.

What:		/sys/class/type-c/usbcN/
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		A /sys/class/type-C/usbcN directory is created for each USB
		Type-C connector in the system. The N is the index of the
		connector.

What:		/sys/class/type-c/usbcN/connected
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		Connection status of the USB Type-C connector usbcN. "yes" when
		connected, otherwise "no".

What:		/sys/class/type-c/usbcN/current_alternate_mode
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		When connected to an Alternate Mode, displays the current Mode
		ID and the Standard of Vendor ID (SVID) of the Alternate Mode in
		form:

		<SVID>,<Mode>

		This file is writable and can be used to enter other modes both
		the connector usbcN and the partner support. The modes are
		Alternate Mode specific, and before they are to be changed with
		this file, the exact details of the modes under the given SVID
		should be known by the user.

What:		/sys/class/type-c/usbcN/current_data_role
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		The current USB data role, host or device. This file can be used
		by the userspace to execute data role swap by writing the
		requested role to it.

		When the connector is not connected, the file is used to store
		the preselected role which the system will attempt to use next
		time when connected.

What:		/sys/class/type-c/usbcN/current_power_role
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		The current power role of the connector, source or sink. This
		file can be used by the userspace to execute power role swap by
		writing the requested role to it. USB power role swap depends on
		USB Power Delivery support. When the connector usbcN, or the
		partner it's connected to, do not support USB Power Delivery,
		the power role is defined by the USB data role, i.e. host is
		source and device is sink, and can not be independently swapped
		with this file.

		When the connector is not connected, the file is used to store
		the preselected role which the system will attempt to use next
		time when connected.

What:		/sys/class/type-c/usbcN/partner_alternate_modes
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		When connected, this file lists the Alternate Modes the partner
		connected to connector usbcN supports in the following form:

		<SVID 0>,<Mode 0>
		<SVID 0>,<Mode 1>
		...
		<SVID 0>,<Mode n>
		<SVID 1>,<Mode 0>
		...
		<SVID n>,<Mode n>

		Where the SVID is the 16 bit Standard of Vendor ID, and 32 bit
		Mode n is the ID of the Mode. The modes are Alternate Mode
		specific and are defined in the separate specifications for the
		Alternate Modes.

What:		/sys/class/type-c/usbcN/partner_type
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		This read only file returns the type of partner when connected.
		The partner can be of type:

		- USB - When connected to normal USB host or device
		- Charger - When connected dedicated charger
		- Alternate Mode - when connected to Alternate Mode
		- Audio Accessory
		- Debug Accessory

What:		/sys/class/type-c/usbcN/power_operation_mode
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		This file is read only, and it returns indication of the current
		power level of the connector when connected. It does not return
		the exact voltage and current in use except in case of 1.5A and
		3.0A modes which are defined in USB Type-C specification. The
		power operation mode can be one of the following:

		- USB - When normal power levels defined in USB specifications
			are in use
		- BC1.2 - When power levels defined in Battery Charging
			  Specification v1.2 are in use
		- USB Type-C 1.5A -  Higher 1.5A current compared to the
				     standard USB currents is possible, as
				     defined in USB Type-C specification
		- USB Type-C 3.0A -  Higher 3.0A current compared to the
				     standard USB currents is possible, as
				     defined in USB Type-C specification
		- USB Power Delivery - The voltages and currents defined in USB
				       Power Delivery specification are in use

		The exact power levels are outside the scope of this interface,
		and if needed by userspace, need to be extracted from an other
		source.

What:		/sys/class/type-c/usbcN/supported_alternate_modes
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		This read only file lists the Alternate Modes the connector usbN
		supports in the following form:

		<SVID 0>,<Mode 0>
		<SVID 0>,<Mode 1>
		...
		<SVID 0>,<Mode n>
		<SVID 1>,<Mode 0>
		...
		<SVID n>,<Mode n>

		Where the SVID is the 16 bit Standard or Vendor ID, and 32 bit
		Mode n is the ID of the Mode. The modes are Alternate Mode
		specific and are defined in the separate specifications for the
		Alternate Modes.

What:		/sys/class/type-c/usbcN/supported_data_roles
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		The USB data roles that the connector support.

		Valid values:
		- host
		- device

What:		/sys/class/type-c/usbcN/supported_power_roles
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		The power roles that the connector usbcN support.

		Valid values:
		- source
		- sink

What:		/sys/class/type-c/usbcN/supports_audio_accessory
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		Returns "yes" if the connector usbcN can be used in Audio
		Accessory Mode, otherwise "no". The file is read only.

		In Audio Accessory Mode the connector usbcN will be used as a
		replacement for 3.5mm audio plug, and the pins of the connector
		that the Audio Accessory Mode in USB Type-C specification defines
		are routed to the audio hardware on the system.

What:		/sys/class/type-c/usbcN/supports_debug_accessory
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		Returns "yes" if the connector usbcN can be used in Debug
		Accessory Mode, otherwise "no". The file is read only.

		USB Type-C Specification does not define any details about Debug
		Accessory Mode, not even the pins on the connector. The Debug
		Accessory Mode will therefore be completely platform specific.
		Possible uses of the connector in this mode could be, serial
		port connected to 16550 compatible UART, or for example JTAG
		interface.

What:		/sys/class/type-c/usbcN/supports_usb_power_delivery
Date:		February 2016
Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Description:
		Returns "yes" if the connector usbcN is USB Power Delivery
		capable, otherwise "no". The file is read only.

		USB Power Delivery support will define if the connector usbcN is
		able to use Alternate Modes and swap the power role
		independently of the USB data role.

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:

Hi,

> The modes that can actually be selected have to be supported by both
> the connector and the partner, and this is where I'm putting the ball
> on the userspace at the moment. I'm not offering a list of
> "possible_alternate_modes" where I list the combination, but instead
> expect the userspace to be figure out that on it's own.
> 
> Do you think we should add "possible_alternate_modes" file?

No, what do we answer to the DFP if we recieve "Discover SVIDs"?
I don't think that we always should answer with all we physically
can. If, for example, the hardware could do Thunderbolt, but the OS
is not prepared to handle it, we shouldn't offer it. So this is
a policy decision to be made in user space. Hence we need
an API to tell it to the kernel.

> P.S. That reminds me, here's my current draft for the
> Documentation/ABI/. Could you take a look?

OK

Here are my comments:

What:		/sys/class/type-c/usbcN/connected

		Connection status of the USB Type-C connector usbcN. "yes" when
		connected, otherwise "no".

Unnecessarily wordy. 0 and 1 would do

What:		/sys/class/type-c/usbcN/current_data_role

Again, 0 and 1 would do

What:		/sys/class/type-c/usbcN/partner_alternate_modes

You should say in which number base the values are given.

What:		/sys/class/type-c/usbcN/partner_type

That could be combined with "connected"

What:		/sys/class/type-c/usbcN/supported_data_roles

A connector can be both. How is that expressed?

What:		/sys/class/type-c/usbcN/supported_power_roles

Again, what if it can do both?

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Thu, Feb 18, 2016 at 10:21:48AM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:
> 
> Hi,
> 
> > The modes that can actually be selected have to be supported by both
> > the connector and the partner, and this is where I'm putting the ball
> > on the userspace at the moment. I'm not offering a list of
> > "possible_alternate_modes" where I list the combination, but instead
> > expect the userspace to be figure out that on it's own.
> > 
> > Do you think we should add "possible_alternate_modes" file?
> 
> No, what do we answer to the DFP if we recieve "Discover SVIDs"?
> I don't think that we always should answer with all we physically
> can. If, for example, the hardware could do Thunderbolt, but the OS
> is not prepared to handle it, we shouldn't offer it. So this is
> a policy decision to be made in user space. Hence we need
> an API to tell it to the kernel.

OK. Makes sense.

> > P.S. That reminds me, here's my current draft for the
> > Documentation/ABI/. Could you take a look?
> 
> OK
> 
> Here are my comments:
> 
> What:		/sys/class/type-c/usbcN/connected
> 
> 		Connection status of the USB Type-C connector usbcN. "yes" when
> 		connected, otherwise "no".
> 
> Unnecessarily wordy. 0 and 1 would do

That works for me.

> What:		/sys/class/type-c/usbcN/current_data_role
> 
> Again, 0 and 1 would do

I disagree with this one. What would 0 mean and what would 1? It would
require us to make an agreement about the "index" of the role, which
creates a small risk of somebody getting it wrong, but for what
purpose?

Why couldn't it be human readable "host" or "device" so there is never
no confusion about it.

> What:		/sys/class/type-c/usbcN/partner_alternate_modes
> 
> You should say in which number base the values are given.
> 
> What:		/sys/class/type-c/usbcN/partner_type
> 
> That could be combined with "connected"

Hmm, so in practice getting rid of "connected" completely.. I guess
it's OK.

> What:		/sys/class/type-c/usbcN/supported_data_roles
> 
> A connector can be both. How is that expressed?

"host, device".

> What:		/sys/class/type-c/usbcN/supported_power_roles
> 
> Again, what if it can do both?

"source, sink".

So these last two are now listing the values that can be entered to
the current_data_role and current_power_role.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:

Hi,

> P.S. That reminds me, here's my current draft for the
> Documentation/ABI/. Could you take a look?

And I am afraid, that I have a few remarks not bound
to a specific entry.

We have port directories for port power switching. How is
the connector directory linked to them?

Likewise, if we have USB PD, we have to know how that
is linked to the connector directory.

In addition, writes to those files have results. We need
the error codes to be described.

Furthermore, do these files support poll?

And lastly we can get "Attention" as a message connected
with a connector in an alternate mode. How does user space
learn about that?

I am sorry to be this obnoxious, but this is an API which
will be with us for a long time, so we better get it right.

	HTH
		Oliver
 

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi,

On Thu, Feb 18, 2016 at 10:35:41AM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:
> 
> Hi,
> 
> > P.S. That reminds me, here's my current draft for the
> > Documentation/ABI/. Could you take a look?
> 
> And I am afraid, that I have a few remarks not bound
> to a specific entry.
> 
> We have port directories for port power switching. How is
> the connector directory linked to them?

I'm sorry, I don't think I understand this point.

> Likewise, if we have USB PD, we have to know how that
> is linked to the connector directory.

So you mean when we have USB PD PHY or controller, right? That
will be the parent of the connector device if we have one on the
platform.

I think I'm misunderstanding this point as well..

> In addition, writes to those files have results. We need
> the error codes to be described.

Yes, I need to document those.

> Furthermore, do these files support poll?
> 
> And lastly we can get "Attention" as a message connected
> with a connector in an alternate mode. How does user space
> learn about that?

The class should notify the userspace with uevent on
connection/disconnection regardless what is being connected, or what
mode the connector enters initially.

So do you want to see that explained in the ABI document?

The uevent does not contain any details, but I thought that it's OK to
expect the userspace to read that separately. If this is not OK, let's
add a new uevent variable that specifies what was just connected.

I hope I did not misunderstand also this one.

> I am sorry to be this obnoxious, but this is an API which
> will be with us for a long time, so we better get it right.

I would not say you are being obnoxious. If you are, feel free :).
Your input is most welcome. Thanks a lot for that. And I agree, we
need to make this solid from the beginning.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-02-18 at 15:25 +0200, Heikki Krogerus wrote:

Hi,


> > We have port directories for port power switching. How is
> > the connector directory linked to them?
> 
> I'm sorry, I don't think I understand this point.

Like this:

oneukum@linux-dtbq:/sys/bus/usb/devices/3-0:1.0> ls -l
total 0
-rw-r--r-- 1 root root 4096 Feb 18 14:34 authorized
-r--r--r-- 1 root root 4096 Feb 18 14:34 bAlternateSetting
-r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceClass
-r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceNumber
-r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceProtocol
-r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceSubClass
-r--r--r-- 1 root root 4096 Feb 18 14:34 bNumEndpoints
lrwxrwxrwx 1 root root    0 Feb 17 15:59 driver
-> ../../../../../bus/usb/drivers/hub
drwxr-xr-x 3 root root    0 Feb 18 09:35 ep_81
-r--r--r-- 1 root root 4096 Feb 18 14:34 modalias
drwxr-xr-x 2 root root    0 Feb 18 09:35 power
lrwxrwxrwx 1 root root    0 Feb 17 15:59 subsystem
-> ../../../../../bus/usb
-r--r--r-- 1 root root 4096 Feb 18 14:34 supports_autosuspend
-rw-r--r-- 1 root root 4096 Feb 18 14:34 uevent
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port1
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port10
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port11
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port12
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port13
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port14
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port15
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port2
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port3
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port4
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port5
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port6
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port7
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port8
drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port9

usb*-port*

They correspond to the connectors a system has.
It seems to me that we want a link connecting them
if the correspondance is known.

> > Likewise, if we have USB PD, we have to know how that
> > is linked to the connector directory.
> 
> So you mean when we have USB PD PHY or controller, right? That
> will be the parent of the connector device if we have one on the
> platform.

So the parentage is different on whether a PD controller is present?
That needs to be documented. And so we cannot deal with separate modules
for a PD driver?

[..]
> > Furthermore, do these files support poll?

At least the current role and mode can change, so in principle
poll() makes sense.

> > And lastly we can get "Attention" as a message connected
> > with a connector in an alternate mode. How does user space
> > learn about that?
> 
> The class should notify the userspace with uevent on
> connection/disconnection regardless what is being connected, or what
> mode the connector enters initially.

Yes, but "Attention" in the sense of 6.4.4.3.6 of the PD spec.
Does this need to be handled in the kernel? Do we generate a uevent
for that?

> So do you want to see that explained in the ABI document?

No.

	Regards
		Oliver

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Thu, Feb 18, 2016 at 02:44:25PM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-18 at 15:25 +0200, Heikki Krogerus wrote:
> 
> Hi,
> 
> 
> > > We have port directories for port power switching. How is
> > > the connector directory linked to them?
> > 
> > I'm sorry, I don't think I understand this point.
> 
> Like this:
> 
> oneukum@linux-dtbq:/sys/bus/usb/devices/3-0:1.0> ls -l
> total 0
> -rw-r--r-- 1 root root 4096 Feb 18 14:34 authorized
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bAlternateSetting
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceClass
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceNumber
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceProtocol
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bInterfaceSubClass
> -r--r--r-- 1 root root 4096 Feb 18 14:34 bNumEndpoints
> lrwxrwxrwx 1 root root    0 Feb 17 15:59 driver
> -> ../../../../../bus/usb/drivers/hub
> drwxr-xr-x 3 root root    0 Feb 18 09:35 ep_81
> -r--r--r-- 1 root root 4096 Feb 18 14:34 modalias
> drwxr-xr-x 2 root root    0 Feb 18 09:35 power
> lrwxrwxrwx 1 root root    0 Feb 17 15:59 subsystem
> -> ../../../../../bus/usb
> -r--r--r-- 1 root root 4096 Feb 18 14:34 supports_autosuspend
> -rw-r--r-- 1 root root 4096 Feb 18 14:34 uevent
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port1
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port10
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port11
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port12
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port13
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port14
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port15
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port2
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port3
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port4
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port5
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port6
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port7
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port8
> drwxr-xr-x 3 root root    0 Feb 18 09:35 usb3-port9
> 
> usb*-port*
> 
> They correspond to the connectors a system has.
> It seems to me that we want a link connecting them
> if the correspondance is known.

Ah, got it. In case of ACPI enumerated UCSI, we will have the actual
ACPI device object for the port as a child device object. So when we
attach the port ACPI companion to the connector device we create in
the class, it will link us directly to the correct usb*-port*.

I have not done it so far because the same port ACPI device object
will also be bound to the usb peripheral once it gets enumerated, and
I was worried if that would cause a problem. But after talking to guys
that know more about ACPI then I do, I'm sure that is not going to be
a problem.

With ACPI, the binding should happen the same way even without UCSI.
What ever device driver registers the connector device should have the
port ACPI device object as it's child. So I'm thinking about doing
this in typec_register_port() and not in UCSI driver.


Thanks,

-- 
heikki

Re: [PATCH 1/3] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi Oliver,

I've missed couple of your questions..

> > > Furthermore, do these files support poll?
> 
> At least the current role and mode can change, so in principle
> poll() makes sense.

Yes I agree. I'll add support for polling.

> > > And lastly we can get "Attention" as a message connected
> > > with a connector in an alternate mode. How does user space
> > > learn about that?
> > 
> > The class should notify the userspace with uevent on
> > connection/disconnection regardless what is being connected, or what
> > mode the connector enters initially.
> 
> Yes, but "Attention" in the sense of 6.4.4.3.6 of the PD spec.
> Does this need to be handled in the kernel? Do we generate a uevent
> for that?

OK, got it. Not for now, but it makes sense, though for example with
UCSI we don't receive any notification in case of Attention, so this
is something that is not going to be always available.

I'm going to be away next week, but I'm planning to prepare v2 of
these patches after that. I'll also propose something for handling
Attention Command and polling then.


Thanks,

-- 
heikki

[PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

USB Type-C Connector System Software Interface (UCSI) is a
specification that defines registers and data structures
used to interface with the USB Type-C connectors on a system.

The specification is public and available at:
http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/type-c/Kconfig  |   8 +
 drivers/usb/type-c/Makefile |   1 +
 drivers/usb/type-c/ucsi.c   | 450 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/type-c/ucsi.h   | 219 +++++++++++++++++++++
 4 files changed, 678 insertions(+)
 create mode 100644 drivers/usb/type-c/ucsi.c
 create mode 100644 drivers/usb/type-c/ucsi.h

diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
index b229fb9..02abd74 100644
--- a/drivers/usb/type-c/Kconfig
+++ b/drivers/usb/type-c/Kconfig
@@ -4,4 +4,12 @@ menu "USB PD and Type-C drivers"
 config TYPEC
 	tristate
 
+config TYPEC_UCSI
+	tristate "USB Type-C Connector System Software Interface"
+	select TYPEC
+	help
+	  USB Type-C Connector System Software Interface (UCSI) describes the
+	  registers and data structures used to interface with the USB Type-C
+	  connectors on a system.
+
 endmenu
diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
index 1012a8b..ab974ba 100644
--- a/drivers/usb/type-c/Makefile
+++ b/drivers/usb/type-c/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
+obj-$(CONFIG_TYPEC_UCSI)	+= ucsi.o
diff --git a/drivers/usb/type-c/ucsi.c b/drivers/usb/type-c/ucsi.c
new file mode 100644
index 0000000..0107a85
--- /dev/null
+++ b/drivers/usb/type-c/ucsi.c
@@ -0,0 +1,450 @@
+/*
+ * ucsi.c - USB Type-C Connector System Software Interface
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb/typec.h>
+
+#include "ucsi.h"
+
+#define UCSI_ERROR 1
+#define UCSI_BUSY 2
+
+#define to_ucsi_connector(_port_) container_of(_port_->cap,                    \
+					       struct ucsi_connector,          \
+					       typec_cap)
+
+#define cci_to_connector(_ucsi_, cci) (_ucsi_->connector +		       \
+					UCSI_CCI_CONNECTOR_CHANGE(cci) - 1)
+
+struct ucsi_connector {
+	unsigned num;
+	struct ucsi *ucsi;
+	struct work_struct work;
+	struct typec_port *port;
+	struct typec_capability typec_cap;
+	struct ucsi_connector_capability cap;
+};
+
+struct ucsi {
+	struct device *dev;
+	struct ucsi_ppm *ppm;
+
+	int status;
+	struct completion complete;
+	struct ucsi_capability cap;
+	struct ucsi_connector *connector;
+};
+
+static int ucsi_ack(struct ucsi *ucsi, u8 cmd)
+{
+	struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
+	int ret;
+
+	ucsi->ppm->data->control = 0;
+	ctrl->cmd = UCSI_ACK_CC_CI;
+	ctrl->data = cmd;
+
+	ret = ucsi->ppm->cmd(ucsi->ppm);
+	if (ret)
+		return ret;
+
+	/* Waiting for ACK also with ACK CMD for now */
+	wait_for_completion(&ucsi->complete);
+	return 0;
+}
+
+static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
+{
+	int status;
+	int ret;
+
+	dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
+		 ucsi->ppm->data->control);
+
+	ret = ucsi->ppm->cmd(ucsi->ppm);
+	if (ret)
+		return ret;
+
+	/* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
+	wait_for_completion(&ucsi->complete);
+
+	status = ucsi->status;
+	if (status != UCSI_ERROR && size)
+		memcpy(data, ucsi->ppm->data->message_in, size);
+
+	ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
+	if (ret)
+		goto out;
+
+	if (status == UCSI_ERROR) {
+		u16 error;
+
+		ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
+		ret = ucsi->ppm->cmd(ucsi->ppm);
+		if (ret)
+			goto out;
+
+		wait_for_completion(&ucsi->complete);
+
+		/* Something has really gone wrong */
+		if (ucsi->status == UCSI_ERROR) {
+			ret = -ENODEV;
+			goto out;
+		}
+
+		memcpy(&error, ucsi->ppm->data->message_in, sizeof(error));
+
+		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
+		if (ret)
+			goto out;
+
+		switch (error) {
+		case UCSI_ERROR_INVALID_CON_NUM:
+			ret = -ENXIO;
+			break;
+		case UCSI_ERROR_INCOMPATIBLE_PARTNER:
+		case UCSI_ERROR_CC_COMMUNICATION_ERR:
+		case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
+			ret = -EIO;
+			break;
+		case UCSI_ERROR_DEAD_BATTERY:
+			dev_warn(ucsi->dev, "Dead Battery Condition!\n");
+			ret = -EPERM;
+			break;
+		case UCSI_ERROR_UNREGONIZED_CMD:
+		case UCSI_ERROR_INVALID_CMD_ARGUMENT:
+		default:
+			ret = -EINVAL;
+			break;
+		}
+	}
+out:
+	ucsi->ppm->data->control = 0;
+	return ret;
+}
+
+static int ucsi_dr_swap(struct typec_port *port)
+{
+	struct ucsi_connector *con = to_ucsi_connector(port);
+	struct ucsi_uor_cmd *ctrl = (void *)&con->ucsi->ppm->data->control;
+
+	ctrl->cmd = UCSI_SET_UOR;
+	ctrl->con_num = con->num;
+	ctrl->role = port->data_role == TYPEC_HOST ?
+			UCSI_UOR_ROLE_UFP : UCSI_UOR_ROLE_DFP;
+	if (port->cap->type == TYPEC_PORT_DRP)
+		ctrl->role |= UCSI_UOR_ROLE_DRP;
+
+	return ucsi_run_cmd(con->ucsi, NULL, 0);
+}
+
+static int ucsi_pr_swap(struct typec_port *port)
+{
+	struct ucsi_connector *con = to_ucsi_connector(port);
+	struct ucsi_uor_cmd *ctrl = (void *)&con->ucsi->ppm->data->control;
+
+	/* The command structure is identical to SET_UOR command structure */
+	ctrl->cmd = UCSI_SET_PDR;
+	ctrl->con_num = con->num;
+	ctrl->role = port->pwr_role == TYPEC_PWR_SOURCE ?
+			UCSI_UOR_ROLE_UFP : UCSI_UOR_ROLE_DFP;
+	/* Always accepting power swap requests from partner for now */
+	ctrl->role |= UCSI_UOR_ROLE_DRP;
+
+	return ucsi_run_cmd(con->ucsi, NULL, 0);
+}
+
+static int ucsi_get_constat(struct ucsi_connector *con,
+			    struct ucsi_connector_status *constat)
+{
+	struct ucsi_control *ctrl = (void *)&con->ucsi->ppm->data->control;
+
+	ctrl->cmd = UCSI_GET_CONNECTOR_STATUS;
+	ctrl->data = con->num;
+
+	return ucsi_run_cmd(con->ucsi, constat, sizeof(*constat));
+}
+
+static int
+ucsi_connect(struct ucsi_connector *con, struct ucsi_connector_status *constat)
+{
+	struct typec_port *port = con->port;
+
+	port->connected = true;
+
+	if (constat->partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
+		port->partner_type = TYPEC_PARTNER_ALTMODE;
+	else
+		port->partner_type = TYPEC_PARTNER_USB;
+
+	switch (constat->partner_type) {
+	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP:
+		/* REVISIT: We don't care about just the cable for now */
+		return 0;
+	case UCSI_CONSTAT_PARTNER_TYPE_DFP:
+	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
+		port->pwr_role = TYPEC_PWR_SINK;
+		port->data_role = TYPEC_DEVICE;
+		break;
+	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
+		port->pwr_role = TYPEC_PWR_SOURCE;
+		port->data_role = TYPEC_HOST;
+		break;
+	case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
+		port->partner_type = TYPEC_PARTNER_DEBUG;
+		goto out;
+	case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
+		port->partner_type = TYPEC_PARTNER_AUDIO;
+		goto out;
+	}
+
+	switch (constat->pwr_op_mode) {
+	case UCSI_CONSTAT_PWR_OPMODE_NONE:
+	case UCSI_CONSTAT_PWR_OPMODE_DEFAULT:
+		port->pwr_opmode = TYPEC_PWR_MODE_USB;
+		break;
+	case UCSI_CONSTAT_PWR_OPMODE_BC:
+		port->partner_type = TYPEC_PARTNER_CHARGER;
+		port->pwr_opmode = TYPEC_PWR_MODE_BC1_2;
+		break;
+	case UCSI_CONSTAT_PWR_OPMODE_PD:
+		port->pwr_opmode = TYPEC_PWR_MODE_PD;
+		break;
+	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3:
+		port->pwr_opmode = TYPEC_PWR_MODE_1_5A;
+		break;
+	case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
+		port->pwr_opmode = TYPEC_PWR_MODE_3_0A;
+		break;
+	default:
+		break;
+	}
+out:
+	return typec_connect(port);
+}
+
+static void ucsi_disconnect(struct ucsi_connector *con)
+{
+	con->port->partner_type = TYPEC_PARTNER_NONE;
+	con->port->connected = false;
+	typec_disconnect(con->port);
+}
+
+static void ucsi_connector_change(struct work_struct *work)
+{
+	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
+						  work);
+	struct ucsi_connector_status constat;
+
+	ucsi_ack(con->ucsi, UCSI_ACK_EVENT);
+
+	if (WARN_ON(ucsi_get_constat(con, &constat) != 0))
+		return;
+
+	if (constat.constat_change & UCSI_CONSTAT_CONNECT_CHANGE) {
+		if (constat.connected)
+			ucsi_connect(con, &constat);
+		else
+			ucsi_disconnect(con);
+	}
+}
+
+/**
+ * ucsi_interrupt - UCSI Notification Handler
+ * @ucsi: Source UCSI Interface for the notifications
+ *
+ * Handle notifications from @ucsi.
+ */
+int ucsi_interrupt(struct ucsi *ucsi)
+{
+	u32 cci = ucsi->ppm->data->cci;
+
+	if (!cci)
+		return 0;
+
+	if (UCSI_CCI_CONNECTOR_CHANGE(cci)) {
+		struct ucsi_connector *con = cci_to_connector(ucsi, cci);
+
+		schedule_work(&con->work);
+		return 1;
+	}
+
+	ucsi->status = 0;
+
+	/* REVISIT: We don't actually do anything with this for now */
+	if (cci & UCSI_CCI_BUSY)
+		ucsi->status = UCSI_BUSY;
+
+	if (cci & UCSI_CCI_ERROR)
+		ucsi->status = UCSI_ERROR;
+
+	if (cci & UCSI_CCI_ACK_CMD || cci & UCSI_CCI_CMD_COMPLETED)
+		complete(&ucsi->complete);
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(ucsi_interrupt);
+
+/**
+ * ucsi_init - Initialize an UCSI Interface
+ * @ucsi: The UCSI Interface
+ *
+ * Registers all the USB Type-C ports governed by the PPM of @ucsi and enables
+ * all the notifications from the PPM.
+ */
+int ucsi_init(struct ucsi *ucsi)
+{
+	struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
+	struct ucsi_connector *con;
+	int ret;
+	int i;
+
+	/* Enable basic notifications */
+	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
+	ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
+	ret = ucsi_run_cmd(ucsi, NULL, 0);
+	if (ret)
+		return ret;
+
+	/* Get PPM capabilities */
+	ctrl->cmd = UCSI_GET_CAPABILITY;
+	ret = ucsi_run_cmd(ucsi, &ucsi->cap, sizeof(ucsi->cap));
+	if (ret)
+		return ret;
+
+	ucsi->connector = kcalloc(ucsi->cap.num_connectors,
+				  sizeof(struct ucsi_connector), GFP_KERNEL);
+	if (!ucsi->connector)
+		return -ENOMEM;
+
+	for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
+	     i++, con++) {
+		struct typec_capability *cap = &con->typec_cap;
+		struct ucsi_connector_status constat;
+
+		/* Get connector capability */
+		ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
+		ctrl->data = i + 1;
+		ret = ucsi_run_cmd(ucsi, &con->cap, sizeof(con->cap));
+		if (ret)
+			goto err;
+
+		/* Register the connector */
+
+		if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
+			cap->type = TYPEC_PORT_DRP;
+		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
+			cap->type = TYPEC_PORT_DFP;
+		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
+			cap->type = TYPEC_PORT_UFP;
+
+		cap->usb_pd = !!(ucsi->cap.attributes &
+				       UCSI_CAP_ATTR_USB_PD);
+		cap->audio_accessory = !!(con->cap.op_mode &
+					  UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
+		cap->debug_accessory = !!(con->cap.op_mode &
+					  UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
+
+		/* TODO: Alt modes */
+
+		cap->dr_swap = ucsi_dr_swap;
+		cap->pr_swap = ucsi_pr_swap;
+
+		con->port = typec_register_port(ucsi->dev, cap);
+		if (IS_ERR(con->port)) {
+			ret = PTR_ERR(con->port);
+			goto err;
+		}
+
+		con->num = i + 1;
+		con->ucsi = ucsi;
+		INIT_WORK(&con->work, ucsi_connector_change);
+
+		/* Check if the connector is connected */
+		if (WARN_ON(ucsi_get_constat(con, &constat) != 0))
+			continue;
+
+		if (constat.connected)
+			ucsi_connect(con, &constat);
+	}
+
+	/* Enable all notifications */
+	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
+	ctrl->data = UCSI_ENABLE_NTFY_ALL;
+	ret = ucsi_run_cmd(ucsi, NULL, 0);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	if (i > 0)
+		for (; i >= 0; i--, con--)
+			typec_unregister_port(con->port);
+
+	kfree(ucsi->connector);
+	return ret;
+}
+EXPORT_SYMBOL(ucsi_init);
+
+/**
+ * ucsi_register_ppm - Register UCSI PPM Interface
+ * @dev: Device interface to the PPM
+ * @ppm: The PPM interface
+ *
+ * Allocates an UCSI instance, associates it with @ppm and returns it to the
+ * caller.
+ */
+struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
+{
+	struct ucsi *ucsi;
+
+	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
+	if (!ucsi)
+		return ERR_PTR(-ENOMEM);
+
+	init_completion(&ucsi->complete);
+	ucsi->dev = dev;
+	ucsi->ppm = ppm;
+
+	return ucsi;
+}
+EXPORT_SYMBOL_GPL(ucsi_register_ppm);
+
+/**
+ * ucsi_unregister_ppm - Unregister UCSI PPM Interface
+ * @ucsi: struct ucsi associated with the PPM
+ *
+ * Unregister an UCSI PPM that was created with ucsi_register().
+ */
+void ucsi_unregister_ppm(struct ucsi *ucsi)
+{
+	struct ucsi_connector *con;
+	int i;
+
+	/* Disable all notifications */
+	ucsi->ppm->data->control = UCSI_SET_NOTIFICATION_ENABLE;
+	ucsi->ppm->cmd(ucsi->ppm);
+
+	for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
+	     i++, con++)
+		typec_unregister_port(con->port);
+
+	kfree(ucsi->connector);
+	kfree(ucsi);
+}
+EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("USB Type-C System Software Interface driver");
diff --git a/drivers/usb/type-c/ucsi.h b/drivers/usb/type-c/ucsi.h
new file mode 100644
index 0000000..0ec6366
--- /dev/null
+++ b/drivers/usb/type-c/ucsi.h
@@ -0,0 +1,219 @@
+
+#include <linux/types.h>
+
+/* -------------------------------------------------------------------------- */
+
+struct ucsi_data {
+	__u16 version;
+	__u16 RESERVED;
+	__u32 cci;
+	__u64 control;
+	__u32 message_in[4];
+	__u32 message_out[4];
+} __packed;
+
+struct ucsi_control {
+	__u8 cmd;
+	__u8 length;
+	__u64 data:48;
+} __packed;
+
+/* Command Status and Connector Change Indication (CCI) bits */
+#define UCSI_CCI_CONNECTOR_CHANGE(c)	((c >> 1) & 0x7f)
+#define UCSI_CCI_DATA_LENGTH(c)		((c >> 8) & 0xff)
+#define UCSI_CCI_NOT_SUPPORTED		BIT(25)
+#define UCSI_CCI_CANCEL_CMD		BIT(26)
+#define UCSI_CCI_RESET_CMD		BIT(27)
+#define UCSI_CCI_BUSY			BIT(28)
+#define UCSI_CCI_ACK_CMD		BIT(29)
+#define UCSI_CCI_ERROR			BIT(30)
+#define UCSI_CCI_CMD_COMPLETED		BIT(31)
+
+/* Commands */
+#define UCSI_PPM_RESET			0x01
+#define UCSI_CANCEL			0x02
+#define UCSI_CONNECTOR_RESET		0x03
+#define UCSI_ACK_CC_CI			0x04
+#define UCSI_SET_NOTIFICATION_ENABLE	0x05
+#define UCSI_GET_CAPABILITY		0x06
+#define UCSI_GET_CONNECTOR_CAPABILITY	0x07
+#define UCSI_SET_UOM			0x08
+#define UCSI_SET_UOR			0x09
+#define UCSI_SET_PDM			0x0A
+#define UCSI_SET_PDR			0x0B
+#define UCSI_GET_ALTERNATE_MODES	0x0C
+#define UCSI_GET_CAM_SUPPORTED		0x0D
+#define UCSI_GET_CURRENT_CAM		0x0E
+#define UCSI_SET_NEW_CAM		0x0F
+#define UCSI_GET_PDOS			0x10
+#define UCSI_GET_CABLE_PROPERTY		0x11
+#define UCSI_GET_CONNECTOR_STATUS	0x12
+#define UCSI_GET_ERROR_STATUS		0x13
+
+/* ACK_CC_CI commands */
+#define UCSI_ACK_EVENT			1
+#define UCSI_ACK_CMD			2
+
+/* Bits for SET_NOTIFICATION_ENABLE command */
+#define UCSI_ENABLE_NTFY_CMD_COMPLETE		BIT(0)
+#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE	BIT(1)
+#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE	BIT(2)
+#define UCSI_ENABLE_NTFY_CAP_CHANGE		BIT(5)
+#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE	BIT(6)
+#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE	BIT(7)
+#define UCSI_ENABLE_NTFY_CAM_CHANGE		BIT(8)
+#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE	BIT(9)
+#define UCSI_ENABLE_NTFY_PARTNER_CHANGE		BIT(11)
+#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE		BIT(12)
+#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE	BIT(14)
+#define UCSI_ENABLE_NTFY_ERROR			BIT(15)
+#define UCSI_ENABLE_NTFY_ALL			0xdbf3
+
+/* Error information returned by PPM in response to GET_ERROR_STATUS command. */
+#define UCSI_ERROR_UNREGONIZED_CMD		BIT(0)
+#define UCSI_ERROR_INVALID_CON_NUM		BIT(1)
+#define UCSI_ERROR_INVALID_CMD_ARGUMENT		BIT(2)
+#define UCSI_ERROR_INCOMPATIBLE_PARTNER		BIT(3)
+#define UCSI_ERROR_CC_COMMUNICATION_ERR		BIT(4)
+#define UCSI_ERROR_DEAD_BATTERY			BIT(5)
+#define UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL	BIT(6)
+
+/* Set USB Operation Role Command structure */
+struct ucsi_uor_cmd {
+	__u8 cmd;
+	__u8 length;
+	__u8 con_num:7;
+	__u64 role:3;
+#define UCSI_UOR_ROLE_DFP			BIT(0)
+#define UCSI_UOR_ROLE_UFP			BIT(1)
+#define UCSI_UOR_ROLE_DRP			BIT(2)
+	__u64 data:38;
+} __packed;
+
+/* Data structure filled by PPM in response to GET_CAPABILITY command. */
+struct ucsi_capability {
+	__u32 attributes;
+#define UCSI_CAP_ATTR_DISABLE_STATE		BIT(0)
+#define UCSI_CAP_ATTR_BATTERY_CHARGING		BIT(1)
+#define UCSI_CAP_ATTR_USB_PD			BIT(2)
+#define UCSI_CAP_ATTR_TYPEC_CURRENT		BIT(6)
+#define UCSI_CAP_ATTR_POWER_AC_SUPPLY		BIT(8)
+#define UCSI_CAP_ATTR_POWER_OTHER		BIT(10)
+#define UCSI_CAP_ATTR_POWER_VBUS		BIT(14)
+	__u8 num_connectors;
+	__u32 features:24;
+#define UCSI_CAP_SET_UOM			BIT(0)
+#define UCSI_CAP_SET_PDM			BIT(1)
+#define UCSI_CAP_ALT_MODE_DETAILS		BIT(2)
+#define UCSI_CAP_ALT_MODE_OVERRIDE		BIT(3)
+#define UCSI_CAP_PDO_DETAILS			BIT(4)
+#define UCSI_CAP_CABLE_DETAILS			BIT(5)
+#define UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS	BIT(6)
+#define UCSI_CAP_PD_RESET			BIT(7)
+	__u8 num_alt_modes;
+	__u8 RESERVED;
+	__u16 bc_version;
+	__u16 pd_version;
+	__u16 typec_version;
+} __packed;
+
+/* Data structure filled by PPM in response to GET_CONNECTOR_CAPABILITY cmd. */
+struct ucsi_connector_capability {
+	__u8 op_mode;
+#define UCSI_CONCAP_OPMODE_DFP			BIT(0)
+#define UCSI_CONCAP_OPMODE_UFP			BIT(1)
+#define UCSI_CONCAP_OPMODE_DRP			BIT(2)
+#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY	BIT(3)
+#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY	BIT(4)
+#define UCSI_CONCAP_OPMODE_USB2			BIT(5)
+#define UCSI_CONCAP_OPMODE_USB3			BIT(6)
+#define UCSI_CONCAP_OPMODE_ALT_MODE		BIT(7)
+	__u8 provider:1;
+	__u8 consumer:1;
+} __packed;
+
+/* Data structure filled by PPM in response to GET_ALTERNATE_MODES command. */
+struct ucsi_alt_modes {
+	__u32 svid0;
+	__u16 mid0;
+	__u32 svid1;
+	__u16 mid1;
+} __packed;
+
+/* Data structure filled by PPM in response to GET_CABLE_PROPERTY command. */
+struct ucsi_cable_property {
+	__u16 speed_supported;
+	__u8 current_capability;
+	__u8 vbus_in_cable:1;
+	__u8 active_cable:1;
+	__u8 directionality:1;
+	__u8 plug_type:2;
+#define UCSI_CABLE_PROPERTY_PLUG_TYPE_A		0
+#define UCSI_CABLE_PROPERTY_PLUG_TYPE_B		1
+#define UCSI_CABLE_PROPERTY_PLUG_TYPE_C		2
+#define UCSI_CABLE_PROPERTY_PLUG_OTHER		3
+	__u8 mode_support:1;
+	__u8 RESERVED_2:2;
+	__u8 latency:4;
+	__u8 RESERVED_4:4;
+} __packed;
+
+/* Data structure filled by PPM in response to GET_CONNECTOR_STATUS command. */
+struct ucsi_connector_status {
+	__u16 constat_change;
+#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE		BIT(1)
+#define UCSI_CONSTAT_POWER_OPMODE_CHANGE	BIT(2)
+#define UCSI_CONSTAT_PDOS_CHANGE		BIT(5)
+#define UCSI_CONSTAT_POWER_LEVEL_CHANGE		BIT(6)
+#define UCSI_CONSTAT_PD_RESET_COMPLETE		BIT(7)
+#define UCSI_CONSTAT_CAM_CHANGE			BIT(8)
+#define UCSI_CONSTAT_BC_CHANGE			BIT(9)
+#define UCSI_CONSTAT_PARTNER_CHANGE		BIT(11)
+#define UCSI_CONSTAT_POWER_DIR_CHANGE		BIT(12)
+#define UCSI_CONSTAT_CONNECT_CHANGE		BIT(14)
+#define UCSI_CONSTAT_ERROR			BIT(15)
+	__u16 pwr_op_mode:3;
+#define UCSI_CONSTAT_PWR_OPMODE_NONE		0
+#define UCSI_CONSTAT_PWR_OPMODE_DEFAULT		1
+#define UCSI_CONSTAT_PWR_OPMODE_BC		2
+#define UCSI_CONSTAT_PWR_OPMODE_PD		3
+#define UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3	4
+#define UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0	5
+	__u16 connected:1;
+	__u16 pwr_dir:1;
+	__u16 partner_flags:8;
+#define UCSI_CONSTAT_PARTNER_FLAG_USB		BIT(0)
+#define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE	BIT(1)
+	__u16 partner_type:3;
+#define UCSI_CONSTAT_PARTNER_TYPE_DFP		1
+#define UCSI_CONSTAT_PARTNER_TYPE_UFP		2
+#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP	3 /* Powered Cable */
+#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP	4 /* Powered Cable */
+#define UCSI_CONSTAT_PARTNER_TYPE_DEBUG		5
+#define UCSI_CONSTAT_PARTNER_TYPE_AUDIO		6
+	__u32 request_data_obj;
+	__u8 bc_status;
+#define UCSI_CONSTAT_BC_NOT_CHARGING		0
+#define UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
+#define UCSI_CONSTAT_BC_SLOW_CHARGING		2
+#define UCSI_CONSTAT_BC_TRICLE_CHARGING		3
+} __packed;
+
+/* -------------------------------------------------------------------------- */
+
+struct ucsi;
+
+/*
+ * struct ucsi_ppm - Interface to an UCSI Platform Policy Manager
+ * @data: memory location to the UCSI data structures
+ * @cmd: UCSI command execution routine
+ */
+struct ucsi_ppm {
+	struct ucsi_data *data;
+	int (*cmd)(struct ucsi_ppm *);
+};
+
+struct ucsi *ucsi_register_ppm(struct device *, struct ucsi_ppm *);
+void ucsi_unregister_ppm(struct ucsi *);
+int ucsi_init(struct ucsi *);
+int ucsi_interrupt(struct ucsi *);
-- 
2.7.0

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
> 
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> 

What does this driver / code actually do?  Why is it needed?  What
interface to the rest of the kernel / userspace does it provide?

Why would we care about this?


You need to describe this a lot better than you did...

thanks,

greg k-h

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > USB Type-C Connector System Software Interface (UCSI) is a
> > specification that defines registers and data structures
> > used to interface with the USB Type-C connectors on a system.
> > 
> > The specification is public and available at:
> > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > 
> 
> What does this driver / code actually do?  Why is it needed?  What
> interface to the rest of the kernel / userspace does it provide?

I will fix this describe these things in the commit message. I'll just
but some UCSI background in case somebody is interested. So UCSI is in
practice a standard for USB Type-C controllers..

UCSI is the control interface for USB Type-C connectors (regardless
was USB PD supported or not) in MS Windows, so most likely all new HW
platforms designed to work also with Windows that are equipped with
USB Type-C will have UCSI device for controlling the USB Type-C ports.
In some cases the hardware for Type-C will be just a PHY like fusb30x
on these platforms (it's cheaper then USB PD or complete USB Type-C
controller), but in those cases the PHY is probable attached to an EC
or is completely controlled by system FW like BIOS together with any
USB PD communication in cases where USB PD is supported, and is in any
case not visible to the OS. Instead UCSI device is exposed to the OS
to give it means to apply its policies to the USB Type-C port.

> Why would we care about this?

I'll try to explain why it's important to export the control of USB
Type-C ports to the user space in my answer to your comments to the
first patch of this series, the one introducing the class.

But surely everybody agrees that decision about the policies regarding
USB Type-C ports, like which data role to use, do we charge or are we
letting the other end charge, etc., belongs to the user? If you plug
your phone to your desktop, I would imagine that you want to see the
phone primarily as the USB device and the desktop as host, and to
achieve the device role, you don't want to be forced to unplug/replug
your phone from the desktop until you achieve device role, right?

> You need to describe this a lot better than you did...

Sure thing. I'm sorry about the poor description. I send these out too
hastily.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 
> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.

There's many millions of devices with type-C without Windows on them, so
don't count on this being everywhere :)

> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user?

No, I don't agree.  It's still unknown if userspace can react fast
though to these types of "policy" changes.  I've heard from some
manufacturers that the response time needed is something that we can't
leave to userspace.

And along those lines, do you have a working userspace user of this
interface?  We don't create interfaces without a user, especially given
that it takes a long time to ensure that a user/kernel api actually is
correct.  We would need to see that to ensure that this kernel
implementation is "correct" and working properly.

> If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?

Why is unplugging somehow required?

thanks,

greg k-h

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Greg,

> > But surely everybody agrees that decision about the policies regarding
> > USB Type-C ports, like which data role to use, do we charge or are we
> > letting the other end charge, etc., belongs to the user?
> 
> No, I don't agree.  It's still unknown if userspace can react fast
> though to these types of "policy" changes.  I've heard from some
> manufacturers that the response time needed is something that we can't
> leave to userspace.

There are no restrictions on when role swapping could to be executed
or when an alt mode can be entered after the connection is made and
an initial role and mode are set. The timing constraints these guys
are most likely talking about are related to the USB PD functions that
need to be executed, for example DR_Swap when the data role swap is
requested and so on. But those are a problem for the drivers that
implement the dr_swap, pr_swap and set_alt_mode, or more likely the
PD stack, and indeed happen inside kernel.

This is probable just a misunderstand. I'm not talking about USB PD
Policy, Device Manager, System Policy Manager or anything else USB PD
spec defines. Those things will indeed happen inside kernel.

My little class is just a high level interface that allows userspace
to request kernel to do things which then end up being executed inside
kernel. There really should not be any problem here.

> And along those lines, do you have a working userspace user of this
> interface?  We don't create interfaces without a user, especially given
> that it takes a long time to ensure that a user/kernel api actually is
> correct.  We would need to see that to ensure that this kernel
> implementation is "correct" and working properly.

No users (well, let me get back on this). I want to force peoples hand
with this early because, if we exclude details about the cable, which
I don't see of any interest to the userspace, the functions and
features USB Type-C spec defines are what I'm presenting, and that's
it. Unless newer versions of USB Type-C connectors bring something
different to the table, the interface is solid. We just need to fine
tune it, agree on what are proper names for the files, etc.

There is just one function that USB Type-C spec has defined that I
have left out of the interface. That is VCONN swapping. I left it out
on purpose as it is cable specific, but I'm now thinking about adding
that as well. It's not like you have to use it, so why not.

> > If you plug
> > your phone to your desktop, I would imagine that you want to see the
> > phone primarily as the USB device and the desktop as host, and to
> > achieve the device role, you don't want to be forced to unplug/replug
> > your phone from the desktop until you achieve device role, right?
> 
> Why is unplugging somehow required?

Because USB Type-C ports (DRP ones) will select the data role randomly
when you connect (to an other DRP port). USB Type-C spec defines that
you can "prefer" host mode, but when both ends prefer host mode, it's
+-0.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> Because USB Type-C ports (DRP ones) will select the data role randomly
> when you connect (to an other DRP port). USB Type-C spec defines that
> you can "prefer" host mode, but when both ends prefer host mode, it's
> +-0.

That question has not been answered. It would be awkward for the OS
to find itself in the slave role, which it is ill equipped for. So
the data role should be switched before the new device is announced
to user space. How is that handled?

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi,

On Mon, Feb 15, 2016 at 04:30:18PM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> > Because USB Type-C ports (DRP ones) will select the data role randomly
> > when you connect (to an other DRP port). USB Type-C spec defines that
> > you can "prefer" host mode, but when both ends prefer host mode, it's
> > +-0.
> 
> That question has not been answered. It would be awkward for the OS
> to find itself in the slave role, which it is ill equipped for. So
> the data role should be switched before the new device is announced
> to user space. How is that handled?

In the class driver, once we add support for preselecting the role,
when the connection happens we compare the initial role to the
preselected one and execute swap if it differs. Only after that we
notify userspace.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > That question has not been answered. It would be awkward for the OS
> > to find itself in the slave role, which it is ill equipped for. So
> > the data role should be switched before the new device is announced
> > to user space. How is that handled?
> 
> In the class driver, once we add support for preselecting the role,
> when the connection happens we compare the initial role to the
> preselected one and execute swap if it differs. Only after that we
> notify userspace.

Yes, but we need an API. We can't keep adding to it. So if that
is to be supported, it needs to be defined now.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > That question has not been answered. It would be awkward for the OS
> > > to find itself in the slave role, which it is ill equipped for. So
> > > the data role should be switched before the new device is announced
> > > to user space. How is that handled?
> > 
> > In the class driver, once we add support for preselecting the role,
> > when the connection happens we compare the initial role to the
> > preselected one and execute swap if it differs. Only after that we
> > notify userspace.
> 
> Yes, but we need an API. We can't keep adding to it. So if that
> is to be supported, it needs to be defined now.

When you say API, do you mean the API the class provides to the
drivers? Or did you mean ABI which would be the sysfs in this case?

For the sysfs I would image we can manage with the current files,
current_data_role and current_power_role. If somebody writes to them
when we are disconnected, we still callback the dr_swap or pr_swap
hooks, and make a rule that when disconnected, it means we are
setting the "preferred" roles.

Would that be OK? Or did I still misunderstood your question?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > > That question has not been answered. It would be awkward for the OS
> > > > to find itself in the slave role, which it is ill equipped for. So
> > > > the data role should be switched before the new device is announced
> > > > to user space. How is that handled?
> > > 
> > > In the class driver, once we add support for preselecting the role,
> > > when the connection happens we compare the initial role to the
> > > preselected one and execute swap if it differs. Only after that we
> > > notify userspace.
> > 
> > Yes, but we need an API. We can't keep adding to it. So if that
> > is to be supported, it needs to be defined now.
> 
> When you say API, do you mean the API the class provides to the
> drivers? Or did you mean ABI which would be the sysfs in this case?

The API to user space. That is the point. We cannot break user space.
Once this sysfs API is upstream we are stuck with it.

> For the sysfs I would image we can manage with the current files,
> current_data_role and current_power_role. If somebody writes to them
> when we are disconnected, we still callback the dr_swap or pr_swap
> hooks, and make a rule that when disconnected, it means we are
> setting the "preferred" roles.
> 
> Would that be OK? Or did I still misunderstood your question?

That would be absolutely OK, but that function needs to be decided on
and documented as all files in sysfs are. And likewise it needs to be
documented that alternate modes behave differently.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]


Hi,

Oliver Neukum <oneukum@suse.com> writes:
> On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
>> > > > That question has not been answered. It would be awkward for the OS
>> > > > to find itself in the slave role, which it is ill equipped for. So
>> > > > the data role should be switched before the new device is announced
>> > > > to user space. How is that handled?
>> > > 
>> > > In the class driver, once we add support for preselecting the role,
>> > > when the connection happens we compare the initial role to the
>> > > preselected one and execute swap if it differs. Only after that we
>> > > notify userspace.
>> > 
>> > Yes, but we need an API. We can't keep adding to it. So if that
>> > is to be supported, it needs to be defined now.
>> 
>> When you say API, do you mean the API the class provides to the
>> drivers? Or did you mean ABI which would be the sysfs in this case?
>
> The API to user space. That is the point. We cannot break user space.
> Once this sysfs API is upstream we are stuck with it.

yeah, in fact I have been wondering if sysfs is the best interface to
userspace. I talked with Heikki a few days back about this; I was
wondering if something like what the NFC folks did with netlink would be
better here.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum <oneukum@suse.com> writes:
> > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:

> >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > is to be supported, it needs to be defined now.
> >> 
> >> When you say API, do you mean the API the class provides to the
> >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >
> > The API to user space. That is the point. We cannot break user space.
> > Once this sysfs API is upstream we are stuck with it.
> 
> yeah, in fact I have been wondering if sysfs is the best interface to

That is the discussion we must have.

> userspace. I talked with Heikki a few days back about this; I was
> wondering if something like what the NFC folks did with netlink would be
> better here.

I doubt that, because the main user is likely to be udev scripts.
They can easily deal with sysfs attributes.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > Oliver Neukum <oneukum@suse.com> writes:
> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> 
> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> > >> > is to be supported, it needs to be defined now.
> > >> 
> > >> When you say API, do you mean the API the class provides to the
> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> > >
> > > The API to user space. That is the point. We cannot break user space.
> > > Once this sysfs API is upstream we are stuck with it.
> > 
> > yeah, in fact I have been wondering if sysfs is the best interface to
> 
> That is the discussion we must have.
> 
> > userspace. I talked with Heikki a few days back about this; I was
> > wondering if something like what the NFC folks did with netlink would be
> > better here.
> 
> I doubt that, because the main user is likely to be udev scripts.
> They can easily deal with sysfs attributes.

IMHO for high level interface like this, sysfs is ideal because of the
simple fact that you only need a shell to access the files. netlink
would make us depend on custom software, no?

I'm not against using netlink, but what would be the benefit from it
in this case?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]


Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> > Hi,
>> > 
>> > Oliver Neukum <oneukum@suse.com> writes:
>> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> 
>> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> > >> > is to be supported, it needs to be defined now.
>> > >> 
>> > >> When you say API, do you mean the API the class provides to the
>> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> > >
>> > > The API to user space. That is the point. We cannot break user space.
>> > > Once this sysfs API is upstream we are stuck with it.
>> > 
>> > yeah, in fact I have been wondering if sysfs is the best interface to
>> 
>> That is the discussion we must have.
>> 
>> > userspace. I talked with Heikki a few days back about this; I was
>> > wondering if something like what the NFC folks did with netlink would be
>> > better here.
>> 
>> I doubt that, because the main user is likely to be udev scripts.
>> They can easily deal with sysfs attributes.
>
> IMHO for high level interface like this, sysfs is ideal because of the
> simple fact that you only need a shell to access the files. netlink
> would make us depend on custom software, no?
>
> I'm not against using netlink, but what would be the benefit from it
> in this case?

With HW we see nowadays, CC stack is hidden on some microcontroller, but
is it too far-fetched to consider a system where this is not the case ?

Specially when we consider things like power delivery which, I know, you
wanted to keep it out of this interface, however we would have two
'stacks' competing for access to the same pins, right ?

IIRC mode and role negotiation goes via CC pins using the power delivery
protocol. If I misunderstand anything, let me know.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> > Hi,
> >> > 
> >> > Oliver Neukum <oneukum@suse.com> writes:
> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> 
> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > >> > is to be supported, it needs to be defined now.
> >> > >> 
> >> > >> When you say API, do you mean the API the class provides to the
> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> > >
> >> > > The API to user space. That is the point. We cannot break user space.
> >> > > Once this sysfs API is upstream we are stuck with it.
> >> > 
> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> 
> >> That is the discussion we must have.
> >> 
> >> > userspace. I talked with Heikki a few days back about this; I was
> >> > wondering if something like what the NFC folks did with netlink would be
> >> > better here.
> >> 
> >> I doubt that, because the main user is likely to be udev scripts.
> >> They can easily deal with sysfs attributes.
> >
> > IMHO for high level interface like this, sysfs is ideal because of the
> > simple fact that you only need a shell to access the files. netlink
> > would make us depend on custom software, no?
> >
> > I'm not against using netlink, but what would be the benefit from it
> > in this case?
> 
> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> is it too far-fetched to consider a system where this is not the case ?

There already are several USB PD stacks out there, like also Greg
pointed out.

> Specially when we consider things like power delivery which, I know, you
> wanted to keep it out of this interface, however we would have two
> 'stacks' competing for access to the same pins, right ?

No. This class would be the top layer for the coming stack, where ever
it ends up coming. The class is only the interface to the user space
and nothing else.

By saying we need to keep USB Type-C separate from USB PD I meant that
the userspace access can not be mixed somewhere in layers of the USB
PD/CC stack like it has been in the USB PD stacks I've seen so far.
They assume that we always use the software USB PD stack with USB
Type-C, which as we can see is not true when the stack is implemented
in EC or firmware or some complex USB PD controller or what ever.
However, the operations the userspace needs to do are exactly the same
in both cases.

- data role swapping
- power role swapping (depends on USB PD)
- Alternate Modes (depends on USB PD)

And we really should not forget that we actually also have USB Type-C
PHYs that can't do any USB PD communication over the CC pin, so USB PD
is simply not always going to be available. But the data role swapping
and also accessories are still available with them, as the do not need
USB PD.

This was the whole point with the class. It allows the different ways
of dealing with Type-C ports to be exposed to userspace in the same
way.

> IIRC mode and role negotiation goes via CC pins using the power delivery
> protocol. If I misunderstand anything, let me know.

The data role swap with USB Type-C connectors is in no way tied to USB
Power Delivery. The USB Type-C spec defines that when USB PD is
available, DR_Swap USB PD function is used to swap the role, otherwise
emulated disconnect will do the trick.

Data role swapping is a must thing to have with USB Type-C connectors
because of the fact that the role is selected randomly. Regardless was
USB PD supported or not.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> 
> > IIRC mode and role negotiation goes via CC pins using the power delivery
> > protocol. If I misunderstand anything, let me know.
> 
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is
> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.
> 

I am interested in how you design role swap on the fly without USB PD.
Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
just echo the /sys to give up current role, and swap to another?

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Peter,

On Thu, Feb 18, 2016 at 05:07:04PM +0800, Peter Chen wrote:
> On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > > IIRC mode and role negotiation goes via CC pins using the power delivery
> > > protocol. If I misunderstand anything, let me know.
> > 
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I am interested in how you design role swap on the fly without USB PD.
> Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
> just echo the /sys to give up current role, and swap to another?

No OTG with USB Type-C. You echo the wanted role to the
/sys/class/type-C/usbcN/data_role.

This operation from userspace is the same regardless was USB PD
supported or not. The actual operations needed for the role swap are
of course platform specific, and the responsibility of the drivers
that register the ports with type-c class.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
>> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> > Hi,
>> >> >
>> >> > Oliver Neukum <oneukum@suse.com> writes:
>> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >>
>> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> > >> > is to be supported, it needs to be defined now.
>> >> > >>
>> >> > >> When you say API, do you mean the API the class provides to the
>> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> >> > >
>> >> > > The API to user space. That is the point. We cannot break user space.
>> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >
>> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >>
>> >> That is the discussion we must have.
>> >>
>> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> > wondering if something like what the NFC folks did with netlink would be
>> >> > better here.
>> >>
>> >> I doubt that, because the main user is likely to be udev scripts.
>> >> They can easily deal with sysfs attributes.
>> >
>> > IMHO for high level interface like this, sysfs is ideal because of the
>> > simple fact that you only need a shell to access the files. netlink
>> > would make us depend on custom software, no?
>> >
>> > I'm not against using netlink, but what would be the benefit from it
>> > in this case?
>>
>> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> is it too far-fetched to consider a system where this is not the case ?
>
> There already are several USB PD stacks out there, like also Greg
> pointed out.
>
>> Specially when we consider things like power delivery which, I know, you
>> wanted to keep it out of this interface, however we would have two
>> 'stacks' competing for access to the same pins, right ?
>
> No. This class would be the top layer for the coming stack, where ever
> it ends up coming. The class is only the interface to the user space
> and nothing else.
>
> By saying we need to keep USB Type-C separate from USB PD I meant that
> the userspace access can not be mixed somewhere in layers of the USB
> PD/CC stack like it has been in the USB PD stacks I've seen so far.
> They assume that we always use the software USB PD stack with USB
> Type-C, which as we can see is not true when the stack is implemented
> in EC or firmware or some complex USB PD controller or what ever.
> However, the operations the userspace needs to do are exactly the same
> in both cases.
>
> - data role swapping
> - power role swapping (depends on USB PD)
> - Alternate Modes (depends on USB PD)
>
> And we really should not forget that we actually also have USB Type-C
> PHYs that can't do any USB PD communication over the CC pin, so USB PD
> is simply not always going to be available. But the data role swapping
> and also accessories are still available with them, as the do not need
> USB PD.
>
> This was the whole point with the class. It allows the different ways
> of dealing with Type-C ports to be exposed to userspace in the same
> way.
>
>> IIRC mode and role negotiation goes via CC pins using the power delivery
>> protocol. If I misunderstand anything, let me know.
>
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is

Its not data role swap i guess its dual role, A Data role swap is tied
with USB PD,

> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.

I doubt a USB host with no device capability implement DRP ?? Also
emulated trick(??) is not spec requirement rt ?

>
> Data role swapping is a must thing to have with USB Type-C connectors

I guess you are referring to Dual role (DRP) and not data role (DRD).

> because of the fact that the role is selected randomly. Regardless was
> USB PD supported or not.
>
>
> Thanks,
>
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> >> > Hi,
> >> >> >
> >> >> > Oliver Neukum <oneukum@suse.com> writes:
> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> >>
> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> >> > >> > is to be supported, it needs to be defined now.
> >> >> > >>
> >> >> > >> When you say API, do you mean the API the class provides to the
> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> >> > >
> >> >> > > The API to user space. That is the point. We cannot break user space.
> >> >> > > Once this sysfs API is upstream we are stuck with it.
> >> >> >
> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> >>
> >> >> That is the discussion we must have.
> >> >>
> >> >> > userspace. I talked with Heikki a few days back about this; I was
> >> >> > wondering if something like what the NFC folks did with netlink would be
> >> >> > better here.
> >> >>
> >> >> I doubt that, because the main user is likely to be udev scripts.
> >> >> They can easily deal with sysfs attributes.
> >> >
> >> > IMHO for high level interface like this, sysfs is ideal because of the
> >> > simple fact that you only need a shell to access the files. netlink
> >> > would make us depend on custom software, no?
> >> >
> >> > I'm not against using netlink, but what would be the benefit from it
> >> > in this case?
> >>
> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> >> is it too far-fetched to consider a system where this is not the case ?
> >
> > There already are several USB PD stacks out there, like also Greg
> > pointed out.
> >
> >> Specially when we consider things like power delivery which, I know, you
> >> wanted to keep it out of this interface, however we would have two
> >> 'stacks' competing for access to the same pins, right ?
> >
> > No. This class would be the top layer for the coming stack, where ever
> > it ends up coming. The class is only the interface to the user space
> > and nothing else.
> >
> > By saying we need to keep USB Type-C separate from USB PD I meant that
> > the userspace access can not be mixed somewhere in layers of the USB
> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
> > They assume that we always use the software USB PD stack with USB
> > Type-C, which as we can see is not true when the stack is implemented
> > in EC or firmware or some complex USB PD controller or what ever.
> > However, the operations the userspace needs to do are exactly the same
> > in both cases.
> >
> > - data role swapping
> > - power role swapping (depends on USB PD)
> > - Alternate Modes (depends on USB PD)
> >
> > And we really should not forget that we actually also have USB Type-C
> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
> > is simply not always going to be available. But the data role swapping
> > and also accessories are still available with them, as the do not need
> > USB PD.
> >
> > This was the whole point with the class. It allows the different ways
> > of dealing with Type-C ports to be exposed to userspace in the same
> > way.
> >
> >> IIRC mode and role negotiation goes via CC pins using the power delivery
> >> protocol. If I misunderstand anything, let me know.
> >
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> 
> Its not data role swap i guess its dual role, A Data role swap is tied
> with USB PD,
> 
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I doubt a USB host with no device capability implement DRP ?? Also
> emulated trick(??) is not spec requirement rt ?
> 
> >
> > Data role swapping is a must thing to have with USB Type-C connectors
> 
> I guess you are referring to Dual role (DRP) and not data role (DRD).

There is no term "DRD" in USB Type-C spec. A quote from Type-C spec
ch. 2.3.3:

"Two methods are defined to allow a USB Type-C DRP to functionally
swap data roles, one managed using USB PD DR_Swap and the other
emulating a disconnect/reconnect sequence (see Figure 4-16)"


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Thu, Feb 18, 2016 at 4:17 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
>> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
>> <heikki.krogerus@linux.intel.com> wrote:
>> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>> >>
>> >> Hi,
>> >>
>> >> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
>> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > Oliver Neukum <oneukum@suse.com> writes:
>> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >> >>
>> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> >> > >> > is to be supported, it needs to be defined now.
>> >> >> > >>
>> >> >> > >> When you say API, do you mean the API the class provides to the
>> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> >> >> > >
>> >> >> > > The API to user space. That is the point. We cannot break user space.
>> >> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >> >
>> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >> >>
>> >> >> That is the discussion we must have.
>> >> >>
>> >> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> >> > wondering if something like what the NFC folks did with netlink would be
>> >> >> > better here.
>> >> >>
>> >> >> I doubt that, because the main user is likely to be udev scripts.
>> >> >> They can easily deal with sysfs attributes.
>> >> >
>> >> > IMHO for high level interface like this, sysfs is ideal because of the
>> >> > simple fact that you only need a shell to access the files. netlink
>> >> > would make us depend on custom software, no?
>> >> >
>> >> > I'm not against using netlink, but what would be the benefit from it
>> >> > in this case?
>> >>
>> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> >> is it too far-fetched to consider a system where this is not the case ?
>> >
>> > There already are several USB PD stacks out there, like also Greg
>> > pointed out.
>> >
>> >> Specially when we consider things like power delivery which, I know, you
>> >> wanted to keep it out of this interface, however we would have two
>> >> 'stacks' competing for access to the same pins, right ?
>> >
>> > No. This class would be the top layer for the coming stack, where ever
>> > it ends up coming. The class is only the interface to the user space
>> > and nothing else.
>> >
>> > By saying we need to keep USB Type-C separate from USB PD I meant that
>> > the userspace access can not be mixed somewhere in layers of the USB
>> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
>> > They assume that we always use the software USB PD stack with USB
>> > Type-C, which as we can see is not true when the stack is implemented
>> > in EC or firmware or some complex USB PD controller or what ever.
>> > However, the operations the userspace needs to do are exactly the same
>> > in both cases.
>> >
>> > - data role swapping
>> > - power role swapping (depends on USB PD)
>> > - Alternate Modes (depends on USB PD)
>> >
>> > And we really should not forget that we actually also have USB Type-C
>> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
>> > is simply not always going to be available. But the data role swapping
>> > and also accessories are still available with them, as the do not need
>> > USB PD.
>> >
>> > This was the whole point with the class. It allows the different ways
>> > of dealing with Type-C ports to be exposed to userspace in the same
>> > way.
>> >
>> >> IIRC mode and role negotiation goes via CC pins using the power delivery
>> >> protocol. If I misunderstand anything, let me know.
>> >
>> > The data role swap with USB Type-C connectors is in no way tied to USB
>> > Power Delivery. The USB Type-C spec defines that when USB PD is
>>
>> Its not data role swap i guess its dual role, A Data role swap is tied
>> with USB PD,
>>
>> > available, DR_Swap USB PD function is used to swap the role, otherwise
>> > emulated disconnect will do the trick.
>>
>> I doubt a USB host with no device capability implement DRP ?? Also
>> emulated trick(??) is not spec requirement rt ?
>>
>> >
>> > Data role swapping is a must thing to have with USB Type-C connectors
>>
>> I guess you are referring to Dual role (DRP) and not data role (DRD).
>
> There is no term "DRD" in USB Type-C spec. A quote from Type-C spec

Yes, not in Spec 1.1 but a new term to differentiate data and power
roles . All I wanted to bring in some difference between data role
swap and DRP

> ch. 2.3.3:
>
> "Two methods are defined to allow a USB Type-C DRP to functionally
> swap data roles, one managed using USB PD DR_Swap and the other
> emulating a disconnect/reconnect sequence (see Figure 4-16)"

Ok I get it. Here in a user perspective its a connect and disconnect
and things are random which user may not prefer.

>
>
> Thanks,
>
> --
> heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]


Hi,

Oliver Neukum <oneukum@suse.com> writes:
>> > The API to user space. That is the point. We cannot break user space.
>> > Once this sysfs API is upstream we are stuck with it.
>> 
>> yeah, in fact I have been wondering if sysfs is the best interface to
>
> That is the discussion we must have.
>
>> userspace. I talked with Heikki a few days back about this; I was
>> wondering if something like what the NFC folks did with netlink would be
>> better here.
>
> I doubt that, because the main user is likely to be udev scripts.

I'm entirely sure about this. I think it's not too far-fetched to think
that, eventually, we will need an actual stack exposed for the CC pin.

> They can easily deal with sysfs attributes.

right

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 15:34 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum <oneukum@suse.com> writes:
> >> > The API to user space. That is the point. We cannot break user space.
> >> > Once this sysfs API is upstream we are stuck with it.
> >> 
> >> yeah, in fact I have been wondering if sysfs is the best interface to
> >
> > That is the discussion we must have.
> >
> >> userspace. I talked with Heikki a few days back about this; I was
> >> wondering if something like what the NFC folks did with netlink would be
> >> better here.
> >
> > I doubt that, because the main user is likely to be udev scripts.
> 
> I'm entirely sure about this. I think it's not too far-fetched to think

What exactly are you sure about about?

> that, eventually, we will need an actual stack exposed for the CC pin.

The raw CC pin? What about the timing requirement?

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]


Hi,

Oliver Neukum <oneukum@suse.com> writes:
>> Oliver Neukum <oneukum@suse.com> writes:
>> >> > The API to user space. That is the point. We cannot break user space.
>> >> > Once this sysfs API is upstream we are stuck with it.
>> >> 
>> >> yeah, in fact I have been wondering if sysfs is the best interface to
>> >
>> > That is the discussion we must have.
>> >
>> >> userspace. I talked with Heikki a few days back about this; I was
>> >> wondering if something like what the NFC folks did with netlink would be
>> >> better here.
>> >
>> > I doubt that, because the main user is likely to be udev scripts.
>> 
>> I'm entirely sure about this. I think it's not too far-fetched to think
>
> What exactly are you sure about about?

heh, missed a NOT there :-)

>> that, eventually, we will need an actual stack exposed for the CC pin.
>
> The raw CC pin? What about the timing requirement?

well, not the _raw_ CC pin, but a situation where the microcontroller
handling CC pin is "dumb", in the sense that it provides an interface
for us to request/start arbitrary transactions. That will, in turn,
shift the actual bits on the CC pin. Or something along these lines.

An example would be the alternate mode thing. CC microcontroller doesn't
have to know what are the available alternate modes, but it needs to be
able to tell processor what request is coming from the other end.

I guess what I'm trying to say is that CC microcontroller might not be
the one controlling the multiplexer which switches USB pins to another
function. IOW:

Change to alternate mode X message
 CC microcontroller interrupts CPU
  read status to get X
   change multiplexer

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 09:08 +0200, Felipe Balbi wrote:

> Oliver Neukum <oneukum@suse.com> writes:
> >> Oliver Neukum <oneukum@suse.com> writes:

Hi,

> > What exactly are you sure about about?
> 
> heh, missed a NOT there :-)

I am still confused :-)
Do you think a sysfs interface is good, bad or good
but insufficient?

> >> that, eventually, we will need an actual stack exposed for the CC pin.
> >
> > The raw CC pin? What about the timing requirement?
> 
> well, not the _raw_ CC pin, but a situation where the microcontroller
> handling CC pin is "dumb", in the sense that it provides an interface
> for us to request/start arbitrary transactions. That will, in turn,
> shift the actual bits on the CC pin. Or something along these lines.

Well, how else would we send vendor specific messages?

> An example would be the alternate mode thing. CC microcontroller doesn't
> have to know what are the available alternate modes, but it needs to be
> able to tell processor what request is coming from the other end.

Indeed.

> I guess what I'm trying to say is that CC microcontroller might not be
> the one controlling the multiplexer which switches USB pins to another
> function. IOW:
> 
> Change to alternate mode X message
>  CC microcontroller interrupts CPU
>   read status to get X
>    change multiplexer
> 

Yes. But it seems to me that in this case we need a kernel driver
without an API to user space. There are necessarily internal users
of the PD controller. There are also external users. So the CC pins
should be seen as a bus, which in essence they are (it reminds me
of ancient ethernet actually), and if you really want full user
space access, you'd need the quivalent of an sg driver.

The issue is orthogonal to the question how we support UCSI,
except that UCSI is a user of the CC pins and PD and frankly
I don't see the firmware and a driver access this sanely simultaneously.
Therefore I'd prefer we make an API here which does not depend on UCSI,
but can, if necessary, work on top of a driver doing full hardware
access.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]


Hi,

Oliver Neukum <oneukum@suse.com> writes:
>> > What exactly are you sure about about?
>> 
>> heh, missed a NOT there :-)
>
> I am still confused :-)
> Do you think a sysfs interface is good, bad or good
> but insufficient?

I'm not sure it's the best interface. My fear is that as new
requirements/features come along, the amount of files will continue to
grow.

>> I guess what I'm trying to say is that CC microcontroller might not be
>> the one controlling the multiplexer which switches USB pins to another
>> function. IOW:
>> 
>> Change to alternate mode X message
>>  CC microcontroller interrupts CPU
>>   read status to get X
>>    change multiplexer
>> 
>
> Yes. But it seems to me that in this case we need a kernel driver
> without an API to user space. There are necessarily internal users

that assumes kernel always knows all possible alternate modes. What do
we about bogus requests (request alternate mode X when X is not
supported) ?

> of the PD controller. There are also external users. So the CC pins
> should be seen as a bus, which in essence they are (it reminds me
> of ancient ethernet actually), and if you really want full user
> space access, you'd need the quivalent of an sg driver.
>
> The issue is orthogonal to the question how we support UCSI, except
> that UCSI is a user of the CC pins and PD and frankly I don't see the
> firmware and a driver access this sanely simultaneously.  Therefore
> I'd prefer we make an API here which does not depend on UCSI, but can,
> if necessary, work on top of a driver doing full hardware access.

fair enough.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 12:30 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum <oneukum@suse.com> writes:
> >> > What exactly are you sure about about?
> >> 
> >> heh, missed a NOT there :-)
> >
> > I am still confused :-)
> > Do you think a sysfs interface is good, bad or good
> > but insufficient?
> 
> I'm not sure it's the best interface. My fear is that as new
> requirements/features come along, the amount of files will continue to
> grow.

That will happen. The alternative, however is a "typectool" or
"usbpdtool" which would need to be updated for new features.

> >> I guess what I'm trying to say is that CC microcontroller might not be
> >> the one controlling the multiplexer which switches USB pins to another
> >> function. IOW:
> >> 
> >> Change to alternate mode X message
> >>  CC microcontroller interrupts CPU
> >>   read status to get X
> >>    change multiplexer
> >> 
> >
> > Yes. But it seems to me that in this case we need a kernel driver
> > without an API to user space. There are necessarily internal users
> 
> that assumes kernel always knows all possible alternate modes. What do
> we about bogus requests (request alternate mode X when X is not
> supported) ?

Do as the spec says: NACK it.

The questions which modes we offer, if we are a slave, still remains.
And I think the API is deficient in that regard. But again that question
is orthogonal of both issue, handling of bogus requests and how the
CC pins are exported.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 

Does this UCSI spec has some similar things with USB Type-C Port
Controller Interface Spec at usb.org? If not, how to co-work
together in future?

Peter

> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.
> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user? If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?
> 
> > You need to describe this a lot better than you did...
> 
> Sure thing. I'm sorry about the poor description. I send these out too
> hastily.
> 
> 
> Thanks,
> 
> -- 
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 17:29 +0800, Peter Chen wrote:

> Does this UCSI spec has some similar things with USB Type-C Port
> Controller Interface Spec at usb.org? If not, how to co-work
> together in future?

USB Type-C Port Controller Interface Spec:

What can a type C connector do?

UCSI spec:

How can the OS use ACPI to tell a type C connector what to do.

Obviously UCSI depends on the type C spec, but they cover different
things.

	HTH
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

> +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> +{
> +	int status;
> +	int ret;
> +
> +	dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> +		 ucsi->ppm->data->control);
> +
> +	ret = ucsi->ppm->cmd(ucsi->ppm);
> +	if (ret)
> +		return ret;
> +
> +	/* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> +	wait_for_completion(&ucsi->complete);
> +
> +	status = ucsi->status;
> +	if (status != UCSI_ERROR && size)
> +		memcpy(data, ucsi->ppm->data->message_in, size);
> +
> +	ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> +	if (ret)
> +		goto out;
> +
> +	if (status == UCSI_ERROR) {
> +		u16 error;
> +
> +		ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> +		ret = ucsi->ppm->cmd(ucsi->ppm);
> +		if (ret)
> +			goto out;
> +
> +		wait_for_completion(&ucsi->complete);
> +
> +		/* Something has really gone wrong */
> +		if (ucsi->status == UCSI_ERROR) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +
> +		memcpy(&error, ucsi->ppm->data->message_in, sizeof(error));
> +
> +		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> +		if (ret)
> +			goto out;
> +
> +		switch (error) {
> +		case UCSI_ERROR_INVALID_CON_NUM:
> +			ret = -ENXIO;
> +			break;
> +		case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> +		case UCSI_ERROR_CC_COMMUNICATION_ERR:
> +		case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> +			ret = -EIO;
> +			break;

This looks like you mapped all the interesting error condition
on a single error code and gave out other error codes to
conditions of lesser importance.

> +		case UCSI_ERROR_DEAD_BATTERY:
> +			dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> +			ret = -EPERM;
> +			break;
> +		case UCSI_ERROR_UNREGONIZED_CMD:
> +		case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}
> +out:
> +	ucsi->ppm->data->control = 0;
> +	return ret;
> +}

[..]

> +/**
> + * ucsi_init - Initialize an UCSI Interface
> + * @ucsi: The UCSI Interface
> + *
> + * Registers all the USB Type-C ports governed by the PPM of @ucsi and enables
> + * all the notifications from the PPM.
> + */
> +int ucsi_init(struct ucsi *ucsi)
> +{
> +	struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
> +	struct ucsi_connector *con;
> +	int ret;
> +	int i;
> +
> +	/* Enable basic notifications */
> +	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> +	ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> +	ret = ucsi_run_cmd(ucsi, NULL, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Get PPM capabilities */
> +	ctrl->cmd = UCSI_GET_CAPABILITY;
> +	ret = ucsi_run_cmd(ucsi, &ucsi->cap, sizeof(ucsi->cap));
> +	if (ret)
> +		return ret;
> +
> +	ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> +				  sizeof(struct ucsi_connector), GFP_KERNEL);
> +	if (!ucsi->connector)
> +		return -ENOMEM;
> +
> +	for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +	     i++, con++) {
> +		struct typec_capability *cap = &con->typec_cap;
> +		struct ucsi_connector_status constat;
> +
> +		/* Get connector capability */
> +		ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> +		ctrl->data = i + 1;
> +		ret = ucsi_run_cmd(ucsi, &con->cap, sizeof(con->cap));
> +		if (ret)
> +			goto err;
> +
> +		/* Register the connector */
> +
> +		if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> +			cap->type = TYPEC_PORT_DRP;
> +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> +			cap->type = TYPEC_PORT_DFP;
> +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> +			cap->type = TYPEC_PORT_UFP;
> +
> +		cap->usb_pd = !!(ucsi->cap.attributes &
> +				       UCSI_CAP_ATTR_USB_PD);
> +		cap->audio_accessory = !!(con->cap.op_mode &
> +					  UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> +		cap->debug_accessory = !!(con->cap.op_mode &
> +					  UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> +
> +		/* TODO: Alt modes */
> +
> +		cap->dr_swap = ucsi_dr_swap;
> +		cap->pr_swap = ucsi_pr_swap;
> +
> +		con->port = typec_register_port(ucsi->dev, cap);
> +		if (IS_ERR(con->port)) {
> +			ret = PTR_ERR(con->port);
> +			goto err;
> +		}
> +
> +		con->num = i + 1;
> +		con->ucsi = ucsi;
> +		INIT_WORK(&con->work, ucsi_connector_change);

You init the work later. Does this depend on the firmware
generating no events before they are enabled? Depending on firmware
is a bad idea.

> +
> +		/* Check if the connector is connected */
> +		if (WARN_ON(ucsi_get_constat(con, &constat) != 0))
> +			continue;
> +
> +		if (constat.connected)
> +			ucsi_connect(con, &constat);
> +	}
> +
> +	/* Enable all notifications */
> +	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> +	ctrl->data = UCSI_ENABLE_NTFY_ALL;
> +	ret = ucsi_run_cmd(ucsi, NULL, 0);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	if (i > 0)
> +		for (; i >= 0; i--, con--)
> +			typec_unregister_port(con->port);
> +
> +	kfree(ucsi->connector);
> +	return ret;
> +}
> +EXPORT_SYMBOL(ucsi_init);
> +

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 12:19:53PM +0100, Oliver Neukum wrote:
> 
> > +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> > +{
> > +	int status;
> > +	int ret;
> > +
> > +	dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> > +		 ucsi->ppm->data->control);
> > +
> > +	ret = ucsi->ppm->cmd(ucsi->ppm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> > +	wait_for_completion(&ucsi->complete);
> > +
> > +	status = ucsi->status;
> > +	if (status != UCSI_ERROR && size)
> > +		memcpy(data, ucsi->ppm->data->message_in, size);
> > +
> > +	ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (status == UCSI_ERROR) {
> > +		u16 error;
> > +
> > +		ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> > +		ret = ucsi->ppm->cmd(ucsi->ppm);
> > +		if (ret)
> > +			goto out;
> > +
> > +		wait_for_completion(&ucsi->complete);
> > +
> > +		/* Something has really gone wrong */
> > +		if (ucsi->status == UCSI_ERROR) {
> > +			ret = -ENODEV;
> > +			goto out;
> > +		}
> > +
> > +		memcpy(&error, ucsi->ppm->data->message_in, sizeof(error));
> > +
> > +		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +		if (ret)
> > +			goto out;
> > +
> > +		switch (error) {
> > +		case UCSI_ERROR_INVALID_CON_NUM:
> > +			ret = -ENXIO;
> > +			break;
> > +		case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> > +		case UCSI_ERROR_CC_COMMUNICATION_ERR:
> > +		case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> > +			ret = -EIO;
> > +			break;
> 
> This looks like you mapped all the interesting error condition
> on a single error code and gave out other error codes to
> conditions of lesser importance.

OK, I'll fix those.

> > +		case UCSI_ERROR_DEAD_BATTERY:
> > +			dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> > +			ret = -EPERM;
> > +			break;
> > +		case UCSI_ERROR_UNREGONIZED_CMD:
> > +		case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> > +		default:
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +	}
> > +out:
> > +	ucsi->ppm->data->control = 0;
> > +	return ret;
> > +}
> 
> [..]
> 
> > +/**
> > + * ucsi_init - Initialize an UCSI Interface
> > + * @ucsi: The UCSI Interface
> > + *
> > + * Registers all the USB Type-C ports governed by the PPM of @ucsi and enables
> > + * all the notifications from the PPM.
> > + */
> > +int ucsi_init(struct ucsi *ucsi)
> > +{
> > +	struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
> > +	struct ucsi_connector *con;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Enable basic notifications */
> > +	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > +	ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> > +	ret = ucsi_run_cmd(ucsi, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Get PPM capabilities */
> > +	ctrl->cmd = UCSI_GET_CAPABILITY;
> > +	ret = ucsi_run_cmd(ucsi, &ucsi->cap, sizeof(ucsi->cap));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> > +				  sizeof(struct ucsi_connector), GFP_KERNEL);
> > +	if (!ucsi->connector)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> > +	     i++, con++) {
> > +		struct typec_capability *cap = &con->typec_cap;
> > +		struct ucsi_connector_status constat;
> > +
> > +		/* Get connector capability */
> > +		ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> > +		ctrl->data = i + 1;
> > +		ret = ucsi_run_cmd(ucsi, &con->cap, sizeof(con->cap));
> > +		if (ret)
> > +			goto err;
> > +
> > +		/* Register the connector */
> > +
> > +		if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> > +			cap->type = TYPEC_PORT_DRP;
> > +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> > +			cap->type = TYPEC_PORT_DFP;
> > +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> > +			cap->type = TYPEC_PORT_UFP;
> > +
> > +		cap->usb_pd = !!(ucsi->cap.attributes &
> > +				       UCSI_CAP_ATTR_USB_PD);
> > +		cap->audio_accessory = !!(con->cap.op_mode &
> > +					  UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> > +		cap->debug_accessory = !!(con->cap.op_mode &
> > +					  UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> > +
> > +		/* TODO: Alt modes */
> > +
> > +		cap->dr_swap = ucsi_dr_swap;
> > +		cap->pr_swap = ucsi_pr_swap;
> > +
> > +		con->port = typec_register_port(ucsi->dev, cap);
> > +		if (IS_ERR(con->port)) {
> > +			ret = PTR_ERR(con->port);
> > +			goto err;
> > +		}
> > +
> > +		con->num = i + 1;
> > +		con->ucsi = ucsi;
> > +		INIT_WORK(&con->work, ucsi_connector_change);
> 
> You init the work later. Does this depend on the firmware
> generating no events before they are enabled? Depending on firmware
> is a bad idea.

If there is a real risk of getting connector change events before we
have enabled them, I think the only thing we can do is add checks to
ucsi_interrupt() to make sure that we don't try to handle events from
connectors that are not yet initialized.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
>
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

+#define to_ucsi_connector(_port_) container_of(_port_->cap,
         \
+                                              struct ucsi_connector,          \
+                                              typec_cap)

Perhaps
static inline ...

+
+#define cci_to_connector(_ucsi_, cci) (_ucsi_->connector +                    \
+                                       UCSI_CCI_CONNECTOR_CHANGE(cci) - 1)
+

If you start you macro on the next line it will look better.

> +static int
> +ucsi_connect(struct ucsi_connector *con, struct ucsi_connector_status *constat)
> +{
> +       struct typec_port *port = con->port;
> +
> +       port->connected = true;
> +
> +       if (constat->partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
> +               port->partner_type = TYPEC_PARTNER_ALTMODE;
> +       else
> +               port->partner_type = TYPEC_PARTNER_USB;
> +
> +       switch (constat->partner_type) {
> +       case UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP:
> +               /* REVISIT: We don't care about just the cable for now */
> +               return 0;
> +       case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> +       case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> +               port->pwr_role = TYPEC_PWR_SINK;
> +               port->data_role = TYPEC_DEVICE;
> +               break;
> +       case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> +               port->pwr_role = TYPEC_PWR_SOURCE;
> +               port->data_role = TYPEC_HOST;
> +               break;
> +       case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> +               port->partner_type = TYPEC_PARTNER_DEBUG;
> +               goto out;
> +       case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
> +               port->partner_type = TYPEC_PARTNER_AUDIO;
> +               goto out;
> +       }
> +
> +       switch (constat->pwr_op_mode) {
> +       case UCSI_CONSTAT_PWR_OPMODE_NONE:
> +       case UCSI_CONSTAT_PWR_OPMODE_DEFAULT:
> +               port->pwr_opmode = TYPEC_PWR_MODE_USB;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_BC:
> +               port->partner_type = TYPEC_PARTNER_CHARGER;
> +               port->pwr_opmode = TYPEC_PWR_MODE_BC1_2;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_PD:
> +               port->pwr_opmode = TYPEC_PWR_MODE_PD;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3:
> +               port->pwr_opmode = TYPEC_PWR_MODE_1_5A;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
> +               port->pwr_opmode = TYPEC_PWR_MODE_3_0A;
> +               break;
> +       default:
> +               break;
> +       }
> +out:

CodingStyle suggests to do a better label naming.

> +       return typec_connect(port);
> +}

> +static void ucsi_connector_change(struct work_struct *work)
> +{
> +       struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> +                                                 work);

> +}


> +int ucsi_init(struct ucsi *ucsi)
> +{
> +       struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
> +       struct ucsi_connector *con;
> +       int ret;

> +       int i;

> +       for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +            i++, con++) {

> +err:
> +       if (i > 0)
> +               for (; i >= 0; i--, con--)
> +                       typec_unregister_port(con->port);

Perhaps

while (--i >= 0) {
 ...
}

But...

> +
> +       kfree(ucsi->connector);
> +       return ret;
> +}

> +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
> +{
> +       struct ucsi *ucsi;
> +
> +       ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> +       if (!ucsi)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init_completion(&ucsi->complete);
> +       ucsi->dev = dev;
> +       ucsi->ppm = ppm;
> +
> +       return ucsi;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_register_ppm);
> +
> +/**
> + * ucsi_unregister_ppm - Unregister UCSI PPM Interface
> + * @ucsi: struct ucsi associated with the PPM
> + *
> + * Unregister an UCSI PPM that was created with ucsi_register().
> + */
> +void ucsi_unregister_ppm(struct ucsi *ucsi)
> +{
> +       struct ucsi_connector *con;
> +       int i;
> +
> +       /* Disable all notifications */
> +       ucsi->ppm->data->control = UCSI_SET_NOTIFICATION_ENABLE;
> +       ucsi->ppm->cmd(ucsi->ppm);
> +

> +       for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +            i++, con++)
> +               typec_unregister_port(con->port);

...looks a code duplication.

> +
> +       kfree(ucsi->connector);
> +       kfree(ucsi);
> +}
> +EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);

> +#include <linux/types.h>
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct ucsi_data {
> +       __u16 version;
> +       __u16 RESERVED;

Small letters?

> +       __u32 cci;
> +       __u64 control;
> +       __u32 message_in[4];
> +       __u32 message_out[4];
> +} __packed;

> +/* Bits for SET_NOTIFICATION_ENABLE command */
> +#define UCSI_ENABLE_NTFY_CMD_COMPLETE          BIT(0)
> +#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE    BIT(1)
> +#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE     BIT(2)
> +#define UCSI_ENABLE_NTFY_CAP_CHANGE            BIT(5)
> +#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE      BIT(6)
> +#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE     BIT(7)
> +#define UCSI_ENABLE_NTFY_CAM_CHANGE            BIT(8)
> +#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE     BIT(9)
> +#define UCSI_ENABLE_NTFY_PARTNER_CHANGE                BIT(11)
> +#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE                BIT(12)
> +#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE      BIT(14)
> +#define UCSI_ENABLE_NTFY_ERROR                 BIT(15)

> +#define UCSI_ENABLE_NTFY_ALL                   0xdbf3

Hmm... I would add the decode part of this, i.e. what kind of
notifications are excluded (and maybe why), e.g. BIT(2).


> +       __u8 num_alt_modes;
> +       __u8 RESERVED;

Small letters?

> +       __u16 bc_version;
> +       __u16 pd_version;
> +       __u16 typec_version;
> +} __packed;

> +       __u8 mode_support:1;
> +       __u8 RESERVED_2:2;
> +       __u8 latency:4;
> +       __u8 RESERVED_4:4;
> +} __packed;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +out:
> 
> CodingStyle suggests to do a better label naming.

Names coming from specs are what they are. There is
no place for coding style here.

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +out:
>>
>> CodingStyle suggests to do a better label naming.
>
> Names coming from specs are what they are. There is
> no place for coding style here.

Yes, and how is it related to C label names?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>>> > +out:
>>>
>>> CodingStyle suggests to do a better label naming.
>>
>> Names coming from specs are what they are. There is
>> no place for coding style here.
>
> Yes, and how is it related to C label names?

It did appear as if you were commenting on the case labels since you
quoted two full switch blocks.  That's how I read your comment as well.

It's now clear that you somehow mean than "out:" is in conflict with
CodingStyle.  It is still very unclear how, and it does not seem like
you intend to make it any clearer since you did not take the opportunity
to explain yourself.

FWIW, I read the CodingStyle recommendation as: use descriptive labels
instead of "foo1", "foo2" etc, where "foo" is typically "err".  I do not
see this as conflicting with the use of "err" or "out" when there is a
single such label in a function.  The meaning of those labels are very
clear IMHO.

Exactly what is it about "out" that is unclear to you here?  Could you
propose a better alternative if you seriously mean that this needs to be
changed?


Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> wrote:
>>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>>>> > +out:
>>>>
>>>> CodingStyle suggests to do a better label naming.

[...]

> Exactly what is it about "out" that is unclear to you here?  Could you
> propose a better alternative if you seriously mean that this needs to be
> changed?

Those switches of course an extra stuff, but I would propose
out_connect:

To me out: sounds out, like printing error and return code, or
something like that. Here the case is different.

And since we have two full switches it might be hard to have on one
screen out code and exact goto. See my point now?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> To me out: sounds out, like printing error and return code, or
> something like that. Here the case is different.
>
> And since we have two full switches it might be hard to have on one
> screen out code and exact goto. See my point now?

No, sorry.  Not really.

The pattern

   out:
      return whatever;

is so common in the kernel that I cannot see any point starting a
discussion about it:

 bjorn@nemi:/usr/local/src/git/linux$ git grep -A1 ^out:|grep return|wc -l
 3622


And I also fail to see any relation between the explanation you come up
with here and the text in CodingStyle, which was what you initially
referred to.  There is no specific interpreation of the label name "out"
there AFAICS.

I apologize if you think I am being an ass now. I am.  I'm just too fed
up with coding style arguments with no other reason than a vague pointer
to some document.  If you're going to point out coding style problems,
then you could at least make an effort explaining why a style change
improves the code.  My bet is that most of the time you'll find that the
comment is unnecessary...



Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +err:
> > +       if (i > 0)
> > +               for (; i >= 0; i--, con--)
> > +                       typec_unregister_port(con->port);
> 
> Perhaps
> 
> while (--i >= 0) {
>  ...
> }

While we are at it. No we should not change the semantics
of conditionals for the sake of appearance.

	Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +err:
>> > +       if (i > 0)
>> > +               for (; i >= 0; i--, con--)
>> > +                       typec_unregister_port(con->port);
>>
>> Perhaps
>>
>> while (--i >= 0) {
>>  ...
>> }
>
> While we are at it. No we should not change the semantics
> of conditionals for the sake of appearance.

I'm sorry I didn't get you.
How this more or less standard pattern to clean up stuff on error path
does with conditional semantics?

I also noticed that this code might be factored out to helper which
will do same things (only number of loops is different) in both cases.
What did I miss?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> >> > +err:
> >> > +       if (i > 0)
> >> > +               for (; i >= 0; i--, con--)
> >> > +                       typec_unregister_port(con->port);
> >>
> >> Perhaps
> >>
> >> while (--i >= 0) {
> >>  ...
> >> }
> >
> > While we are at it. No we should not change the semantics
> > of conditionals for the sake of appearance.
> 
> I'm sorry I didn't get you.
> How this more or less standard pattern to clean up stuff on error path
> does with conditional semantics?

You change a postdecrement to a predecrement. The highest
number the loop is executed for is changed.

	Oliver

Fwd: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

---------- Forwarded message ----------
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Thu, Feb 11, 2016 at 10:10 AM
Subject: Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System
Software Interface
To: Oliver Neukum <oneukum@suse.de>


On Wed, Feb 10, 2016 at 5:08 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
>> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> > On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> >> > +err:
>> >> > +       if (i > 0)
>> >> > +               for (; i >= 0; i--, con--)
>> >> > +                       typec_unregister_port(con->port);
>> >>
>> >> Perhaps
>> >>
>> >> while (--i >= 0) {
>> >>  ...
>> >> }
>> >
>> > While we are at it. No we should not change the semantics
>> > of conditionals for the sake of appearance.
>>
>> I'm sorry I didn't get you.
>> How this more or less standard pattern to clean up stuff on error path
>> does with conditional semantics?
>
> You change a postdecrement to a predecrement. The highest
> number the loop is executed for is changed.

I still didn't get.
Variable i is just counter here,

And it seems there is a bug, since when i == 1, we will have

i = 1, con == connector[0]:
typec_unregister_port(con->port);

i = 0, con == connector[1]:
typec_unregister_port(con->port); <<< It wasn't registered yet!

The correct code should be something like
if (i > 0)
 for (--i; i >= 0; i--) {}

Which
a) makes conditional redundant;
b) classical pattern of while (--i >= 0) {}

So where am I wrong?

--
With Best Regards,
Andy Shevchenko


-- 
With Best Regards,
Andy Shevchenko

Re: Fwd: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Thu, Feb 11, 2016 at 10:13:11AM +0200, Andy Shevchenko wrote:
> ---------- Forwarded message ----------
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Date: Thu, Feb 11, 2016 at 10:10 AM
> Subject: Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System
> Software Interface
> To: Oliver Neukum <oneukum@suse.de>
> 
> 
> On Wed, Feb 10, 2016 at 5:08 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
> >> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> wrote:
> >> > On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> >> >> > +err:
> >> >> > +       if (i > 0)
> >> >> > +               for (; i >= 0; i--, con--)
> >> >> > +                       typec_unregister_port(con->port);
> >> >>
> >> >> Perhaps
> >> >>
> >> >> while (--i >= 0) {
> >> >>  ...
> >> >> }
> >> >
> >> > While we are at it. No we should not change the semantics
> >> > of conditionals for the sake of appearance.
> >>
> >> I'm sorry I didn't get you.
> >> How this more or less standard pattern to clean up stuff on error path
> >> does with conditional semantics?
> >
> > You change a postdecrement to a predecrement. The highest
> > number the loop is executed for is changed.
> 
> I still didn't get.
> Variable i is just counter here,
> 
> And it seems there is a bug, since when i == 1, we will have
> 
> i = 1, con == connector[0]:
> typec_unregister_port(con->port);
> 
> i = 0, con == connector[1]:
> typec_unregister_port(con->port); <<< It wasn't registered yet!
> 
> The correct code should be something like
> if (i > 0)
>  for (--i; i >= 0; i--) {}
> 
> Which
> a) makes conditional redundant;
> b) classical pattern of while (--i >= 0) {}
> 
> So where am I wrong?

I think Andy has a point here.

Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> +#define UCSI_CONSTAT_BC_NOT_CHARGING           0
> +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING       1
> +#define UCSI_CONSTAT_BC_SLOW_CHARGING          2
> +#define UCSI_CONSTAT_BC_TRICLE_CHARGING                3

typo. It is spelled TRICKLE

	Regards
		Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 02:04:07PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > +#define UCSI_CONSTAT_BC_NOT_CHARGING           0
> > +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING       1
> > +#define UCSI_CONSTAT_BC_SLOW_CHARGING          2
> > +#define UCSI_CONSTAT_BC_TRICLE_CHARGING                3
> 
> typo. It is spelled TRICKLE

Thanks! I'll fix it.

-- 
heikki

[PATCH 3/3] usb: type-c: UCSI ACPI driver

[Heikki Krogerus]

Driver for ACPI enumerated UCSI devices.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/type-c/Kconfig     |  10 ++++
 drivers/usb/type-c/Makefile    |   1 +
 drivers/usb/type-c/ucsi_acpi.c | 133 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 drivers/usb/type-c/ucsi_acpi.c

diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
index 02abd74..72b002e 100644
--- a/drivers/usb/type-c/Kconfig
+++ b/drivers/usb/type-c/Kconfig
@@ -12,4 +12,14 @@ config TYPEC_UCSI
 	  registers and data structures used to interface with the USB Type-C
 	  connectors on a system.
 
+if TYPEC_UCSI
+
+config TYPEC_UCSI_ACPI
+	tristate "UCSI ACPI Driver"
+	depends on ACPI
+	help
+	  Driver for ACPI enumerated UCSI devices.
+
+endif
+
 endmenu
diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
index ab974ba..17933dc 100644
--- a/drivers/usb/type-c/Makefile
+++ b/drivers/usb/type-c/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
 obj-$(CONFIG_TYPEC_UCSI)	+= ucsi.o
+obj-$(CONFIG_TYPEC_UCSI_ACPI)	+= ucsi_acpi.o
diff --git a/drivers/usb/type-c/ucsi_acpi.c b/drivers/usb/type-c/ucsi_acpi.c
new file mode 100644
index 0000000..8445a7d
--- /dev/null
+++ b/drivers/usb/type-c/ucsi_acpi.c
@@ -0,0 +1,133 @@
+/*
+ * UCSI ACPI driver
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/acpi.h>
+
+#include "ucsi.h"
+
+struct ucsi_acpi {
+	struct device *dev;
+	struct ucsi *ucsi;
+	struct ucsi_ppm ppm;
+};
+
+static const u8 ucsi_uuid[] = {
+	0xc2, 0x98, 0x83, 0x6f,	0xa4, 0x7c, 0xe4, 0x11,
+	0xad, 0x36, 0x63, 0x10, 0x42, 0xb5, 0x00, 0x8f,
+};
+
+static int ucsi_acpi_cmd(struct ucsi_ppm *ppm)
+{
+	struct ucsi_acpi *ua = container_of(ppm, struct ucsi_acpi, ppm);
+	union acpi_object *obj;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(ua->dev), ucsi_uuid, 1, 1, NULL);
+	if (!obj) {
+		dev_err(ua->dev, "%s: failed to evaluate _DSM\n", __func__);
+		return -EIO;
+	}
+
+	ACPI_FREE(obj);
+	return 0;
+}
+
+static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct ucsi_acpi *ua = data;
+
+	if (!ucsi_interrupt(ua->ucsi))
+		dev_err(ua->dev, "spurious ACPI notification\n");
+}
+
+static int ucsi_acpi_probe(struct platform_device *pdev)
+{
+	struct ucsi_acpi *ua;
+	struct resource *res;
+	acpi_status status;
+	int ret;
+
+	ua = devm_kzalloc(&pdev->dev, sizeof(*ua), GFP_KERNEL);
+	if (!ua)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "missing memory resource\n");
+		return -ENODEV;
+	}
+
+	ua->ppm.data = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!ua->ppm.data)
+		return -ENOMEM;
+
+	ua->ppm.cmd = ucsi_acpi_cmd;
+	ua->dev = &pdev->dev;
+
+	ua->ucsi = ucsi_register_ppm(&pdev->dev, &ua->ppm);
+	if (IS_ERR(ua->ucsi))
+		return PTR_ERR(ua->ucsi);
+
+	status = acpi_install_notify_handler(ACPI_HANDLE(&pdev->dev),
+					     ACPI_DEVICE_NOTIFY,
+					     ucsi_acpi_notify, ua);
+	if (ACPI_FAILURE(status)) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = ucsi_init(ua->ucsi);
+	if (ret) {
+		acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev),
+					   ACPI_DEVICE_NOTIFY,
+					   ucsi_acpi_notify);
+		goto err;
+	}
+
+	platform_set_drvdata(pdev, ua);
+	return 0;
+err:
+	ucsi_unregister_ppm(ua->ucsi);
+	return ret;
+}
+
+static int ucsi_acpi_remove(struct platform_device *pdev)
+{
+	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
+
+	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev),
+				   ACPI_DEVICE_NOTIFY, ucsi_acpi_notify);
+	ucsi_unregister_ppm(ua->ucsi);
+	return 0;
+}
+
+static const struct acpi_device_id ucsi_acpi_match[] = {
+	{ "PNP0CA0", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, ucsi_acpi_match);
+
+static struct platform_driver ucsi_acpi_platform_driver = {
+	.driver = {
+		.name = "ucsi_acpi",
+		.acpi_match_table = ACPI_PTR(ucsi_acpi_match),
+	},
+	.probe = ucsi_acpi_probe,
+	.remove = ucsi_acpi_remove,
+};
+
+module_platform_driver(ucsi_acpi_platform_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("UCSI ACPI driver");
-- 
2.7.0

Re: [PATCH 3/3] usb: type-c: UCSI ACPI driver

[Greg KH]

On Tue, Feb 09, 2016 at 07:01:23PM +0200, Heikki Krogerus wrote:
> Driver for ACPI enumerated UCSI devices.

What does this mean?

What does the driver do?  Why would we care?

> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/type-c/Kconfig     |  10 ++++
>  drivers/usb/type-c/Makefile    |   1 +
>  drivers/usb/type-c/ucsi_acpi.c | 133 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/usb/type-c/ucsi_acpi.c
> 
> diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
> index 02abd74..72b002e 100644
> --- a/drivers/usb/type-c/Kconfig
> +++ b/drivers/usb/type-c/Kconfig
> @@ -12,4 +12,14 @@ config TYPEC_UCSI
>  	  registers and data structures used to interface with the USB Type-C
>  	  connectors on a system.
>  
> +if TYPEC_UCSI
> +
> +config TYPEC_UCSI_ACPI
> +	tristate "UCSI ACPI Driver"
> +	depends on ACPI
> +	help
> +	  Driver for ACPI enumerated UCSI devices.

Worst help text ever :(

thanks,

greg k-h

Re: [PATCH 3/3] usb: type-c: UCSI ACPI driver

[Heikki Krogerus]

On Tue, Feb 09, 2016 at 10:22:46AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:23PM +0200, Heikki Krogerus wrote:
> > Driver for ACPI enumerated UCSI devices.
> 
> What does this mean?
> 
> What does the driver do?  Why would we care?
> 
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/type-c/Kconfig     |  10 ++++
> >  drivers/usb/type-c/Makefile    |   1 +
> >  drivers/usb/type-c/ucsi_acpi.c | 133 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 144 insertions(+)
> >  create mode 100644 drivers/usb/type-c/ucsi_acpi.c
> > 
> > diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
> > index 02abd74..72b002e 100644
> > --- a/drivers/usb/type-c/Kconfig
> > +++ b/drivers/usb/type-c/Kconfig
> > @@ -12,4 +12,14 @@ config TYPEC_UCSI
> >  	  registers and data structures used to interface with the USB Type-C
> >  	  connectors on a system.
> >  
> > +if TYPEC_UCSI
> > +
> > +config TYPEC_UCSI_ACPI
> > +	tristate "UCSI ACPI Driver"
> > +	depends on ACPI
> > +	help
> > +	  Driver for ACPI enumerated UCSI devices.
> 
> Worst help text ever :(

I'm sorry for that. I was not planning to leave it like that. I was
more consearned with the class then these drivers, and forgot finish
these parts.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> Hi,
> 
> The OS, or more precisely the user space, needs to be able to control
> a few things regarding USB Type-C ports. The first thing that must be
> allowed to be controlled is the data role. USB Type-C ports will
> select the data role randomly with DRP ports. When USB PD is
> supported, also independent (from data role) power role swapping can
> be supported together with Alternate Mode control.

What about S4? We need to restore the alternate mode upon resume,
if we are the DFP and as that might involve storage devices it
needs to be done in kernel space.

	Regards
		Oliver

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 07:53:47PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > The OS, or more precisely the user space, needs to be able to control
> > a few things regarding USB Type-C ports. The first thing that must be
> > allowed to be controlled is the data role. USB Type-C ports will
> > select the data role randomly with DRP ports. When USB PD is
> > supported, also independent (from data role) power role swapping can
> > be supported together with Alternate Mode control.
> 
> What about S4? We need to restore the alternate mode upon resume,
> if we are the DFP and as that might involve storage devices it
> needs to be done in kernel space.

That I'm expecting to be the responsibility of the drivers registering
the ports at them moment, because I'm afraid of all the possible
different kind of platform specific oddities that need to be
considered.

But I guess all we would need to do in the class is store the roles
and mode during suspend, and restore them during resuming with
dr_swap, pr_swap and set_alt_mode hooks if they changed.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Rajaram R]

On Tue, Feb 9, 2016 at 10:31 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> Hi,
>
> The OS, or more precisely the user space, needs to be able to control
> a few things regarding USB Type-C ports. The first thing that must be
> allowed to be controlled is the data role. USB Type-C ports will
> select the data role randomly with DRP ports. When USB PD is
> supported, also independent (from data role) power role swapping can
> be supported together with Alternate Mode control.
>
> I'm proposing with this set a Class for the Type-C connectors that
> gives the user space control over those things on top of getting basic
> details about the USB Type-C connectors and also partners. The details
> include the capabilities of the port, the supported data and power
> roles, supported accessories (audio and debug), supported Alternate
> Modes, USB PD support and of course the type of the partner (USB, Alt
> Mode, Accessory or Charger), and more or less the same details about
> the partner.
>
> I'm not considering cables with this Class, and I have deliberately

Since we have capability details of ports in user space, I believe
cable capability is also necessary for policy decision(power, alt
mode). Is that something we are cautiously leaving out ? pls explain

> left out some more technical details, like cable orientation, firstly
> because I did not see much use for the user space from knowing that
> an secondly because that kind of details are not always available for
> example with UCSI.
>
> So the interface to the user space is kept as simple as I dared to
> make it.
>
> NOTE: In case there is somebody wondering, this is not adding USB PD
> support to Linux kernel. This is just about USB Type-C.
>
>
> Heikki Krogerus (3):
>   usb: USB Type-C Connector Class
>   usb: type-c: USB Type-C Connector System Software Interface
>   usb: type-c: UCSI ACPI driver
>
>  drivers/usb/Kconfig            |   2 +
>  drivers/usb/Makefile           |   2 +
>  drivers/usb/type-c/Kconfig     |  25 +++
>  drivers/usb/type-c/Makefile    |   3 +
>  drivers/usb/type-c/typec.c     | 446 ++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/type-c/ucsi.c      | 450 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/type-c/ucsi.h      | 219 ++++++++++++++++++++
>  drivers/usb/type-c/ucsi_acpi.c | 133 ++++++++++++
>  include/linux/usb/typec.h      | 114 +++++++++++
>  9 files changed, 1394 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 drivers/usb/type-c/ucsi.c
>  create mode 100644 drivers/usb/type-c/ucsi.h
>  create mode 100644 drivers/usb/type-c/ucsi_acpi.c
>  create mode 100644 include/linux/usb/typec.h
>
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

Hi Rajaram,

On Thu, Feb 18, 2016 at 01:04:48AM +0530, Rajaram R wrote:
> On Tue, Feb 9, 2016 at 10:31 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > Hi,
> >
> > The OS, or more precisely the user space, needs to be able to control
> > a few things regarding USB Type-C ports. The first thing that must be
> > allowed to be controlled is the data role. USB Type-C ports will
> > select the data role randomly with DRP ports. When USB PD is
> > supported, also independent (from data role) power role swapping can
> > be supported together with Alternate Mode control.
> >
> > I'm proposing with this set a Class for the Type-C connectors that
> > gives the user space control over those things on top of getting basic
> > details about the USB Type-C connectors and also partners. The details
> > include the capabilities of the port, the supported data and power
> > roles, supported accessories (audio and debug), supported Alternate
> > Modes, USB PD support and of course the type of the partner (USB, Alt
> > Mode, Accessory or Charger), and more or less the same details about
> > the partner.
> >
> > I'm not considering cables with this Class, and I have deliberately
> 
> Since we have capability details of ports in user space, I believe
> cable capability is also necessary for policy decision(power, alt
> mode). Is that something we are cautiously leaving out ? pls explain

Adding the cable control to this interface will make it more complex
from users perspective. However, nothing forces the user to control
also the cable.

I already decided to add vconn_swap support, so I'll try to add cable
alt mode control and capabilities as well.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Oliver Neukum]

On Thu, 2016-02-18 at 13:05 +0200, Heikki Krogerus wrote:

> > Since we have capability details of ports in user space, I believe
> > cable capability is also necessary for policy decision(power, alt
> > mode). Is that something we are cautiously leaving out ? pls explain
> 
> Adding the cable control to this interface will make it more complex
> from users perspective. However, nothing forces the user to control
> also the cable.

But we would like to indicate to the user that we cannot run
an alternate mode because the cable is incapable as opposed to the
device.

	Regards
		Oliver

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> The OS, or more precisely the user space, needs to be able to control
> a few things regarding USB Type-C ports. The first thing that must be
> allowed to be controlled is the data role. USB Type-C ports will
> select the data role randomly with DRP ports. When USB PD is
> supported, also independent (from data role) power role swapping can
> be supported together with Alternate Mode control.
> 
> I'm proposing with this set a Class for the Type-C connectors that
> gives the user space control over those things on top of getting basic
> details about the USB Type-C connectors and also partners. The details
> include the capabilities of the port, the supported data and power
> roles, supported accessories (audio and debug), supported Alternate
> Modes, USB PD support and of course the type of the partner (USB, Alt
> Mode, Accessory or Charger), and more or less the same details about
> the partner.
> 
> I'm not considering cables with this Class, and I have deliberately
> left out some more technical details, like cable orientation, firstly
> because I did not see much use for the user space from knowing that
> an secondly because that kind of details are not always available for
> example with UCSI.
> 
> So the interface to the user space is kept as simple as I dared to
> make it.
> 
> NOTE: In case there is somebody wondering, this is not adding USB PD
> support to Linux kernel. This is just about USB Type-C.
> 

Hello Heikki,

we have implemented a prototype TCPM (USB Type-C Protocol Manager)
software on top of your patch set. It will support TCPCI as well
as other USB-C controllers such as FUSB302. The plan is to use
this software in systems where no separate controller is available.

Is there any chance to advance this patch set ? It would be instrumental
to get a unified interface to user space.

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]


Hi Guenter,

Guenter Roeck <linux@roeck-us.net> writes:
> On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
>> Hi,
>> 
>> The OS, or more precisely the user space, needs to be able to control
>> a few things regarding USB Type-C ports. The first thing that must be
>> allowed to be controlled is the data role. USB Type-C ports will
>> select the data role randomly with DRP ports. When USB PD is
>> supported, also independent (from data role) power role swapping can
>> be supported together with Alternate Mode control.
>> 
>> I'm proposing with this set a Class for the Type-C connectors that
>> gives the user space control over those things on top of getting basic
>> details about the USB Type-C connectors and also partners. The details
>> include the capabilities of the port, the supported data and power
>> roles, supported accessories (audio and debug), supported Alternate
>> Modes, USB PD support and of course the type of the partner (USB, Alt
>> Mode, Accessory or Charger), and more or less the same details about
>> the partner.
>> 
>> I'm not considering cables with this Class, and I have deliberately
>> left out some more technical details, like cable orientation, firstly
>> because I did not see much use for the user space from knowing that
>> an secondly because that kind of details are not always available for
>> example with UCSI.
>> 
>> So the interface to the user space is kept as simple as I dared to
>> make it.
>> 
>> NOTE: In case there is somebody wondering, this is not adding USB PD
>> support to Linux kernel. This is just about USB Type-C.
>> 
>
> Hello Heikki,
>
> we have implemented a prototype TCPM (USB Type-C Protocol Manager)
> software on top of your patch set. It will support TCPCI as well
> as other USB-C controllers such as FUSB302. The plan is to use
> this software in systems where no separate controller is available.
>
> Is there any chance to advance this patch set ? It would be instrumental
> to get a unified interface to user space.

A newer version of $subject is already in Greg's queue [1]

[1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=0c1849a8c7af652c92ad0265a7ca5934fd773c69

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

Felipe,

On 05/05/2016 11:50 PM, Felipe Balbi wrote:
>
> Hi Guenter,
>
> Guenter Roeck <linux@roeck-us.net> writes:
>> On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> The OS, or more precisely the user space, needs to be able to control
>>> a few things regarding USB Type-C ports. The first thing that must be
>>> allowed to be controlled is the data role. USB Type-C ports will
>>> select the data role randomly with DRP ports. When USB PD is
>>> supported, also independent (from data role) power role swapping can
>>> be supported together with Alternate Mode control.
>>>
>>> I'm proposing with this set a Class for the Type-C connectors that
>>> gives the user space control over those things on top of getting basic
>>> details about the USB Type-C connectors and also partners. The details
>>> include the capabilities of the port, the supported data and power
>>> roles, supported accessories (audio and debug), supported Alternate
>>> Modes, USB PD support and of course the type of the partner (USB, Alt
>>> Mode, Accessory or Charger), and more or less the same details about
>>> the partner.
>>>
>>> I'm not considering cables with this Class, and I have deliberately
>>> left out some more technical details, like cable orientation, firstly
>>> because I did not see much use for the user space from knowing that
>>> an secondly because that kind of details are not always available for
>>> example with UCSI.
>>>
>>> So the interface to the user space is kept as simple as I dared to
>>> make it.
>>>
>>> NOTE: In case there is somebody wondering, this is not adding USB PD
>>> support to Linux kernel. This is just about USB Type-C.
>>>
>>
>> Hello Heikki,
>>
>> we have implemented a prototype TCPM (USB Type-C Protocol Manager)
>> software on top of your patch set. It will support TCPCI as well
>> as other USB-C controllers such as FUSB302. The plan is to use
>> this software in systems where no separate controller is available.
>>
>> Is there any chance to advance this patch set ? It would be instrumental
>> to get a unified interface to user space.
>
> A newer version of $subject is already in Greg's queue [1]
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=0c1849a8c7af652c92ad0265a7ca5934fd773c69
>
I am aware of that patch.

Unfortunately, unlike the original submission, the new patch is not an
infrastructure, it is just a driver supporting Intel's UCSI. Unlike the
original series, it does not provide an infrastructure, and it does not
support other implementations of USB Type-C port management systems.

In our system, we'll have (at least) three such implementations:

- TCPM and TCPC implemented in EC and/or microcontrollers.
   This is currently implemented and shipping with some Chromebooks.
- TCPM implemented in Linux, interfacing to a standard TCPC, using TCPCI
   for TCPM-TCPC communication
   This will be needed for systems with no EC and a standard Type-C port
   controller.
- TCPM implemented in Linux, interfacing to FUSB302.
   This will be needed for systems with no EC, utilizing a FUSB302
   port controller.

All those fit nicely into the infrastructure provided by the original
patch series, where UCSI was just one possible implementation of a
USB Type-C port management system.

The original patch series had the tremendous advantage of presenting a
unified ABI to user space. With the new patch, this is no longer the case.
All implementations would be completely separate and thus effectively
guarantee ABI fragmentation (Fairchild's code supporting FUSB302 in Linux
is a good example. The existing implementation of Type-C support in the
Chromebooks mentioned above is another).

I know there has been a lengthy discussion about the patch set, but I may
have missed the conclusion. Is there some reason to _not_ advance it
that I may have missed ?

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

On Fri, May 06, 2016 at 01:05:05AM -0700, Guenter Roeck wrote:
> Felipe,
> 
> On 05/05/2016 11:50 PM, Felipe Balbi wrote:
> > 
> > Hi Guenter,
> > 
> > Guenter Roeck <linux@roeck-us.net> writes:
> > > On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
> > > > Hi,
> > > > 
> > > > The OS, or more precisely the user space, needs to be able to control
> > > > a few things regarding USB Type-C ports. The first thing that must be
> > > > allowed to be controlled is the data role. USB Type-C ports will
> > > > select the data role randomly with DRP ports. When USB PD is
> > > > supported, also independent (from data role) power role swapping can
> > > > be supported together with Alternate Mode control.
> > > > 
> > > > I'm proposing with this set a Class for the Type-C connectors that
> > > > gives the user space control over those things on top of getting basic
> > > > details about the USB Type-C connectors and also partners. The details
> > > > include the capabilities of the port, the supported data and power
> > > > roles, supported accessories (audio and debug), supported Alternate
> > > > Modes, USB PD support and of course the type of the partner (USB, Alt
> > > > Mode, Accessory or Charger), and more or less the same details about
> > > > the partner.
> > > > 
> > > > I'm not considering cables with this Class, and I have deliberately
> > > > left out some more technical details, like cable orientation, firstly
> > > > because I did not see much use for the user space from knowing that
> > > > an secondly because that kind of details are not always available for
> > > > example with UCSI.
> > > > 
> > > > So the interface to the user space is kept as simple as I dared to
> > > > make it.
> > > > 
> > > > NOTE: In case there is somebody wondering, this is not adding USB PD
> > > > support to Linux kernel. This is just about USB Type-C.
> > > > 
> > > 
> > > Hello Heikki,
> > > 
> > > we have implemented a prototype TCPM (USB Type-C Protocol Manager)
> > > software on top of your patch set. It will support TCPCI as well
> > > as other USB-C controllers such as FUSB302. The plan is to use
> > > this software in systems where no separate controller is available.
> > > 
> > > Is there any chance to advance this patch set ? It would be instrumental
> > > to get a unified interface to user space.
> > 
> > A newer version of $subject is already in Greg's queue [1]
> > 
> > [1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=0c1849a8c7af652c92ad0265a7ca5934fd773c69
> > 
> I am aware of that patch.
> 
> Unfortunately, unlike the original submission, the new patch is not an
> infrastructure, it is just a driver supporting Intel's UCSI. Unlike the
> original series, it does not provide an infrastructure, and it does not
> support other implementations of USB Type-C port management systems.
> 
> In our system, we'll have (at least) three such implementations:
> 
> - TCPM and TCPC implemented in EC and/or microcontrollers.
>   This is currently implemented and shipping with some Chromebooks.
> - TCPM implemented in Linux, interfacing to a standard TCPC, using TCPCI
>   for TCPM-TCPC communication
>   This will be needed for systems with no EC and a standard Type-C port
>   controller.
> - TCPM implemented in Linux, interfacing to FUSB302.
>   This will be needed for systems with no EC, utilizing a FUSB302
>   port controller.
> 
> All those fit nicely into the infrastructure provided by the original
> patch series, where UCSI was just one possible implementation of a
> USB Type-C port management system.
> 
> The original patch series had the tremendous advantage of presenting a
> unified ABI to user space. With the new patch, this is no longer the case.
> All implementations would be completely separate and thus effectively
> guarantee ABI fragmentation (Fairchild's code supporting FUSB302 in Linux
> is a good example. The existing implementation of Type-C support in the
> Chromebooks mentioned above is another).
> 
> I know there has been a lengthy discussion about the patch set, but I may
> have missed the conclusion. Is there some reason to _not_ advance it
> that I may have missed ?

No, we are still continuing with the class driver. We just descided to
split the UCIS into separate driver for now, just because we needed it
to be supported fast. But I did mention in the commit message of the
UCSI patch that the goal is to merge that into a Type-C framework once
it's awailable.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

Hello Heikki,

On 05/06/2016 01:29 AM, Heikki Krogerus wrote:
> On Fri, May 06, 2016 at 01:05:05AM -0700, Guenter Roeck wrote:
[ ... ]
>> I know there has been a lengthy discussion about the patch set, but I may
>> have missed the conclusion. Is there some reason to _not_ advance it
>> that I may have missed ?
>
> No, we are still continuing with the class driver. We just descided to
> split the UCIS into separate driver for now, just because we needed it
> to be supported fast. But I did mention in the commit message of the
> UCSI patch that the goal is to merge that into a Type-C framework once
> it's awailable.
>
Yes, I noticed. I had suspected that your need for a driver now was the
reason for the stand-alone driver. Felipe's response got me concerned
for a minute, though.

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

On Fri, May 06, 2016 at 09:50:00AM +0300, Felipe Balbi wrote:
> 
> Hi Guenter,
> 
> Guenter Roeck <linux@roeck-us.net> writes:
> > On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
> >> Hi,
> >> 
> >> The OS, or more precisely the user space, needs to be able to control
> >> a few things regarding USB Type-C ports. The first thing that must be
> >> allowed to be controlled is the data role. USB Type-C ports will
> >> select the data role randomly with DRP ports. When USB PD is
> >> supported, also independent (from data role) power role swapping can
> >> be supported together with Alternate Mode control.
> >> 
> >> I'm proposing with this set a Class for the Type-C connectors that
> >> gives the user space control over those things on top of getting basic
> >> details about the USB Type-C connectors and also partners. The details
> >> include the capabilities of the port, the supported data and power
> >> roles, supported accessories (audio and debug), supported Alternate
> >> Modes, USB PD support and of course the type of the partner (USB, Alt
> >> Mode, Accessory or Charger), and more or less the same details about
> >> the partner.
> >> 
> >> I'm not considering cables with this Class, and I have deliberately
> >> left out some more technical details, like cable orientation, firstly
> >> because I did not see much use for the user space from knowing that
> >> an secondly because that kind of details are not always available for
> >> example with UCSI.
> >> 
> >> So the interface to the user space is kept as simple as I dared to
> >> make it.
> >> 
> >> NOTE: In case there is somebody wondering, this is not adding USB PD
> >> support to Linux kernel. This is just about USB Type-C.
> >> 
> >
> > Hello Heikki,
> >
> > we have implemented a prototype TCPM (USB Type-C Protocol Manager)
> > software on top of your patch set. It will support TCPCI as well
> > as other USB-C controllers such as FUSB302. The plan is to use
> > this software in systems where no separate controller is available.
> >
> > Is there any chance to advance this patch set ? It would be instrumental
> > to get a unified interface to user space.
> 
> A newer version of $subject is already in Greg's queue [1]
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=0c1849a8c7af652c92ad0265a7ca5934fd773c69

No that only has the separated UCSI driver, not the class.



-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

Hi,

On Wed, May 04, 2016 at 08:05:44PM -0700, Guenter Roeck wrote:
> On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > The OS, or more precisely the user space, needs to be able to control
> > a few things regarding USB Type-C ports. The first thing that must be
> > allowed to be controlled is the data role. USB Type-C ports will
> > select the data role randomly with DRP ports. When USB PD is
> > supported, also independent (from data role) power role swapping can
> > be supported together with Alternate Mode control.
> > 
> > I'm proposing with this set a Class for the Type-C connectors that
> > gives the user space control over those things on top of getting basic
> > details about the USB Type-C connectors and also partners. The details
> > include the capabilities of the port, the supported data and power
> > roles, supported accessories (audio and debug), supported Alternate
> > Modes, USB PD support and of course the type of the partner (USB, Alt
> > Mode, Accessory or Charger), and more or less the same details about
> > the partner.
> > 
> > I'm not considering cables with this Class, and I have deliberately
> > left out some more technical details, like cable orientation, firstly
> > because I did not see much use for the user space from knowing that
> > an secondly because that kind of details are not always available for
> > example with UCSI.
> > 
> > So the interface to the user space is kept as simple as I dared to
> > make it.
> > 
> > NOTE: In case there is somebody wondering, this is not adding USB PD
> > support to Linux kernel. This is just about USB Type-C.
> > 
> 
> Hello Heikki,
> 
> we have implemented a prototype TCPM (USB Type-C Protocol Manager)
> software on top of your patch set. It will support TCPCI as well
> as other USB-C controllers such as FUSB302. The plan is to use
> this software in systems where no separate controller is available.

Interesting. So I'm guessing TCPM is actually an implementation USB PD
software stack, no? AFAIK FUSB302 has USB PD transceiver, right?

> Is there any chance to advance this patch set ? It would be instrumental
> to get a unified interface to user space.

I don't have not made any new code for the class driver yet, but I'm
attempting to prepare v2 next week.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> Hi,
>
> On Wed, May 04, 2016 at 08:05:44PM -0700, Guenter Roeck wrote:
>> On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> The OS, or more precisely the user space, needs to be able to control
>>> a few things regarding USB Type-C ports. The first thing that must be
>>> allowed to be controlled is the data role. USB Type-C ports will
>>> select the data role randomly with DRP ports. When USB PD is
>>> supported, also independent (from data role) power role swapping can
>>> be supported together with Alternate Mode control.
>>>
>>> I'm proposing with this set a Class for the Type-C connectors that
>>> gives the user space control over those things on top of getting basic
>>> details about the USB Type-C connectors and also partners. The details
>>> include the capabilities of the port, the supported data and power
>>> roles, supported accessories (audio and debug), supported Alternate
>>> Modes, USB PD support and of course the type of the partner (USB, Alt
>>> Mode, Accessory or Charger), and more or less the same details about
>>> the partner.
>>>
>>> I'm not considering cables with this Class, and I have deliberately
>>> left out some more technical details, like cable orientation, firstly
>>> because I did not see much use for the user space from knowing that
>>> an secondly because that kind of details are not always available for
>>> example with UCSI.
>>>
>>> So the interface to the user space is kept as simple as I dared to
>>> make it.
>>>
>>> NOTE: In case there is somebody wondering, this is not adding USB PD
>>> support to Linux kernel. This is just about USB Type-C.
>>>
>>
>> Hello Heikki,
>>
>> we have implemented a prototype TCPM (USB Type-C Protocol Manager)
>> software on top of your patch set. It will support TCPCI as well
>> as other USB-C controllers such as FUSB302. The plan is to use
>> this software in systems where no separate controller is available.
>
> Interesting. So I'm guessing TCPM is actually an implementation USB PD
> software stack, no? AFAIK FUSB302 has USB PD transceiver, right?
>
Yes, both correct.

>> Is there any chance to advance this patch set ? It would be instrumental
>> to get a unified interface to user space.
>
> I don't have not made any new code for the class driver yet, but I'm
> attempting to prepare v2 next week.
>

Excellent! Please cc me - either this e-mail address or groeck@chromium.org.

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

Heikki,

On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> Hi,
>
[ ... ]
>
> I don't have not made any new code for the class driver yet, but I'm
> attempting to prepare v2 next week.
>
Would it make sense to send feedback about v1 now, or should I wait for v2 ?

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote:
> Heikki,
> 
> On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> [ ... ]
> > 
> > I don't have not made any new code for the class driver yet, but I'm
> > attempting to prepare v2 next week.
> > 
> Would it make sense to send feedback about v1 now, or should I wait for v2 ?

I don't think I'm able to send v2 today, or even tomorrow, so feel
free to give the feedback. Just be aware that I've rewritten the
alternate mode part completely.

I'm creating a separate device for the partner and also the cable
during connection. I'm also already going to introduce a small bus for
the AltModes. It's clear that we need to have AltMode specific
drivers. The generic parts can't take care of all the AltMode specific
requirements and VDMs. The bus will give us a nice way to bind those
drivers to the actual AltModes a partner and the cable plugs offer.

So if there are dependencies between the altmodes, for example if the
cable plugs needs to be in a certain mode in order for the partner to
be able to function in some specific mode, the responsibility of
taking care of those will fall primarily to in the AltMode drivers.
So not userspace.

The AltMode drivers actually are useful also as they can be part of
the relevant frameworks, for example DP in some graphics framework.
For example in case of DP, the number of lanes (I guess 2 or 4) should
be ideally known if I have understood correctly. Knowledge about the
connection seems to also be needed, and I've so far seen some pretty
weird solutions for hotplug events with the DP AltMode. With the
driver we should be able to avoid those.

But in any case, every SVIDs a partner (or plug) offers will have
their own device registered with the partner (or cable) itself as
parent in this design. I'm expecting a little bit of conversation
about this plan, but right now I feel confident about it.

How does this sound to you?


Cheers,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

Hi,

On Wed, May 11, 2016 at 12:40:11PM +0300, Heikki Krogerus wrote:
> On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote:
> > Heikki,
> > 
> > On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> > > Hi,
> > > 
> > [ ... ]
> > > 
> > > I don't have not made any new code for the class driver yet, but I'm
> > > attempting to prepare v2 next week.
> > > 
> > Would it make sense to send feedback about v1 now, or should I wait for v2 ?
> 
> I don't think I'm able to send v2 today, or even tomorrow, so feel
> free to give the feedback. Just be aware that I've rewritten the
> alternate mode part completely.
> 
Alternate mode handling was my major concern, actually.

> I'm creating a separate device for the partner and also the cable
> during connection. I'm also already going to introduce a small bus for
> the AltModes. It's clear that we need to have AltMode specific
> drivers. The generic parts can't take care of all the AltMode specific
> requirements and VDMs. The bus will give us a nice way to bind those
> drivers to the actual AltModes a partner and the cable plugs offer.
> 
> So if there are dependencies between the altmodes, for example if the
> cable plugs needs to be in a certain mode in order for the partner to
> be able to function in some specific mode, the responsibility of
> taking care of those will fall primarily to in the AltMode drivers.
> So not userspace.
> 
> The AltMode drivers actually are useful also as they can be part of
> the relevant frameworks, for example DP in some graphics framework.
> For example in case of DP, the number of lanes (I guess 2 or 4) should
> be ideally known if I have understood correctly. Knowledge about the
> connection seems to also be needed, and I've so far seen some pretty
> weird solutions for hotplug events with the DP AltMode. With the
> driver we should be able to avoid those.
> 
> But in any case, every SVIDs a partner (or plug) offers will have
> their own device registered with the partner (or cable) itself as
> parent in this design. I'm expecting a little bit of conversation
> about this plan, but right now I feel confident about it.
> 
> How does this sound to you?
> 
Looking forward to it. My major problem so far was that alternate mode
handling is very platform specific, which didn't seem to be well supported
in v1 of your patch. I thought about implementing a hierarchy of drivers
below the type-c class to solve that problem. Looks like you just solved
it for me.

Other than that, my major concern is the lack of synchronization/protection
between the type-c class and the drivers. Setting port parameters (data role,
power role, operational power role, partner alternate modes, partner type)
from registered drivers may need to be synchronzed/protected. For example,
data and power role are set during connection establishment, but can be
overwritten from the typec class code. Right now I am just setting the
respective variables in struct typec_port directly, but that doesn't seem
right.

For partner_type, I don't really know how to map the options to the identity
reported by the partner. The reported product types are unknown / hub /
peripheral / passive cable / active cable / alternate mode adapter.
The available partner types are unknown / USB / Charger / Alternate Mode /
Audio Accessory / Debug Accessory. What am I missing here ?

The rest is just nitpicks.

- alternate_modes_show() and partner_alt_modes_show() discard the last byte
  of the generated string and replace it with \0.
- s/Accessroy/Accessory/
- typec_connect() and typec_disconnect() should probably also set
  port->connected.

Thanks,
Guenter

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

Hi,

On Wed, May 11, 2016 at 07:47:18AM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Wed, May 11, 2016 at 12:40:11PM +0300, Heikki Krogerus wrote:
> > On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote:
> > > Heikki,
> > > 
> > > On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> > > > Hi,
> > > > 
> > > [ ... ]
> > > > 
> > > > I don't have not made any new code for the class driver yet, but I'm
> > > > attempting to prepare v2 next week.
> > > > 
> > > Would it make sense to send feedback about v1 now, or should I wait for v2 ?
> > 
> > I don't think I'm able to send v2 today, or even tomorrow, so feel
> > free to give the feedback. Just be aware that I've rewritten the
> > alternate mode part completely.
> > 
> Alternate mode handling was my major concern, actually.
> 
> > I'm creating a separate device for the partner and also the cable
> > during connection. I'm also already going to introduce a small bus for
> > the AltModes. It's clear that we need to have AltMode specific
> > drivers. The generic parts can't take care of all the AltMode specific
> > requirements and VDMs. The bus will give us a nice way to bind those
> > drivers to the actual AltModes a partner and the cable plugs offer.
> > 
> > So if there are dependencies between the altmodes, for example if the
> > cable plugs needs to be in a certain mode in order for the partner to
> > be able to function in some specific mode, the responsibility of
> > taking care of those will fall primarily to in the AltMode drivers.
> > So not userspace.
> > 
> > The AltMode drivers actually are useful also as they can be part of
> > the relevant frameworks, for example DP in some graphics framework.
> > For example in case of DP, the number of lanes (I guess 2 or 4) should
> > be ideally known if I have understood correctly. Knowledge about the
> > connection seems to also be needed, and I've so far seen some pretty
> > weird solutions for hotplug events with the DP AltMode. With the
> > driver we should be able to avoid those.
> > 
> > But in any case, every SVIDs a partner (or plug) offers will have
> > their own device registered with the partner (or cable) itself as
> > parent in this design. I'm expecting a little bit of conversation
> > about this plan, but right now I feel confident about it.
> > 
> > How does this sound to you?
> > 
> Looking forward to it. My major problem so far was that alternate mode
> handling is very platform specific, which didn't seem to be well supported
> in v1 of your patch. I thought about implementing a hierarchy of drivers
> below the type-c class to solve that problem. Looks like you just solved
> it for me.
> 
> Other than that, my major concern is the lack of synchronization/protection
> between the type-c class and the drivers. Setting port parameters (data role,
> power role, operational power role, partner alternate modes, partner type)
> from registered drivers may need to be synchronzed/protected. For example,
> data and power role are set during connection establishment, but can be
> overwritten from the typec class code. Right now I am just setting the
> respective variables in struct typec_port directly, but that doesn't seem
> right.

I'm actually struggling with this same question. I decided to protect
the whole struct typec_port by not allowing the drivers to touch it,
but I'm still working on it..

> For partner_type, I don't really know how to map the options to the identity
> reported by the partner. The reported product types are unknown / hub /
> peripheral / passive cable / active cable / alternate mode adapter.
> The available partner types are unknown / USB / Charger / Alternate Mode /
> Audio Accessory / Debug Accessory. What am I missing here ?

The partner types don't map directly to the USB PD Product Types. We
need to describe the partner even when USB PD is not available.
Accessory Modes are for example out side the scope of USB PD.

But I'll try to propose something for those.

> The rest is just nitpicks.
> 
> - alternate_modes_show() and partner_alt_modes_show() discard the last byte
>   of the generated string and replace it with \0.
> - s/Accessroy/Accessory/
> - typec_connect() and typec_disconnect() should probably also set
>   port->connected.

OK. I'll check these.

So I failed to finish my proposal for v2 this week. On top of the
sync/protection problems, I'm also still trying to tune my AltMode
bus. I'm going to attempt to send it as an RFC on Monday or Tuesday in
any case.


Thanks,

-- 
heikki

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Guenter Roeck]

Hi,

On Fri, May 13, 2016 at 05:23:21PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, May 11, 2016 at 07:47:18AM -0700, Guenter Roeck wrote:
> > Hi,
> > 
> > On Wed, May 11, 2016 at 12:40:11PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote:
> > > > Heikki,
> > > > 
> > > > On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> > > > > Hi,
> > > > > 
> > > > [ ... ]
> > > > > 
> > > > > I don't have not made any new code for the class driver yet, but I'm
> > > > > attempting to prepare v2 next week.
> > > > > 
> > > > Would it make sense to send feedback about v1 now, or should I wait for v2 ?
> > > 
> > > I don't think I'm able to send v2 today, or even tomorrow, so feel
> > > free to give the feedback. Just be aware that I've rewritten the
> > > alternate mode part completely.
> > > 
> > Alternate mode handling was my major concern, actually.
> > 
> > > I'm creating a separate device for the partner and also the cable
> > > during connection. I'm also already going to introduce a small bus for
> > > the AltModes. It's clear that we need to have AltMode specific
> > > drivers. The generic parts can't take care of all the AltMode specific
> > > requirements and VDMs. The bus will give us a nice way to bind those
> > > drivers to the actual AltModes a partner and the cable plugs offer.
> > > 
> > > So if there are dependencies between the altmodes, for example if the
> > > cable plugs needs to be in a certain mode in order for the partner to
> > > be able to function in some specific mode, the responsibility of
> > > taking care of those will fall primarily to in the AltMode drivers.
> > > So not userspace.
> > > 
> > > The AltMode drivers actually are useful also as they can be part of
> > > the relevant frameworks, for example DP in some graphics framework.
> > > For example in case of DP, the number of lanes (I guess 2 or 4) should
> > > be ideally known if I have understood correctly. Knowledge about the
> > > connection seems to also be needed, and I've so far seen some pretty
> > > weird solutions for hotplug events with the DP AltMode. With the
> > > driver we should be able to avoid those.
> > > 
> > > But in any case, every SVIDs a partner (or plug) offers will have
> > > their own device registered with the partner (or cable) itself as
> > > parent in this design. I'm expecting a little bit of conversation
> > > about this plan, but right now I feel confident about it.
> > > 
> > > How does this sound to you?
> > > 
> > Looking forward to it. My major problem so far was that alternate mode
> > handling is very platform specific, which didn't seem to be well supported
> > in v1 of your patch. I thought about implementing a hierarchy of drivers
> > below the type-c class to solve that problem. Looks like you just solved
> > it for me.
> > 
> > Other than that, my major concern is the lack of synchronization/protection
> > between the type-c class and the drivers. Setting port parameters (data role,
> > power role, operational power role, partner alternate modes, partner type)
> > from registered drivers may need to be synchronzed/protected. For example,
> > data and power role are set during connection establishment, but can be
> > overwritten from the typec class code. Right now I am just setting the
> > respective variables in struct typec_port directly, but that doesn't seem
> > right.
> 
> I'm actually struggling with this same question. I decided to protect
> the whole struct typec_port by not allowing the drivers to touch it,
> but I'm still working on it..
> 
Sounds good to me, as long as you provide APIs to change the values.

> > For partner_type, I don't really know how to map the options to the identity
> > reported by the partner. The reported product types are unknown / hub /
> > peripheral / passive cable / active cable / alternate mode adapter.
> > The available partner types are unknown / USB / Charger / Alternate Mode /
> > Audio Accessory / Debug Accessory. What am I missing here ?
> 
> The partner types don't map directly to the USB PD Product Types. We
> need to describe the partner even when USB PD is not available.
> Accessory Modes are for example out side the scope of USB PD.
> 
> But I'll try to propose something for those.
> 
Ok.

> > The rest is just nitpicks.
> > 
> > - alternate_modes_show() and partner_alt_modes_show() discard the last byte
> >   of the generated string and replace it with \0.
> > - s/Accessroy/Accessory/
> > - typec_connect() and typec_disconnect() should probably also set
> >   port->connected.
> 
> OK. I'll check these.
> 
> So I failed to finish my proposal for v2 this week. On top of the
> sync/protection problems, I'm also still trying to tune my AltMode
> bus. I'm going to attempt to send it as an RFC on Monday or Tuesday in
> any case.
> 
No worries. It turns out the PD code in existing type-c devices isn't
exactly stable or predictable, so I end up spending a lot of time
trying to stabilize my state machine anyway.

Thanks,
Guenter