Skip to contentSkip to conversation

usb: typec: Add USB Power Delivery sink port support

PATCH Superseded Bin Gao 4 participants 18 messages Jul 15, 2016 – Jul 19, 2016

Subsystems (2)
USB USB Type-C class

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Bin Gao

author
#0 lore raw

PATCH 1/2 usb: typec: Add USB Power Delivery sink port support

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 <[email protected]>
---
 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
drivers/usb/typec/Kconfig +13 -0
index 7a345a4..a04a900 100644
@@ -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
drivers/usb/typec/Makefile +1 -0
index b9cb862..a62eb57 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
 obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
+obj-$(CONFIG_USB_PD_SINK)	+= pd_sink.o
drivers/usb/typec/pd_sink.c new file +967 -0

967 lines not on this page · raw message

include/linux/usb/pd_message.h new file +371 -0

371 lines not on this page · raw message

include/linux/usb/pd_sink.h new file +286 -0

286 lines not on this page · raw message

Signature · 1 line
1.9.1
#1 lore raw
Quoted 35 lines · Bin Gao

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.

drivers/usb/typec/pd_sink.c new file +28 -0
+/*
+ * 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);
+	}

drivers/usb/typec/pd_sink.c line 215

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+
+	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

Quoted 93 lines
drivers/usb/typec/pd_sink.c new file +93 -0
+
+	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.
+ */

drivers/usb/typec/pd_sink.c line 318

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+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.

Quoted 75 lines
drivers/usb/typec/pd_sink.c new file +75 -0
+
+	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);

drivers/usb/typec/pd_sink.c line 403

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+}
+
+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?

Quoted 66 lines
drivers/usb/typec/pd_sink.c new file +66 -0
+	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
+ */

drivers/usb/typec/pd_sink.c line 479

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+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

Quoted 37 lines
drivers/usb/typec/pd_sink.c new file +37 -0
+	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 */

drivers/usb/typec/pd_sink.c line 526

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+	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

author
#2 lore raw
Quoted 6 lines · Oliver Neukum

drivers/usb/typec/pd_sink.c line 215

Quoted 3 lines
drivers/usb/typec/pd_sink.c new file +3 -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.

Quoted 8 lines

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.

Quoted 5 lines

drivers/usb/typec/pd_sink.c line 318

Quoted 2 lines
drivers/usb/typec/pd_sink.c new file +2 -0
+	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.

Quoted 4 lines
Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+static void handle_soft_reset(struct pd_sink_port *port)
+{
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
+
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 403

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+	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.

Quoted 4 lines

drivers/usb/typec/pd_sink.c line 479

Quoted 2 lines
drivers/usb/typec/pd_sink.c new file +2 -0
+	struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
+					PD_OBJ_SIZE, GFP_KERNEL);

GFP_NOIO, same reasons

Yes.

Quoted 4 lines
Quoted 1 line

+

HTH
Oliver

Thanks for your review.

#3 lore raw
In reply to Bin Gao
Quoted 27 lines · Bin Gao

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 <[email protected]>
---
 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
drivers/usb/typec/Kconfig +0 -0
index 7a345a4..a04a900 100644

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 8

Quoted 5 lines
drivers/usb/typec/Kconfig +2 -0
@@ -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?

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 9

Quoted 1 line
drivers/usb/typec/Kconfig +1 -0
+	select TYPEC

this should depend on TYPEC, not select it.

Quoted 6 lines
drivers/usb/typec/Kconfig +6 -0
+	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

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 25

Quoted 10 lines
drivers/usb/typec/Kconfig +4 -0
+	  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.

Quoted 113 lines
drivers/usb/typec/pd_sink.c new file +107 -0
new file mode 100644
index 0000000..374bdef
@@ -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 <[email protected]>
+ *
+ * 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];

drivers/usb/typec/pd_sink.c line 117

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+	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.

drivers/usb/typec/pd_sink.c line 162

Quoted 11 lines
drivers/usb/typec/pd_sink.c new file +11 -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,

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

Quoted 8 lines
drivers/usb/typec/pd_sink.c new file +8 -0
+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;
+}
+
+/*

drivers/usb/typec/pd_sink.c line 188

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+ * 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.

Quoted 6 lines
drivers/usb/typec/pd_sink.c new file +6 -0
+ */
+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);

drivers/usb/typec/pd_sink.c line 204

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+		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.

Quoted 7 lines
drivers/usb/typec/pd_sink.c new file +7 -0
+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");

drivers/usb/typec/pd_sink.c line 894

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+		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?

Signature · 1 line
balbi
#4 lore raw
Quoted 13 lines · Felipe Balbi
Quoted 13 lines
drivers/usb/typec/pd_sink.c new file +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 894

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+     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

#5 lore raw

Hi,

Quoted 23 lines · Oliver Neukum
Quoted 13 lines · Felipe Balbi
Quoted 13 lines
drivers/usb/typec/pd_sink.c new file +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 894

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+     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?

Quoted 1 line

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.

Signature · 1 line
balbi
#6 lore raw

Hi again,

Quoted 24 lines · Felipe Balbi
Quoted 23 lines · Oliver Neukum
Quoted 21 lines · Felipe Balbi
Quoted 7 lines
drivers/usb/typec/pd_sink.c new file +7 -0
+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");

