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

Groups > linux.kernel > #1433742 > unrolled thread

[PATCHv4 0/2] USB Type-C Connector class

Back to article view | Back to linux.kernel

Contents

  [PATCHv4 0/2] USB Type-C Connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-29 15:40 +0200
    [PATCHv4 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-29 15:40 +0200
    Re: [PATCHv4 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-06-30 19:20 +0200
      Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-01 09:40 +0200
        Re: [PATCHv4 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-07-01 16:50 +0200
    Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-01 09:20 +0200
      Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-01 14:10 +0200
        Re: [PATCHv4 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-07-01 16:40 +0200
          Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-03 21:40 +0200
            Re: [PATCHv4 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-07-03 23:30 +0200
              Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-04 19:20 +0200
                Re: [PATCHv4 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-07-04 19:50 +0200
                  Re: [PATCHv4 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-07-05 20:50 +0200

#1433742 — [PATCHv4 0/2] USB Type-C Connector class

Hi,

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.



Heikki Krogerus (2):
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  236 +++++
 Documentation/usb/typec.txt                 |  103 +++
 MAINTAINERS                                 |    9 +
 drivers/usb/Kconfig                         |    2 +
 drivers/usb/Makefile                        |    2 +
 drivers/usb/typec/Kconfig                   |   21 +
 drivers/usb/typec/Makefile                  |    2 +
 drivers/usb/typec/typec.c                   | 1277 +++++++++++++++++++++++++++
 drivers/usb/typec/typec_wcove.c             |  371 ++++++++
 include/linux/usb/typec.h                   |  260 ++++++
 10 files changed, 2283 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.8.1

[toc] | [next] | [standalone]

#1433743 — [PATCHv4 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/Kconfig       |  14 ++
 drivers/usb/typec/Makefile      |   1 +
 drivers/usb/typec/typec_wcove.c | 371 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 386 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index b229fb9..7a345a4 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB PD and Type-C drivers"
 config TYPEC
 	tristate
 
+config TYPEC_WCOVE
+	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+	depends on ACPI
+	depends on INTEL_SOC_PMIC
+	depends on INTEL_PMC_IPC
+	select TYPEC
+	help
+	  This driver adds support for USB Type-C detection on Intel Broxton
+	  platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+	  role and cable orientation.
+
+	  To compile this driver as module, choose M here: the module will be
+	  called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8b..b9cb862 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index 0000000..c7c2d28
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,371 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY 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/acpi.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/usb/typec.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/intel_soc_pmic.h>
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0		0x4e09
+#define WCOVE_PHYCTRL		0x5e07
+
+#define USBC_CONTROL1		0x7001
+#define USBC_CONTROL2		0x7002
+#define USBC_CONTROL3		0x7003
+#define USBC_CC1_CTRL		0x7004
+#define USBC_CC2_CTRL		0x7005
+#define USBC_STATUS1		0x7007
+#define USBC_STATUS2		0x7008
+#define USBC_STATUS3		0x7009
+#define USBC_IRQ1		0x7015
+#define USBC_IRQ2		0x7016
+#define USBC_IRQMASK1		0x7017
+#define USBC_IRQMASK2		0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)	((r & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNK		BIT(0)
+#define USBC_CONTROL2_UNATT_SRC		BIT(1)
+#define USBC_CONTROL2_DIS_ST		BIT(2)
+
+#define USBC_CONTROL3_PD_DIS		BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN		BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING	BIT(6)
+#define USBC_STATUS1_RSLT(r)		(r & 0xf)
+#define USBC_RSLT_NOTHING		0
+#define USBC_RSLT_SRC_DEFAULT		1
+#define USBC_RSLT_SRC_1_5A		2
+#define USBC_RSLT_SRC_3_0A		3
+#define USBC_RSLT_SNK			4
+#define USBC_RSLT_DEBUG_ACC		5
+#define USBC_RSLT_AUDIO_ACC		6
+#define USBC_RSLT_UNDEF			15
+#define USBC_STATUS1_ORIENT(r)		((r >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL		1
+#define USBC_ORIENT_REVERSE		2
+
+#define USBC_STATUS2_VBUS_REQ		BIT(5)
+
+#define USBC_IRQ1_ADCDONE1		BIT(2)
+#define USBC_IRQ1_OVERTEMP		BIT(1)
+#define USBC_IRQ1_SHORT			BIT(0)
+
+#define USBC_IRQ2_CC_CHANGE		BIT(7)
+#define USBC_IRQ2_RX_PD			BIT(6)
+#define USBC_IRQ2_RX_HR			BIT(5)
+#define USBC_IRQ2_RX_CR			BIT(4)
+#define USBC_IRQ2_TX_SUCCESS		BIT(3)
+#define USBC_IRQ2_TX_FAIL		BIT(2)
+
+#define USBC_IRQMASK1_ALL	(USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+				 USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL	(USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+				 USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+				 USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+	struct mutex lock; /* device lock */
+	struct device *dev;
+	struct regmap *regmap;
+	struct typec_port *port;
+	struct typec_capability cap;
+	struct typec_connection con;
+	struct typec_partner partner;
+};
+
+enum wcove_typec_func {
+	WCOVE_FUNC_DRIVE_VBUS = 1,
+	WCOVE_FUNC_ORIENTATION,
+	WCOVE_FUNC_ROLE,
+	WCOVE_FUNC_DRIVE_VCONN,
+};
+
+enum wcove_typec_orientation {
+	WCOVE_ORIENTATION_NORMAL,
+	WCOVE_ORIENTATION_REVERSE,
+};
+
+enum wcove_typec_role {
+	WCOVE_ROLE_HOST,
+	WCOVE_ROLE_DEVICE,
+};
+
+static uuid_le uuid = UUID_LE(0x482383f0, 0x2876, 0x4e49,
+			      0x86, 0x85, 0xdb, 0x66, 0x21, 0x1a, 0xf0, 0x37);
+
+static int wcove_typec_func(struct wcove_typec *wcove,
+			    enum wcove_typec_func func, int param)
+{
+	union acpi_object *obj;
+	union acpi_object tmp;
+	union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
+
+	tmp.type = ACPI_TYPE_INTEGER;
+	tmp.integer.value = param;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(wcove->dev), uuid.b, 1, func,
+				&argv4);
+	if (!obj) {
+		dev_err(wcove->dev, "%s: failed to evaluate _DSM\n", __func__);
+		return -EIO;
+	}
+
+	ACPI_FREE(obj);
+	return 0;
+}
+
+static void wcove_typec_device_mode(struct wcove_typec *wcove)
+{
+	wcove->partner.type = TYPEC_PARTNER_USB;
+	wcove->con.partner = &wcove->partner;
+	wcove->con.pwr_role = TYPEC_SINK;
+	wcove->con.vconn_role = TYPEC_SINK;
+	wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_DEVICE);
+	typec_connect(wcove->port, &wcove->con);
+}
+
+static irqreturn_t wcove_typec_irq(int irq, void *data)
+{
+	struct wcove_typec *wcove = data;
+	unsigned int cc1_ctrl;
+	unsigned int cc2_ctrl;
+	unsigned int cc_irq1;
+	unsigned int cc_irq2;
+	unsigned int status1;
+	unsigned int status2;
+	int ret;
+
+	mutex_lock(&wcove->lock);
+
+	ret = regmap_read(wcove->regmap, USBC_IRQ1, &cc_irq1);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_IRQ2, &cc_irq2);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_STATUS1, &status1);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_STATUS2, &status2);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_CC1_CTRL, &cc1_ctrl);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_CC2_CTRL, &cc2_ctrl);
+	if (ret)
+		goto err;
+
+	if (cc_irq1) {
+		if (cc_irq1 & USBC_IRQ1_OVERTEMP)
+			dev_err(wcove->dev, "VCONN Switch Over Temperature!\n");
+		if (cc_irq1 & USBC_IRQ1_SHORT)
+			dev_err(wcove->dev, "VCONN Switch Short Circuit!\n");
+		regmap_write(wcove->regmap, USBC_IRQ1, cc_irq1);
+	}
+
+	if (cc_irq2) {
+		regmap_write(wcove->regmap, USBC_IRQ2, cc_irq2);
+		/*
+		 * Ingoring any PD communication interrupts until the PD stack
+		 * is in place
+		 */
+		if (cc_irq2 & ~USBC_IRQ2_CC_CHANGE) {
+			dev_WARN(wcove->dev, "USB PD handling missing\n");
+			goto err;
+		}
+	}
+
+	if (status1 & USBC_STATUS1_DET_ONGOING)
+		goto out;
+
+	if (USBC_STATUS1_RSLT(status1) == USBC_RSLT_NOTHING) {
+		if (wcove->con.partner) {
+			typec_disconnect(wcove->port);
+			memset(&wcove->con, 0, sizeof(wcove->con));
+			memset(&wcove->partner, 0, sizeof(wcove->partner));
+		}
+
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_NORMAL);
+		/* Host mode by default */
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+		goto out;
+	}
+
+	if (wcove->con.partner)
+		goto out;
+
+	switch (USBC_STATUS1_ORIENT(status1)) {
+	case USBC_ORIENT_NORMAL:
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_NORMAL);
+		break;
+	case USBC_ORIENT_REVERSE:
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_REVERSE);
+	default:
+		break;
+	}
+
+	switch (USBC_STATUS1_RSLT(status1)) {
+	case USBC_RSLT_SRC_DEFAULT:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_USB;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SRC_1_5A:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_1_5A;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SRC_3_0A:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_3_0A;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SNK:
+		wcove->partner.type = TYPEC_PARTNER_USB;
+		wcove->con.partner = &wcove->partner;
+		wcove->con.data_role = TYPEC_HOST;
+		wcove->con.pwr_role = TYPEC_SOURCE;
+		wcove->con.vconn_role = TYPEC_SOURCE;
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	case USBC_RSLT_DEBUG_ACC:
+		wcove->partner.accessory = TYPEC_ACCESSORY_DEBUG;
+		wcove->partner.type = TYPEC_PARTNER_ACCESSORY;
+		wcove->con.partner = &wcove->partner;
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	case USBC_RSLT_AUDIO_ACC:
+		wcove->partner.accessory = TYPEC_ACCESSORY_AUDIO;
+		wcove->partner.type = TYPEC_PARTNER_ACCESSORY;
+		wcove->con.partner = &wcove->partner;
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	default:
+		dev_WARN(wcove->dev, "%s Undefined result\n", __func__);
+		goto err;
+	}
+out:
+	/* If either CC pins is requesting VCONN, we turn it on */
+	if ((cc1_ctrl & USBC_CC_CTRL_VCONN_EN) ||
+	    (cc2_ctrl &	USBC_CC_CTRL_VCONN_EN))
+		wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VCONN, true);
+	else
+		wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VCONN, false);
+
+	/* Relying on the FSM to know when we need to drive VBUS. */
+	wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VBUS,
+			 !!(status2 & USBC_STATUS2_VBUS_REQ));
+err:
+	/* REVISIT: Clear WhiskeyCove CHGR Type-C interrupt */
+	regmap_write(wcove->regmap, WCOVE_CHGRIRQ0, BIT(5));
+
+	mutex_unlock(&wcove->lock);
+	return IRQ_HANDLED;
+}
+
+static int wcove_typec_probe(struct platform_device *pdev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	struct wcove_typec *wcove;
+	unsigned int val;
+	int ret;
+
+	wcove = devm_kzalloc(&pdev->dev, sizeof(*wcove), GFP_KERNEL);
+	if (!wcove)
+		return -ENOMEM;
+
+	mutex_init(&wcove->lock);
+	wcove->dev = &pdev->dev;
+	wcove->regmap = pmic->regmap;
+
+	ret = regmap_irq_get_virq(pmic->irq_chip_data_level2,
+				  platform_get_irq(pdev, 0));
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_threaded_irq(&pdev->dev, ret, NULL,
+					wcove_typec_irq, IRQF_ONESHOT,
+					"wcove_typec", wcove);
+	if (ret)
+		return ret;
+
+	wcove->cap.type = TYPEC_PORT_DRP;
+
+	wcove->port = typec_register_port(&pdev->dev, &wcove->cap);
+	if (IS_ERR(wcove->port))
+		return PTR_ERR(wcove->port);
+
+	if (!acpi_check_dsm(ACPI_HANDLE(&pdev->dev), uuid.b, 0, 0x1f)) {
+		dev_err(&pdev->dev, "Missing _DSM functions\n");
+		return -ENODEV;
+	}
+
+	/* Make sure the PD PHY is disabled until PD stack is ready */
+	regmap_read(wcove->regmap, USBC_CONTROL3, &val);
+	regmap_write(wcove->regmap, USBC_CONTROL3, val | USBC_CONTROL3_PD_DIS);
+
+	/* DRP mode without accessory support */
+	regmap_read(wcove->regmap, USBC_CONTROL1, &val);
+	regmap_write(wcove->regmap, USBC_CONTROL1, USBC_CONTROL1_MODE_DRP(val));
+
+	/* Unmask everything */
+	regmap_read(wcove->regmap, USBC_IRQMASK1, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK1, val & ~USBC_IRQMASK1_ALL);
+	regmap_read(wcove->regmap, USBC_IRQMASK2, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK2, val & ~USBC_IRQMASK2_ALL);
+
+	platform_set_drvdata(pdev, wcove);
+	return 0;
+}
+
+static int wcove_typec_remove(struct platform_device *pdev)
+{
+	struct wcove_typec *wcove = platform_get_drvdata(pdev);
+	unsigned int val;
+
+	/* Mask everything */
+	regmap_read(wcove->regmap, USBC_IRQMASK1, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK1, val | USBC_IRQMASK1_ALL);
+	regmap_read(wcove->regmap, USBC_IRQMASK2, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK2, val | USBC_IRQMASK2_ALL);
+
+	typec_unregister_port(wcove->port);
+	return 0;
+}
+
+static struct platform_driver wcove_typec_driver = {
+	.driver = {
+		.name		= "bxt_wcove_usbc",
+	},
+	.probe			= wcove_typec_probe,
+	.remove			= wcove_typec_remove,
+};
+
+module_platform_driver(wcove_typec_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("WhiskeyCove PMIC USB Type-C PHY driver");
-- 
2.8.1

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

#1434747 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
[ ... ]

> +
> +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the partner supports USB Power Delivery.
> +		- 1 if USB Power Delivery is supported
> +		- 0 when it's not
> +
> +
> +What:		/sys/class/typec/<port>-partner/id_header_vdo
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		If the partner supports USB Power Deliver, shows the VDO
> +		returned from Discover Identity USB Power Delivery command.
> +
> +		If the partner does not support USB Power Delivery, the
> +		attribute is hidden.
> +
On second thought, and after merging the code (and realizing that I don't get
the raw data from the Type-C Port Manager), I am not sure if a raw attribute
is that useful here. It also doesn't provide all information either.

Would it make sense to split it into multiple decoded attributes ?

- vendor-id
  [bit 0..15 of ID header VDO]
- product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
	passive/active for cable plugs)
  Might map into typec_partner_type, but I don't see a 1:1 match.
  [bit 27..29 of ID header VDO]
