Hi, On Thu, May 25, 2017 at 06:20:16AM -0700, Guenter Roeck wrote: > On 05/16/2017 05:26 AM, Heikki Krogerus wrote: > > UCSI - USB Type-C Connector System Software Interface - is a > > specification that defines set of registers and data > > structures for controlling the USB Type-C ports. It's > > designed for systems where an embedded controller (EC) is in > > charge of the USB Type-C PHY or USB Power Delivery > > controller. It is designed for systems with EC, but it is > > not limited to them, and for example some USB Power Delivery > > controllers will use it as their direct control interface. > > > > With UCSI the EC (or USB PD controller) acts as the port > > manager, implementing all USB Type-C and Power Delivery state > > machines. The OS can use the interfaces for reading the > > status of the ports and controlling basic operations like > > role swapping. > > > > The UCSI specification highlights the fact that it does not > > define the interface method (PCI/I2C/ACPI/etc.). > > Therefore the driver is implemented as library and every > > supported interface method needs its own driver. Driver for > > ACPI is provided in separate patch following this one. > > > > The initial driver includes support for all required > > features from UCSI specification version 1.0 (getting > > connector capabilities and status, and support for power and > > data role swapping), but none of the optional UCSI features > > (alternate modes, power source capabilities, and cable > > capabilities). > > > > Signed-off-by: Heikki Krogerus <heikki.kroge...@linux.intel.com> > > --- > > drivers/usb/typec/Kconfig | 2 + > > drivers/usb/typec/Makefile | 1 + > > drivers/usb/typec/ucsi/Kconfig | 22 ++ > > drivers/usb/typec/ucsi/Makefile | 7 + > > drivers/usb/typec/ucsi/debug.h | 64 ++++ > > drivers/usb/typec/ucsi/trace.c | 2 + > > drivers/usb/typec/ucsi/trace.h | 143 ++++++++ > > drivers/usb/typec/ucsi/ucsi.c | 770 > > ++++++++++++++++++++++++++++++++++++++++ > > drivers/usb/typec/ucsi/ucsi.h | 329 +++++++++++++++++ > > 9 files changed, 1340 insertions(+) > > create mode 100644 drivers/usb/typec/ucsi/Kconfig > > create mode 100644 drivers/usb/typec/ucsi/Makefile > > create mode 100644 drivers/usb/typec/ucsi/debug.h > > create mode 100644 drivers/usb/typec/ucsi/trace.c > > create mode 100644 drivers/usb/typec/ucsi/trace.h > > create mode 100644 drivers/usb/typec/ucsi/ucsi.c > > create mode 100644 drivers/usb/typec/ucsi/ucsi.h > > > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > > index dfcfe459b7cf..bc1b7745f1d4 100644 > > --- a/drivers/usb/typec/Kconfig > > +++ b/drivers/usb/typec/Kconfig > > @@ -19,4 +19,6 @@ config TYPEC_WCOVE > > To compile this driver as module, choose M here: the module will be > > called typec_wcove > > +source "drivers/usb/typec/ucsi/Kconfig" > > + > > endmenu > > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile > > index b9cb862221af..bc214f15f1b5 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_TYPEC_UCSI) += ucsi/ > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > new file mode 100644 > > index 000000000000..679ba6648396 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/Kconfig > > @@ -0,0 +1,22 @@ > > +config TYPEC_UCSI > > + tristate "USB Type-C Connector System Software Interface driver" > > + select TYPEC > > + help > > + USB Type-C Connector System Software Interface (UCSI) is a > > + specification for an interface that allows the operating system to > > + control the USB Type-C ports. On UCSI system the USB Type-C ports > > + function autonomously by default, but in order to get the status of > > + the ports and support basic operations like role swapping, the driver > > + is required. UCSI is available on most of the new Intel based systems > > + that are equipped with Embedded Controller and USB Type-C ports. > > + > > + UCSI specification does not define the interface method, so depending > > + on the platform, ACPI, PCI, I2C, etc. may be used. Therefore this > > + driver only provides the core part, and separate drivers are needed > > + for every supported interface method. > > + > > + The UCSI specification can be downloaded from: > > + > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html > > + > > + To compile the driver as a module, choose M here: the module will be > > + called typec_ucsi. > > diff --git a/drivers/usb/typec/ucsi/Makefile > > b/drivers/usb/typec/ucsi/Makefile > > new file mode 100644 > > index 000000000000..87dd6ee6c9f3 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/Makefile > > @@ -0,0 +1,7 @@ > > +CFLAGS_trace.o := -I$(src) > > + > > +obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o > > + > > +typec_ucsi-y := ucsi.o > > + > > +typec_ucsi-$(CONFIG_FTRACE) += trace.o > > diff --git a/drivers/usb/typec/ucsi/debug.h b/drivers/usb/typec/ucsi/debug.h > > new file mode 100644 > > index 000000000000..87d0cd20597a > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/debug.h > > @@ -0,0 +1,64 @@ > > +#ifndef __UCSI_DEBUG_H > > +#define __UCSI_DEBUG_H > > + > > +#include "ucsi.h" > > + > > +static const char * const ucsi_cmd_strs[] = { > > + [0] = "Unknown command", > > + [UCSI_PPM_RESET] = "PPM_RESET", > > + [UCSI_CANCEL] = "CANCEL", > > + [UCSI_CONNECTOR_RESET] = "CONNECTOR_RESET", > > + [UCSI_ACK_CC_CI] = "ACK_CC_CI", > > + [UCSI_SET_NOTIFICATION_ENABLE] = "SET_NOTIFICATION_ENABLE", > > + [UCSI_GET_CAPABILITY] = "GET_CAPABILITY", > > + [UCSI_GET_CONNECTOR_CAPABILITY] = "GET_CONNECTOR_CAPABILITY", > > + [UCSI_SET_UOM] = "SET_UOM", > > + [UCSI_SET_UOR] = "SET_UOR", > > + [UCSI_SET_PDM] = "SET_PDM", > > + [UCSI_SET_PDR] = "SET_PDR", > > + [UCSI_GET_ALTERNATE_MODES] = "GET_ALTERNATE_MODES", > > + [UCSI_GET_CAM_SUPPORTED] = "GET_CAM_SUPPORTED", > > + [UCSI_GET_CURRENT_CAM] = "GET_CURRENT_CAM", > > + [UCSI_SET_NEW_CAM] = "SET_NEW_CAM", > > + [UCSI_GET_PDOS] = "GET_PDOS", > > + [UCSI_GET_CABLE_PROPERTY] = "GET_CABLE_PROPERTY", > > + [UCSI_GET_CONNECTOR_STATUS] = "GET_CONNECTOR_STATUS", > > + [UCSI_GET_ERROR_STATUS] = "GET_ERROR_STATUS", > > +}; > > + > > +static inline const char *ucsi_cmd_str(u64 raw_cmd) > > +{ > > + u8 cmd = raw_cmd & GENMASK(7, 0); > > + > > + return ucsi_cmd_strs[(cmd > ARRAY_SIZE(ucsi_cmd_strs)) ? 0 : cmd]; > > >= ? > > > +} > > + > > +static const char * const ucsi_ack_strs[] = { > > + [0] = "", > > + [UCSI_ACK_EVENT] = "event", > > + [UCSI_ACK_CMD] = "command", > > +}; > > + > > +static inline const char *ucsi_ack_str(u8 ack) > > +{ > > + return ucsi_ack_strs[(ack > ARRAY_SIZE(ucsi_ack_strs)) ? 0 : ack]; > > >= ? > > > +} > > + > > +static inline const char *ucsi_cci_str(u32 cci) > > +{ > > + if (cci & GENMASK(7, 0)) { > > + if (cci & BIT(29)) > > + return "Event pending (ACK completed)"; > > + if (cci & BIT(31)) > > + return "Event pending (command completed)"; > > + return "Connector Change"; > > + } > > + if (cci & BIT(29)) > > + return "ACK completed"; > > + if (cci & BIT(31)) > > + return "Command completed"; > > + > > + return ""; > > +} > > + > > What happens if trace is not enabled ? If I recall discussions around CLANG > correctly, it complains about unused static inline functions.
Nothing happens if trace is not enable. Everything continues to compile just like before. Maybe you were trying to define the tracepoints in C code? That won't work. > Anyway, is it really necessary to have an include file for this instead of > keeping the code in trace.c ? I am somewhat wary of variable declarations > in include files - include twice and there will be two instances. The tracepoint documentation actually states that you need to place the definitions of the tracepoints in a header. I have the trace.c file only because it was a convenient place to put the tracepoint statement in. I don't know the inner workings of tracepoints, but they do work fine for me. In case you want more info on the topic, I never read these articles, but they were mentioned in Documentation/trace/tracepoints.txt: http://lwn.net/Articles/379903 http://lwn.net/Articles/381064 http://lwn.net/Articles/383362 I believe they provide at least some kind of description on how tracepoints actually work. > > +#endif /* __UCSI_DEBUG_H */ > > diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c > > new file mode 100644 > > index 000000000000..006f65c72a34 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/trace.c > > @@ -0,0 +1,2 @@ > > +#define CREATE_TRACE_POINTS > > +#include "trace.h" > > diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h > > new file mode 100644 > > index 000000000000..30a24e8fadc7 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/trace.h > > @@ -0,0 +1,143 @@ > > + > > +#undef TRACE_SYSTEM > > +#define TRACE_SYSTEM ucsi > > + > > +#if !defined(__UCSI_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) > > +#define __UCSI_TRACE_H > > + > > +#include <linux/tracepoint.h> > > +#include "ucsi.h" > > +#include "debug.h" > > + > > +DECLARE_EVENT_CLASS(ucsi_log_ack, > > + TP_PROTO(u8 ack), > > + TP_ARGS(ack), > > + TP_STRUCT__entry( > > + __field(u8, ack) > > + ), > > + TP_fast_assign( > > + __entry->ack = ack; > > + ), > > + TP_printk("ACK %s", ucsi_ack_str(__entry->ack)) > > +); > > + > > +DEFINE_EVENT(ucsi_log_ack, ucsi_ack, > > + TP_PROTO(u8 ack), > > + TP_ARGS(ack) > > +); > > + > > +DECLARE_EVENT_CLASS(ucsi_log_control, > > + TP_PROTO(struct ucsi_control *ctrl), > > + TP_ARGS(ctrl), > > + TP_STRUCT__entry( > > + __field(u64, ctrl) > > + ), > > + TP_fast_assign( > > + __entry->ctrl = ctrl->raw_cmd; > > + ), > > + TP_printk("control=%08llx (%s)", __entry->ctrl, > > + ucsi_cmd_str(__entry->ctrl)) > > +); > > + > > +DEFINE_EVENT(ucsi_log_control, ucsi_command, > > + TP_PROTO(struct ucsi_control *ctrl), > > + TP_ARGS(ctrl) > > +); > > + > > +DECLARE_EVENT_CLASS(ucsi_log_command, > > + TP_PROTO(struct ucsi_control *ctrl, int ret), > > + TP_ARGS(ctrl, ret), > > + TP_STRUCT__entry( > > + __field(u64, ctrl) > > + __field(int, ret) > > + ), > > + TP_fast_assign( > > + __entry->ctrl = ctrl->raw_cmd; > > + __entry->ret = ret; > > + ), > > + TP_printk("%s -> %s (err=%d)", ucsi_cmd_str(__entry->ctrl), > > + __entry->ret < 0 ? "FAIL" : "OK", > > + __entry->ret < 0 ? __entry->ret : 0) > > +); > > + > > +DEFINE_EVENT(ucsi_log_command, ucsi_run_command, > > + TP_PROTO(struct ucsi_control *ctrl, int ret), > > + TP_ARGS(ctrl, ret) > > +); > > + > > +DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm, > > + TP_PROTO(struct ucsi_control *ctrl, int ret), > > + TP_ARGS(ctrl, ret) > > +); > > + > > +DECLARE_EVENT_CLASS(ucsi_log_cci, > > + TP_PROTO(u32 cci), > > + TP_ARGS(cci), > > + TP_STRUCT__entry( > > + __field(u32, cci) > > + ), > > + TP_fast_assign( > > + __entry->cci = cci; > > + ), > > + TP_printk("CCI=%08x %s", __entry->cci, ucsi_cci_str(__entry->cci)) > > +); > > + > > +DEFINE_EVENT(ucsi_log_cci, ucsi_notify, > > + TP_PROTO(u32 cci), > > + TP_ARGS(cci) > > +); > > + > > +DECLARE_EVENT_CLASS(ucsi_log_connector_status, > > + TP_PROTO(int port, struct ucsi_connector_status *status), > > + TP_ARGS(port, status), > > + TP_STRUCT__entry( > > + __field(int, port) > > + __field(u16, change) > > + __field(u8, opmode) > > + __field(u8, connected) > > + __field(u8, pwr_dir) > > + __field(u8, partner_flags) > > + __field(u8, partner_type) > > + __field(u32, request_data_obj) > > + __field(u8, bc_status) > > + ), > > + TP_fast_assign( > > + __entry->port = port - 1; > > + __entry->change = status->change; > > + __entry->opmode = status->pwr_op_mode; > > + __entry->connected = status->connected; > > + __entry->pwr_dir = status->pwr_dir; > > + __entry->partner_flags = status->partner_flags; > > + __entry->partner_type = status->partner_type; > > + __entry->request_data_obj = status->request_data_obj; > > + __entry->bc_status = status->bc_status; > > + ), > > + TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, " > > + "sourcing=%d, partner_flags=%x, partner_type=%x, " > > + "reguest_data_obj=%08x, BC status=%x", __entry->port, > > + __entry->change, __entry->opmode, __entry->connected, > > + __entry->pwr_dir, __entry->partner_flags, __entry->partner_type, > > + __entry->request_data_obj, __entry->bc_status) > > +); > > + > > +DEFINE_EVENT(ucsi_log_connector_status, ucsi_connector_change, > > + TP_PROTO(int port, struct ucsi_connector_status *status), > > + TP_ARGS(port, status) > > +); > > + > > +DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port, > > + TP_PROTO(int port, struct ucsi_connector_status *status), > > + TP_ARGS(port, status) > > +); > > + > > +#endif /* __UCSI_TRACE_H */ > > + > > +/* This part must be outside protection */ > > + > > +#undef TRACE_INCLUDE_PATH > > +#define TRACE_INCLUDE_PATH . > > + > > +#undef TRACE_INCLUDE_FILE > > +#define TRACE_INCLUDE_FILE trace > > + > > +#include <trace/define_trace.h> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > new file mode 100644 > > index 000000000000..dff1d99d304f > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -0,0 +1,770 @@ > > +/* > > + * USB Type-C Connector System Software Interface driver > > + * > > + * Copyright (C) 2017, Intel Corporation > > + * Author: Heikki Krogerus <heikki.kroge...@linux.intel.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/completion.h> > > +#include <linux/property.h> > > +#include <linux/device.h> > > +#include <linux/module.h> > > +#include <linux/delay.h> > > +#include <linux/slab.h> > > +#include <linux/usb/typec.h> > > Any preference in usb about alphabetic order of include files ? I don't see any mention of it in the documentation, so this is probable just matter of taste in the end. One likes the mother, one likes the daughter. > > + > > +#include "ucsi.h" > > +#include "trace.h" > > + > > +#define to_ucsi_connector(_cap_) container_of(_cap_, struct > > ucsi_connector, \ > > + typec_cap) > > + > > +/* > > + * UCSI_TIMEOUT - PPM communication timeout > > + * > > + * Ideally we could use MIN_TIME_TO_RESPOND_WITH_BUSY (which is defined in > > UCSI > > + * specification) here as reference, but unfortunately we can't. It is very > > + * difficult to estimate the time it takes for the system to process the > > command > > + * before it is actually passed to the PPM. > > + */ > > +#define UCSI_TIMEOUT 1000 > > Might add a note saying that this is in milli-seconds, and/or add a _MS to > the define. > > > + > > +/* > > + * UCSI_SWAP_TIMEOUT - Timeout for role swap requests > > + * > > + * 5 seconds is close to the time it takes for CapsCounter to reach 0, so > > even > > + * if the PPM does not generate Connector Change events before that with > > + * partners that do not support USB Power Delivery, this should still work. > > + */ > > +#define UCSI_SWAP_TIMEOUT 5000 > > Same here. Sure I'll fix both. > > + > > +enum ucsi_status { > > + UCSI_IDLE = 0, > > + UCSI_BUSY, > > + UCSI_ERROR, > > +}; > > + > > +struct ucsi_connector { > > + int num; > > + > > + struct ucsi *ucsi; > > + struct work_struct work; > > + struct completion complete; > > + > > + struct typec_port *port; > > + struct typec_partner *partner; > > + > > + struct typec_capability typec_cap; > > + > > + struct ucsi_connector_status status; > > + struct ucsi_connector_capability cap; > > +}; > > + > > +struct ucsi { > > + struct device *dev; > > + struct ucsi_ppm *ppm; > > + > > + enum ucsi_status status; > > + struct completion complete; > > + struct ucsi_capability cap; > > + struct ucsi_connector *connector; > > + > > + struct work_struct work; > > + > > + /* PPM Communication lock */ > > + struct mutex ppm_lock; > > + > > + /* PPM communication flags */ > > + unsigned long flags; > > +#define EVENT_PENDING 0 > > +#define COMMAND_PENDING 1 > > +}; > > + > > +static inline int ucsi_sync(struct ucsi *ucsi) > > +{ > > + if (ucsi->ppm && ucsi->ppm->sync) > > + return ucsi->ppm->sync(ucsi->ppm); > > + return 0; > > +} > > + > > +static int ucsi_command(struct ucsi *ucsi, struct ucsi_control *ctrl) > > +{ > > + trace_ucsi_command(ctrl); > > + > > + return ucsi->ppm->cmd(ucsi->ppm, ctrl); > > +} > > + > > +static int ucsi_ack(struct ucsi *ucsi, u8 ack) > > +{ > > + struct ucsi_control ctrl; > > + int ret; > > + > > + trace_ucsi_ack(ack); > > + > > + UCSI_CMD_ACK(ctrl, ack); > > + ret = ucsi->ppm->cmd(ucsi->ppm, &ctrl); > > + if (ret) > > + return ret; > > + > > + /* Waiting for ACK with ACK CMD, but not with EVENT for now */ > > + if (ack == UCSI_ACK_EVENT) > > + return 0; > > + > > + if (!wait_for_completion_timeout(&ucsi->complete, > > + msecs_to_jiffies(UCSI_TIMEOUT))) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl, > > + void *data, size_t size) > > +{ > > + struct ucsi_control _ctrl; > > + u8 data_length; > > + u16 error; > > + int ret; > > + > > + set_bit(COMMAND_PENDING, &ucsi->flags); > > + > > + ret = ucsi_command(ucsi, ctrl); > > + if (ret) > > + goto err_clear_flag; > > + > > + if (!wait_for_completion_timeout(&ucsi->complete, > > + msecs_to_jiffies(UCSI_TIMEOUT))) { > > + dev_WARN(ucsi->dev, "PPM NOT RESPONDING\n"); > > Is a traceback here really warranted ? I'll change it to dev_warn(). > > + ret = -ETIMEDOUT; > > + goto err_clear_flag; > > + } > > + > > + switch (ucsi->status) { > > + case UCSI_IDLE: > > + ret = ucsi_sync(ucsi); > > + if (ret) > > + dev_warn(ucsi->dev, "%s: sync failed\n", __func__); > > + > > + if (data) > > + memcpy(data, ucsi->ppm->data->message_in, size); > > + > > + data_length = ucsi->ppm->data->cci.data_length; > > + > > + ret = ucsi_ack(ucsi, UCSI_ACK_CMD); > > + if (!ret) > > + ret = data_length; > > + break; > > + case UCSI_BUSY: > > + /* The caller decides whether to cancel or not */ > > + ret = -EBUSY; > > + goto err_clear_flag; > > + case UCSI_ERROR: > > + ret = ucsi_ack(ucsi, UCSI_ACK_CMD); > > + if (ret) > > + goto err_clear_flag; > > + > > + _ctrl.raw_cmd = 0; > > + _ctrl.cmd.cmd = UCSI_GET_ERROR_STATUS; > > + ret = ucsi_command(ucsi, &_ctrl); > > + if (ret) > > + goto err_clear_flag; > > + > > + ret = wait_for_completion_timeout(&ucsi->complete, > > + msecs_to_jiffies(UCSI_TIMEOUT)); > > + if (!ret) { > > + dev_warn(ucsi->dev, "reading error failed!\n"); > > + ret = -ETIMEDOUT; > > + goto err_clear_flag; > > + } > > + > > + memcpy(&error, ucsi->ppm->data->message_in, sizeof(error)); > > + > > + /* Something has really gone wrong */ > > + if (WARN_ON(ucsi->status == UCSI_ERROR)) { > > + ret = -ENODEV; > > + goto err_clear_flag; > > + } > > + > > + ret = ucsi_ack(ucsi, UCSI_ACK_CMD); > > + if (ret) > > + goto err_clear_flag; > > + > > Why goto here and above instead of break ? No reason. I'll fix it. > > + switch (error) { > > + case UCSI_ERROR_INCOMPATIBLE_PARTNER: > > + ret = -EOPNOTSUPP; > > + break; > > + case UCSI_ERROR_CC_COMMUNICATION_ERR: > > + ret = -ECOMM; > > + break; > > + case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL: > > + ret = -EIO; > > + break; > > + case UCSI_ERROR_DEAD_BATTERY: > > + dev_warn(ucsi->dev, "Dead battery condition!\n"); > > + ret = -EPERM; > > + break; > > + /* The following mean a bug in this driver */ > > + case UCSI_ERROR_INVALID_CON_NUM: > > + case UCSI_ERROR_UNREGONIZED_CMD: > > + case UCSI_ERROR_INVALID_CMD_ARGUMENT: > > + dev_warn(ucsi->dev, > > + "%s: possible UCSI driver bug - error 0x%x\n", > > + __func__, error); > > + ret = -EINVAL; > > + break; > > + default: > > + dev_warn(ucsi->dev, > > + "%s: error without status\n", __func__); > > + ret = -EIO; > > + break; > > + } > > + break; > > + } > > + > > +err_clear_flag: > > + trace_ucsi_run_command(ctrl, ret); > > + > > + clear_bit(COMMAND_PENDING, &ucsi->flags); > > + > > + return ret; > > +} > > + > > +/* > > -------------------------------------------------------------------------- > > */ > > + > > +static void ucsi_pwr_opmode_change(struct ucsi_connector *con) > > +{ > > + switch (con->status.pwr_op_mode) { > > + case UCSI_CONSTAT_PWR_OPMODE_PD: > > + typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD); > > + break; > > + case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5: > > + typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_1_5A); > > + break; > > + case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0: > > + typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_3_0A); > > + break; > > + default: > > + typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_USB); > > break; ? Sure. > > + } > > +} > > + > > +static int ucsi_register_partner(struct ucsi_connector *con) > > +{ > > + struct typec_partner_desc partner; > > + > > + if (con->partner) > > + return 0; > > + > > + memset(&partner, 0, sizeof(partner)); > > + > > + switch (con->status.partner_type) { > > + case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: > > + partner.accessory = TYPEC_ACCESSORY_DEBUG; > > + break; > > + case UCSI_CONSTAT_PARTNER_TYPE_AUDIO: > > + partner.accessory = TYPEC_ACCESSORY_AUDIO; > > + break; > > + default: > > + break; > > + } > > + > > + partner.usb_pd = con->status.pwr_op_mode == UCSI_CONSTAT_PWR_OPMODE_PD; > > + > > + con->partner = typec_register_partner(con->port, &partner); > > + if (!con->partner) { > > + dev_err(con->ucsi->dev, "con%d: failed to register partner\n", > > + con->num); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static void ucsi_unregister_partner(struct ucsi_connector *con) > > +{ > > + typec_unregister_partner(con->partner); > > + con->partner = NULL; > > +} > > + > > +static void ucsi_connector_change(struct work_struct *work) > > +{ > > + struct ucsi_connector *con = container_of(work, struct ucsi_connector, > > + work); > > + struct ucsi *ucsi = con->ucsi; > > + struct ucsi_control ctrl; > > + int ret; > > + > > + mutex_lock(&ucsi->ppm_lock); > > + > > + UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); > > + ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); > > + if (ret < 0) > > + goto out_unlock; > > + > > + if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE) > > + ucsi_pwr_opmode_change(con); > > + > > + if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) { > > + typec_set_pwr_role(con->port, con->status.pwr_dir); > > + > > + /* Complete pending power role swap */ > > + if (!completion_done(&con->complete)) > > + complete(&con->complete); > > + } > > + > > + if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { > > + switch (con->status.partner_type) { > > + case UCSI_CONSTAT_PARTNER_TYPE_UFP: > > + typec_set_data_role(con->port, TYPEC_HOST); > > + break; > > + case UCSI_CONSTAT_PARTNER_TYPE_DFP: > > + typec_set_data_role(con->port, TYPEC_DEVICE); > > + break; > > + default: > > + break; > > + } > > + > > + /* Complete pending data role swap */ > > + if (!completion_done(&con->complete)) > > + complete(&con->complete); > > + } > > + > > + if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { > > + if (con->status.connected) > > + ucsi_register_partner(con); > > + else > > + ucsi_unregister_partner(con); > > + } > > + > > + ret = ucsi_ack(ucsi, UCSI_ACK_EVENT); > > + if (ret) > > + dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret); > > + > > + trace_ucsi_connector_change(con->num, &con->status); > > + > > +out_unlock: > > + clear_bit(EVENT_PENDING, &ucsi->flags); > > + mutex_unlock(&ucsi->ppm_lock); > > +} > > + > > +/** > > + * ucsi_notify - PPM notification handler > > + * @ucsi: Source UCSI Interface for the notifications > > + * > > + * Handle notifications from PPM of @ucsi. > > + */ > > +void ucsi_notify(struct ucsi *ucsi) > > +{ > > + struct ucsi_cci *cci; > > + > > + /* There is no requirement to sync here, so only warning if it fails */ > > + if (ucsi_sync(ucsi)) > > + dev_warn(ucsi->dev, "%s: sync failed\n", __func__); > > + > Why even a warning if it is not a requirement ? It still should not "ever" fail. Why not create the warning? It is after all indication that something is really wrong, and we probable want to know about it. > > + cci = &ucsi->ppm->data->cci; > > + > > + if (cci->error) > > + ucsi->status = UCSI_ERROR; > > + else if (cci->busy) > > + ucsi->status = UCSI_BUSY; > > + else > > + ucsi->status = UCSI_IDLE; > > + > > + if ((cci->ack_complete || cci->cmd_complete) && > > + test_bit(COMMAND_PENDING, &ucsi->flags)) { > > + complete(&ucsi->complete); > > + } else if (cci->connector_change) { > > + struct ucsi_connector *con; > > + > > + con = &ucsi->connector[cci->connector_change - 1]; > > + > > + if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags)) > > + schedule_work(&con->work); > > + } > > + > > + trace_ucsi_notify(ucsi->ppm->data->raw_cci); > > +} > > +EXPORT_SYMBOL_GPL(ucsi_notify); > > + > > +/* > > -------------------------------------------------------------------------- > > */ > > + > > +static int ucsi_reset_connector(struct ucsi_connector *con, bool hard) > > +{ > > + struct ucsi_control ctrl; > > + > > + UCSI_CMD_CONNECTOR_RESET(ctrl, con, hard); > > + > > + return ucsi_run_command(con->ucsi, &ctrl, NULL, 0); > > +} > > + > > +static int ucsi_reset_ppm(struct ucsi *ucsi) > > +{ > > + struct ucsi_control ctrl; > > + unsigned long tmo; > > + int ret = 0; > > Unnecessary variable initialization True. > > + > > + ctrl.raw_cmd = 0; > > + ctrl.cmd.cmd = UCSI_PPM_RESET; > > + ret = ucsi_command(ucsi, &ctrl); > > + if (ret) > > + goto err; > > + > > + tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT); > > + > > + do { > > + /* Here sync is critical. */ > > + ret = ucsi_sync(ucsi); > > + if (ret) > > + goto err; > > + > > + if (ucsi->ppm->data->cci.reset_complete) > > + break; > > + > > + /* If the PPM is still doing something else, reset it again. */ > > + if (ucsi->ppm->data->raw_cci) { > > + dev_warn(ucsi->dev, > > + "Failed to reset PPM! Trying again..\n"); > > + > > Maybe rate limit ? This can generate a lot of warnings. OK. > > + ret = ucsi_command(ucsi, &ctrl); > > + if (ret) > > + goto err; > > + } > > + > > + /* Letting the PPM settle down. */ > > + msleep(20); > > + } while (time_is_after_jiffies(tmo)); > > + > > + if (time_is_before_eq_jiffies(tmo)) > > + ret = -ETIMEDOUT; > > This can result in a timeout error if the command is successful right > when the timeout expires (break from above reset_complete check at > the 'wromg' time. maybe just drop this and set "ret = -ETIMEDOUT;" > right after msleep() ? Makes sense. I'll change it. > > + > > +err: > > + trace_ucsi_reset_ppm(&ctrl, ret); > > + > > + return ret; > > +} > > + > > +static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control > > *ctrl) > > +{ > > + int ret; > > + > > + ret = ucsi_run_command(con->ucsi, ctrl, NULL, 0); > > + if (ret == -ETIMEDOUT) { > > + struct ucsi_control c; > > + > > + /* PPM most likely stopped responding. Resetting everything. */ > > + ucsi_reset_ppm(con->ucsi); > > + > > + UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL); > > + ucsi_run_command(con->ucsi, &c, NULL, 0); > > + > > + ucsi_reset_connector(con, true); > > + } > > + > > + return ret; > > +} > > + > > +static int > > +ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role) > > +{ > > + struct ucsi_connector *con = to_ucsi_connector(cap); > > + struct ucsi_control ctrl; > > + int ret = 0; > > + > > + if (!con->partner) > > + return -EAGAIN; > > Doesn't this result in an immediate retry by user space ? Isn't that what tcpm.c reports? Maybe I misunderstood the code. I'll change that to something else. > > + > > + mutex_lock(&con->ucsi->ppm_lock); > > + > > + if ((con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP && > > + role == TYPEC_DEVICE) || > > + (con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP && > > + role == TYPEC_HOST)) > > + goto out_unlock; > > + > > + UCSI_CMD_SET_UOR(ctrl, con, role); > > + ret = ucsi_role_cmd(con, &ctrl); > > + if (ret < 0) > > + goto out_unlock; > > + > > + mutex_unlock(&con->ucsi->ppm_lock); > > + > > + if (!wait_for_completion_timeout(&con->complete, > > + msecs_to_jiffies(UCSI_SWAP_TIMEOUT))) > > + return -ETIMEDOUT; > > + > > + return 0; > > + > > +out_unlock: > > + mutex_unlock(&con->ucsi->ppm_lock); > > + > > + return ret; > > +} > > + > > +static int > > +ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role) > > +{ > > + struct ucsi_connector *con = to_ucsi_connector(cap); > > + struct ucsi_control ctrl; > > + int ret = 0; > > + > > + if (!con->partner) > > + return -EAGAIN; > > + > > + mutex_lock(&con->ucsi->ppm_lock); > > + > > + if (con->status.pwr_dir == role) > > + goto out_unlock; > > + > > + UCSI_CMD_SET_PDR(ctrl, con, role); > > + ret = ucsi_role_cmd(con, &ctrl); > > + if (ret < 0) > > + goto out_unlock; > > + > > + mutex_unlock(&con->ucsi->ppm_lock); > > + > > + if (!wait_for_completion_timeout(&con->complete, > > + msecs_to_jiffies(UCSI_SWAP_TIMEOUT))) > > + return -ETIMEDOUT; > > + > > + mutex_lock(&con->ucsi->ppm_lock); > > + > > + /* Something has gone wrong while swapping the role */ > > + if (con->status.pwr_op_mode != UCSI_CONSTAT_PWR_OPMODE_PD) { > > + ucsi_reset_connector(con, true); > > + ret = -EPROTO; > > + } > > + > > +out_unlock: > > + mutex_unlock(&con->ucsi->ppm_lock); > > + > > + return ret; > > +} > > + > > +static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con) > > +{ > > + struct fwnode_handle *fwnode; > > + int i = 1; > > + > > + device_for_each_child_node(con->ucsi->dev, fwnode) > > + if (i++ == con->num) > > + return fwnode; > > + return NULL; > > +} > > + > > +static int ucsi_register_port(struct ucsi *ucsi, int index) > > +{ > > + struct ucsi_connector *con = &ucsi->connector[index]; > > + struct typec_capability *cap = &con->typec_cap; > > + enum typec_accessory *accessory = cap->accessory; > > + struct ucsi_control ctrl; > > + int ret; > > + > > + INIT_WORK(&con->work, ucsi_connector_change); > > + init_completion(&con->complete); > > + con->num = index + 1; > > + con->ucsi = ucsi; > > + > > + /* Get connector capability */ > > + UCSI_CMD_GET_CONNECTOR_CAPABILITY(ctrl, con->num); > > + ret = ucsi_run_command(ucsi, &ctrl, &con->cap, sizeof(con->cap)); > > + if (ret < 0) > > + return ret; > > + > > + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP) > > + cap->type = TYPEC_PORT_DRP; > > + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP) > > + cap->type = TYPEC_PORT_DFP; > > + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP) > > + cap->type = TYPEC_PORT_UFP; > > + > > + cap->revision = ucsi->cap.typec_version; > > + cap->pd_revision = ucsi->cap.pd_version; > > + cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; > > + > > + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY) > > + *accessory++ = TYPEC_ACCESSORY_AUDIO; > > + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY) > > + *accessory = TYPEC_ACCESSORY_DEBUG; > > + > > + cap->fwnode = ucsi_find_fwnode(con); > > Is it ok for fwnode to be NULL ? Yes, there isn't a fw companion for every device. For UCSI, at least in case of ACPI, you may or may not have them for the ports. But in general, think about for example USB devices attached to an external hub. You will not have any kind of fw node for those. Not in case of ACPI or DT. > > + cap->dr_set = ucsi_dr_swap; > > + cap->pr_set = ucsi_pr_swap; > > + > > + /* Register the connector */ > > + con->port = typec_register_port(ucsi->dev, cap); > > + if (!con->port) > > + return -ENODEV; > > + > > + /* Get the status */ > > + UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); > > + ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); > > + if (ret < 0) { > > + dev_err(ucsi->dev, "con%d: failed to get status\n", con->num); > > + return 0; > > + } > > + > > + ucsi_pwr_opmode_change(con); > > + typec_set_pwr_role(con->port, con->status.pwr_dir); > > + > > + switch (con->status.partner_type) { > > + case UCSI_CONSTAT_PARTNER_TYPE_UFP: > > + typec_set_data_role(con->port, TYPEC_HOST); > > + break; > > + case UCSI_CONSTAT_PARTNER_TYPE_DFP: > > + typec_set_data_role(con->port, TYPEC_DEVICE); > > + break; > > + default: > > + break; > > + } > > + > > + /* Check if there is already something connected */ > > + if (con->status.connected) > > + ucsi_register_partner(con); > > + > > + trace_ucsi_register_port(con->num, &con->status); > > + > > + return 0; > > +} > > + > > +static void ucsi_init(struct work_struct *work) > > +{ > > + struct ucsi *ucsi = container_of(work, struct ucsi, work); > > + struct ucsi_connector *con; > > + struct ucsi_control ctrl; > > + int ret; > > + int i; > > + > > + mutex_lock(&ucsi->ppm_lock); > > + > > + /* Reset the PPM */ > > + ret = ucsi_reset_ppm(ucsi); > > + if (ret) { > > + dev_err(ucsi->dev, "failed to reset PPM!\n"); > > + goto err; > > + } > > + > > + /* Enable basic notifications */ > > + UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE | > > + UCSI_ENABLE_NTFY_ERROR); > > + ret = ucsi_run_command(ucsi, &ctrl, NULL, 0); > > + if (ret < 0) > > + goto err_reset; > > + > > + /* Get PPM capabilities */ > > + UCSI_CMD_GET_CAPABILITY(ctrl); > > + ret = ucsi_run_command(ucsi, &ctrl, &ucsi->cap, sizeof(ucsi->cap)); > > + if (ret < 0) > > + goto err_reset; > > + > > + if (!ucsi->cap.num_connectors) { > > + ret = -ENODEV; > > + goto err_reset; > > + } > > + > > + /* Register all connectors */ > > + ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1, > > + sizeof(*ucsi->connector), GFP_KERNEL); > > + if (!ucsi->connector) { > > + ret = -ENOMEM; > > + goto err_reset; > > + } > > + > > + for (i = 0; i < ucsi->cap.num_connectors; i++) { > > + ret = ucsi_register_port(ucsi, i); > > + if (ret) > > + goto err_unregister; > > + } > > + > > + /* Enable all notifications */ > > + UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_ALL); > > + ret = ucsi_run_command(ucsi, &ctrl, NULL, 0); > > + if (ret < 0) > > + goto err_unregister; > > + > > + mutex_unlock(&ucsi->ppm_lock); > > + > > + return; > > + > > +err_unregister: > > + for (con = ucsi->connector; con->port; con++) { > > + ucsi_unregister_partner(con); > > + typec_unregister_port(con->port); > > + con->port = NULL; > > + } > > + > > +err_reset: > > + ucsi_reset_ppm(ucsi); > > +err: > > + mutex_unlock(&ucsi->ppm_lock); > > + dev_err(ucsi->dev, "PPM init failed (%d)\n", ret); > > +} > > + > > +/** > > + * ucsi_register_ppm - Register UCSI PPM Interface > > + * @dev: Device interface to the PPM > > + * @ppm: The PPM interface > > + * > > + * Allocates UCSI instance, associates it with @ppm and returns it to the > > + * caller, and schedules initialization of the interface. > > + */ > > +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm) > > +{ > > + struct ucsi *ucsi; > > + > > + ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL); > > + if (!ucsi) > > + return ERR_PTR(-ENOMEM); > > + > > + INIT_WORK(&ucsi->work, ucsi_init); > > + init_completion(&ucsi->complete); > > + mutex_init(&ucsi->ppm_lock); > > + > > + ucsi->dev = dev; > > + ucsi->ppm = ppm; > > + > > + queue_work(system_long_wq, &ucsi->work); > > This is kind of weird/odd. A failure won't be reported to the caller. > Does this have to be asynchronous ? unregister is synchronous, after all. The communication with the PPM is painfully slow, especially during boot time probable because there are many other things trying to communicate with EC at the same time. It quite often takes something like one second per port to finish the probe on average. With four ports it has taken as much as six seconds. Even one second is totally unacceptable. We are being pushed to fix our drivers that exceed 50ms probe time, and as a way to achieve it with slow drivers, handling things in a work was suggested. So that's the reason for this approach. I have not come up with anything better for it, and nobody has proposed anything else. Do you have any suggestions? > I am also a bit concerned about race conditions. > > > + > > + return ucsi; > > +} > > +EXPORT_SYMBOL_GPL(ucsi_register_ppm); > > + > > +/** > > + * ucsi_unregister_ppm - Unregister UCSI PPM Interface > > + * @ucsi: struct ucsi associated with the PPM > > + * > > + * Unregister UCSI PPM that was created with ucsi_register(). > > + */ > > +void ucsi_unregister_ppm(struct ucsi *ucsi) > > +{ > > + struct ucsi_control ctrl; > > + int i; > > + > > + cancel_work_sync(&ucsi->work); > > + > > + mutex_lock(&ucsi->ppm_lock); > > + > > + /* Disable everything except command complete notification */ > > + UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE) > > + ucsi_run_command(ucsi, &ctrl, NULL, 0); > > + > > + mutex_unlock(&ucsi->ppm_lock); > > + > > + for (i = 0; i < ucsi->cap.num_connectors; i++) { > > + cancel_work_sync(&ucsi->connector[i].work); > > + ucsi_unregister_partner(&ucsi->connector[i]); > > + typec_unregister_port(ucsi->connector[i].port); > > + } > > + > > + ucsi_reset_ppm(ucsi); > > + > > + kfree(ucsi->connector); > > + kfree(ucsi); > > +} > > +EXPORT_SYMBOL_GPL(ucsi_unregister_ppm); > > + > > +MODULE_AUTHOR("Heikki Krogerus <heikki.kroge...@linux.intel.com>"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("USB Type-C Connector System Software Interface > > driver"); > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > new file mode 100644 > > index 000000000000..d5d03d368882 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -0,0 +1,329 @@ > > + > > +#ifndef __DRIVER_USB_TYPEC_UCSI_H > > +#define __DRIVER_USB_TYPEC_UCSI_H > > + > > +#include <linux/types.h> > > Should include bitops.h ? Sure. Thanks a lot for the review. I'll wait for your feedback before preparing a fixed version. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
