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

[Felipe Balbi]

Bin Gao <bin.gao@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxx>
> + *
> + * 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: signature.asc
Description: PGP signature