- alternate-mode-supported
  [bit 26 of ID header VDO]
- capabilities (ufp, dfp, drp, none (?))
  [bit 30/31 of ID header VDO]
- product-id
  [bit 16..31 of Product VDO]

Does this make any sense ?

Thanks,
Guenter

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

#1435082 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> [ ... ]
> 
> > +
> > +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> > +Date:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +		Shows if the partner supports USB Power Delivery.
> > +		- 1 if USB Power Delivery is supported
> > +		- 0 when it's not
> > +
> > +
> > +What:		/sys/class/typec/<port>-partner/id_header_vdo
> > +Date:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +		If the partner supports USB Power Deliver, shows the VDO
> > +		returned from Discover Identity USB Power Delivery command.
> > +
> > +		If the partner does not support USB Power Delivery, the
> > +		attribute is hidden.
> > +
> On second thought, and after merging the code (and realizing that I don't get
> the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> is that useful here. It also doesn't provide all information either.

Yeah, I don't think it's available with UCSI either..

> Would it make sense to split it into multiple decoded attributes ?
> 
> - vendor-id
>   [bit 0..15 of ID header VDO]
> - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> 	passive/active for cable plugs)
>   Might map into typec_partner_type, but I don't see a 1:1 match.
>   [bit 27..29 of ID header VDO]
> - alternate-mode-supported
>   [bit 26 of ID header VDO]
> - capabilities (ufp, dfp, drp, none (?))
>   [bit 30/31 of ID header VDO]
> - product-id
>   [bit 16..31 of Product VDO]
> 
> Does this make any sense ?

