All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
@ 2016-07-15  2:14 Bin Gao
  2016-07-15  6:31 ` Oliver Neukum
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Bin Gao @ 2016-07-15  2:14 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel
  Cc: Bin Gao, Chandra Sekhar Anagani

This patch implements a simple USB Power Delivery sink port state machine.
It assumes the hardware only handles PD packet transmitting and receiving
over the CC line of the USB Type-C connector. The state transition is
completely controlled by software. This patch only implement the sink port
function and it doesn't support source port and port swap yet.

This patch depends on these two patches:
https://lkml.org/lkml/2016/6/29/349
https://lkml.org/lkml/2016/6/29/350

Signed-off-by: Bin Gao <bin.gao@intel.com>
---
 drivers/usb/typec/Kconfig      |  13 +
 drivers/usb/typec/Makefile     |   1 +
 drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/pd_message.h | 371 ++++++++++++++++
 include/linux/usb/pd_sink.h    | 286 ++++++++++++
 5 files changed, 1638 insertions(+)
 create mode 100644 drivers/usb/typec/pd_sink.c
 create mode 100644 include/linux/usb/pd_message.h
 create mode 100644 include/linux/usb/pd_sink.h

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 7a345a4..a04a900 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
 config TYPEC
 	tristate
 
+config USB_PD_SINK
+	bool "USB Power Delivery Sink Port State Machine Driver"
+	select TYPEC
+	help
+	  Enable this to support USB PD(Power Delivery) Sink port.
+	  This driver implements a simple USB PD sink state machine.
+	  The underlying TypeC phy driver is responsible for cable
+	  plug/unplug event, port orientation detection, transmitting
+	  and receiving PD messages. This driver only process messages
+	  received by the TypeC phy driver and maintain the sink port's
+	  state machine.
+
 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
+	select USB_PD_SINK
 	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
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index b9cb862..a62eb57 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
 obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
+obj-$(CONFIG_USB_PD_SINK)	+= pd_sink.o
diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
new file mode 100644
index 0000000..374bdef
--- /dev/null
+++ b/drivers/usb/typec/pd_sink.c
@@ -0,0 +1,967 @@
+/*
+ * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
+ *
+ * This driver implements a simple USB PD sink port state machine.
+ * It assumes the upper layer, i.e. the user of this driver, handles
+ * the PD message receiving and transmitting. The upper layer receives
+ * PD messages from the Source, queues them to us, and when processing
+ * the received message we'll call upper layer's transmitting function
+ * to send PD messages to the source.
+ * The sink port state machine is maintained in this driver but we also
+ * broadcast some important PD messages to upper layer as events.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Bin Gao <bin.gao@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/usb/pd_sink.h>
+
+#define MAKE_HEADER(port, header, msg, objs) \
+do { \
+	header->type = msg; \
+	header->data_role = PD_DATA_ROLE_UFP; \
+	header->revision = port->pd_rev; \
+	header->power_role = PD_POWER_ROLE_SINK; \
+	header->id = roll_msg_id(port); \
+	header->nr_objs = objs; \
+	header->extended = PD_MSG_NOT_EXTENDED; \
+} while (0)
+
+static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
+static int nr_ports;
+
+BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
+
+static char *state_strings[] = {
+	"WAIT_FOR_SOURCE_CAPABILITY",
+	"REQUEST_SENT",
+	"ACCEPT_RECEIVED",
+	"POWER_SUPPLY_READY",
+};
+
+/* Control messages */
+static char *cmsg_strings[] = {
+	"GOODCRC",			/* 1 */
+	"GOTOMIN",			/* 2 */
+	"ACCEPT",			/* 3 */
+	"REJECT",			/* 4 */
+	"PING",				/* 5 */
+	"PS_RDY",			/* 6 */
+	"GET_SRC_CAP",			/* 7 */
+	"GET_SINK_CAP",			/* 8 */
+	"DR_SWAP",			/* 9 */
+	"PR_SWAP",			/* 10 */
+	"VCONN_SWAP",			/* 11 */
+	"WAIT",				/* 12 */
+	"SOFT_RESET",			/* 13 */
+	"RESERVED",			/* 14 */
+	"RESERVED",			/* 15 */
+	"NOT_SUPPORTED",		/* 16 */
+	"GET_SOURCE_CAP_EXTENDED",	/* 17 */
+	"GET_STATUS",			/* 18 */
+	"FR_SWAP",			/* 19 */
+	/* RESERVED			20 - 31 */
+};
+
+/* Data messages */
+static char *dmsg_strings[] = {
+	"SOURCE_CAP",		/* 1 */
+	"REQUEST",		/* 2 */
+	"BIST",			/* 3 */
+	"SINK_CAP",		/* 4 */
+	"BATTERY_STATUS",	/* 5 */
+	"ALERT",		/* 6 */
+	"RESERVED",		/* 7 */
+	"RESERVED",		/* 8 */
+	"RESERVED",		/* 9 */
+	"RESERVED",		/* 10 */
+	"RESERVED",		/* 11 */
+	"RESERVED",		/* 12 */
+	"RESERVED",		/* 13 */
+	"RESERVED",		/* 14 */
+	"VENDOR_DEFINED",	/* 15 */
+	/* RESERVED		16 - 31 */
+};
+
+static char *state_to_string(enum pd_sink_state state)
+{
+	if (state < ARRAY_SIZE(state_strings))
+		return state_strings[state];
+	else
+		return NULL;
+}
+
+static char *msg_to_string(bool is_cmsg, u8 msg)
+{
+	int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) : ARRAY_SIZE(dmsg_strings);
+
+	if (msg <= nr)
+		return is_cmsg ? cmsg_strings[msg - 1] : dmsg_strings[msg - 1];
+	else
+		return "RESERVED";
+}
+
+static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
+{
+	pr_info("sink port %d: %s message %s %s\n", port,
+				is_cmsg ? "Control" : "Data",
+				msg_to_string(is_cmsg, msg),
+		 recv ? "received" : "sent(wait GOODCRC)");
+}
+
+static inline bool fixed_ps_equal(struct sink_ps *p1,
+				struct sink_ps *p2)
+{
+	return p1->ps_type == p2->ps_type &&
+		p1->ps_fixed.voltage_fixed == p2->ps_fixed.voltage_fixed &&
+		p1->ps_fixed.current_default == p2->ps_fixed.current_default;
+}
+
+/* The message ID increments each time we send out a new message */
+static u8 roll_msg_id(struct pd_sink_port *port)
+{
+	u8 msg_id = port->msg_id;
+
+	if (msg_id == PD_MSG_ID_MAX)
+		msg_id = PD_MSG_ID_MIN;
+	else
+		msg_id++;
+
+	port->msg_id = msg_id;
+	return msg_id;
+}
+
+static inline bool is_waiting_goodcrc(struct pd_sink_port *port, int msg_id)
+{
+	return (port->waiting_goodcrc) && (msg_id == port->last_sent_msg);
+}
+
+static inline void clear_waiting_goodcrc(struct pd_sink_port *port)
+{
+	port->waiting_goodcrc = 0;
+}
+
+static void start_timer(struct pd_sink_port *port, int timeout,
+		enum hrtimer_restart (*f)(struct hrtimer *))
+{
+	if (hrtimer_active(&port->tx_timer)) {
+		pr_err("Error: previous timer is still active\n");
+		return;
+	}
+
+	port->tx_timer.function = f;
+	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
+	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,
+		(timeout % 1000) * 1000000), HRTIMER_MODE_REL);
+}
+
+static inline void stop_timer(struct pd_sink_port *port)
+{
+	hrtimer_cancel(&port->tx_timer);
+}
+
+static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
+{
+	pr_err("GOODCRC message is not received in %d ms: timeout\n",
+						PD_TIMEOUT_GOODCRC);
+	return HRTIMER_NORESTART;
+}
+
+/*
+ * For any message we send, we must get a GOODCRC message from the Source.
+ * The USB PD spec says the time should be measured between the last bit
+ * of the sending message's EOP has been transmitted and the last bit of
+ * the receiving GOODCRC message's EOP has been received. The allowed time
+ * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
+ * performed in physical layer. When it reaches to the OS and this driver,
+ * the actual time is difficult to predict because of the scheduling,
+ * context switch, interrupt preemption and nesting, etc. So we only define
+ * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
+ * account of all software related latency.
+ */
+static int send_message(struct pd_sink_port *port, void *buf, int len,
+			u8 msg, bool ctrl_msg, enum sop_type sop_type)
+{
+	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
+		port->tx(port->port, port->tx_priv, buf, len, sop_type);
+		print_message(port->port, true, msg, false);
+		return 0;
+	}
+
+	port->tx(port->port, port->tx_priv, buf, len, sop_type);
+	print_message(port->port, ctrl_msg, msg, false);
+
+	if (!port->hw_goodcrc_rx) {
+		port->waiting_goodcrc = true;
+		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);
+	}
+
+	port->last_sent_msg = msg;
+	port->last_msg_ctrl = ctrl_msg;
+
+	return 0;
+}
+
+static void ack_message(struct pd_sink_port *port, int msg_id)
+{
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
+
+	if (!header) {
+		MAKE_HEADER(port, header, PD_CMSG_GOODCRC, 0);
+		send_message(port, header, PD_MSG_HEADER_LEN,
+				PD_CMSG_GOODCRC, true, SOP);
+}	}
+
+static int handle_goodcrc(struct pd_sink_port *port, u8 msg_id)
+{
+	if (is_waiting_goodcrc(port, msg_id)) {
+		hrtimer_cancel(&port->tx_timer);
+		clear_waiting_goodcrc(port);
+		switch (msg_id) {
+		case PD_DMSG_REQUEST:
+			port->state = PD_SINK_STATE_REQUEST_SENT;
+			break;
+		case PD_DMSG_SINK_CAP:
+			pr_info("Got GOODCRC for SINK_CAPABILITY message\n");
+		default:
+			break;
+		}
+	} else
+		pr_warn("Unexpected GOODCRC message received.\n");
+
+	return 0;
+}
+
+static void handle_accept(struct pd_sink_port *port)
+{
+	enum pd_sink_state state = port->state;
+
+	if (state != PD_SINK_STATE_REQUEST_SENT) {
+		pr_err("ACCEPT received but not expected, sink state: %s\n",
+						state_to_string(state));
+		return;
+	}
+
+	port->state = PD_SINK_STATE_ACCEPT_RECEIVED;
+}
+
+/*
+ * The Source may send REJECT message as a response to REQUEST, PR_SWAP,
+ * DR_SWAP or VCONN_SWAP message. Since we only send REQUEST to the Source
+ * so we're sure the REJECT is for our REQUEST message.
+ */
+static void handle_reject(struct pd_sink_port *port)
+{
+	enum pd_sink_state state = port->state;
+
+	if (state == PD_SINK_STATE_REQUEST_SENT)
+		/* Broadcast to upper layer */
+		blocking_notifier_call_chain(&pd_sink_notifier_list,
+				PD_SINK_EVENT_REJECT | port->port << 8, NULL);
+	else
+		pr_err("REJECT received but not expected, sink state: %s\n",
+						state_to_string(state));
+}
+
+static void handle_not_supported(struct pd_sink_port *port)
+{
+	pr_err("sink port %d: %s message %s is not supported by Source\n",
+			port->port, port->last_msg_ctrl ? "Control" : "Data",
+				msg_to_string(port->last_msg_ctrl,
+			port->last_sent_msg & PD_MSG_TYPE_MASK));
+}
+
+/*
+ * The Wait Message is a valid response to a Request, a PR_Swap, DR_Swap or
+ * VCONN_Swap Message. We don't support PR_Swap, DR_Swap and VCONN_Swap
+ * messages so we only handle Wait message from Source responding to our
+ * Request message.
+ */
+static void handle_wait(struct pd_sink_port *port)
+{
+	pr_info("WAIT message received\n");
+}
+
+/*
+ * We repond the GET_SINK_CAPABILITY message by sending the SINK_CAPABILITY
+ * message. The PDOs in the SINK_CAPABILITY message shall be in the following
+ * order:
+ * 1. The vSafe5V Fixed Supply Object shall always be the first object.
+ * 2. The remaining Fixed Supply Objects, if present, shall be sent in voltage
+ *    order; lowest to highest.
+ * 3. The Battery Supply Objects, if present shall be sent in Minimum Voltage
+ *    order; lowest to highest.
+ * 4. The Variable Supply (non-Battery) Objects, if present, shall be sent in
+ *    Minimum Voltage order; lowest to highest.
+ *
+ * The upper layer calling pd_sink_register_port() must ensure the profiles
+ * provided come with the above order as we don't re-order the profiles here
+ * when we compose the SINK_CAPABILITY message.
+ */
+static void handle_get_sink_cap(struct pd_sink_port *port)
+{
+	u8 *pdo;
+	int i, nr_objs = 0;
+	struct sink_ps *ps;
+	struct pd_pdo_sink_fixed *fixed;
+	struct pd_pdo_variable *variable;
+	struct pd_pdo_battery *battery;
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
+		port->nr_ps * PD_OBJ_SIZE, GFP_KERNEL);
+
+	if (!header) {
+		pr_crit("%s(): kzalloc() failed\n", __func__);
+		return;
+	}
+
+	MAKE_HEADER(port, header, PD_DMSG_SINK_CAP, port->nr_ps);
+	pdo = (u8 *)header + PD_MSG_HEADER_LEN;
+
+	for (i = 0; i < port->nr_ps; i++) {
+		ps = port->ps_reqs + i;
+		switch (ps->ps_type) {
+		case PS_TYPE_FIXED:
+			fixed = (struct pd_pdo_sink_fixed *)pdo;
+			fixed->current_max = ps->ps_fixed.current_max;
+			fixed->voltage = ps->ps_fixed.voltage_fixed;
+			fixed->fast_role_swap = FAST_ROLE_SWAP_DISABLE;
+			fixed->usb_capable = 1;
+			fixed->higher_capability =
+				(ps->ps_fixed.voltage_fixed >
+						PD_VSAFE5V) ? 1 : 0;
+			fixed->type = PS_TYPE_FIXED;
+			break;
+		case PS_TYPE_BATTERY:
+			battery = (struct pd_pdo_battery *)pdo;
+			battery->power = ps->ps_battery.power;
+			battery->voltage_max = ps->ps_battery.voltage_max;
+			battery->voltage_min = ps->ps_battery.voltage_min;
+			battery->type = PS_TYPE_BATTERY;
+			break;
+		case PS_TYPE_VARIABLE:
+			variable = (struct pd_pdo_variable *)pdo;
+			variable->current_mo =
+				ps->ps_variable.current_default;
+			variable->voltage_max = ps->ps_variable.voltage_max;
+			variable->voltage_min = ps->ps_variable.voltage_min;
+			variable->type = PS_TYPE_VARIABLE;
+			break;
+		default:
+			continue;
+		}
+
+		pdo += PD_OBJ_SIZE;
+		nr_objs++;
+	}
+
+	if (nr_objs == 0) {
+		kfree(header);
+		pr_err("no valid sink PDOs to send in SINK_CAPABILITY\n");
+		return;
+	}
+
+	header->nr_objs = nr_objs;
+	send_message(port, header, PD_MSG_HEADER_LEN + nr_objs * PD_OBJ_SIZE,
+						PD_DMSG_SINK_CAP, false, SOP);
+}
+
+static void handle_ps_ready(struct pd_sink_port *port)
+{
+	if (port->state != PD_SINK_STATE_ACCEPT_RECEIVED) {
+		pr_err("PS_READY received but sink state is not in %s\n",
+			state_to_string(PD_SINK_STATE_ACCEPT_RECEIVED));
+		return;
+	}
+
+	/* Broadcast to upper layer */
+	blocking_notifier_call_chain(&pd_sink_notifier_list,
+		PD_SINK_EVENT_PS_READY | port->port << 8, NULL);
+}
+
+static void handle_gotomin(struct pd_sink_port *port)
+{
+	/* Broadcast to upper layer */
+	blocking_notifier_call_chain(&pd_sink_notifier_list,
+		PD_SINK_EVENT_GOTOMIN | port->port << 8, NULL);
+}
+
+static void handle_soft_reset(struct pd_sink_port *port)
+{
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
+
+	if (!header)
+		return;
+
+	flush_workqueue(port->rx_wq);
+	stop_timer(port);
+	port->msg_id = PD_MSG_ID_MIN;
+	port->state = PD_SINK_STATE_WAIT_FOR_SRC_CAP;
+	MAKE_HEADER(port, header, PD_CMSG_ACCEPT, 0);
+	send_message(port, header, PD_MSG_HEADER_LEN,
+			PD_CMSG_ACCEPT, true, SOP);
+
+	/* Broadcast to upper layer */
+	blocking_notifier_call_chain(&pd_sink_notifier_list,
+		PD_SINK_EVENT_SOFT_RESET | port->port << 8, NULL);
+}
+
+static void ctl_msg_handler(struct pd_sink_port *port, u8 msg_type)
+{
+	print_message(port->port, true, msg_type, true);
+
+	switch (msg_type) {
+	case PD_CMSG_ACCEPT:
+		handle_accept(port);
+		break;
+	case PD_CMSG_GOTOMIN:
+		handle_gotomin(port);
+		break;
+	case PD_CMSG_REJECT:
+		handle_reject(port);
+		break;
+	case PD_CMSG_GET_SINK_CAP:
+		handle_get_sink_cap(port);
+		break;
+	case PD_CMSG_PS_RDY:
+		handle_ps_ready(port);
+		break;
+	case PD_CMSG_SOFT_RESET:
+		handle_soft_reset(port);
+		break;
+	case PD_CMSG_NOT_SUPPORTED:
+		handle_not_supported(port);
+		break;
+	case PD_CMSG_WAIT:
+		/*
+		 * Sent by Source to Sink to indicate it can't meet the
+		 * requirement for the Request. The Sink is allowed to
+		 * repeat the Request Message using the SinkRequestTimer
+		 * to ensure that there is tSinkRequest between requests.
+		 */
+		handle_wait(port);
+		break;
+	/* Below messages are simply ignored */
+	case PD_CMSG_GET_SOURCE_CAP_EXTENDED:
+	case PD_CMSG_GET_STATUS:
+	case PD_CMSG_PING:
+	case PD_CMSG_GET_SRC_CAP:
+	case PD_CMSG_DR_SWAP:
+	case PD_CMSG_PR_SWAP:
+	case PD_CMSG_VCONN_SWAP:
+	case PD_CMSG_FR_SWAP:
+		break;
+
+	}
+}
+
+/**
+ * send_request() - send REQUEST message to Source.
+ * @port: the sink port number
+ * Return: none
+ */
+static int send_request(struct pd_sink_port *port)
+{
+	int i;
+	struct pd_request *request;
+	struct pd_pdo_src_fixed *src;
+	struct sink_ps *ps =
+		&port->ps_reqs[port->active_ps];
+	struct pd_source_cap *cap;
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
+					PD_OBJ_SIZE, GFP_KERNEL);
+	bool voltage_matched = false, current_matched = false;
+
+	/* Only support fixed PDO for now */
+	if (ps->ps_type != PS_TYPE_FIXED) {
+		pr_err("%s(): We only support fixed PDO now\n", __func__);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < port->nr_source_caps; i++) {
+		cap = &port->source_caps[i];
+		if (cap->ps_type != PS_TYPE_FIXED)
+			continue;
+		src = &cap->fixed;
+		if (src->voltage != ps->ps_fixed.voltage_fixed)
+			continue;
+		voltage_matched = true;
+		/* We matched the voltage, now match the current */
+		if (src->current_max >= ps->ps_fixed.current_max) {
+			current_matched = true;
+			break;
+		}
+	}
+
+	if (!voltage_matched) {
+		pr_err("Can't match any PDO from source caps\n");
+		return -EINVAL;
+	}
+
+	MAKE_HEADER(port, header, PD_DMSG_REQUEST, 1);
+	request = (struct pd_request *)((u8 *)header + PD_MSG_HEADER_LEN);
+	request->pos = i + 1; /* PD protocol RDO index starts from 1 */
+	request->give_back = port->support_gotomin ? 1 : 0;
+	request->usb_comm_capable = port->usb_comm_capable ? 1 : 0;
+	if (port->pd_rev == PD_REVISION_3)
+		request->unchunked_ext_msg_supported = 1;
+
+	/* If current_matched is false, we have to set the mismatch flag */
+	if (!current_matched)
+		request->cap_mismatch = 1;
+
+	request->operating_default = ps->ps_fixed.current_default;
+	request->operating_min_max = ps->ps_fixed.current_max;
+
+	return send_message(port, header, PD_MSG_HEADER_LEN + PD_OBJ_SIZE,
+						PD_DMSG_REQUEST, false, SOP);
+}
+
+static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
+						u8 nr_objs, u8 *buf)
+{
+	int i;
+	u32 *obj;
+	u8 type;
+	struct pd_source_cap *cap = port->source_caps;
+
+	/*
+	 * The PD spec revision included in SOURCE_CAPABILITY message is the
+	 * highest revision that the Source supports.
+	 */
+	port->pd_rev = msg_revision;
+
+	/*
+	 * First we need to save all PDOs - they may be used in the future.
+	 * USB PD spec says we must use PDOs in the most recent
+	 * SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
+	 */
+	port->nr_source_caps = 0;
+	for (i = 0; i < nr_objs; i++) {
+		obj = (u32 *)(buf + i * PD_OBJ_SIZE);
+		type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
+		switch (type) {
+		case PS_TYPE_FIXED:
+			cap->ps_type = PS_TYPE_FIXED;
+			cap->fixed = *(struct pd_pdo_src_fixed *)obj;
+			break;
+		case PS_TYPE_VARIABLE:
+			cap->ps_type = PS_TYPE_VARIABLE;
+			cap->variable = *(struct pd_pdo_variable *)obj;
+			break;
+		case PS_TYPE_BATTERY:
+			cap->ps_type = PS_TYPE_BATTERY;
+			cap->battery = *(struct pd_pdo_battery *)obj;
+			break;
+		default: /* shouldn't come here */
+			pr_err("Invalid Source Capability type: %u.\n", type);
+			continue;
+		}
+		port->nr_source_caps++;
+		cap++;
+	}
+
+	if (port->nr_source_caps == 0) {
+		pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
+		return;
+	}
+
+	/* If a contract is not established, we need send a REQUEST message */
+	if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {
+		if (!send_request(port))
+			port->state = PD_SINK_STATE_REQUEST_SENT;
+	}
+}
+
+static void handle_alert(struct pd_sink_port *port, int nr_objs, u8 *buf)
+{
+	u8 type;
+	struct alert_msg *alert;
+
+	if (nr_objs != 1) {
+		pr_err("Invalid ALERT message\n");
+		return;
+	}
+
+	alert = (struct alert_msg *)buf;
+	type = alert->type;
+
+	pr_info("ALERT received on port %d:\n"
+		"Battery Status Changes: %u\nOCP: %u\nOTP: %u\nOCC: %u\n"
+		"Source Input Change: %u\nOVP: %u\n", port->port,
+		type & PD_ALERT_BATTERY ? 1 : 0,
+		type & PD_ALERT_OCP ? 1 : 0,
+		type & PD_ALERT_OTP ? 1 : 0,
+		type & PD_ALERT_OCC ? 1 : 0,
+		type & PD_ALERT_SRC_INPUT ? 1 : 0,
+		type & PD_ALERT_OVP ? 1 : 0);
+
+	if (type & PD_ALERT_BATTERY)
+		pr_info("Battery Status Change: Fixed Batteries 0x%x\n"
+			"Battery Status Change: Hot Swappable Batteries 0x%x\n",
+			alert->fixed_batteries, alert->hot_swappable_batteries);
+
+	/* Broadcast to upper layer */
+	blocking_notifier_call_chain(&pd_sink_notifier_list,
+		PD_SINK_EVENT_ALERT | port->port << 8, buf);
+}
+
+static void data_msg_handler(struct pd_sink_port *port, u8 msg_revision,
+					u8 msg_type, u8 nr_objs, u8 *buf)
+{
+	print_message(port->port, false, msg_type, true);
+
+	switch (msg_type) {
+	case PD_DMSG_SOURCE_CAP:
+		/* Save source's power supply capability */
+		handle_source_cap(port, msg_revision, nr_objs, buf);
+		break;
+	case PD_DMSG_REQUEST:
+	case PD_DMSG_SINK_CAP:
+		/* Sink shouldn't receive REQUEST, ignore */
+		pr_warn("Ignored REQUEST message.\n");
+		break;
+	case PD_DMSG_BIST:
+		pr_warn("Ignored BIST message.\n");
+		break;
+	case PD_DMSG_BATTERY_STATUS:
+		/*
+		 * The Battery_Status Message shall be sent in response
+		 * to a Get_Battery_Status Message. We(Sink) don't send
+		 * Get_Battery_Status message so we won't receive
+		 * Battery_Status message.
+		 */
+		pr_warn("Ignored BATTERY_STATUS message.\n");
+		break;
+	case PD_DMSG_ALERT:
+		handle_alert(port, nr_objs, buf);
+		break;
+	case PD_DMSG_VENDOR_DEFINED:
+		pr_warn("Ignored VENDOR_DEFINED message.\n");
+		break;
+	default:
+		pr_err("Unknown data message type: %u\n", msg_type);
+	}
+}
+
+static inline bool is_extended_msg(struct pd_msg_header *header)
+{
+	return header->extended == PD_MSG_EXTENDED;
+}
+
+static inline bool is_ctrl_msg(struct pd_msg_header *header)
+{
+	return header->nr_objs == 0;
+}
+
+static inline bool is_data_msg(struct pd_msg_header *header)
+{
+	return header->nr_objs != 0;
+}
+
+static inline void free_msg(struct pd_sink_msg *msg)
+{
+	kfree(msg);
+}
+
+static int msg_handler(struct pd_sink_msg *msg)
+{
+	struct pd_sink_port *port;
+	struct pd_msg_header *header;
+	int len, ret = 0;
+
+	len = msg->msg_len;
+	if (len < PD_MSG_HEADER_LEN) {
+		pr_err("Invalid message: len=%d(should be at least 2)\n", len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	port = sink_ports[msg->port];
+	header = (struct pd_msg_header *)msg->buf;
+
+	if (is_ctrl_msg(header) && (header->type == PD_CMSG_GOODCRC))
+		handle_goodcrc(port, header->id);
+
+	/*
+	 * If HW supports auto GOODCRC generation and transmitting then we
+	 * don't need to send GOODCRC by software. This is indicated by
+	 * the .hw_goodcrc_tx field of the pd_sink_port structure.
+	 */
+	if (is_data_msg(header) || (is_ctrl_msg(header) &&
+			(header->type != PD_CMSG_GOODCRC))) {
+		if (!port->hw_goodcrc_tx)
+			ack_message(port, header->id);
+	}
+
+	if (is_extended_msg(header)) {
+		pr_err("Extended message received, but no support for it\n");
+		goto out;
+	}
+
+	if (header->power_role != PD_POWER_ROLE_SOURCE) {
+		pr_err("Message is not from a Source, ignored\n");
+		goto out;
+	}
+
+	if (header->nr_objs == 0)
+		ctl_msg_handler(port, header->type);
+	else {
+		if (PD_MSG_HEADER_LEN + header->nr_objs * PD_OBJ_SIZE != len) {
+			pr_err("Invalid message: unexpected length.\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		data_msg_handler(port, header->revision, header->type,
+			header->nr_objs, msg->buf + PD_MSG_HEADER_LEN);
+	}
+
+out:
+	free_msg(msg);
+	return ret;
+}
+
+static void rx_msg_worker(struct work_struct *work)
+{
+	unsigned long flags;
+	struct pd_sink_msg *msg, *n;
+	struct pd_sink_port *port = container_of(work,
+			struct pd_sink_port, rx_work);
+
+	/*
+	 * We process all rx messages in the list.
+	 * Lock is held only when operating the list.
+	 */
+	spin_lock_irqsave(&port->rx_lock, flags);
+	list_for_each_entry_safe(msg, n, &port->rx_list, list) {
+		list_del(&msg->list);
+		spin_unlock_irqrestore(&port->rx_lock, flags);
+		msg_handler(msg);
+		spin_lock_irqsave(&port->rx_lock, flags);
+	}
+	spin_unlock_irqrestore(&port->rx_lock, flags);
+}
+
+int pd_sink_register_port(struct pd_sink_profile *profile,
+	int (*tx)(int port, void *tx_priv, void *buf, int len,
+			enum sop_type sop_type), void *tx_priv)
+{
+	int i, nr_ps;
+	struct pd_sink_port *port;
+
+	if (nr_ports >= MAX_NR_SINK_PORTS) {
+		pr_err("Can't register sink port: maximum number of ports reached\n");
+		return -EINVAL;
+	}
+
+	if (!profile) {
+		pr_err("%s(): profiles pointer can't be NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * USB PD spec says in SINK_CAPABILITY message(as a response to
+	 * GET_SINK_CAPABILITY message) the Sink shall include one Power
+	 * Data Object that reports vSafe5V even if the sink requires
+	 * additional power to operate fully. So we'll always have the
+	 * vSafe5V PDO as ps_reqs[0] unless the Sink provides the exact
+	 * same PDO with vSafe5V.
+	 */
+	nr_ps = profile->nr_ps;
+	if (nr_ps <= 0) {
+		pr_err("At least 1 Sink power supply requirement required\n");
+		return -EINVAL;
+	}
+
+	if (nr_ps >= MAX_NR_SINK_PS_REQS) {
+		pr_warn("Shrink Sink power supply requirements size %d to %d\n",
+						nr_ps, MAX_NR_SINK_PS_REQS);
+		nr_ps = MAX_NR_SINK_PS_REQS;
+	}
+
+	if (profile->active_ps <= 0 || profile->active_ps >=
+					 MAX_NR_SINK_PS_REQS) {
+		pr_err("Invalid active_ps in the profile\n");
+		return -EINVAL;
+	}
+
+	port = kmalloc(sizeof(struct pd_sink_port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->nr_ps = nr_ps;
+	port->hw_goodcrc_tx = profile->hw_goodcrc_tx;
+	port->hw_goodcrc_rx = profile->hw_goodcrc_rx;
+	port->support_gotomin = profile->gotomin;
+	port->usb_comm_capable = profile->usb_comm;
+	port->pd_rev = profile->pd_rev;
+	port->usb_spec = profile->spec;
+	port->last_sent_msg = 0;
+	port->waiting_goodcrc = false;
+	port->tx = tx;
+	port->tx_priv = tx_priv;
+	port->active_ps = profile->active_ps;
+	port->state = PD_SINK_STATE_WAIT_FOR_SRC_CAP;
+
+	for (i = 0; i < nr_ps; i++)
+		port->ps_reqs[i] = *(profile->ps + i);
+
+	port->msg_id = PD_MSG_ID_MIN;
+	hrtimer_init(&port->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	INIT_LIST_HEAD(&port->rx_list);
+	INIT_WORK(&port->rx_work, rx_msg_worker);
+	spin_lock_init(&port->rx_lock);
+
+	port->rx_wq = alloc_workqueue("sink_wq: port %d",
+				WQ_HIGHPRI, 0, nr_ports);
+	if (!port->rx_wq) {
+		pr_err(" Failed to allocate workqueue for sink port\n");
+		kfree(port);
+		return -EINVAL;
+	}
+
+	port->port = nr_ports;
+	sink_ports[nr_ports] = port;
+	nr_ports++;
+
+	return port->port;
+}
+EXPORT_SYMBOL_GPL(pd_sink_register_port);
+
+int pd_sink_unregister_port(int port)
+{
+	struct pd_sink_port *pport;
+
+	if (port < 0 || port >= MAX_NR_SINK_PORTS) {
+		pr_err("Invalid port number\n");
+		return -EINVAL;
+	}
+
+	pport = sink_ports[port];
+	flush_workqueue(pport->rx_wq);
+	destroy_workqueue(pport->rx_wq);
+	stop_timer(pport);
+	kfree(pport);
+	sink_ports[port] = NULL;
+	nr_ports--;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pd_sink_unregister_port);
+
+struct pd_sink_msg *pd_sink_alloc_msg(int port, int msg_len)
+{
+	struct pd_sink_msg *msg = kmalloc(sizeof(struct pd_sink_msg) + msg_len,
+								GFP_KERNEL);
+	if (!msg)
+		return NULL;
+
+	msg->port = port;
+	msg->msg_len = msg_len;
+	msg->buf = (u8 *)msg + sizeof(struct pd_sink_msg);
+
+	return msg;
+}
+EXPORT_SYMBOL_GPL(pd_sink_alloc_msg);
+
+/*
+ * pd_queue_msg() may be called in an interrupt handler, so we use
+ * spin lock to protect the message list.
+ */
+int pd_sink_queue_msg(struct pd_sink_msg *msg)
+{
+	unsigned long flags;
+	struct pd_sink_port *port;
+
+	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
+		pr_err("Invalid port number\n");
+		return -EINVAL;
+	}
+
+	port = sink_ports[msg->port];
+
+	spin_lock_irqsave(&port->rx_lock, flags);
+	list_add_tail(&msg->list, &port->rx_list);
+	spin_unlock_irqrestore(&port->rx_lock, flags);
+
+	queue_work(port->rx_wq, &port->rx_work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pd_sink_queue_msg);
+
+/*
+ * Upper layer requests to change to a different power supply from Source.
+ * We send a new REQUEST message to the Source and the upper layer can start
+ * using the new power supply once PD_SINK_EVENT_PS_READY event is received.
+ */
+int pd_sink_change_ps(int port, int ps)
+{
+	struct pd_sink_port *pport;
+
+	if (ps < 0 || ps >= MAX_NR_SINK_PS_REQS) {
+		pr_err("Invalid power supply index\n");
+		return -EINVAL;
+	}
+
+	pport =	sink_ports[port];
+	pport->active_ps = ps;
+
+	if (!send_request(pport))
+		pport->state = PD_SINK_STATE_REQUEST_SENT;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pd_sink_change_ps);
+
+/* Reset the port to initial state(PD_SINK_STATE_WAIT_FOR_SRC_CAP) */
+int pd_sink_reset_state(int port)
+{
+	struct pd_sink_port *pport;
+
+	if (port < 0 || port >= MAX_NR_SINK_PORTS) {
+		pr_err("Invalid port number");
+		return -EINVAL;
+	}
+
+	pport = sink_ports[port];
+	flush_workqueue(pport->rx_wq);
+
+	if (!pport->hw_goodcrc_rx) {
+		stop_timer(pport);
+		clear_waiting_goodcrc(pport);
+	}
+
+	pport->state = PD_SINK_STATE_WAIT_FOR_SRC_CAP;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pd_sink_reset_state);
+
+int pd_sink_tx_complete(int port)
+{
+	struct pd_sink_port *pport;
+
+	if (port < 0 || port >= MAX_NR_SINK_PORTS) {
+		pr_err("Invalid port number");
+		return -EINVAL;
+	}
+
+	pport = sink_ports[port];
+
+	if (!pport->hw_goodcrc_rx && pport->last_msg_ctrl &&
+		pport->last_sent_msg == PD_CMSG_GOODCRC) {
+		stop_timer(pport);
+		clear_waiting_goodcrc(pport);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pd_sink_tx_complete);
diff --git a/include/linux/usb/pd_message.h b/include/linux/usb/pd_message.h
new file mode 100644
index 0000000..b64591e
--- /dev/null
+++ b/include/linux/usb/pd_message.h
@@ -0,0 +1,371 @@
+/*
+ * pd_messae.h - USB PD message header file.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Bin Gao <bin.gao@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __USB_PD_MESSAGE_H
+#define __USB_PD_MESSAGE_H
+
+/* All length definitions are in bytes */
+#define PD_MAX_EXTENDED_MSG_LEN		260
+#define PD_MAX_EXTENDED_MSG_CHUNK_LEN	26
+#define PD_MAX_EXTENDED_MSG_LEGACY_LEN	26
+#define PD_MSG_HEADER_LEN		2
+#define PD_MSG_EXT_HEADER_LEN		2
+
+/* Control Messages */
+/* 0: reserved */
+#define PD_CMSG_GOODCRC		1
+#define PD_CMSG_GOTOMIN		2
+#define PD_CMSG_ACCEPT		3
+#define PD_CMSG_REJECT		4
+#define PD_CMSG_PING		5
+#define PD_CMSG_PS_RDY		6
+#define PD_CMSG_GET_SRC_CAP	7
+#define PD_CMSG_GET_SINK_CAP	8
+#define PD_CMSG_DR_SWAP		9
+#define PD_CMSG_PR_SWAP		10
+#define PD_CMSG_VCONN_SWAP	11
+#define PD_CMSG_WAIT		12
+#define PD_CMSG_SOFT_RESET	13
+/* 14-15: reserved */
+/* 16-19: Revision 3.0 only */
+#define PD_CMSG_NOT_SUPPORTED	16
+#define PD_CMSG_GET_SOURCE_CAP_EXTENDED	17
+#define PD_CMSG_GET_STATUS	18
+#define PD_CMSG_FR_SWAP		19
+/* 20-31: reserved */
+#define PD_NR_CMSGS		32
+
+/* Data messages */
+/* 0: reserved */
+#define PD_DMSG_SOURCE_CAP	1
+#define PD_DMSG_REQUEST		2
+#define PD_DMSG_BIST		3
+#define PD_DMSG_SINK_CAP	4
+/* 5-6: Revision 3.0 only */
+#define PD_DMSG_BATTERY_STATUS	5
+#define PD_DMSG_ALERT		6
+/* 7-14: reserved */
+#define PD_DMSG_VENDOR_DEFINED	15
+/* 16-31: reserved */
+#define PD_NR_DMSGS		32
+
+#define PD_MSG_ID_MIN		0
+#define PD_MSG_ID_MAX		7
+
+/* Both control message and data message are 5-bit long */
+#define PD_MSG_TYPE_MASK	0x1F
+
+/* Source Capability bits: 31:30 */
+#define SOURCE_CAP_TYPE_BIT	30
+#define SOURCE_CAP_TYPE_MASK	0x3
+
+#define PD_OBJ_SIZE		4 /* each data object is 32bit long */
+
+/* All timeouts are in milli-seconds, unless speically commented */
+#define PD_TIMEOUT_GOODCRC	500
+#define PD_TIMEOUT_ACCEPT	500
+#define PD_TIMEOUT_PS_READY	1000
+
+#define PD_VSAFE5V			5 /* Default VBUS voltage: 5V */
+
+/*
+ * Message Header (16 bits)
+ * When SOP type is SOP' or SOP'':
+ * The 1bit data_role field is defined
+ * as "Cable Plug": 1-Message originated from a Cable Plug
+ *                  0-Message originated from a DFP or UFP
+ * The 1bit Port Data Role is defined as "Reserved".
+ */
+struct pd_msg_header {
+	/*
+	 * All fields are for SOP type: SOP, except:
+	 * When SOP type is SOP' or SOP'', the following bits have
+	 * different meaning:
+	 * power_role:1 - "Cable Plug"
+	 *   1-Message originated from a Cable Plug
+	 *   0-Message originated from a DFP or UFP
+	 * data_role:1 - "Reserved"
+	 */
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	/*
+	 * According to the USB PD spec Rev 3.0 V1.0a, extended bit is
+	 * the most significant bit(bit 15) and message type bits are the
+	 * least significant 5 bits (bit 4:0).
+	 */
+	u16 type:5;		/* Message type */
+	u16 data_role:1;	/* Port data role: 1-DFP 0-UFP */
+	u16 revision:2;		/* Specification revision */
+	u16 power_role:1;	/* Port power role: 1-Source 0-Sink */
+	u16 id:3;		/* Message ID */
+	u16 nr_objs:3;		/* Number of 32bit data objects */
+	u16 extended:1;		/* Revision 2.0 - reserved, Revision 3.0 -
+				 * extended message: 1-Extended 0-NonExtended
+				 */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u16 extended:1;
+	u16 nr_objs:3;
+	u16 id:3;
+	u16 power_role:1;
+	u16 revision:2;
+	u16 data_role:1;
+	u16 type:5;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/* Extended message is only for PD Revision 3.0 */
+#define PD_MSG_EXTENDED		1
+#define PD_MSG_NOT_EXTENDED	0
+
+#define PD_POWER_ROLE_SINK	0 /* Consume power */
+#define PD_POWER_ROLE_SOURCE	1 /* Supply power */
+
+#define PD_DATA_ROLE_UFP	0 /* Up facing port: device */
+#define PD_DATA_ROLE_DFP	1 /* Down facing port: host */
+
+#define PD_REVISION_1		0 /* USB PD Specification Revision 1.0 */
+#define PD_REVISION_2		1 /* USB PD Specification Revision 2.0 */
+#define PD_REVISION_3		2 /* USB PD Specification Revision 3.0 */
+
+/* Fixed supply PDO peak current capability */
+#define PEAK_CURRENT_DEFAULT	0 /* default operating current, no overload */
+#define PEAK_CURRENT_1		1 /* low overload */
+#define PEAK_CURRENT_2		2 /* medium overload */
+#define PEAK_CURRENT_3		3 /* high overload */
+
+/* Fast Role Swap coding in Fixed Supply PDO (for Sink) */
+#define FAST_ROLE_SWAP_DISABLE	0
+#define FAST_ROLE_SWAP_DEFAULT	1 /* Default USB Power */
+#define FAST_ROLE_SWAP_1P5A_5V	2 /* 1.5A @ 5V */
+#define FAST_ROLE_SWAP_3A_5V	3 /* 3.0A @ 5V */
+
+/* ALERT message: PD Revision 3.0 only */
+#define PD_ALERT_BATTERY	BIT(1) /* Battery status change */
+#define PD_ALERT_OCP		BIT(2) /* Source only */
+#define PD_ALERT_OTP		BIT(3)
+#define PD_ALERT_OCC		BIT(4) /* Operating condition change */
+#define PD_ALERT_SRC_INPUT	BIT(5) /* Source input change */
+#define PD_ALERT_OVP		BIT(6) /* OVP, Sink only */
+
+/*
+ * Power supply type:
+ * Fixed power Supply is the most commonly used to expose well-regulated fixed
+ * voltage power supplies.
+ * Variable power supply is used to expose very poorly regulated power supplies.
+ * Battery power supply is used to expose batteries that can be directly
+ * connected to VBUS.
+ * The definition below is used in both SOURCE_CAPABILITY and SINK_CAPABILITY
+ * messages.
+ */
+#define PS_TYPE_FIXED		0
+#define PS_TYPE_BATTERY		1
+#define PS_TYPE_VARIABLE	2
+
+enum sop_type {
+	SOP,	/* SOP */
+	SOP_P,	/* SOP' */
+	SOP_PP,	/* SOP'' */
+};
+
+/* Extended Message Header (16 bits), PD Rev 3.0 only */
+struct pd_ext_msg_header {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	u16 data_size:9;
+	u16 reserved:1;
+	u16 request_chunk:1;
+	u16 chunk_number:4;
+	u16 chunked:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u16 chunked:1;
+	u16 chunk_number:4;
+	u16 request_chunk:1;
+	u16 reserved:1;
+	u16 data_size:9;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/* Fixed power supply PDO(Power Data Object) in SOURCE_CAPABILITY message */
+struct pd_pdo_src_fixed {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	u32 current_max:10;	/* Maximum current in 10mA units */
+	u32 voltage:10;		/* Voltage in 50mV units */
+	u32 current_peak:2;	/* Peak Current */
+	u32 reserved:2;		/* Reserved, shall be zero */
+	u32 unchunked:1;	/* Unchunked extended message supported */
+	u32 drd:1;		/* Dual-Role Data */
+	u32 usb_capable:1;	/* USB communication capable */
+	u32 ext_powered:1;	/* Externally powered */
+	u32 usb_suspend:1;	/* USB suspend supported */
+	u32 drw:1;		/* Dual-Role Power */
+	u32 type:2;		/* Fixed supply PDO - 00b */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 type:2;
+	u32 drw:1;
+	u32 usb_suspend:1;
+	u32 ext_powered:1;
+	u32 usb_capable:1;
+	u32 drd:1;
+	u32 unchunked:1;
+	u32 reserved:2;
+	u32 current_peak:2;
+	u32 voltage:10;
+	u32 current_max:10;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/* Fixed power supply PDO(Power Data Object) in SINK_CAPABILITY message */
+struct pd_pdo_sink_fixed {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	u32 current_max:10;	/* Maximum current in 10mA units */
+	u32 voltage:10;		/* Voltage in 50mV units */
+	u32 reserved:3;		/* Reserved, shall be zero */
+	u32 fast_role_swap:2;	/* Fast Role Swap required USB Type-C Current*/
+	u32 drd:1;		/* Dual-Role Data */
+	u32 usb_capable:1;	/* USB communication capable */
+	/* Externally powered(e.g. an AC supply is connected to the Sink) */
+	u32 ext_powered:1;
+	u32 higher_capability:1;	/* Sink needs more than vSafe5V */
+	u32 drw:1;		/* Dual-Role Power */
+	u32 type:2;		/* Fixed supply PDO - 00b */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 type:2;
+	u32 drw:1;
+	u32 higher_capability:1;
+	u32 ext_powered:1;
+	u32 usb_capable:1;
+	u32 drd:1;
+	u32 fast_role_swap:2;
+	u32 reserved:3;
+	u32 voltage:10;
+	u32 current_max:10;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/*
+ * Variable power supply PDO in both SOURCE_CAPABILITY and
+ * SINK_CAPABILITY messages.
+ */
+struct pd_pdo_variable {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	/* current in 10mA units, Source: maximum, Sink: operational */
+	u32 current_mo:10;
+	u32 voltage_min:10;	/* Mimimum voltage in 50mV units */
+	u32 voltage_max:10;	/* Maximum voltage in 50mV units */
+	u32 type:2;		/* Variable supply PDO - 10b */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 type:2;
+	u32 voltage_max:10;
+	u32 voltage_min:10;
+	u32 current_max:10;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/*
+ * Battery power supply PDO in both SOURCE_CAPABILITY and
+ * SINK_CAPABILITY messages.
+ */
+struct pd_pdo_battery {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	/*  Power in 250mW units: Source - maximum, Sink - operational */
+	u32 power:10;
+	u32 voltage_min:10;	/* Minimum voltage in 50mV units */
+	u32 voltage_max:10;	/* Maximum voltage in 50mV units */
+	u32 type:2;		/* Battery supply PDO - 01b */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 type:2;
+	u32 voltage_max:10;
+	u32 voltage_min:10;
+	u32 power_max:10;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/* Request Data Object for Fixed, Variable or Battery power supply */
+struct pd_request {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	/*
+	 * Minimum operating current in 10mA units for Fixed and Variable PS.
+	 * Maximum operating power in 250mW units for Battery PS.
+	 */
+	u32 operating_min_max:10;
+	/*
+	 * Default operating current in 10mA units for Fixed and Variable PS.
+	 * Default operating power in 250mW units for Battery PS.
+	 */
+	u32 operating_default:10;
+	u32 reserved2:3;
+	/*
+	 * 1 - Support unchunked extended message.
+	 * 0 - No support for unchunked extended message.
+	 * This bit is only for PD Rev 3.0.
+	 */
+	u32 unchunked_ext_msg_supported:1;
+	/*
+	 * 1 - Request to use power during USB suspend. This is for purposes
+	 *     other than USB communication e.g. for charging a Battery.
+	 * 0 - Don't use power during USB suspend.
+	 */
+	u32 no_suspend:1;
+	u32 usb_comm_capable:1; /* USB communication capable from this port */
+	/*
+	 * 1 - No PDO from SOURCE_CAPABILITY fits Sink's requirement.
+	 * 0 - One of PDOs fits.
+	 */
+	u32 cap_mismatch:1;
+	u32 give_back:1; /* 1 - Support GOTOMIN message, 0 - No support */
+	u32 pos:3; /* Index of the selected PDO in the SOURCE_CAPABILITY msg */
+	u32 reserved1:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 reserved1:1;
+	u32 pos:3;
+	u32 give_back:1;
+	u32 cap_mismatch:1;
+	u32 usb_comm_capable:1;
+	u32 no_suspend:1;
+	u32 unchunked_ext_msg_supported:1;
+	u32 reserved2:3;
+	u32 operating_default:10;
+	u32 operating_min_max:10;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+/* ALERT message: may sent by Source or Sink, PD Revision 3.0 only */
+struct alert_msg {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	u32 reserved:16;
+	u32 hot_swappable_batteries:4;
+	u32 fixed_batteries:4;
+	u32 type:8;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u32 type:8;
+	u32 fixed_batteries:4;
+	u32 hot_swappable_batteries:4;
+	u32 reserved:16;
+#else
+	#error "Please fix <asm/byteorder.h>"
+#endif
+} __packed;
+
+#endif /* __USB_PD_MESSAGE_H */
diff --git a/include/linux/usb/pd_sink.h b/include/linux/usb/pd_sink.h
new file mode 100644
index 0000000..6545684
--- /dev/null
+++ b/include/linux/usb/pd_sink.h
@@ -0,0 +1,286 @@
+/*
+ * pd_sink.h - USB PD Sink port state machine header file.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Bin Gao <bin.gao@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __USB_PD_SINK_H
+#define __USB_PD_SINK_H
+
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/usb/pd_message.h>
+#include <linux/workqueue.h>
+#include <linux/hrtimer.h>
+
+#define MAX_NR_SINK_PORTS	4
+#define MAX_NR_SINK_PS_REQS	6
+#define MAX_NR_SOURCE_CAPS	6
+
+enum usb_spec {
+	USB_SPEC_1X = 1,
+	USB_SPEC_2X,
+	USB_SPEC_3X,
+};
+
+enum pd_sink_state {
+	PD_SINK_STATE_WAIT_FOR_SRC_CAP,
+	PD_SINK_STATE_REQUEST_SENT,
+	PD_SINK_STATE_ACCEPT_RECEIVED,
+	PD_SINK_STATE_PS_READY,
+};
+
+enum pd_sink_event {
+	PD_SINK_EVENT_PS_READY,	/* PS_READY message received */
+	PD_SINK_EVENT_GOTOMIN,	/* GOTOMIN message received */
+	PD_SINK_EVENT_REJECT,	/* REJECT message received */
+	PD_SINK_EVENT_ALERT,	/* ALERT message received */
+	PD_SINK_EVENT_SOFT_RESET,	/* SOFT_RESET message received */
+};
+
+enum pd_tx_state {
+	PD_TX_SUCCESS, /* PD PHY transmission succeeded */
+	PD_TX_FAILURE, /* PD PHY transmission failed */
+};
+
+/**
+ * struct pd_sink_msg - a structure describing a PD message
+ * @port:	Sink port index
+ * @buf:	pointer of the buffer having the message data
+ * @msg_len:	length of the message
+ * @sop_type:	SOP type - SOP, SOP' or SOP''
+ * @list:	the list_head of the message list
+ */
+struct pd_sink_msg {
+	int port;
+	u8 *buf;
+	int msg_len;
+	enum sop_type sop_type;
+	struct list_head list;
+};
+
+/* Sink port's power supply requirement */
+struct sink_ps {
+	u8 ps_type; /* fixed, variable or battery */
+	union {
+		struct ps_fixed {
+			u16 voltage_fixed; /* Fixed voltage in 50mV units */
+			/* Operational current in 10mA units */
+			u16 current_default;
+			/* Maximum current in 10mA units */
+			u16 current_max;
+		} ps_fixed;
+		struct ps_variable {
+			u16 voltage_max; /* Maximum voltage in 50mV units */
+			u16 voltage_min; /* Minimum voltage in 50mV units */
+			/* Operational current in 10mA units */
+			u16 current_default;
+		} ps_variable;
+		struct ps_battery {
+			u16 voltage_max; /* Maximum voltage in 50mV units */
+			u16 voltage_min; /* Minimum voltage in 50mV units */
+			u16 power; /* Operational power in 250mW units */
+		} ps_battery;
+	};
+};
+
+/* We save PDOs in SOURCE_CAPABILITY message into pd_source_cap structure */
+struct pd_source_cap {
+	u8 ps_type; /* fixed, variable or battery */
+	union {
+		struct pd_pdo_src_fixed fixed;
+		struct pd_pdo_variable variable;
+		struct pd_pdo_battery battery;
+	};
+};
+
+/**
+ * struct pd_sink_profile - a structure describing a Sink port's profile.
+ * @hw_goodcrc_tx: outgoing GOODCRC is sent by HW automatically
+ * @hw_goodcrc_rx: incoming GOODCRC is received and checked by HW, i.e.
+ *                 GOODCRC message won't be reported to software
+ * @gotomin: true if the port supports GOTOMIN message
+ * @usb_comm: true if the port is usb data communication capable
+ * @pd_rev: PD revision(rev 1.0, 2.0 or 3.0) the hardware supports
+ * @spec: highest USB spec(1.x, 2.x or 3.x) the port supports
+ * @ps: An array of power supply requirements
+ * @nr_ps: size of the power supply requirement array
+ * @active_ps: index to the array, indicating the power supply that will
+ *	sent in the REQUEST message
+ */
+struct pd_sink_profile {
+	bool hw_goodcrc_tx;
+	bool hw_goodcrc_rx;
+	bool gotomin;
+	bool usb_comm;
+	u8 pd_rev;
+	enum usb_spec spec;
+	struct sink_ps *ps;
+	int nr_ps;
+	int active_ps;
+};
+
+/**
+ * struct pd_sink_port - the Sink port structure, used by PD sink only,
+ *	not exposed to upper layer(e.g. type-C PHY driver).
+ * @hw_goodcrc_tx: outgoing GOODCRC is sent by HW automatically
+ * @hw_goodcrc_rx: incoming GOODCRC is received and checked by HW, i.e.
+ *                 GOODCRC message won't be reported to software
+ * @support_gotomin: the port will take action, e.g. reduce current
+ *	drawing, when a GOTOMIN message is received
+ * @usb_comm_capable: the port is capable of USB data communication
+ * @pd_rev: PD revision(rev 1.0, 2.0 or 3.0) the hardware supports
+ * @usb_spec: the highest USB spec the port supports(USB 1.X, 2.X or 3.X)
+ * @ps_reqs[]: an array of power supply requirements of the port
+ * @nr_ps: size of ps_reqs[]
+ * @active_ps: index to ps_reqs[] indicating the perferred power supply
+ *	requirement that will be used for the REQUEST message. It can be
+ *	change later by calling pd_sink_change_ps().
+ * @port: the port number (0 based) assigned by the PD sink driver
+ * @last_sent_msg: the most recent message(message type) that was sent
+ * @last_msg_ctrl: true if last_sent_msg is a control message
+ * @msg_id: message ID of last_sent_msg
+ * @state: port state, see the structrue enum pd_sink_state
+ * @waiting_goodcrc: true if a message was sent but a GOODCRC for that
+ *	message is not received yet
+ * @source_caps[]: power supply capability extracted from SOURCE_CAPABILITY
+ *	message sent by the Source
+ * @nr_source_caps: size of the soruce_caps[] array
+ * @rx_wq: workqueue for handing received messages
+ * @rx_work: the work struct for handing received messages
+ * @rx_list: message list head
+ * @tx_timer: a timer for monitoring transmission timeout
+ * @tx: the PD message transmission function, provided by upper layer
+ *	during port registration
+ * @tx_priv: a private data pointer provided by upper layer. This pointer
+ *	will be passed down to the tx() function
+ */
+struct pd_sink_port {
+	bool hw_goodcrc_tx;
+	bool hw_goodcrc_rx;
+	bool support_gotomin;
+	bool usb_comm_capable;
+	u8 pd_rev;
+	enum usb_spec usb_spec;
+	struct sink_ps ps_reqs[MAX_NR_SINK_PS_REQS];
+	int nr_ps;
+	int active_ps;
+	int port;
+	u8 last_sent_msg;
+	bool last_msg_ctrl;
+	u16 msg_id;
+	int state;
+	bool waiting_goodcrc;
+	struct pd_source_cap source_caps[MAX_NR_SOURCE_CAPS];
+	int nr_source_caps;
+	struct workqueue_struct *rx_wq;
+	struct work_struct rx_work;
+	spinlock_t rx_lock;
+	struct list_head rx_list;
+	struct hrtimer tx_timer;
+	int (*tx)(int port, void *tx_priv, void *buf, int len,
+					enum sop_type sop_type);
+	void *tx_priv;
+};
+
+/**
+ * pd_sink_register_port() - Function to register a Sink port
+ * @profile: pointer to a pd_sink_profile structure describing the port, see
+ *	struct pd_sink_profile {} for details
+ * @tx: the function for transmitting PD messages
+ * @tx_priv: a private data pointer that is required by the tx function. This
+ *	pointer will be passed down to the tx function when tx() is called.
+ *
+ * The type-C phy driver calls this function to register a Sink port.
+ * PD message receiving interrupt can be enabled only after this function
+ * call is returned.
+ *
+ * Return: a port number(>=0) if succeeded or a negative number if failed
+ */
+int pd_sink_register_port(struct pd_sink_profile *profile,
+		int (*tx)(int port, void *tx_priv, void *buf, int len,
+				enum sop_type sop_type), void *tx_priv);
+
+/**
+ * pd_sink_unregister_port() - Function to unregister a Sink port
+ * @port: the port number
+ *
+ * The type-C phy driver is supposed to call this function to unregister
+ * the port whenever it detects the cable is unplugged or the VBUS has gone.
+ *
+ * Return: 0 if succeeded or a negative number if failed
+ */
+int pd_sink_unregister_port(int port);
+
+/**
+ * pd_sink_alloc_msg() - allocate a message structure from the PD sink stack
+ * @port: the port number
+ * @msg_len: the length of the buffer to hold the message
+ *
+ * This function is used by Type-C phy driver during receiving a message from
+ * the PD hardware. The received message is saved in the pd_sink_msg structure
+ * which is then pushed to the PD sink stack by calling pd_sink_queue_msg().
+ *
+ * Return: pointer of a pd_sink_msg structure or NULL if failed
+ */
+struct pd_sink_msg *pd_sink_alloc_msg(int port, int msg_len);
+
+/**
+ * pd_sink_queue_msg() - Queue a received message to the PD sink stack
+ * @msg: the message to queue
+ *
+ * The caller must fill the .sop_type field and copy the message data to
+ * the buffer pointed by .buf field.
+ *
+ * Return: 0 if succeeded or a negative number if failed
+ */
+int pd_sink_queue_msg(struct pd_sink_msg *msg);
+
+/**
+ * pd_sink_change_ps() - Function to change a port's power supply requirement
+ * @port: the port number
+ * @ps: index to the power supply requirement array previously provided to
+ *	the PD sink stack
+ *
+ * Type-C phy driver calls this function to request a different power supply
+ * from Source. This may happen for example the port which is in regular
+ * operation state starts charging the battery. In this case the port requests
+ * to change to a power supply with higher power.
+ *
+ * Return: 0 if a REQUEST message is successfully sent, negative value if failed
+ */
+int pd_sink_change_ps(int port, int ps);
+
+/**
+ * pd_sink_reset_state() - Reset the port to initial
+ * state(PD_SINK_STATE_WAIT_FOR_SRC_CAP)
+ * @port: the port number
+ *
+ * Type-C phy driver typically calls this function when the VBUS of the
+ * port has gone, the cable is unplugged or a hard reset is detected. This
+ * function reset the port's state to a initial state(wiat for src_cap) in
+ * the PD sink stack thus it can correctly process SOURCE_CAPABILITY message
+ * once the port becomes active.
+ *
+ * Return: 0 on success or a negtive number on failure
+ */
+int pd_sink_reset_state(int port);
+
+/**
+ * pd_sink_tx_complete() - Type-C phy driver reports message transmission
+ *                         is completed
+ *
+ * @port: the port number
+ *
+ * Return: 0 on success or a negtive number on failure
+ */
+int pd_sink_tx_complete(int port);
+
+#endif /* __USB_PD_SINK_H */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  2:14 [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao
@ 2016-07-15  6:31 ` Oliver Neukum
  2016-07-15 22:33   ` Bin Gao
  2016-07-15  7:25 ` Felipe Balbi
  2016-07-15 10:38 ` Felipe Balbi
  2 siblings, 1 reply; 18+ messages in thread
From: Oliver Neukum @ 2016-07-15  6:31 UTC (permalink / raw)
  To: Bin Gao
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-kernel, linux-usb,
	Bin Gao, Chandra Sekhar Anagani

On Thu, 2016-07-14 at 19:14 -0700, Bin Gao wrote:
> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
> 

> +/*
> + * For any message we send, we must get a GOODCRC message from the Source.
> + * The USB PD spec says the time should be measured between the last bit
> + * of the sending message's EOP has been transmitted and the last bit of
> + * the receiving GOODCRC message's EOP has been received. The allowed time
> + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> + * performed in physical layer. When it reaches to the OS and this driver,
> + * the actual time is difficult to predict because of the scheduling,
> + * context switch, interrupt preemption and nesting, etc. So we only define
> + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> + * account of all software related latency.
> + */
> +static int send_message(struct pd_sink_port *port, void *buf, int len,
> +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> +{
> +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +		print_message(port->port, true, msg, false);
> +		return 0;
> +	}
> +
> +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +	print_message(port->port, ctrl_msg, msg, false);
> +
> +	if (!port->hw_goodcrc_rx) {
> +		port->waiting_goodcrc = true;
> +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);
> +	}
> +
> +	port->last_sent_msg = msg;
> +	port->last_msg_ctrl = ctrl_msg;
> +
> +	return 0;
> +}
> +
> +static void ack_message(struct pd_sink_port *port, int msg_id)
> +{
> +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);

This must be GFP_NOIO. We are in a cycle that can lead to deadlock.

Assume we are waiting for a request for more power to process IO
which we need to ack.

1. memory allocation leads to laundering, blocks on freeing memory
2. launderer decides to perform IO which needs more power
3. more power has already been requested, wait for it to be granted

4. BANG - DEADLOCK


> +
> +	if (!header) {
> +		MAKE_HEADER(port, header, PD_CMSG_GOODCRC, 0);
> +		send_message(port, header, PD_MSG_HEADER_LEN,
> +				PD_CMSG_GOODCRC, true, SOP);
> +}	}
> +
> +static int handle_goodcrc(struct pd_sink_port *port, u8 msg_id)
> +{
> +	if (is_waiting_goodcrc(port, msg_id)) {
> +		hrtimer_cancel(&port->tx_timer);
> +		clear_waiting_goodcrc(port);
> +		switch (msg_id) {
> +		case PD_DMSG_REQUEST:
> +			port->state = PD_SINK_STATE_REQUEST_SENT;
> +			break;
> +		case PD_DMSG_SINK_CAP:
> +			pr_info("Got GOODCRC for SINK_CAPABILITY message\n");
> +		default:
> +			break;
> +		}
> +	} else
> +		pr_warn("Unexpected GOODCRC message received.\n");
> +
> +	return 0;
> +}
> +
> +static void handle_accept(struct pd_sink_port *port)
> +{
> +	enum pd_sink_state state = port->state;
> +
> +	if (state != PD_SINK_STATE_REQUEST_SENT) {
> +		pr_err("ACCEPT received but not expected, sink state: %s\n",
> +						state_to_string(state));
> +		return;
> +	}
> +
> +	port->state = PD_SINK_STATE_ACCEPT_RECEIVED;
> +}
> +
> +/*
> + * The Source may send REJECT message as a response to REQUEST, PR_SWAP,
> + * DR_SWAP or VCONN_SWAP message. Since we only send REQUEST to the Source
> + * so we're sure the REJECT is for our REQUEST message.
> + */
> +static void handle_reject(struct pd_sink_port *port)
> +{
> +	enum pd_sink_state state = port->state;
> +
> +	if (state == PD_SINK_STATE_REQUEST_SENT)
> +		/* Broadcast to upper layer */
> +		blocking_notifier_call_chain(&pd_sink_notifier_list,
> +				PD_SINK_EVENT_REJECT | port->port << 8, NULL);
> +	else
> +		pr_err("REJECT received but not expected, sink state: %s\n",
> +						state_to_string(state));
> +}
> +
> +static void handle_not_supported(struct pd_sink_port *port)
> +{
> +	pr_err("sink port %d: %s message %s is not supported by Source\n",
> +			port->port, port->last_msg_ctrl ? "Control" : "Data",
> +				msg_to_string(port->last_msg_ctrl,
> +			port->last_sent_msg & PD_MSG_TYPE_MASK));
> +}
> +
> +/*
> + * The Wait Message is a valid response to a Request, a PR_Swap, DR_Swap or
> + * VCONN_Swap Message. We don't support PR_Swap, DR_Swap and VCONN_Swap
> + * messages so we only handle Wait message from Source responding to our
> + * Request message.
> + */
> +static void handle_wait(struct pd_sink_port *port)
> +{
> +	pr_info("WAIT message received\n");
> +}
> +
> +/*
> + * We repond the GET_SINK_CAPABILITY message by sending the SINK_CAPABILITY
> + * message. The PDOs in the SINK_CAPABILITY message shall be in the following
> + * order:
> + * 1. The vSafe5V Fixed Supply Object shall always be the first object.
> + * 2. The remaining Fixed Supply Objects, if present, shall be sent in voltage
> + *    order; lowest to highest.
> + * 3. The Battery Supply Objects, if present shall be sent in Minimum Voltage
> + *    order; lowest to highest.
> + * 4. The Variable Supply (non-Battery) Objects, if present, shall be sent in
> + *    Minimum Voltage order; lowest to highest.
> + *
> + * The upper layer calling pd_sink_register_port() must ensure the profiles
> + * provided come with the above order as we don't re-order the profiles here
> + * when we compose the SINK_CAPABILITY message.
> + */
> +static void handle_get_sink_cap(struct pd_sink_port *port)
> +{
> +	u8 *pdo;
> +	int i, nr_objs = 0;
> +	struct sink_ps *ps;
> +	struct pd_pdo_sink_fixed *fixed;
> +	struct pd_pdo_variable *variable;
> +	struct pd_pdo_battery *battery;
> +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> +		port->nr_ps * PD_OBJ_SIZE, GFP_KERNEL);

Must be GFP_NOIO. For the same reason as above. We may be asked
this to resolve a mismatch due to needing more power for IO.

> +
> +	if (!header) {
> +		pr_crit("%s(): kzalloc() failed\n", __func__);
> +		return;
> +	}
> +
> +	MAKE_HEADER(port, header, PD_DMSG_SINK_CAP, port->nr_ps);
> +	pdo = (u8 *)header + PD_MSG_HEADER_LEN;
> +
> +	for (i = 0; i < port->nr_ps; i++) {
> +		ps = port->ps_reqs + i;
> +		switch (ps->ps_type) {
> +		case PS_TYPE_FIXED:
> +			fixed = (struct pd_pdo_sink_fixed *)pdo;
> +			fixed->current_max = ps->ps_fixed.current_max;
> +			fixed->voltage = ps->ps_fixed.voltage_fixed;
> +			fixed->fast_role_swap = FAST_ROLE_SWAP_DISABLE;
> +			fixed->usb_capable = 1;
> +			fixed->higher_capability =
> +				(ps->ps_fixed.voltage_fixed >
> +						PD_VSAFE5V) ? 1 : 0;
> +			fixed->type = PS_TYPE_FIXED;
> +			break;
> +		case PS_TYPE_BATTERY:
> +			battery = (struct pd_pdo_battery *)pdo;
> +			battery->power = ps->ps_battery.power;
> +			battery->voltage_max = ps->ps_battery.voltage_max;
> +			battery->voltage_min = ps->ps_battery.voltage_min;
> +			battery->type = PS_TYPE_BATTERY;
> +			break;
> +		case PS_TYPE_VARIABLE:
> +			variable = (struct pd_pdo_variable *)pdo;
> +			variable->current_mo =
> +				ps->ps_variable.current_default;
> +			variable->voltage_max = ps->ps_variable.voltage_max;
> +			variable->voltage_min = ps->ps_variable.voltage_min;
> +			variable->type = PS_TYPE_VARIABLE;
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		pdo += PD_OBJ_SIZE;
> +		nr_objs++;
> +	}
> +
> +	if (nr_objs == 0) {
> +		kfree(header);
> +		pr_err("no valid sink PDOs to send in SINK_CAPABILITY\n");
> +		return;
> +	}
> +
> +	header->nr_objs = nr_objs;
> +	send_message(port, header, PD_MSG_HEADER_LEN + nr_objs * PD_OBJ_SIZE,
> +						PD_DMSG_SINK_CAP, false, SOP);
> +}
> +
> +static void handle_ps_ready(struct pd_sink_port *port)
> +{
> +	if (port->state != PD_SINK_STATE_ACCEPT_RECEIVED) {
> +		pr_err("PS_READY received but sink state is not in %s\n",
> +			state_to_string(PD_SINK_STATE_ACCEPT_RECEIVED));
> +		return;
> +	}
> +
> +	/* Broadcast to upper layer */
> +	blocking_notifier_call_chain(&pd_sink_notifier_list,
> +		PD_SINK_EVENT_PS_READY | port->port << 8, NULL);
> +}
> +
> +static void handle_gotomin(struct pd_sink_port *port)
> +{
> +	/* Broadcast to upper layer */
> +	blocking_notifier_call_chain(&pd_sink_notifier_list,
> +		PD_SINK_EVENT_GOTOMIN | port->port << 8, NULL);
> +}
> +
> +static void handle_soft_reset(struct pd_sink_port *port)
> +{
> +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
> +
> +	if (!header)
> +		return;
> +
> +	flush_workqueue(port->rx_wq);

That is problematic. We may be here precisely because something is wrong
blocking progress. In particular what happens if another soft reset
is queued?

> +	stop_timer(port);
> +	port->msg_id = PD_MSG_ID_MIN;
> +	port->state = PD_SINK_STATE_WAIT_FOR_SRC_CAP;
> +	MAKE_HEADER(port, header, PD_CMSG_ACCEPT, 0);
> +	send_message(port, header, PD_MSG_HEADER_LEN,
> +			PD_CMSG_ACCEPT, true, SOP);
> +
> +	/* Broadcast to upper layer */
> +	blocking_notifier_call_chain(&pd_sink_notifier_list,
> +		PD_SINK_EVENT_SOFT_RESET | port->port << 8, NULL);
> +}
> +
> +static void ctl_msg_handler(struct pd_sink_port *port, u8 msg_type)
> +{
> +	print_message(port->port, true, msg_type, true);
> +
> +	switch (msg_type) {
> +	case PD_CMSG_ACCEPT:
> +		handle_accept(port);
> +		break;
> +	case PD_CMSG_GOTOMIN:
> +		handle_gotomin(port);
> +		break;
> +	case PD_CMSG_REJECT:
> +		handle_reject(port);
> +		break;
> +	case PD_CMSG_GET_SINK_CAP:
> +		handle_get_sink_cap(port);
> +		break;
> +	case PD_CMSG_PS_RDY:
> +		handle_ps_ready(port);
> +		break;
> +	case PD_CMSG_SOFT_RESET:
> +		handle_soft_reset(port);
> +		break;
> +	case PD_CMSG_NOT_SUPPORTED:
> +		handle_not_supported(port);
> +		break;
> +	case PD_CMSG_WAIT:
> +		/*
> +		 * Sent by Source to Sink to indicate it can't meet the
> +		 * requirement for the Request. The Sink is allowed to
> +		 * repeat the Request Message using the SinkRequestTimer
> +		 * to ensure that there is tSinkRequest between requests.
> +		 */
> +		handle_wait(port);
> +		break;
> +	/* Below messages are simply ignored */
> +	case PD_CMSG_GET_SOURCE_CAP_EXTENDED:
> +	case PD_CMSG_GET_STATUS:
> +	case PD_CMSG_PING:
> +	case PD_CMSG_GET_SRC_CAP:
> +	case PD_CMSG_DR_SWAP:
> +	case PD_CMSG_PR_SWAP:
> +	case PD_CMSG_VCONN_SWAP:
> +	case PD_CMSG_FR_SWAP:
> +		break;
> +
> +	}
> +}
> +
> +/**
> + * send_request() - send REQUEST message to Source.
> + * @port: the sink port number
> + * Return: none
> + */
> +static int send_request(struct pd_sink_port *port)
> +{
> +	int i;
> +	struct pd_request *request;
> +	struct pd_pdo_src_fixed *src;
> +	struct sink_ps *ps =
> +		&port->ps_reqs[port->active_ps];
> +	struct pd_source_cap *cap;
> +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> +					PD_OBJ_SIZE, GFP_KERNEL);

GFP_NOIO, same reasons

> +	bool voltage_matched = false, current_matched = false;
> +
> +	/* Only support fixed PDO for now */
> +	if (ps->ps_type != PS_TYPE_FIXED) {
> +		pr_err("%s(): We only support fixed PDO now\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < port->nr_source_caps; i++) {
> +		cap = &port->source_caps[i];
> +		if (cap->ps_type != PS_TYPE_FIXED)
> +			continue;
> +		src = &cap->fixed;
> +		if (src->voltage != ps->ps_fixed.voltage_fixed)
> +			continue;
> +		voltage_matched = true;
> +		/* We matched the voltage, now match the current */
> +		if (src->current_max >= ps->ps_fixed.current_max) {
> +			current_matched = true;
> +			break;
> +		}
> +	}
> +
> +	if (!voltage_matched) {
> +		pr_err("Can't match any PDO from source caps\n");
> +		return -EINVAL;
> +	}
> +
> +	MAKE_HEADER(port, header, PD_DMSG_REQUEST, 1);
> +	request = (struct pd_request *)((u8 *)header + PD_MSG_HEADER_LEN);
> +	request->pos = i + 1; /* PD protocol RDO index starts from 1 */
> +	request->give_back = port->support_gotomin ? 1 : 0;
> +	request->usb_comm_capable = port->usb_comm_capable ? 1 : 0;
> +	if (port->pd_rev == PD_REVISION_3)
> +		request->unchunked_ext_msg_supported = 1;
> +
> +	/* If current_matched is false, we have to set the mismatch flag */
> +	if (!current_matched)
> +		request->cap_mismatch = 1;
> +
> +	request->operating_default = ps->ps_fixed.current_default;
> +	request->operating_min_max = ps->ps_fixed.current_max;
> +
> +	return send_message(port, header, PD_MSG_HEADER_LEN + PD_OBJ_SIZE,
> +						PD_DMSG_REQUEST, false, SOP);
> +}
> +

	HTH
		Oliver

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  2:14 [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao
  2016-07-15  6:31 ` Oliver Neukum