drivers/usb/typec/pd_sink.c line 894

Quoted 10 lines
drivers/usb/typec/pd_sink.c new file +10 -0
+             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?

Quoted 8 lines
Quoted 1 line

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.

Signature · 1 line
balbi
author
#7 lore raw
In reply to Felipe Balbi
Quoted 51 lines · Felipe Balbi
Quoted 27 lines · Bin Gao

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 <[email protected]>
---
 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
drivers/usb/typec/Kconfig +0 -0
index 7a345a4..a04a900 100644

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 8

Quoted 5 lines
drivers/usb/typec/Kconfig +2 -0
@@ -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?

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 9

Quoted 1 line
drivers/usb/typec/Kconfig +1 -0
+	select TYPEC

this should depend on TYPEC, not select it.

Quoted 10 lines
drivers/usb/typec/Kconfig +9 -0
+	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
Quoted 8 lines

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers" · line 25

Quoted 6 lines
drivers/usb/typec/Kconfig +1 -0
 	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.

Quoted 9 lines

drivers/usb/typec/Kconfig menu "USB PD and Type-C drivers"

Quoted 6 lines
drivers/usb/typec/Kconfig +6 -0
+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().

Quoted 14 lines

drivers/usb/typec/Kconfig

Quoted 12 lines
drivers/usb/typec/Kconfig +11 -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,

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:-)

Quoted 17 lines
Quoted 17 lines
drivers/usb/typec/Kconfig +16 -0
+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
Quoted 8 lines
Quoted 2 lines

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

Quoted 34 lines
Quoted 7 lines

+ */

drivers/usb/typec/Kconfig +5 -0
+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);

drivers/usb/typec/Kconfig

Quoted 10 lines
drivers/usb/typec/Kconfig +10 -0
+		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.

Quoted 13 lines
drivers/usb/typec/Kconfig +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/Kconfig

Quoted 4 lines
drivers/usb/typec/Kconfig +4 -0
+	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.

Quoted 3 lines
Signature · 2 lines
-- 
balbi

Thanks for your review.

#8 lore raw

Hi,

Quoted 21 lines · Bin Gao
Quoted 13 lines
Quoted 13 lines
drivers/usb/typec/pd_sink.c new file +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 894

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+	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?

Quoted 7 lines

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.

Signature · 1 line
balbi
author
#9 lore raw
Quoted 33 lines · Felipe Balbi

Hi,

Quoted 21 lines · Bin Gao
Quoted 13 lines
Quoted 13 lines
drivers/usb/typec/pd_sink.c new file +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 894

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+	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?

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 3 lines
Signature · 2 lines
-- 
balbi
#10 lore raw

Hi,

Quoted 34 lines · Bin Gao
Quoted 33 lines · Felipe Balbi

Hi,

Quoted 28 lines · Bin Gao
Quoted 13 lines
Quoted 13 lines
drivers/usb/typec/pd_sink.c new file +13 -0
+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);
Quoted 8 lines

drivers/usb/typec/pd_sink.c line 894

Quoted 4 lines
drivers/usb/typec/pd_sink.c new file +4 -0
+	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.

Quoted 6 lines
Quoted 2 lines

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.

Signature · 1 line
balbi
#11 lore raw
In reply to Bin Gao

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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.

Signature · 1 line
balbi
maintainer
#12 lore raw
Quoted 16 lines · Felipe Balbi

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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:

Quoted 6 lines

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

Quoted 1 line

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

#13 lore raw

Hi,

Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Felipe Balbi

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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:

Quoted 8 lines
Quoted 6 lines

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.

Quoted 1 line

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

right, that too.

Signature · 1 line
balbi
author
#14 lore raw
Quoted 27 lines · Felipe Balbi
Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Felipe Balbi

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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:

Quoted 8 lines
Quoted 6 lines

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.

Quoted 10 lines

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.

Quoted 1 line

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

right, that too.

Signature · 1 line
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().

maintainer
#15 lore raw
Quoted 32 lines · Bin Gao
Quoted 31 lines · Felipe Balbi
Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Felipe Balbi

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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:

Quoted 8 lines
Quoted 6 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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

right, that too.

Signature · 1 line
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

author
#16 lore raw
Quoted 35 lines · Greg Kroah-Hartman
Quoted 34 lines · Bin Gao
Quoted 33 lines · Felipe Balbi
Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Felipe Balbi

Hi,

drivers/usb/typec/pd_sink.c line 118

Quoted 7 lines · Bin Gao
drivers/usb/typec/pd_sink.c new file +7 -0
+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:

Quoted 8 lines
Quoted 6 lines

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.

Quoted 1 line

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

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

right, that too.

Signature · 1 line
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

Quoted 4 lines

thanks,

greg k-h

#17 lore raw

Hi,

Quoted 38 lines · Bin Gao
Quoted 37 lines · Greg Kroah-Hartman
Quoted 36 lines · Bin Gao
Quoted 35 lines · Felipe Balbi
Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Felipe Balbi

Hi,

Review

Quoted 7 lines · Bin Gao
(quoted diff) +7 -0
+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:

Quoted 8 lines
Quoted 6 lines

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.

Quoted 1 line

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

right, that too.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Signature · 1 line
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.

Quoted 3 lines

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.

Signature · 1 line
balbi