I feel a bit uncomfortable exposing so many attribute like that which
will give details that we can only know when both ends support USB
PD...

Here's my proposal for this:
There has to be a special group for these devices, partners and
cables, a directory named "pd" or "power_deliver" (or something like
that), which exposes those. That group will not be responsibility of
this class, but instead the PD framework that you are working on
(right?).

So basically, in this class we will not expose those attributes, and
we'll also get rid of the "supports_usb_power_delivery" attribute (the
"pd" groups should only exist when USB PD is actually supported).

Initially that group needs to be assigned to the "groups" member of
struct device of the partners in the drivers before they are
registered. That member is meant for optional attribute groups, but we
can later think of something better, a way perhaps to bind that group
the device types "groups" member for partners, cables, etc. or
something similar in case using the "groups" member of struct device
is not ideal. But initially we can use that.

How would that sound to you?


Thanks,

-- 
heikki

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

#1435387 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Fri, Jul 01, 2016 at 10:38:03AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > [ ... ]
> > 
> > > +
> > > +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +		Shows if the partner supports USB Power Delivery.
> > > +		- 1 if USB Power Delivery is supported
> > > +		- 0 when it's not
> > > +
> > > +
> > > +What:		/sys/class/typec/<port>-partner/id_header_vdo
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +		If the partner supports USB Power Deliver, shows the VDO
> > > +		returned from Discover Identity USB Power Delivery command.
> > > +
> > > +		If the partner does not support USB Power Delivery, the
> > > +		attribute is hidden.
> > > +
> > On second thought, and after merging the code (and realizing that I don't get
> > the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> > is that useful here. It also doesn't provide all information either.
> 
> Yeah, I don't think it's available with UCSI either..
> 
> > Would it make sense to split it into multiple decoded attributes ?
> > 
> > - vendor-id
> >   [bit 0..15 of ID header VDO]
> > - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> > 	passive/active for cable plugs)
> >   Might map into typec_partner_type, but I don't see a 1:1 match.
> >   [bit 27..29 of ID header VDO]
> > - alternate-mode-supported
> >   [bit 26 of ID header VDO]
> > - capabilities (ufp, dfp, drp, none (?))
> >   [bit 30/31 of ID header VDO]
> > - product-id
> >   [bit 16..31 of Product VDO]
> > 
> > Does this make any sense ?
> 
> I feel a bit uncomfortable exposing so many attribute like that which
> will give details that we can only know when both ends support USB
> PD...
> 
> Here's my proposal for this:
> There has to be a special group for these devices, partners and
> cables, a directory named "pd" or "power_deliver" (or something like
> that), which exposes those. That group will not be responsibility of
> this class, but instead the PD framework that you are working on
> (right?).
> 
> So basically, in this class we will not expose those attributes, and
> we'll also get rid of the "supports_usb_power_delivery" attribute (the
> "pd" groups should only exist when USB PD is actually supported).
> 
> Initially that group needs to be assigned to the "groups" member of
> struct device of the partners in the drivers before they are
> registered. That member is meant for optional attribute groups, but we
> can later think of something better, a way perhaps to bind that group
> the device types "groups" member for partners, cables, etc. or
> something similar in case using the "groups" member of struct device
> is not ideal. But initially we can use that.
> 
> How would that sound to you?
> 
Quite frankly I don't know. At first glance it sounds like a bad idea - it means
that PD specific atttributes would not be standardized, which would reduce the
valus of the class code substantially. I'll have to think about this.

At the same time I'll have to find a somewhat working solution for the locking
issues. I'll have to focus on that for the time being, because it affects
the core architecture of our code. So I don't really have the time to look
into attribute support right now.

Guenter

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

#1435075 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

Hi Guenter,

On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > +static ssize_t
> > +preferred_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_role role;
> > +	int ret;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->cap->try_role) {
> > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> 
> With this, 'echo "sink" > preferred_role' no longer works because
> match_string() tries to match the entire string, including the newline
> generated by the echo command. One has to use "echo -n" instead.
> Is this on purpose ? It is quite unusual.

For some reason I though 'echo -n is expected. I guess I'm still
living in the past in the days when the kernel version was still 2.4.
It did not use to be so unusual, and there are still plenty of sysfs
attributes that do expect 'echo -n..'.

But I'll fix this.

> > +	if (ret < 0) {
> > +		port->prefer_role = -1;
> > +		ret = size;
> > +		goto out;
> > +	}
> > +
> > +	role = ret;
> > +
> > +	ret = port->cap->try_role(port->cap, role);
> > +	if (ret)
> > +		goto out;
> > +
> > +	port->prefer_role = role;
> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t
> > +preferred_role_show(struct device *dev, struct device_attribute *attr,
> > +		    char *buf)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	ssize_t ret = 0;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	if (port->prefer_role < 0)
> > +		goto out;
> > +
> > +	ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(preferred_role);
> > +
> > +static ssize_t
> > +current_data_role_store(struct device *dev, struct device_attribute *attr,
> > +			const char *buf, size_t size)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	int ret = size;
> > +
> > +	mutex_lock(&port->lock);
> > +
> With this lock, the code in the driver can no longer call any state updates
> during the call to dr_set(), because those (eg typec_set_data_role()) set
> the lock as well. This means that the dr_set call and all other similar calls
> now have to be asynchronous. As a result, those calls can not return an error
> if the role change request fails. Is this on purpose ? I see it as a major
> drawback, not only because errors can no longer be returned, but also because
> I very much preferred the call to be synchronous.

But the drivers should not call typec_set_data_role() in these
cases? That API the drivers should only use if the partners execute
the swaps.

So in this case, we need to set the role and notify sysfs here. This
function has to return successfully only if the role swap really
happened, and dr_set() of course can not return before the driver
actually knows the result of, for example, DR_swap.

> > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->cap->dr_set) {
> > +		dev_dbg(dev, "data role swapping not supported\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->connected)
> > +		goto out;
> > +
> > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = port->cap->dr_set(port->cap, ret);
> > +	if (ret)
> > +		goto out;

I could have sworn I was setting the port->data_role here, but I guess
it was removed at some point...

Need to fix that.

> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}


Thanks,

-- 
heikki

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

#1435284 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Fri, Jul 01, 2016 at 10:13:48AM +0300, Heikki Krogerus wrote:
> Hi Guenter,
> 
> On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > > +static ssize_t
> > > +preferred_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_role role;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&port->lock);
> > > +
> > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->cap->try_role) {
> > > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > 
> > With this, 'echo "sink" > preferred_role' no longer works because
> > match_string() tries to match the entire string, including the newline
> > generated by the echo command. One has to use "echo -n" instead.
> > Is this on purpose ? It is quite unusual.
> 
> For some reason I though 'echo -n is expected. I guess I'm still
> living in the past in the days when the kernel version was still 2.4.
> It did not use to be so unusual, and there are still plenty of sysfs
> attributes that do expect 'echo -n..'.
> 
> But I'll fix this.

<snip>

> > > +static ssize_t
> > > +current_data_role_store(struct device *dev, struct device_attribute *attr,
> > > +			const char *buf, size_t size)
> > > +{
> > > +	struct typec_port *port = to_typec_port(dev);
> > > +	int ret = size;
> > > +
> > > +	mutex_lock(&port->lock);
> > > +
> > With this lock, the code in the driver can no longer call any state updates
> > during the call to dr_set(), because those (eg typec_set_data_role()) set
> > the lock as well. This means that the dr_set call and all other similar calls
> > now have to be asynchronous. As a result, those calls can not return an error
> > if the role change request fails. Is this on purpose ? I see it as a major
> > drawback, not only because errors can no longer be returned, but also because
> > I very much preferred the call to be synchronous.
> 
> But the drivers should not call typec_set_data_role() in these
> cases? That API the drivers should only use if the partners execute
> the swaps.
> 
> So in this case, we need to set the role and notify sysfs here. This
> function has to return successfully only if the role swap really
> happened, and dr_set() of course can not return before the driver
> actually knows the result of, for example, DR_swap.
> 
> > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->cap->dr_set) {
> > > +		dev_dbg(dev, "data role swapping not supported\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->connected)
> > > +		goto out;
> > > +
> > > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > > +	if (ret < 0)
> > > +		goto out;
> > > +
> > > +	ret = port->cap->dr_set(port->cap, ret);
> > > +	if (ret)
> > > +		goto out;
> 
> I could have sworn I was setting the port->data_role here, but I guess
> it was removed at some point...
> 
> Need to fix that.

I've updated my github branch with a commit where both of these issues
should be fixed. Can you give it a try?

I've also removed the supports_usb_power_delivery and id_header_vdo
attributes from partners and cables. We really have to put all the USB
PD specific attributes to its own group, and that group can't be in
control of this class. We will simply not always have access to the
kind of USB PD specific details you want to show, for example details
that you get from discover identity command, even when USB PD is fully
supported. For example on systems that use UCSI.

The alternate mode VDO is the only that we can for fairly certainly
always get, and the only things purely tied to USB PD (the plugs are
also, but the only purpose for them is to expose the alternate modes
they have).

Man, I was always against coupling this thing too tightly with USB PD.
I'm slipping.


Thanks,

-- 
heikki

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

#1435384 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> On Fri, Jul 01, 2016 at 10:13:48AM +0300, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > > > +static ssize_t
> > > > +preferred_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_role role;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->try_role) {
> > > > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > > 
> > > With this, 'echo "sink" > preferred_role' no longer works because
> > > match_string() tries to match the entire string, including the newline
> > > generated by the echo command. One has to use "echo -n" instead.
> > > Is this on purpose ? It is quite unusual.
> > 
> > For some reason I though 'echo -n is expected. I guess I'm still
> > living in the past in the days when the kernel version was still 2.4.
> > It did not use to be so unusual, and there are still plenty of sysfs
> > attributes that do expect 'echo -n..'.
> > 
> > But I'll fix this.
> 
> <snip>
> 
> > > > +static ssize_t
> > > > +current_data_role_store(struct device *dev, struct device_attribute *attr,
> > > > +			const char *buf, size_t size)
> > > > +{
> > > > +	struct typec_port *port = to_typec_port(dev);
> > > > +	int ret = size;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > With this lock, the code in the driver can no longer call any state updates
> > > during the call to dr_set(), because those (eg typec_set_data_role()) set
> > > the lock as well. This means that the dr_set call and all other similar calls
> > > now have to be asynchronous. As a result, those calls can not return an error
> > > if the role change request fails. Is this on purpose ? I see it as a major
> > > drawback, not only because errors can no longer be returned, but also because
> > > I very much preferred the call to be synchronous.
> > 
> > But the drivers should not call typec_set_data_role() in these
> > cases? That API the drivers should only use if the partners execute
> > the swaps.
> > 
> > So in this case, we need to set the role and notify sysfs here. This
> > function has to return successfully only if the role swap really
> > happened, and dr_set() of course can not return before the driver
> > actually knows the result of, for example, DR_swap.
> > 
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->dr_set) {
> > > > +		dev_dbg(dev, "data role swapping not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->connected)
> > > > +		goto out;
> > > > +
> > > > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	ret = port->cap->dr_set(port->cap, ret);
> > > > +	if (ret)
> > > > +		goto out;
> > 
> > I could have sworn I was setting the port->data_role here, but I guess
> > it was removed at some point...
> > 
> > Need to fix that.
> 
> I've updated my github branch with a commit where both of these issues
> should be fixed. Can you give it a try?
> 

This is going to be very difficult. So far, calls such as
typec_set_data_role() were independent (asynchronous) of role change
requests through sysfs, meaning they happened whenever lower level
confirmed that a role change was complete, for whatever reason. Now
I have to check if a role change request through the class code
is pending and not call typec_set_data_role() and friends in that case.

This becomes even more complicated in situations with parallel role
change requests both from the class code and from the partner.
In such cases the class code may have acquired the lock, and before it
calls the driver, a role change complete is reported. The call to
typec_set_XXX_role() will then get stuck. So I'll have to add another
flag before calling typec_set_data_role() and friends, and then reject
the call to dr_set and all other set requests with -EBUSY.
race conditions.

Thinking more ... ultimately, this means that I'll have to reject role
change requests from the class code whenever the state machine is busy,
because I never know if the state machine activity would result in a
call into the typec class code. At the same time, the protocol driver
will have to reject incoming role change requests while a locally
requested role change request is pending, even if that internal
role change request has not yet resulted in a PD message being sent.

Is this really necessary ? It creates a lot of interdependencies.

In summary, this means a substantial amount of code and testing, which
is going to take a while.

You might want to mention all this in the API documentation.

> I've also removed the supports_usb_power_delivery and id_header_vdo
> attributes from partners and cables. We really have to put all the USB
> PD specific attributes to its own group, and that group can't be in
> control of this class. We will simply not always have access to the
> kind of USB PD specific details you want to show, for example details
> that you get from discover identity command, even when USB PD is fully
> supported. For example on systems that use UCSI.
> 

I think we should have a single unified ABI, independent of the underlying
driver, that informs the user about the partner device. I really don't
quite understand why the class code should not be able to report what device
it is connected to (while at the same time being able to report the alternate
modes it supports).

Following your logic, one could argue that all alternate mode handling
should be taken out as well. After all, alternate modes are also tied
to PD protocol support, and discovering the partner identity is a
prerequisite of discovering the alternate modes it supports.
That really doesn't make any sense.

Guenter

> The alternate mode VDO is the only that we can for fairly certainly
> always get, and the only things purely tied to USB PD (the plugs are
> also, but the only purpose for them is to expose the alternate modes
> they have).
> 
> Man, I was always against coupling this thing too tightly with USB PD.
> I'm slipping.
> 
> 
> Thanks,
> 
> -- 
> heikki

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

#1436030 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
> On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> > I've updated my github branch with a commit where both of these issues
> > should be fixed. Can you give it a try?
> > 
> 
> This is going to be very difficult. So far, calls such as
> typec_set_data_role() were independent (asynchronous) of role change
> requests through sysfs, meaning they happened whenever lower level
> confirmed that a role change was complete, for whatever reason. Now
> I have to check if a role change request through the class code
> is pending and not call typec_set_data_role() and friends in that case.

I'm sorry about the misunderstanding.

What you want is basically that we only support non-blocking mode with
this interface, and we can't do that.

Would you even be able to make it work? How would you report errors
for example?

> This becomes even more complicated in situations with parallel role
> change requests both from the class code and from the partner.
> In such cases the class code may have acquired the lock, and before it
> calls the driver, a role change complete is reported. The call to
> typec_set_XXX_role() will then get stuck. So I'll have to add another
> flag before calling typec_set_data_role() and friends, and then reject
> the call to dr_set and all other set requests with -EBUSY.
> race conditions.

Or we'll just export the port locks somehow, or provide separate API
for handling them.

..typec_get_port_lock(..
{
        return &port->lock;
}

or

typec_lock(..
typec_test_and_lock(..
typec_unlock(..
...

> Thinking more ... ultimately, this means that I'll have to reject role
> change requests from the class code whenever the state machine is busy,
> because I never know if the state machine activity would result in a
> call into the typec class code.

Yes, why is that a problem? The other option would be that you queue
the requests from users, and I'm pretty sure we want to avoid that. It
would mean you have to consider a lot of conditions to avoid any
races, and you would also have to make a lot of extra decisions. As an
example, what do you do if two role change requests are in the queue
for the same role? If the first requests is successful, you probable
can just report success to the second request also, _maybe_! But if
the first one fails, do you try again for the second request, and in
that case do you wait for the result of the second request before
reporting to the first, or do you just fail both of them? etc.

> At the same time, the protocol driver
> will have to reject incoming role change requests while a locally
> requested role change request is pending, even if that internal
> role change request has not yet resulted in a PD message being sent.

Why?

> Is this really necessary ? It creates a lot of interdependencies.
> 
> In summary, this means a substantial amount of code and testing, which
> is going to take a while.
> 
> You might want to mention all this in the API documentation.

OK.

> > I've also removed the supports_usb_power_delivery and id_header_vdo
> > attributes from partners and cables. We really have to put all the USB
> > PD specific attributes to its own group, and that group can't be in
> > control of this class. We will simply not always have access to the
> > kind of USB PD specific details you want to show, for example details
> > that you get from discover identity command, even when USB PD is fully
> > supported. For example on systems that use UCSI.
> > 
> 
> I think we should have a single unified ABI, independent of the underlying
> driver, that informs the user about the partner device. I really don't
> quite understand why the class code should not be able to report what device
> it is connected to (while at the same time being able to report the alternate
> modes it supports).

OK, let's plan this more then. Maybe we can make for example a layer
that creates the groups for the PD specific details to the class?

The problem is still that we can only provide results from for example
request identity command reliably when the PD protocol layer is
completely inside kernel, and that is not always the case. So we
really need to group those details in its own group, and that group
will basically indicate is the PD stack inside the kernel or not.

We should not forget also that the userspace can never rely on those
details because of the fact that they simply will not always be
available.


Thanks,

-- 
heikki

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

#1436062 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On 07/03/2016 12:38 PM, Heikki Krogerus wrote:
> On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
>> On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
>>> I've updated my github branch with a commit where both of these issues
>>> should be fixed. Can you give it a try?
>>>
>>
>> This is going to be very difficult. So far, calls such as
>> typec_set_data_role() were independent (asynchronous) of role change
>> requests through sysfs, meaning they happened whenever lower level
>> confirmed that a role change was complete, for whatever reason. Now
>> I have to check if a role change request through the class code
>> is pending and not call typec_set_data_role() and friends in that case.
>
> I'm sorry about the misunderstanding.
>
> What you want is basically that we only support non-blocking mode with
> this interface, and we can't do that.
>

No, that is not what I said or asked for. The problems I am seeing are due
to locking in the class code. "Asynchronous" above referred to the state
machine vs. role change requests by the class code, which operates independent
of each other in my code. Set requests from the class code still wait for
the state machine to complete.

The problem is that the state machine now needs to know if the class code
has a set role request pending, because in that case it can no longer report
role changes directly to the class code. This includes role changes unrelated
to the actual set role request. That code is now much more complicated, especially
since each callback into the typec code is handled differently. For example,
typec_disconnect() does not require a lock (unless I missed it), but many
other functions do.

> Would you even be able to make it work? How would you report errors
> for example?
>

It worked just great when you had no locks. All I had to do is handle
role changes in the driver, and to implement the necessary locks there.
Actually, before you introduced the locks, you had suggested that this
was the way to go, which is why I implemented it that way.

>> This becomes even more complicated in situations with parallel role
>> change requests both from the class code and from the partner.
>> In such cases the class code may have acquired the lock, and before it
>> calls the driver, a role change complete is reported. The call to
>> typec_set_XXX_role() will then get stuck. So I'll have to add another
>> flag before calling typec_set_data_role() and friends, and then reject
>> the call to dr_set and all other set requests with -EBUSY.
>> race conditions.
>
> Or we'll just export the port locks somehow, or provide separate API
> for handling them.
>
> ..typec_get_port_lock(..
> {
>          return &port->lock;
> }
>
> or
>
> typec_lock(..
> typec_test_and_lock(..
> typec_unlock(..
> ...
>
>> Thinking more ... ultimately, this means that I'll have to reject role
>> change requests from the class code whenever the state machine is busy,
>> because I never know if the state machine activity would result in a
>> call into the typec class code.
>
> Yes, why is that a problem? The other option would be that you queue
> the requests from users, and I'm pretty sure we want to avoid that. It
> would mean you have to consider a lot of conditions to avoid any
> races, and you would also have to make a lot of extra decisions. As an
> example, what do you do if two role change requests are in the queue
> for the same role? If the first requests is successful, you probable
> can just report success to the second request also, _maybe_! But if
> the first one fails, do you try again for the second request, and in
> that case do you wait for the result of the second request before
> reporting to the first, or do you just fail both of them? etc.
>

No, that is not correct. Again, all I had to do was to implement a lock
in the driver which waited until the state machine was idle, and _then_
check if the set command needed to do anything. No queuing necessary,
no race conditions.

Keep in mind there are no role _change_ requests, the requests are to set
a specific role - and I can do that once I have the state machine lock.
The second request would again be a request to set a specific role. It waits
for the state machine lock, and after it is acquired decides if a role
change is necessary or not.

Sure, I can switch to your method, and I guess I'll have to.
I just think that the resulting -EBUSY responses are unnecessary.

I will reiterate, though, that this all worked just fine with the previous
version of your code, where sysfs role change requests did not require
or implement a lock in the class code.

>> At the same time, the protocol driver
>> will have to reject incoming role change requests while a locally
>> requested role change request is pending, even if that internal
>> role change request has not yet resulted in a PD message being sent.
>
> Why?
>
Example:

Class code requests data role change, remote partner requests power role
change. Both are handled in parallel by the state machine. State machine
tries to report power role change to class code and gets stuck because the
class code holds a lock due to the pending data role change.

Therefore, a role change request from the partner now has to be rejected
while a (different) role change request from the class code is pending.

>> Is this really necessary ? It creates a lot of interdependencies.
>>
>> In summary, this means a substantial amount of code and testing, which
>> is going to take a while.
>>
>> You might want to mention all this in the API documentation.
>
> OK.
>
>>> I've also removed the supports_usb_power_delivery and id_header_vdo
>>> attributes from partners and cables. We really have to put all the USB
>>> PD specific attributes to its own group, and that group can't be in
>>> control of this class. We will simply not always have access to the
>>> kind of USB PD specific details you want to show, for example details
>>> that you get from discover identity command, even when USB PD is fully
>>> supported. For example on systems that use UCSI.
>>>
>>
>> I think we should have a single unified ABI, independent of the underlying
>> driver, that informs the user about the partner device. I really don't
>> quite understand why the class code should not be able to report what device
>> it is connected to (while at the same time being able to report the alternate
>> modes it supports).
>
> OK, let's plan this more then. Maybe we can make for example a layer
> that creates the groups for the PD specific details to the class?
>
> The problem is still that we can only provide results from for example
> request identity command reliably when the PD protocol layer is
> completely inside kernel, and that is not always the case. So we
> really need to group those details in its own group, and that group
> will basically indicate is the PD stack inside the kernel or not.
>
> We should not forget also that the userspace can never rely on those
> details because of the fact that they simply will not always be
> available.
>
On the other side, not being able to rely on a well defined ABI makes the
ABI much less useful.

Guenter

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

#1436417 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

Hi Guenter,

On Sun, Jul 03, 2016 at 02:28:44PM -0700, Guenter Roeck wrote:
> On 07/03/2016 12:38 PM, Heikki Krogerus wrote:
> > On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
> > > On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> > > > I've updated my github branch with a commit where both of these issues
> > > > should be fixed. Can you give it a try?
> > > > 
> > > 
> > > This is going to be very difficult. So far, calls such as
> > > typec_set_data_role() were independent (asynchronous) of role change
> > > requests through sysfs, meaning they happened whenever lower level
> > > confirmed that a role change was complete, for whatever reason. Now
> > > I have to check if a role change request through the class code
> > > is pending and not call typec_set_data_role() and friends in that case.
> > 
> > I'm sorry about the misunderstanding.
> > 
> > What you want is basically that we only support non-blocking mode with
> > this interface, and we can't do that.
> > 
> 
> No, that is not what I said or asked for. The problems I am seeing are due
> to locking in the class code. "Asynchronous" above referred to the state
> machine vs. role change requests by the class code, which operates independent
> of each other in my code. Set requests from the class code still wait for
> the state machine to complete.
> 
> The problem is that the state machine now needs to know if the class code
> has a set role request pending, because in that case it can no longer report
> role changes directly to the class code. This includes role changes unrelated
> to the actual set role request. That code is now much more complicated, especially
> since each callback into the typec code is handled differently. For example,
> typec_disconnect() does not require a lock (unless I missed it), but many
> other functions do.

It seems that I have misunderstood your whole point. I'm really sorry.
I was in a hurry to start my vacation.

So let's just fix the locking.

<snip>

> > > > I've also removed the supports_usb_power_delivery and id_header_vdo
> > > > attributes from partners and cables. We really have to put all the USB
> > > > PD specific attributes to its own group, and that group can't be in
> > > > control of this class. We will simply not always have access to the
> > > > kind of USB PD specific details you want to show, for example details
> > > > that you get from discover identity command, even when USB PD is fully
> > > > supported. For example on systems that use UCSI.
> > > > 
> > > 
> > > I think we should have a single unified ABI, independent of the underlying
> > > driver, that informs the user about the partner device. I really don't
> > > quite understand why the class code should not be able to report what device
> > > it is connected to (while at the same time being able to report the alternate
> > > modes it supports).
> > 
> > OK, let's plan this more then. Maybe we can make for example a layer
> > that creates the groups for the PD specific details to the class?
> > 
> > The problem is still that we can only provide results from for example
> > request identity command reliably when the PD protocol layer is
> > completely inside kernel, and that is not always the case. So we
> > really need to group those details in its own group, and that group
> > will basically indicate is the PD stack inside the kernel or not.
> > 
> > We should not forget also that the userspace can never rely on those
> > details because of the fact that they simply will not always be
> > available.
> > 
> On the other side, not being able to rely on a well defined ABI makes the
> ABI much less useful.

What do we do about this?


Thanks,

-- 
heikki

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

#1436456 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
[ ... ]

>>> We should not forget also that the userspace can never rely on those
>>> details because of the fact that they simply will not always be
>>> available.
>>>
>> On the other side, not being able to rely on a well defined ABI makes the
>> ABI much less useful.
>
> What do we do about this?
>
I'll have to think about it. Unfortunately, I'll have to put this on the
back-burner for the time being. My primary problem is that I need
a functional  driver _now_ (ie by the end of this week), to meet an
upcoming deadline. Due to to the locking problem, I can not rely on the
typec infrastructure ... meaning I have to take two steps back and get
my code working with the Android infrastructure (which, coincidentally,
does not require locking and thus doesn't have all the resulting
limitations and complexities).

Guenter

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

#1437229 — Re: [PATCHv4 1/2] usb: USB Type-C connector class

On Mon, Jul 04, 2016 at 10:45:02AM -0700, Guenter Roeck wrote:
> On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
> [ ... ]
> 
> > > > We should not forget also that the userspace can never rely on those
> > > > details because of the fact that they simply will not always be
> > > > available.
> > > > 
> > > On the other side, not being able to rely on a well defined ABI makes the
> > > ABI much less useful.
> > 
> > What do we do about this?
> > 
> I'll have to think about it. Unfortunately, I'll have to put this on the
> back-burner for the time being. My primary problem is that I need
> a functional  driver _now_ (ie by the end of this week), to meet an
> upcoming deadline. Due to to the locking problem, I can not rely on the
> typec infrastructure ... meaning I have to take two steps back and get
> my code working with the Android infrastructure (which, coincidentally,
> does not require locking and thus doesn't have all the resulting
> limitations and complexities).

That is unfortunate.

I added a commit where I remove the lock completely (at least for
now) to my github.


Cheers,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web