@ 2016-07-15  7:25 ` Felipe Balbi
  2016-07-15  8:44   ` Oliver Neukum
  2016-07-15 23:49   ` Bin Gao
  2016-07-15 10:38 ` Felipe Balbi
  2 siblings, 2 replies; 18+ messages in thread
From: Felipe Balbi @ 2016-07-15  7:25 UTC (permalink / raw)
  To: Bin Gao, Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel
  Cc: Bin Gao, Chandra Sekhar Anagani

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

Bin Gao <bin.gao@linux.intel.com> writes:

> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao <bin.gao@intel.com>
> ---
>  drivers/usb/typec/Kconfig      |  13 +
>  drivers/usb/typec/Makefile     |   1 +
>  drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/pd_message.h | 371 ++++++++++++++++
>  include/linux/usb/pd_sink.h    | 286 ++++++++++++
>  5 files changed, 1638 insertions(+)
>  create mode 100644 drivers/usb/typec/pd_sink.c
>  create mode 100644 include/linux/usb/pd_message.h
>  create mode 100644 include/linux/usb/pd_sink.h
>
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 7a345a4..a04a900 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
>  config TYPEC
>  	tristate
>  
> +config USB_PD_SINK
> +	bool "USB Power Delivery Sink Port State Machine Driver"

