Re: [PATCHv2] usb: Add driver for UCSI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20.04.2016 13:19, Heikki Krogerus wrote:
USB Type-C Connector System Software Interface (UCSI) is
specification that defines the registers and data structures
that can be used to control USB Type-C ports on a system.
UCSI is used on several Intel Broxton SoC based platforms.
Things that UCSI can be used to control include at least USB
Data Role swapping, Power Role swapping and controlling of
Alternate Modes on top of providing general details about
the port and the partners that are attached to it.

The initial purpose of the UCSI driver is to make sure USB
is in host mode on desktop and server systems that are USB
dual role capable, and provide UCSI interface.

The goal is to integrate the driver later to an USB Type-C
framework for Linux kernel, and at the same time add support
for more extensive USB Type-C port control that UCSI offers,
for example data role swapping, power role swapping,
Alternate Mode control etc.

The UCSI specification is public can be obtained from here:
http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

Some small comments inline.

---
  drivers/usb/misc/Kconfig  |  26 +++
  drivers/usb/misc/Makefile |   1 +
  drivers/usb/misc/ucsi.c   | 467 ++++++++++++++++++++++++++++++++++++++++++++++
  drivers/usb/misc/ucsi.h   | 215 +++++++++++++++++++++
  4 files changed, 709 insertions(+)
  create mode 100644 drivers/usb/misc/ucsi.c
  create mode 100644 drivers/usb/misc/ucsi.h

Changes since v1:
- Now just always using host mode. Got rid of the modules parameter.
- Replaced the atomic_t with bitop "flags". It's used to sync
   communication with the PPMs.
- Refactored the notifier handler based on suggestions from Felipe
   and Andy
- Added data structure for CCI as suggested by Felipe

...


