On Thu, Jun 15, 2017 at 08:08:19AM -0700, Guenter Roeck wrote:
> On Mon, Jun 12, 2017 at 05:40:08PM +0300, 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.krogerus@xxxxxxxxxxxxxxx>
>
> Couple of nitpicks, but I am also still concerned about the use
> of bit fields, epecially when the bit field size does not align
> with register sizes. Maybe my concerns are overblown; if so, I
> would kindly ask for someone from Intel (or any other listener)
> to provide a Reviewed-by: tag.
You do have a point, I will need to fix the structures...
> > 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,
>
> s/reguest_data_obj/request_data_obj/
OK,
<snip>
> > +#define __UCSI_CMD(_ctrl_, _cmd_) \
> > +{ \
> > + _ctrl_.raw_cmd = 0; \
> > + _ctrl_.cmd.cmd = _cmd_; \
>
> (_ctrl_) ?
>
> > +}
> > +
> > +/* Helper for preparing ucsi_control for CONNECTOR_RESET command. */
> > +#define UCSI_CMD_CONNECTOR_RESET(_ctrl_, _con_, _hard_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_CONNECTOR_RESET) \
> > + _ctrl_.con_rst.con_num = _con_->num; \
>
> (_ctrl_), (_con_) ?
>
> > + _ctrl_.con_rst.hard_reset = _hard_; \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for ACK_CC_CI command. */
> > +#define UCSI_CMD_ACK(_ctrl_, _ack_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_ACK_CC_CI) \
> > + _ctrl_.ack.cci_ack = (_ack_ == UCSI_ACK_EVENT); \
> > + _ctrl_.ack.cmd_ack = (_ack_ == UCSI_ACK_CMD); \
>
> ... and so on
Sure, I'll fix all of them.
> > +}
> > +
> > +/* Helper for preparing ucsi_control for SET_NOTIFY_ENABLE command. */
> > +#define UCSI_CMD_SET_NTFY_ENABLE(_ctrl_, _ntfys_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_SET_NOTIFICATION_ENABLE) \
> > + _ctrl_.cmd.data = _ntfys_; \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for GET_CAPABILITY command. */
> > +#define UCSI_CMD_GET_CAPABILITY(_ctrl_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_GET_CAPABILITY) \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for GET_CONNECTOR_CAPABILITY command. */
> > +#define UCSI_CMD_GET_CONNECTOR_CAPABILITY(_ctrl_, _con_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_GET_CONNECTOR_CAPABILITY) \
> > + _ctrl_.cmd.data = _con_; \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for GET_CONNECTOR_STATUS command. */
> > +#define UCSI_CMD_GET_CONNECTOR_STATUS(_ctrl_, _con_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, UCSI_GET_CONNECTOR_STATUS) \
> > + _ctrl_.cmd.data = _con_; \
> > +}
> > +
> > +#define __UCSI_ROLE(_ctrl_, _cmd_, _con_num_) \
> > +{ \
> > + __UCSI_CMD(_ctrl_, _cmd_) \
> > + _ctrl_.uor.con_num = _con_num_; \
> > + _ctrl_.uor.role = UCSI_UOR_ROLE_DRP; \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for SET_UOR command. */
> > +#define UCSI_CMD_SET_UOR(_ctrl_, _con_, _role_) \
> > +{ \
> > + __UCSI_ROLE(_ctrl_, UCSI_SET_UOR, _con_->num) \
> > + _ctrl_.uor.role |= _role_ == TYPEC_HOST ? UCSI_UOR_ROLE_DFP : \
> > + UCSI_UOR_ROLE_UFP; \
> > +}
> > +
> > +/* Helper for preparing ucsi_control for SET_PDR command. */
> > +#define UCSI_CMD_SET_PDR(_ctrl_, _con_, _role_) \
> > +{ \
> > + __UCSI_ROLE(_ctrl_, UCSI_SET_PDR, _con_->num) \
> > + _ctrl_.uor.role |= _role_ == TYPEC_SOURCE ? UCSI_UOR_ROLE_DFP : \
> > + UCSI_UOR_ROLE_UFP; \
> > +}
> > +
> > +/* Commands */
> > +#define UCSI_PPM_RESET 0x01
> > +#define UCSI_CANCEL 0x02
> > +#define UCSI_CONNECTOR_RESET 0x03
> > +#define UCSI_ACK_CC_CI 0x04
> > +#define UCSI_SET_NOTIFICATION_ENABLE 0x05
> > +#define UCSI_GET_CAPABILITY 0x06
> > +#define UCSI_GET_CONNECTOR_CAPABILITY 0x07
> > +#define UCSI_SET_UOM 0x08
> > +#define UCSI_SET_UOR 0x09
> > +#define UCSI_SET_PDM 0x0a
> > +#define UCSI_SET_PDR 0x0b
> > +#define UCSI_GET_ALTERNATE_MODES 0x0c
> > +#define UCSI_GET_CAM_SUPPORTED 0x0d
> > +#define UCSI_GET_CURRENT_CAM 0x0e
> > +#define UCSI_SET_NEW_CAM 0x0f
> > +#define UCSI_GET_PDOS 0x10
> > +#define UCSI_GET_CABLE_PROPERTY 0x11
> > +#define UCSI_GET_CONNECTOR_STATUS 0x12
> > +#define UCSI_GET_ERROR_STATUS 0x13
> > +
> > +/* ACK_CC_CI commands */
> > +#define UCSI_ACK_EVENT 1
> > +#define UCSI_ACK_CMD 2
> > +
> > +/* Bits for SET_NOTIFICATION_ENABLE command */
> > +#define UCSI_ENABLE_NTFY_CMD_COMPLETE BIT(0)
> > +#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE BIT(1)
> > +#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE BIT(2)
> > +#define UCSI_ENABLE_NTFY_CAP_CHANGE BIT(5)
> > +#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE BIT(6)
> > +#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE BIT(7)
> > +#define UCSI_ENABLE_NTFY_CAM_CHANGE BIT(8)
> > +#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE BIT(9)
> > +#define UCSI_ENABLE_NTFY_PARTNER_CHANGE BIT(11)
> > +#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE BIT(12)
> > +#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE BIT(14)
> > +#define UCSI_ENABLE_NTFY_ERROR BIT(15)
> > +#define UCSI_ENABLE_NTFY_ALL 0xdbe7
> > +
> > +/* Error information returned by PPM in response to GET_ERROR_STATUS command. */
> > +#define UCSI_ERROR_UNREGONIZED_CMD BIT(0)
> > +#define UCSI_ERROR_INVALID_CON_NUM BIT(1)
> > +#define UCSI_ERROR_INVALID_CMD_ARGUMENT BIT(2)
> > +#define UCSI_ERROR_INCOMPATIBLE_PARTNER BIT(3)
> > +#define UCSI_ERROR_CC_COMMUNICATION_ERR BIT(4)
> > +#define UCSI_ERROR_DEAD_BATTERY BIT(5)
> > +#define UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL BIT(6)
> > +
> > +/* Data structure filled by PPM in response to GET_CAPABILITY command. */
> > +struct ucsi_capability {
> > + u32 attributes;
> > +#define UCSI_CAP_ATTR_DISABLE_STATE BIT(0)
> > +#define UCSI_CAP_ATTR_BATTERY_CHARGING BIT(1)
> > +#define UCSI_CAP_ATTR_USB_PD BIT(2)
> > +#define UCSI_CAP_ATTR_TYPEC_CURRENT BIT(6)
> > +#define UCSI_CAP_ATTR_POWER_AC_SUPPLY BIT(8)
> > +#define UCSI_CAP_ATTR_POWER_OTHER BIT(10)
> > +#define UCSI_CAP_ATTR_POWER_VBUS BIT(14)
> > + u8 num_connectors;
> > + u32 features:24;
>
> Still wonder what this kind of construct is doing.
> Is it guaranteed that it only allocates 24 bit, overlapping
> with num_conectors ? If so, why not "u32 num_connectors:8;" ?
True. I'll fix it.
> Or does it allocate 32 bit, with 8 bit unused ?
> In the latter case, it might be useful to fill the field with
> "u32 unused:8;" or similar for clarification.
>
> > +#define UCSI_CAP_SET_UOM BIT(0)
> > +#define UCSI_CAP_SET_PDM BIT(1)
> > +#define UCSI_CAP_ALT_MODE_DETAILS BIT(2)
> > +#define UCSI_CAP_ALT_MODE_OVERRIDE BIT(3)
> > +#define UCSI_CAP_PDO_DETAILS BIT(4)
> > +#define UCSI_CAP_CABLE_DETAILS BIT(5)
> > +#define UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS BIT(6)
> > +#define UCSI_CAP_PD_RESET BIT(7)
> > + u8 num_alt_modes;
> > + u8 reserved;
> > + u16 bc_version;
> > + u16 pd_version;
> > + u16 typec_version;
> > +} __packed;
> > +
> > +/* Data structure filled by PPM in response to GET_CONNECTOR_CAPABILITY cmd. */
> > +struct ucsi_connector_capability {
> > + u8 op_mode;
> > +#define UCSI_CONCAP_OPMODE_DFP BIT(0)
> > +#define UCSI_CONCAP_OPMODE_UFP BIT(1)
> > +#define UCSI_CONCAP_OPMODE_DRP BIT(2)
> > +#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY BIT(3)
> > +#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY BIT(4)
> > +#define UCSI_CONCAP_OPMODE_USB2 BIT(5)
> > +#define UCSI_CONCAP_OPMODE_USB3 BIT(6)
> > +#define UCSI_CONCAP_OPMODE_ALT_MODE BIT(7)
> > + u8 provider:1;
> > + u8 consumer:1;
> > +} __packed;
> > +
> > +struct ucsi_altmode {
> > + u16 svid;
> > + u32 mid;
> > +} __packed;
> > +
> > +/* Data structure filled by PPM in response to GET_CABLE_PROPERTY command. */
> > +struct ucsi_cable_property {
> > + u16 speed_supported;
> > + u8 current_capability;
> > + u8 vbus_in_cable:1;
> > + u8 active_cable:1;
> > + u8 directionality:1;
> > + u8 plug_type:2;
> > +#define UCSI_CABLE_PROPERTY_PLUG_TYPE_A 0
> > +#define UCSI_CABLE_PROPERTY_PLUG_TYPE_B 1
> > +#define UCSI_CABLE_PROPERTY_PLUG_TYPE_C 2
> > +#define UCSI_CABLE_PROPERTY_PLUG_OTHER 3
> > + u8 mode_support:1;
> > + u8:2; /* reserved */
> > + u8 latency:4;
>
> And another 4 bit reserved ?
>
> Overall the mixed use of bit fields makes me a bit uneasy.
> The bit fields often don't align with the real world (it is
> unlikely that struct ucsi_cable_property, as defined by the
> protocol, has a length of 36 bit). Since, presumably, the
> structures are supposed to reflect the protocol, I would find
> it useful to have the structures filled with "reserved" fields.
>
> That is of course just my personal opinion, but I think it would
> be _really_ helpful in cases where an incomplete bit field is followed
> by more variables, like further above. It would also help understanding
> the complete protocol (is the above a 5-byte field per UCSI ? or 6 ? 8 ?).
You are correct. I'll fill the structures with reserved fields.
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