tristate?

> +	select TYPEC

this should depend on TYPEC, not select it.

> +	help
> +	  Enable this to support USB PD(Power Delivery) Sink port.
> +	  This driver implements a simple USB PD sink state machine.
> +	  The underlying TypeC phy driver is responsible for cable
> +	  plug/unplug event, port orientation detection, transmitting
> +	  and receiving PD messages. This driver only process messages
> +	  received by the TypeC phy driver and maintain the sink port's
> +	  state machine.
> +
>  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
> +	select USB_PD_SINK

TYPEC without PD is valid, let user select PD support.

> diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
> new file mode 100644
> index 0000000..374bdef
> --- /dev/null
> +++ b/drivers/usb/typec/pd_sink.c
> @@ -0,0 +1,967 @@
> +/*
> + * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
> + *
> + * This driver implements a simple USB PD sink port state machine.
> + * It assumes the upper layer, i.e. the user of this driver, handles
> + * the PD message receiving and transmitting. The upper layer receives
> + * PD messages from the Source, queues them to us, and when processing
> + * the received message we'll call upper layer's transmitting function
> + * to send PD messages to the source.
> + * The sink port state machine is maintained in this driver but we also
> + * broadcast some important PD messages to upper layer as events.
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Bin Gao <bin.gao@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/usb/pd_sink.h>
> +
> +#define MAKE_HEADER(port, header, msg, objs) \
> +do { \
> +	header->type = msg; \
> +	header->data_role = PD_DATA_ROLE_UFP; \
> +	header->revision = port->pd_rev; \
> +	header->power_role = PD_POWER_ROLE_SINK; \
> +	header->id = roll_msg_id(port); \
> +	header->nr_objs = objs; \
> +	header->extended = PD_MSG_NOT_EXTENDED; \
> +} while (0)
> +
> +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> +static int nr_ports;
> +
> +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> +
> +static char *state_strings[] = {
> +	"WAIT_FOR_SOURCE_CAPABILITY",
> +	"REQUEST_SENT",
> +	"ACCEPT_RECEIVED",
> +	"POWER_SUPPLY_READY",
> +};
> +
> +/* Control messages */
> +static char *cmsg_strings[] = {
> +	"GOODCRC",			/* 1 */
> +	"GOTOMIN",			/* 2 */
> +	"ACCEPT",			/* 3 */
> +	"REJECT",			/* 4 */
> +	"PING",				/* 5 */
> +	"PS_RDY",			/* 6 */
> +	"GET_SRC_CAP",			/* 7 */
> +	"GET_SINK_CAP",			/* 8 */
> +	"DR_SWAP",			/* 9 */
> +	"PR_SWAP",			/* 10 */
> +	"VCONN_SWAP",			/* 11 */
> +	"WAIT",				/* 12 */
> +	"SOFT_RESET",			/* 13 */
> +	"RESERVED",			/* 14 */
> +	"RESERVED",			/* 15 */
> +	"NOT_SUPPORTED",		/* 16 */
> +	"GET_SOURCE_CAP_EXTENDED",	/* 17 */
> +	"GET_STATUS",			/* 18 */
> +	"FR_SWAP",			/* 19 */
> +	/* RESERVED			20 - 31 */
> +};
> +
> +/* Data messages */
> +static char *dmsg_strings[] = {
> +	"SOURCE_CAP",		/* 1 */
> +	"REQUEST",		/* 2 */
> +	"BIST",			/* 3 */
> +	"SINK_CAP",		/* 4 */
> +	"BATTERY_STATUS",	/* 5 */
> +	"ALERT",		/* 6 */
> +	"RESERVED",		/* 7 */
> +	"RESERVED",		/* 8 */
> +	"RESERVED",		/* 9 */
> +	"RESERVED",		/* 10 */
> +	"RESERVED",		/* 11 */
> +	"RESERVED",		/* 12 */
> +	"RESERVED",		/* 13 */
> +	"RESERVED",		/* 14 */
> +	"VENDOR_DEFINED",	/* 15 */
> +	/* RESERVED		16 - 31 */
> +};
> +
> +static char *state_to_string(enum pd_sink_state state)
> +{
> +	if (state < ARRAY_SIZE(state_strings))
> +		return state_strings[state];
> +	else
> +		return NULL;
> +}
> +
> +static char *msg_to_string(bool is_cmsg, u8 msg)
> +{
> +	int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) : ARRAY_SIZE(dmsg_strings);
> +
> +	if (msg <= nr)
> +		return is_cmsg ? cmsg_strings[msg - 1] : dmsg_strings[msg - 1];
> +	else
> +		return "RESERVED";
> +}
> +
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +	pr_info("sink port %d: %s message %s %s\n", port,
> +				is_cmsg ? "Control" : "Data",
> +				msg_to_string(is_cmsg, msg),
> +		 recv ? "received" : "sent(wait GOODCRC)");

looks like a debugging message to me. We don't want to spam dmesg with
every single message transmission.

> +static void start_timer(struct pd_sink_port *port, int timeout,
> +		enum hrtimer_restart (*f)(struct hrtimer *))
> +{
> +	if (hrtimer_active(&port->tx_timer)) {
> +		pr_err("Error: previous timer is still active\n");
> +		return;
> +	}
> +
> +	port->tx_timer.function = f;
> +	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> +	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,

I don't think you need HR timers here. A normal mod_timer() should do.

> +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> +{
> +	pr_err("GOODCRC message is not received in %d ms: timeout\n",
> +						PD_TIMEOUT_GOODCRC);
> +	return HRTIMER_NORESTART;
> +}
> +
> +/*
> + * For any message we send, we must get a GOODCRC message from the Source.
> + * The USB PD spec says the time should be measured between the last bit
> + * of the sending message's EOP has been transmitted and the last bit of
> + * the receiving GOODCRC message's EOP has been received. The allowed time
> + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> + * performed in physical layer. When it reaches to the OS and this driver,
> + * the actual time is difficult to predict because of the scheduling,
> + * context switch, interrupt preemption and nesting, etc. So we only define
> + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> + * account of all software related latency.

that's true. But here's the thing. From 1ms to 500ms there are two
orders of magnitude. If we take that long to realize we received
GoodCRC, it's already too late. Remember that if we don't e.g. request
power within 30ms after reception of GoodCRC, Source will issue a
HardReset.

> + */
> +static int send_message(struct pd_sink_port *port, void *buf, int len,
> +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> +{
> +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +		print_message(port->port, true, msg, false);
> +		return 0;
> +	}
> +
> +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +	print_message(port->port, ctrl_msg, msg, false);
> +
> +	if (!port->hw_goodcrc_rx) {
> +		port->waiting_goodcrc = true;
> +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);

Every port waits for goodcrc, it's just that some will receive it and
process it in SW, while others will just notify of its reception.

> +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> +{
> +	unsigned long flags;
> +	struct pd_sink_port *port;
> +
> +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> +		pr_err("Invalid port number\n");
> +		return -EINVAL;
> +	}
> +
> +	port = sink_ports[msg->port];
> +
> +	spin_lock_irqsave(&port->rx_lock, flags);
> +	list_add_tail(&msg->list, &port->rx_list);
> +	spin_unlock_irqrestore(&port->rx_lock, flags);
> +
> +	queue_work(port->rx_wq, &port->rx_work);

can we really queue several messages at a time? It seems unfeasible to
me. It's not like we can queue several power request in a role. Why do
you need this workqueue? Why don't you process message here, in place?

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  7:25 ` Felipe Balbi
@ 2016-07-15  8:44   ` Oliver Neukum
  2016-07-15 10:30     ` Felipe Balbi
  2016-07-15 23:49   ` Bin Gao
  1 sibling, 1 reply; 18+ messages in thread
From: Oliver Neukum @ 2016-07-15  8:44 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Bin Gao, Heikki Krogerus, Greg Kroah-Hartman, linux-kernel,
	linux-usb, Bin Gao, Chandra Sekhar Anagani

On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > +     unsigned long flags;
> > +     struct pd_sink_port *port;
> > +
> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > +             pr_err("Invalid port number\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     port = sink_ports[msg->port];
> > +
> > +     spin_lock_irqsave(&port->rx_lock, flags);
> > +     list_add_tail(&msg->list, &port->rx_list);
> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > +     queue_work(port->rx_wq, &port->rx_work);
> 
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?

A reset can come at any time.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  8:44   ` Oliver Neukum
@ 2016-07-15 10:30     ` Felipe Balbi
  2016-07-15 13:13       ` Felipe Balbi
  0 siblings, 1 reply; 18+ messages in thread
From: Felipe Balbi @ 2016-07-15 10:30 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Bin Gao, Heikki Krogerus, Greg Kroah-Hartman, linux-kernel,
	linux-usb, Bin Gao, Chandra Sekhar Anagani

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


Hi,

Oliver Neukum <oneukum@suse.com> writes:
> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > +     unsigned long flags;
>> > +     struct pd_sink_port *port;
>> > +
>> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > +             pr_err("Invalid port number\n");
>> > +             return -EINVAL;
>> > +     }
>> > +
>> > +     port = sink_ports[msg->port];
>> > +
>> > +     spin_lock_irqsave(&port->rx_lock, flags);
>> > +     list_add_tail(&msg->list, &port->rx_list);
>> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > +     queue_work(port->rx_wq, &port->rx_work);
>> 
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
>
> A reset can come at any time.

right, but that's not how this is being used. IMHO, rx_work is a
misnomer. If you look at how typec_wcove (patch 2 in this series) uses
it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
message that was *already* received. We can't have two replies, right?

In any case, this is a minor problem.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  2:14 [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao
  2016-07-15  6:31 ` Oliver Neukum
  2016-07-15  7:25 ` Felipe Balbi
@ 2016-07-15 10:38 ` Felipe Balbi
  2016-07-15 11:11   ` Greg Kroah-Hartman
  2 siblings, 1 reply; 18+ messages in thread
From: Felipe Balbi @ 2016-07-15 10:38 UTC (permalink / raw)
  To: Bin Gao, Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel
  Cc: Bin Gao, Chandra Sekhar Anagani

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


Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +	pr_info("sink port %d: %s message %s %s\n", port,
> +				is_cmsg ? "Control" : "Data",
> +				msg_to_string(is_cmsg, msg),
> +		 recv ? "received" : "sent(wait GOODCRC)");
> +}

this is problematic. By default, we're all using 115200 8N1 baud
rate. This message alone prints anywhere from 50 to 100 characters (I
didn't really count properly, these are rough numbers), and that takes:

n50chars_time = 50 / (115200 / 10) = 4.3ms
n100chars_time = 100 / (115200 / 10) = 8.6ms

Considering you have 30ms to reply with Power Request after GoodCRC, and
considering you're printing several of these messages, they become
really expensive and eat up valuable time from tSenderReply.

This should really be a pr_debug() or, better yet, a tracepoint.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 10:38 ` Felipe Balbi
@ 2016-07-15 11:11   ` Greg Kroah-Hartman
  2016-07-15 11:21     ` Felipe Balbi
  0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2016-07-15 11:11 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Bin Gao, Heikki Krogerus, linux-usb, linux-kernel, Bin Gao,
	Chandra Sekhar Anagani

On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao <bin.gao@linux.intel.com> writes:
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > +				is_cmsg ? "Control" : "Data",
> > +				msg_to_string(is_cmsg, msg),
> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > +}
> 
> this is problematic. By default, we're all using 115200 8N1 baud
> rate. This message alone prints anywhere from 50 to 100 characters (I
> didn't really count properly, these are rough numbers), and that takes:
> 
> n50chars_time = 50 / (115200 / 10) = 4.3ms
> n100chars_time = 100 / (115200 / 10) = 8.6ms
> 
> Considering you have 30ms to reply with Power Request after GoodCRC, and
> considering you're printing several of these messages, they become
> really expensive and eat up valuable time from tSenderReply.

printk() should be async, so it shouldn't be that big of a deal.

What is wrong is that this isn't using dev_info().

> This should really be a pr_debug() or, better yet, a tracepoint.

Yes, that would be best (dev_dbg() or a tracepoint.)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 11:11   ` Greg Kroah-Hartman
@ 2016-07-15 11:21     ` Felipe Balbi
  2016-07-15 22:41       ` Bin Gao
  0 siblings, 1 reply; 18+ messages in thread
From: Felipe Balbi @ 2016-07-15 11:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Bin Gao, Heikki Krogerus, linux-usb, linux-kernel, Bin Gao,
	Chandra Sekhar Anagani

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


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Bin Gao <bin.gao@linux.intel.com> writes:
>> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
>> > +{
>> > +	pr_info("sink port %d: %s message %s %s\n", port,
>> > +				is_cmsg ? "Control" : "Data",
>> > +				msg_to_string(is_cmsg, msg),
>> > +		 recv ? "received" : "sent(wait GOODCRC)");
>> > +}
>> 
>> this is problematic. By default, we're all using 115200 8N1 baud
>> rate. This message alone prints anywhere from 50 to 100 characters (I
>> didn't really count properly, these are rough numbers), and that takes:
>> 
>> n50chars_time = 50 / (115200 / 10) = 4.3ms
>> n100chars_time = 100 / (115200 / 10) = 8.6ms
>> 
>> Considering you have 30ms to reply with Power Request after GoodCRC, and
>> considering you're printing several of these messages, they become
>> really expensive and eat up valuable time from tSenderReply.
>
> printk() should be async, so it shouldn't be that big of a deal.

I can actually see this causing problems ;-) With this pr_info(),
sometimes tSenderReply times out and Source gives a HardReset. Without
pr_info(), type-c analyzer tells me we reply in less than 1ms.

> What is wrong is that this isn't using dev_info().

right, that too.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 10:30     ` Felipe Balbi
@ 2016-07-15 13:13       ` Felipe Balbi
  0 siblings, 0 replies; 18+ messages in thread
From: Felipe Balbi @ 2016-07-15 13:13 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Bin Gao, Heikki Krogerus, Greg Kroah-Hartman, linux-kernel,
	linux-usb, Bin Gao, Chandra Sekhar Anagani

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


Hi again,

Felipe Balbi <felipe.balbi@linux.intel.com> writes:
> Oliver Neukum <oneukum@suse.com> writes:
>> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>>> > +{
>>> > +     unsigned long flags;
>>> > +     struct pd_sink_port *port;
>>> > +
>>> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>>> > +             pr_err("Invalid port number\n");
>>> > +             return -EINVAL;
>>> > +     }
>>> > +
>>> > +     port = sink_ports[msg->port];
>>> > +
>>> > +     spin_lock_irqsave(&port->rx_lock, flags);
>>> > +     list_add_tail(&msg->list, &port->rx_list);
>>> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
>>> > +
>>> > +     queue_work(port->rx_wq, &port->rx_work);
>>> 
>>> can we really queue several messages at a time? It seems unfeasible to
>>> me. It's not like we can queue several power request in a role. Why do
>>> you need this workqueue? Why don't you process message here, in place?
>>
>> A reset can come at any time.
>
> right, but that's not how this is being used. IMHO, rx_work is a
> misnomer. If you look at how typec_wcove (patch 2 in this series) uses
> it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
> message that was *already* received. We can't have two replies, right?
>
> In any case, this is a minor problem.

oh wait, it's not a minor problem. If CPU is busy, this workqueue might
take longer than 30ms to get scheduled. This is another problem I just
reproduced, even after changing that pr_info() in print_message() to a
pr_debug().

Everything worked fine when I called rx_msg_worker() directly, instead
of queueing it to the workqueue.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  6:31 ` Oliver Neukum
@ 2016-07-15 22:33   ` Bin Gao
  0 siblings, 0 replies; 18+ messages in thread
From: Bin Gao @ 2016-07-15 22:33 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-kernel, linux-usb,
	Bin Gao, Chandra Sekhar Anagani

On Fri, Jul 15, 2016 at 08:31:08AM +0200, Oliver Neukum wrote:
> > +static void ack_message(struct pd_sink_port *port, int msg_id)
> > +{
> > +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
> 
> This must be GFP_NOIO. We are in a cycle that can lead to deadlock.
> 
> Assume we are waiting for a request for more power to process IO
> which we need to ack.
> 
> 1. memory allocation leads to laundering, blocks on freeing memory
> 2. launderer decides to perform IO which needs more power
> 3. more power has already been requested, wait for it to be granted
> 
> 4. BANG - DEADLOCK
Agree, I'll change the GFP flag in next revision.

> > +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> > +		port->nr_ps * PD_OBJ_SIZE, GFP_KERNEL);
> 
> Must be GFP_NOIO. For the same reason as above. We may be asked
> this to resolve a mismatch due to needing more power for IO.
Yes will do.

> > +static void handle_soft_reset(struct pd_sink_port *port)
> > +{
> > +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
> > +
> > +	if (!header)
> > +		return;
> > +
> > +	flush_workqueue(port->rx_wq);
> 
> That is problematic. We may be here precisely because something is wrong
> blocking progress. In particular what happens if another soft reset
> is queued?
I'm going to remove the workqueue.

> > +	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> > +					PD_OBJ_SIZE, GFP_KERNEL);
> 
> GFP_NOIO, same reasons
Yes.

> > +
> 
> 	HTH
> 		Oliver
Thanks for your review.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 11:21     ` Felipe Balbi
@ 2016-07-15 22:41       ` Bin Gao
  2016-07-15 23:49         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 18+ messages in thread
From: Bin Gao @ 2016-07-15 22:41 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Greg Kroah-Hartman, Heikki Krogerus, linux-usb, linux-kernel,
	Bin Gao, Chandra Sekhar Anagani

On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> >> 
> >> Hi,
> >> 
> >> Bin Gao <bin.gao@linux.intel.com> writes:
> >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> >> > +{
> >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> >> > +				is_cmsg ? "Control" : "Data",
> >> > +				msg_to_string(is_cmsg, msg),
> >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> >> > +}
> >> 
> >> this is problematic. By default, we're all using 115200 8N1 baud
> >> rate. This message alone prints anywhere from 50 to 100 characters (I
> >> didn't really count properly, these are rough numbers), and that takes:
> >> 
> >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> >> 
> >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> >> considering you're printing several of these messages, they become
> >> really expensive and eat up valuable time from tSenderReply.
> >
> > printk() should be async, so it shouldn't be that big of a deal.
> 
> I can actually see this causing problems ;-) With this pr_info(),
> sometimes tSenderReply times out and Source gives a HardReset. Without
> pr_info(), type-c analyzer tells me we reply in less than 1ms.
> 
> > What is wrong is that this isn't using dev_info().
> 
> right, that too.
> 
> -- 
> balbi

When we don't have a struct device pointer for this driver,
a dev_info(NULL, fmt, ...) is equivalent to pr_info(). So we have to
use dev_info() here?
But I agree at least it should be pr_debug().

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 22:41       ` Bin Gao
@ 2016-07-15 23:49         ` Greg Kroah-Hartman
  2016-07-19  5:30           ` Bin Gao
  0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2016-07-15 23:49 UTC (permalink / raw)
  To: Bin Gao
  Cc: Felipe Balbi, Heikki Krogerus, linux-usb, linux-kernel, Bin Gao,
	Chandra Sekhar Anagani

On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > >> 
> > >> Hi,
> > >> 
> > >> Bin Gao <bin.gao@linux.intel.com> writes:
> > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > >> > +{
> > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > >> > +				is_cmsg ? "Control" : "Data",
> > >> > +				msg_to_string(is_cmsg, msg),
> > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > >> > +}
> > >> 
> > >> this is problematic. By default, we're all using 115200 8N1 baud
> > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > >> didn't really count properly, these are rough numbers), and that takes:
> > >> 
> > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > >> 
> > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> > >> considering you're printing several of these messages, they become
> > >> really expensive and eat up valuable time from tSenderReply.
> > >
> > > printk() should be async, so it shouldn't be that big of a deal.
> > 
> > I can actually see this causing problems ;-) With this pr_info(),
> > sometimes tSenderReply times out and Source gives a HardReset. Without
> > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> > 
> > > What is wrong is that this isn't using dev_info().
> > 
> > right, that too.
> > 
> > -- 
> > balbi
> 
> When we don't have a struct device pointer for this driver,

Then you should fix that, as this is a driver for hardware :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15  7:25 ` Felipe Balbi
  2016-07-15  8:44   ` Oliver Neukum
@ 2016-07-15 23:49   ` Bin Gao
  2016-07-18  7:07     ` Felipe Balbi
  1 sibling, 1 reply; 18+ messages in thread
From: Bin Gao @ 2016-07-15 23:49 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Bin Gao, Chandra Sekhar Anagani

On Fri, Jul 15, 2016 at 10:25:36AM +0300, Felipe Balbi wrote:
> Bin Gao <bin.gao@linux.intel.com> writes:
> 
> > This patch implements a simple USB Power Delivery sink port state machine.
> > It assumes the hardware only handles PD packet transmitting and receiving
> > over the CC line of the USB Type-C connector. The state transition is
> > completely controlled by software. This patch only implement the sink port
> > function and it doesn't support source port and port swap yet.
> >
> > This patch depends on these two patches:
> > https://lkml.org/lkml/2016/6/29/349
> > https://lkml.org/lkml/2016/6/29/350
> >
> > Signed-off-by: Bin Gao <bin.gao@intel.com>
> > ---
> >  drivers/usb/typec/Kconfig      |  13 +
> >  drivers/usb/typec/Makefile     |   1 +
> >  drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/pd_message.h | 371 ++++++++++++++++
> >  include/linux/usb/pd_sink.h    | 286 ++++++++++++
> >  5 files changed, 1638 insertions(+)
> >  create mode 100644 drivers/usb/typec/pd_sink.c
> >  create mode 100644 include/linux/usb/pd_message.h
> >  create mode 100644 include/linux/usb/pd_sink.h
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 7a345a4..a04a900 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
> >  config TYPEC
> >  	tristate
> >  
> > +config USB_PD_SINK
> > +	bool "USB Power Delivery Sink Port State Machine Driver"
> 
> tristate?
> 
> > +	select TYPEC
> 
> this should depend on TYPEC, not select it.
> 
> > +	help
> > +	  Enable this to support USB PD(Power Delivery) Sink port.
> > +	  This driver implements a simple USB PD sink state machine.
> > +	  The underlying TypeC phy driver is responsible for cable
> > +	  plug/unplug event, port orientation detection, transmitting
> > +	  and receiving PD messages. This driver only process messages
> > +	  received by the TypeC phy driver and maintain the sink port's
> > +	  state machine.
> > +
> >  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
> > +	select USB_PD_SINK
> 
> TYPEC without PD is valid, let user select PD support.
Yes will fix the Kconfig in next revision.

> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > +				is_cmsg ? "Control" : "Data",
> > +				msg_to_string(is_cmsg, msg),
> > +		 recv ? "received" : "sent(wait GOODCRC)");
> 
> looks like a debugging message to me. We don't want to spam dmesg with
> every single message transmission.
This should be a pr_debug().

> 
> > +static void start_timer(struct pd_sink_port *port, int timeout,
> > +		enum hrtimer_restart (*f)(struct hrtimer *))
> > +{
> > +	if (hrtimer_active(&port->tx_timer)) {
> > +		pr_err("Error: previous timer is still active\n");
> > +		return;
> > +	}
> > +
> > +	port->tx_timer.function = f;
> > +	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> > +	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,
> 
> I don't think you need HR timers here. A normal mod_timer() should do.
When hrtimer is in place, the old timer becomes "legacy". And the old timer
APIs are implemented on top of hrtimer. It's no harm to use hrtimers anywhere
in the kernel and it would be encouraged in my opinion:-)

> 
> > +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> > +{
> > +	pr_err("GOODCRC message is not received in %d ms: timeout\n",
> > +						PD_TIMEOUT_GOODCRC);
> > +	return HRTIMER_NORESTART;
> > +}
> > +
> > +/*
> > + * For any message we send, we must get a GOODCRC message from the Source.
> > + * The USB PD spec says the time should be measured between the last bit
> > + * of the sending message's EOP has been transmitted and the last bit of
> > + * the receiving GOODCRC message's EOP has been received. The allowed time
> > + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> > + * performed in physical layer. When it reaches to the OS and this driver,
> > + * the actual time is difficult to predict because of the scheduling,
> > + * context switch, interrupt preemption and nesting, etc. So we only define
> > + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> > + * account of all software related latency.
> 
> that's true. But here's the thing. From 1ms to 500ms there are two
> orders of magnitude. If we take that long to realize we received
> GoodCRC, it's already too late. Remember that if we don't e.g. request
> power within 30ms after reception of GoodCRC, Source will issue a
> HardReset.
This part is for sure to be revisited even in the beginning of the design.
The hardware(Type-C PHY) we tested on forces auto GOODCRC transmission
and reception and doesn't report to software so we actually have no way
to test it. But any way this timeout value will be rivisited. 

> 
> > + */
> > +static int send_message(struct pd_sink_port *port, void *buf, int len,
> > +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> > +{
> > +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> > +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +		print_message(port->port, true, msg, false);
> > +		return 0;
> > +	}
> > +
> > +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +	print_message(port->port, ctrl_msg, msg, false);
> > +
> > +	if (!port->hw_goodcrc_rx) {
> > +		port->waiting_goodcrc = true;
> > +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);
> 
> Every port waits for goodcrc, it's just that some will receive it and
> process it in SW, while others will just notify of its reception.
> 
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > +	unsigned long flags;
> > +	struct pd_sink_port *port;
> > +
> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > +		pr_err("Invalid port number\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	port = sink_ports[msg->port];
> > +
> > +	spin_lock_irqsave(&port->rx_lock, flags);
> > +	list_add_tail(&msg->list, &port->rx_list);
> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > +	queue_work(port->rx_wq, &port->rx_work);
> 
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?
Some Type-C chargers send two messages in a short duration(less than 1 ms),
e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
message to PD stack by Type-C phy driver typically happens in a interrupt
context. So in this case a nested interrupt may happen. Our whole PD
stack while processing one message is not re-entrant so the nested
interrupt would cause a problem.
But I'll re-check and see if it will really happen.
Yes I agree the workqueue will introduce delay so we could miss the
tSenderResponse timeout deadline.
Most likey I'll remove the workqueue and directly process message one
bye one in the next revision.

> 
> -- 
> balbi
Thanks for your review.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 23:49   ` Bin Gao
@ 2016-07-18  7:07     ` Felipe Balbi
  2016-07-19  5:39       ` Bin Gao
  0 siblings, 1 reply; 18+ messages in thread
From: Felipe Balbi @ 2016-07-18  7:07 UTC (permalink / raw)
  To: Bin Gao
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Bin Gao, Chandra Sekhar Anagani

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


Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > +	unsigned long flags;
>> > +	struct pd_sink_port *port;
>> > +
>> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > +		pr_err("Invalid port number\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	port = sink_ports[msg->port];
>> > +
>> > +	spin_lock_irqsave(&port->rx_lock, flags);
>> > +	list_add_tail(&msg->list, &port->rx_list);
>> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > +	queue_work(port->rx_wq, &port->rx_work);
>> 
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
> Some Type-C chargers send two messages in a short duration(less than 1 ms),
> e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> message to PD stack by Type-C phy driver typically happens in a interrupt
> context. So in this case a nested interrupt may happen. Our whole PD
> stack while processing one message is not re-entrant so the nested
> interrupt would cause a problem.

keep interrupts masked for as long as necessary until your message is
processed.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-15 23:49         ` Greg Kroah-Hartman
@ 2016-07-19  5:30           ` Bin Gao
  2016-07-19  8:30             ` Felipe Balbi
  0 siblings, 1 reply; 18+ messages in thread
From: Bin Gao @ 2016-07-19  5:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Felipe Balbi, Heikki Krogerus, linux-usb, linux-kernel, Bin Gao,
	Chandra Sekhar Anagani

On Sat, Jul 16, 2016 at 08:49:53AM +0900, Greg Kroah-Hartman wrote:
> On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> > On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > > >> 
> > > >> Hi,
> > > >> 
> > > >> Bin Gao <bin.gao@linux.intel.com> writes:
> > > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > > >> > +{
> > > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > > >> > +				is_cmsg ? "Control" : "Data",
> > > >> > +				msg_to_string(is_cmsg, msg),
> > > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > > >> > +}
> > > >> 
> > > >> this is problematic. By default, we're all using 115200 8N1 baud
> > > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > > >> didn't really count properly, these are rough numbers), and that takes:
> > > >> 
> > > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > > >> 
> > > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> > > >> considering you're printing several of these messages, they become
> > > >> really expensive and eat up valuable time from tSenderReply.
> > > >
> > > > printk() should be async, so it shouldn't be that big of a deal.
> > > 
> > > I can actually see this causing problems ;-) With this pr_info(),
> > > sometimes tSenderReply times out and Source gives a HardReset. Without
> > > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> > > 
> > > > What is wrong is that this isn't using dev_info().
> > > 
> > > right, that too.
> > > 
> > > -- 
> > > balbi
> > 
> > When we don't have a struct device pointer for this driver,
> 
> Then you should fix that, as this is a driver for hardware :)
This is actualy a software stack to implement the USB PD spec.
Only the USB Type-C phy driver has a device pointer.
The PD stack vs. USB Type-C phy driver is similar to TCP/IP stack
vs. ethernet driver in the kernel. We don't have a device pointer
for TCP/IP stack code either.

Thanks,
Bin

> 
> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-18  7:07     ` Felipe Balbi
@ 2016-07-19  5:39       ` Bin Gao
  2016-07-19  8:29         ` Felipe Balbi
  0 siblings, 1 reply; 18+ messages in thread
From: Bin Gao @ 2016-07-19  5:39 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Bin Gao, Chandra Sekhar Anagani

On Mon, Jul 18, 2016 at 10:07:24AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao <bin.gao@linux.intel.com> writes:
> >> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> >> > +{
> >> > +	unsigned long flags;
> >> > +	struct pd_sink_port *port;
> >> > +
> >> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> >> > +		pr_err("Invalid port number\n");
> >> > +		return -EINVAL;
> >> > +	}
> >> > +
> >> > +	port = sink_ports[msg->port];
> >> > +
> >> > +	spin_lock_irqsave(&port->rx_lock, flags);
> >> > +	list_add_tail(&msg->list, &port->rx_list);
> >> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
> >> > +
> >> > +	queue_work(port->rx_wq, &port->rx_work);
> >> 
> >> can we really queue several messages at a time? It seems unfeasible to
> >> me. It's not like we can queue several power request in a role. Why do
> >> you need this workqueue? Why don't you process message here, in place?
> > Some Type-C chargers send two messages in a short duration(less than 1 ms),
> > e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> > GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> > message to PD stack by Type-C phy driver typically happens in a interrupt
> > context. So in this case a nested interrupt may happen. Our whole PD
> > stack while processing one message is not re-entrant so the nested
> > interrupt would cause a problem.
> 
> keep interrupts masked for as long as necessary until your message is
> processed.

Yes, that's a right way to go. 
We'll have to document this because there might be other Type-C
PHY drivers(other than Intel Whiskey Cove PMIC) to use the PD stack.

> 
> -- 
> balbi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-19  5:39       ` Bin Gao
@ 2016-07-19  8:29         ` Felipe Balbi
  0 siblings, 0 replies; 18+ messages in thread
From: Felipe Balbi @ 2016-07-19  8:29 UTC (permalink / raw)
  To: Bin Gao
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Bin Gao, Chandra Sekhar Anagani

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


Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> On Mon, Jul 18, 2016 at 10:07:24AM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Bin Gao <bin.gao@linux.intel.com> writes:
>> >> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> >> > +{
>> >> > +	unsigned long flags;
>> >> > +	struct pd_sink_port *port;
>> >> > +
>> >> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> >> > +		pr_err("Invalid port number\n");
>> >> > +		return -EINVAL;
>> >> > +	}
>> >> > +
>> >> > +	port = sink_ports[msg->port];
>> >> > +
>> >> > +	spin_lock_irqsave(&port->rx_lock, flags);
>> >> > +	list_add_tail(&msg->list, &port->rx_list);
>> >> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
>> >> > +
>> >> > +	queue_work(port->rx_wq, &port->rx_work);
>> >> 
>> >> can we really queue several messages at a time? It seems unfeasible to
>> >> me. It's not like we can queue several power request in a role. Why do
>> >> you need this workqueue? Why don't you process message here, in place?
>> > Some Type-C chargers send two messages in a short duration(less than 1 ms),
>> > e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
>> > GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
>> > message to PD stack by Type-C phy driver typically happens in a interrupt
>> > context. So in this case a nested interrupt may happen. Our whole PD
>> > stack while processing one message is not re-entrant so the nested
>> > interrupt would cause a problem.
>> 
>> keep interrupts masked for as long as necessary until your message is
>> processed.
>
> Yes, that's a right way to go. 
> We'll have to document this because there might be other Type-C
> PHY drivers(other than Intel Whiskey Cove PMIC) to use the PD stack.

that's a requirement for *any* driver. You _must_ keep interrupts masked
(or disabled) while you're processing the interrupts themselves.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
  2016-07-19  5:30           ` Bin Gao
@ 2016-07-19  8:30             ` Felipe Balbi
  0 siblings, 0 replies; 18+ messages in thread
From: Felipe Balbi @ 2016-07-19  8:30 UTC (permalink / raw)
  To: Bin Gao, Greg Kroah-Hartman
  Cc: Heikki Krogerus, linux-usb, linux-kernel, Bin Gao,
	Chandra Sekhar Anagani

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


Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> On Sat, Jul 16, 2016 at 08:49:53AM +0900, Greg Kroah-Hartman wrote:
>> On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
>> > On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
>> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> > > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>> > > >> 
>> > > >> Hi,
>> > > >> 
>> > > >> Bin Gao <bin.gao@linux.intel.com> writes:
>> > > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
>> > > >> > +{
>> > > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
>> > > >> > +				is_cmsg ? "Control" : "Data",
>> > > >> > +				msg_to_string(is_cmsg, msg),
>> > > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
>> > > >> > +}
>> > > >> 
>> > > >> this is problematic. By default, we're all using 115200 8N1 baud
>> > > >> rate. This message alone prints anywhere from 50 to 100 characters (I
>> > > >> didn't really count properly, these are rough numbers), and that takes:
>> > > >> 
>> > > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
>> > > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
>> > > >> 
>> > > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
>> > > >> considering you're printing several of these messages, they become
>> > > >> really expensive and eat up valuable time from tSenderReply.
>> > > >
>> > > > printk() should be async, so it shouldn't be that big of a deal.
>> > > 
>> > > I can actually see this causing problems ;-) With this pr_info(),
>> > > sometimes tSenderReply times out and Source gives a HardReset. Without
>> > > pr_info(), type-c analyzer tells me we reply in less than 1ms.
>> > > 
>> > > > What is wrong is that this isn't using dev_info().
>> > > 
>> > > right, that too.
>> > > 
>> > > -- 
>> > > balbi
>> > 
>> > When we don't have a struct device pointer for this driver,
>> 
>> Then you should fix that, as this is a driver for hardware :)
> This is actualy a software stack to implement the USB PD spec.
> Only the USB Type-C phy driver has a device pointer.

what Greg is saying is that you should register yourself to the PD stack
with something that passes along a pointer to the actual device.

> The PD stack vs. USB Type-C phy driver is similar to TCP/IP stack
> vs. ethernet driver in the kernel. We don't have a device pointer
> for TCP/IP stack code either.

This is the wrong analogy.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-07-19  8:32 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-15  2:14 [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao
2016-07-15  6:31 ` Oliver Neukum
2016-07-15 22:33   ` Bin Gao
2016-07-15  7:25 ` Felipe Balbi
2016-07-15  8:44   ` Oliver Neukum
2016-07-15 10:30     ` Felipe Balbi
2016-07-15 13:13       ` Felipe Balbi
2016-07-15 23:49   ` Bin Gao
2016-07-18  7:07     ` Felipe Balbi
2016-07-19  5:39       ` Bin Gao
2016-07-19  8:29         ` Felipe Balbi
2016-07-15 10:38 ` Felipe Balbi
2016-07-15 11:11   ` Greg Kroah-Hartman
2016-07-15 11:21     ` Felipe Balbi
2016-07-15 22:41       ` Bin Gao
2016-07-15 23:49         ` Greg Kroah-Hartman
2016-07-19  5:30           ` Bin Gao
2016-07-19  8:30             ` Felipe Balbi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.