+static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct ucsi *ucsi = data;
+	struct ucsi_cci *cci;
+
+	spin_lock(&ucsi->dev_lock);
+
+	ucsi->status = UCSI_IDLE;
+	cci = &ucsi->data->cci;
+
+	/*
+	 * REVISIT: This is not documented behavior, but all known PPMs ACK
+	 * asynchronous events by sending notification with cleared CCI.
+	 */
+	if (!ucsi->data->raw_cci) {
+		if (test_bit(EVENT_PENDING, &ucsi->flags))
+			complete(&ucsi->complete);
+		else
+			dev_WARN(ucsi->dev, "spurious notification\n");
+		goto out_unlock;
+	}
+
+	if (test_bit(COMMAND_PENDING, &ucsi->flags)) {
+		if (cci->busy) {

So if I understood correctly there is no risk of missing a connector change
here because a cci->busy bit is set there can be no other ones, right?

+			ucsi->status = UCSI_BUSY;
+			complete(&ucsi->complete);
+
+			goto out_unlock;
+		} else if (cci->ack_complete || cci->cmd_complete) {
+			/* Error Indication is only valid with commands */
+			if (cci->error && cci->cmd_complete)
+				ucsi->status = UCSI_ERROR;
+
+			ucsi->data->ctrl.raw_cmd = 0;
+			complete(&ucsi->complete);
+		}
+	}
+
+	if (cci->connector_change) {
+		struct ucsi_connector *con;
+
+		/*
+		 * This is workaround for buggy PPMs that create asynchronous
+		 * event notifications before OPM has enabled them.
+		 */
+		if (!ucsi->connector)
+			goto out_unlock;
+
+		con = ucsi->connector + (cci->connector_change - 1);
+
+		/*
+		 * PPM will not clear the connector specific bit in Connector
+		 * Change Indication field of CCI until the driver has ACK it,
+		 * and the driver can not ACK it before it has been processed.
+		 * The PPM will not generate new events before the first has
+		 * been acknowledged, even if they are for an other connector.
+		 * So only one event at a time.
+		 */
+		if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+			schedule_work(&con->work);

Was there any possibility that we miss fast consecutive connection change, such
as a new connection change work not being scheduled if the previous is not yet
ACKed and EVENT_PENDING flag cleared?
Or does hw/fw make sure no new connection change notification is queued before
previous is ACKed?
+	}
+out_unlock:
+	spin_unlock(&ucsi->dev_lock);
+}
+
+static int ucsi_ack(struct ucsi *ucsi, u8 cmd)
+{
+	struct ucsi_control ctrl;
+	int ret;
+
+	ctrl.cmd.cmd = UCSI_ACK_CC_CI;
+	ctrl.cmd.length = 0;
+	ctrl.cmd.data = cmd;
+	ret = ucsi_acpi_cmd(ucsi, &ctrl);
+	if (ret)
+		return ret;
+
+	/* Waiting for ACK also with ACK CMD for now */
+	ret = wait_for_completion_timeout(&ucsi->complete,
+					  msecs_to_jiffies(UCSI_TIMEOUT_MS));
+	if (!ret)
+		return -ETIMEDOUT;
+	return 0;
+}
+
+static int ucsi_run_cmd(struct ucsi *ucsi, struct ucsi_control *ctrl,
+			void *data, size_t size)
+{
+	int timeout = UCSI_TIMEOUT_MS;
+	u16 err_value = 0;
+	int ret;
+
+	while (test_and_set_bit(COMMAND_PENDING, &ucsi->flags) && timeout--)
+		usleep_range(1000, 2000);

Basically a home made mutex, but as the flags is needed anyways I guess this
works as well.

+
+	if (!timeout)
+		return -ETIMEDOUT;
+
+	ret = ucsi_acpi_cmd(ucsi, ctrl);
+	if (ret)
+		goto err_clear_flag;
+
+	ret = wait_for_completion_timeout(&ucsi->complete,
+					  msecs_to_jiffies(UCSI_TIMEOUT_MS));
+	if (!ret) {
+		ret = -ETIMEDOUT;
+		goto err_clear_flag;
+	}
+
+	switch (ucsi->status) {
+	case UCSI_IDLE:
+		if (data)
+			memcpy(data, ucsi->data->message_in, size);
+
+		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
+		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->cmd.cmd = UCSI_GET_ERROR_STATUS;
+		ctrl->cmd.length = 0;
+		ctrl->cmd.data = 0;
+		ret = ucsi_acpi_cmd(ucsi, ctrl);
+		if (ret)
+			goto err_clear_flag;
+
+		ret = wait_for_completion_timeout(&ucsi->complete,
+					msecs_to_jiffies(UCSI_TIMEOUT_MS));
+		if (!ret) {
+			ret = -ETIMEDOUT;
+			goto err_clear_flag;
+		}
+
+		memcpy(&err_value, ucsi->data->message_in, sizeof(err_value));
+
+		/* Something has really gone wrong */
+		if (WARN_ON(ucsi->status == UCSI_ERROR))
+			return -ENODEV;

Is it OK to _not_ clear the COMMAND_PENDING  flag before return?
Are we in such a bad state that it doesn't matter?

+
+		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
+		if (ret)
+			goto err_clear_flag;
+
+		switch (err_value) {
+		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:
+		default:
+			dev_warn(ucsi->dev,
+				 "%s: possible UCSI driver bug - error %hu\n",
+				 __func__, err_value);
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	}
+	ctrl->raw_cmd = 0;
+err_clear_flag:
+	clear_bit(COMMAND_PENDING, &ucsi->flags);
+	return ret;
+}
+
+static void ucsi_connector_change(struct work_struct *work)
+{
+	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
+						  work);
+	struct ucsi_connector_status constat;
+	struct ucsi *ucsi = con->ucsi;
+	struct ucsi_control ctrl;
+	int ret;
+
+	ctrl.cmd.cmd = UCSI_GET_CONNECTOR_STATUS;
+	ctrl.cmd.length = 0;
+	ctrl.cmd.data = con->num;
+
+	ret = ucsi_run_cmd(con->ucsi, &ctrl, &constat, sizeof(constat));
+	if (ret) {
+		dev_err(ucsi->dev, "%s: failed to read connector status (%d)\n",
+			__func__, ret);
+		goto out_ack_event;
+	}
+

ucsi_connector_change() is scheduled when a connector change event is received,
EVENT_PENDING bit is set, so we prevent new usci_connector_change() handlers from being
scheduled. COMMAND_PENDIG is anyway cleared in between (must be torun these commands).
Is there any risk that commands run before this handler is scheduled, and that they
mess up reading the connector change done here? or is it OK to do other things in between
getting a connect change event, and actually reading the connector status in here?


+	/* Ignoring disconnections and Alternate Modes */
+	if (!constat.connected || !(constat.change &
+	    (UCSI_CONSTAT_PARTNER_CHANGE | UCSI_CONSTAT_CONNECT_CHANGE)) ||
+	    constat.partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
+		goto out_ack_event;
+
+	/* If the partner got USB Host role, attempting swap */
+	if (constat.partner_type & UCSI_CONSTAT_PARTNER_TYPE_DFP) {
+		ctrl.uor.cmd = UCSI_SET_UOR;
+		ctrl.uor.con_num = con->num;
+		ctrl.uor.role = UCSI_UOR_ROLE_DFP;
+

Same here, it it OK to run some completely different command in between this and the
previous ucsi_run_cmd()

+		ret = ucsi_run_cmd(con->ucsi, &ctrl, NULL, 0);
+		if (ret)
+			dev_err(ucsi->dev, "%s: failed to swap role (%d)\n",
+				__func__, ret);
+	}
+out_ack_event:
+	ucsi_ack(ucsi, UCSI_ACK_EVENT);
+	clear_bit(EVENT_PENDING, &ucsi->flags);
+}
+
+static int ucsi_reset_ppm(struct ucsi *ucsi)
+{
+	struct ucsi_control ctrl;
+
+	memset(&ctrl, 0, sizeof(ctrl));
+	ctrl.cmd.cmd = UCSI_PPM_RESET;
+	return ucsi_acpi_cmd(ucsi, &ctrl);

Not waiting for any completion, or polling for a bit here like with other commands?

+}
+
+static int ucsi_init(struct ucsi *ucsi)
+{
+	struct ucsi_connector *con;
+	int timeout = UCSI_TIMEOUT_MS;
+	struct ucsi_control ctrl;
+	int ret;
+	int i;
+
+	init_completion(&ucsi->complete);
+	spin_lock_init(&ucsi->dev_lock);
+
+	/* Reset the PPM */
+	ret = ucsi_reset_ppm(ucsi);
+	if (ret)
+		return ret;
+
+	/* There is no quarantee the PPM will ever set the RESET_COMPLETE bit */
+	while (!(ucsi->data->cci.reset_complete) && timeout--)
+		usleep_range(1000, 2000);
+
+	/*
+	 * REVISIT: Executing second reset to WA an issue seen on some of the
+	 * Broxton based platforms, where the first reset puts the PPM into a
+	 * state where it's unable to recognise some of the commands.
+	 */
+	ret = ucsi_reset_ppm(ucsi);
+	if (ret)
+		return ret;

Not polling for the cci.reset_complete here after ucsi_reset_ppm()?
Not needed anymore for the second bugfix reset?

-Mathias

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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux