[v9,2/2] usb: typec: ucsi: add support for Cypress CCGx

Hi Peter,

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Friday, September 7, 2018 2:13 AM
To: Ajay Gupta <ajayg@nvidia.com>; wsa@the-dreams.de;
heikki.krogerus@linux.intel.com
Cc: linux-usb@vger.kernel.org; linux-i2c@vger.kernel.org
Subject: Re: [PATCH v9 2/2] usb: typec: ucsi: add support for Cypress CCGx

On 2018-09-07 01:56, Ajay Gupta wrote:

quoted

Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller over I2C
interface.

This UCSI I2C driver uses I2C bus driver interface for communicating
with Type-C controller.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
Reviewed-by: Andy Shevchenko <redacted>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
Changes from v1 -> v2
	Fixed identation in drivers/usb/typec/ucsi/Kconfig Changes from v2 ->
v3
	Fixed most of comments from Heikki
	Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
Changes from v3 -> v4
	Fixed comments from Andy
Changes from v4 -> v5
	Fixed comments from Andy
Changes from v5 -> v6
	Fixed review comments from Greg
Changes from v6 -> v7
	None
Changes from v7 -> v8
	Fixed review comments from Peter
	- Removed empty STOP message
	- Using stack memory for i2c_transfer() Changes from v8 -> v9
	None

 drivers/usb/typec/ucsi/Kconfig    |  10 ++
 drivers/usb/typec/ucsi/Makefile   |   2 +
 drivers/usb/typec/ucsi/ucsi_ccg.c | 335
++++++++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c

diff --git a/drivers/usb/typec/ucsi/Kconfig

b/drivers/usb/typec/ucsi/Kconfig index e36d6c7..7811888 100644

--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig

@@ -23,6 +23,16 @@ config TYPEC_UCSI

 if TYPEC_UCSI

+config UCSI_CCG
+	tristate "UCSI Interface Driver for Cypress CCGx"
+	depends on I2C
+	help
+	  This driver enables UCSI support on platforms that expose a
+	  Cypress CCGx Type-C controller over I2C interface.
+
+	  To compile the driver as a module, choose M here: the module will

be

quoted

+	  called ucsi_ccg.
+
 config UCSI_ACPI
 	tristate "UCSI ACPI Interface Driver"
 	depends on ACPI

diff --git a/drivers/usb/typec/ucsi/Makefile

b/drivers/usb/typec/ucsi/Makefile index 7afbea5..2f4900b 100644

--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile

@@ -8,3 +8,5 @@ typec_ucsi-y			:= ucsi.o
 typec_ucsi-$(CONFIG_TRACING)	+= trace.o

 obj-$(CONFIG_UCSI_ACPI)		+= ucsi_acpi.o
+
+obj-$(CONFIG_UCSI_CCG)		+= ucsi_ccg.o

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c

b/drivers/usb/typec/ucsi/ucsi_ccg.c
new file mode 100644
index 0000000..387b6fd

--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c

@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for Cypress CCGx Type-C controller
+ *
+ * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta <ajayg@nvidia.com>
+ *
+ * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
+ */
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include <asm/unaligned.h>
+#include "ucsi.h"
+
+struct ucsi_ccg {
+	struct device *dev;
+	struct ucsi *ucsi;
+	struct ucsi_ppm ppm;
+	struct i2c_client *client;
+	int irq;
+};
+
+#define CCGX_I2C_RAB_DEVICE_MODE			0x00
+#define CCGX_I2C_RAB_READ_SILICON_ID			0x2
+#define CCGX_I2C_RAB_INTR_REG				0x06
+#define CCGX_I2C_RAB_FW1_VERSION			0x28
+#define CCGX_I2C_RAB_FW2_VERSION			0x20
+#define CCGX_I2C_RAB_UCSI_CONTROL			0x39
+#define CCGX_I2C_RAB_UCSI_CONTROL_START			BIT(0)
+#define CCGX_I2C_RAB_UCSI_CONTROL_STOP			BIT(1)
+#define CCGX_I2C_RAB_RESPONSE_REG			0x7E
+#define CCGX_I2C_RAB_UCSI_DATA_BLOCK			0xf000
+
+static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+	struct i2c_client *client = uc->client;
+	unsigned char buf[2];
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.len	= 0x2,
+			.buf	= buf,
+		},
+		{
+			.addr	= client->addr,
+			.flags  = I2C_M_RD,
+			.buf	= data,
+		},
+	};
+	u32 rlen, rem_len = len;
+	int status;
+
+	while (rem_len > 0) {
+		msgs[1].buf = &data[len - rem_len];
+		rlen = min_t(u16, rem_len, 4);
+		msgs[1].len = rlen;
+		put_unaligned_le16(rab, buf);

Why not simply do whichever is correct of

	buf[0] = rab >> 8;
	buf[1] = rab;

and

	buf[0] = rab;
	buf[1] = rab >> 8;

and feed rab as a cpu-native value and get rid of the endianess crap.

It was like that but was changed to put_unaligned_le16() in one of
review comments from Andy at
https://marc.info/?l=linux-usb&m=153561689418696&w=2

I would rather stay with put_unaligned_le16() which looks better to me
and is similar to your suggestion of using i2c_8bit_addr_from_msg() in 1/2 
patch of series.
 

quoted

+		status = i2c_transfer(client->adapter, msgs,

ARRAY_SIZE(msgs));

quoted

+		if (status < 0) {
+			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+			return status;
+		}
+		rab += rlen;
+		rem_len -= rlen;
+	}
+
+	return 0;
+}
+
+static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+	struct i2c_client *client = uc->client;
+	unsigned char buf[2];
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.len	= 0x2,
+			.buf	= buf,
+		},
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.buf	= data,
+			.len	= len,
+		},
+	};
+	int status;
+
+	put_unaligned_le16(rab, buf);

Dito.

See above.

quoted

+	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (status < 0) {
+		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+		return status;
+	}
+
+	return 0;
+}
+
+static int ucsi_ccg_init(struct ucsi_ccg *uc) {
+	struct device *dev = uc->dev;
+	unsigned int count = 10;
+	u8 data[64];
+	int status;
+
+	/*
+	 * Selectively issue device reset
+	 * - if RESPONSE register is RESET_COMPLETE, do not issue device

reset

quoted

+	 *   (will cause usb device disconnect / reconnect)
+	 * - if RESPONSE register is not RESET_COMPLETE, issue device reset
+	 *   (causes PPC to resync device connect state by re-issuing
+	 *   set mux command)
+	 */
+	data[0] = 0x00;
+	data[1] = 0x00;

Why do you need these assigments? Will not ccg_read just overwrite this
anyway?

ok

quoted

+
+	status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
+	if (status < 0)
+		return status;
+
+	memset(data, 0, sizeof(data));

Dito.

ok

quoted

+	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,

sizeof(data));

quoted

+	if (status < 0)
+		return status;
+
+	dev_dbg(dev, "Silicon id %2ph", data +

CCGX_I2C_RAB_READ_SILICON_ID);

quoted

+	dev_dbg(dev, "FW1 version %8ph\n", data +

CCGX_I2C_RAB_FW1_VERSION);

quoted

+	dev_dbg(dev, "FW2 version %8ph\n", data +

CCGX_I2C_RAB_FW2_VERSION);

quoted

+
+	data[0] = 0x0;
+	data[1] = 0x0;

Dito.

ok

quoted

+	status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
+	if (status < 0)
+		return status;
+
+	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
+	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+	if (status < 0)
+		return status;
+
+	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
+	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+	if (status < 0)
+		return status;
+
+	/*
+	 * Flush CCGx RESPONSE queue by acking interrupts
+	 * - above ucsi control register write will push response
+	 * which must be flushed
+	 * - affects f/w update which reads response register
+	 */
+	data[0] = 0xff;
+	do {
+		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+		if (status < 0)
+			return status;
+
+		usleep_range(10000, 11000);
+
+		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+		if (status < 0)
+			return status;
+	} while ((data[0] != 0x00) && count--);
+
+	return 0;
+}
+
+static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
+	int status;
+	unsigned char buf[4] = {
+		0x20, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
+		0x8, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
+	};
+	unsigned char buf1[16];
+	unsigned char buf2[8];
+
+	memcpy(buf1, ((const void *)uc->ppm.data) + 0x20, sizeof(buf1));
+	memcpy(buf2, ((const void *)uc->ppm.data) + 0x8, sizeof(buf2));
+
+	status = ccg_write(uc, *(u16 *)buf, buf1, sizeof(buf1));

This seems to be endian-dependent. May I suggest that you do as suggested
above for ccg_read, and then somthing like

#define CCGX_I2C_RAB_USCI_DATA_BLOCK(xxx) (0xf000 | ((xxx) & <mask>))

where you of course use an appropriate value for <mask> (perhaps 0xff, or
0xfff, what do I know) and a better name for the field than xxx (perhaps len,
what do I know), and then finally do

	status = ccg_write(uc, CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20), ...

Also, the 0x20 and 0x8 are repeated and are some magic numbers that really
should be given a name or some explanation. They appear to be data lengths,
but again, what do I know?

I will check on this.

quoted

+	if (status < 0)
+		return status;
+
+	return ccg_write(uc, *(u16 *)(buf + 2), buf2, sizeof(buf2)); }
+
+static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
+	u8 *ppm = (u8 *)uc->ppm.data;
+	int status;
+	unsigned char buf[6] = {
+		0x0, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
+		0x4, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
+		0x10, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
+	};
+
+	status = ccg_read(uc, *(u16 *)buf, ppm, 0x2);

There are plenty magic numbers, but this call does not follow the pattern.
Should perhaps buf[0] be 0x2, or should perhaps the last 0x2 argument be
0x0? All other ...DATA_BLOCK calls seem to have the len in the other byte of
the rab argument. Why does this call not follow the pattern?

We are reading message IN data from Type-C controller in response to a
UCSI command. You can find details at
https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/usb-type-c-ucsi-spec.pdf
 

quoted

+	if (status < 0)
+		return status;
+
+	status = ccg_read(uc, *(u16 *)(buf + 2), ppm + 0x4, 0x4);
+	if (status < 0)
+		return status;
+
+	return ccg_read(uc, *(u16 *)(buf + 4), ppm + 0x10, 0x10); }
+
+static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
+	int status;
+	unsigned char buf[2] = {
+		CCGX_I2C_RAB_INTR_REG, CCGX_I2C_RAB_INTR_REG >> 8};
+	unsigned char buf2[1] = {0x0};
+
+	status = ccg_read(uc, *(u16 *)buf, buf2, 0x1);

This becomes
	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf2, 0x1); and you
can drop the buf variable (or perhaps rename buf2 to buf)

[time passes]

Hmm, you already do it like that in ucsi_ccg_init, so this function can be
cleaned up regardless of any endian cleanup.

Ok

Thanks
Ajay
--
nvpublic
--

Cheers,
Peter

quoted

+	if (status < 0)
+		return status;
+
+	return ccg_write(uc, *(u16 *)buf, buf2, 0x1); }
+
+static int ucsi_ccg_sync(struct ucsi_ppm *ppm) {
+	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
+	int status;
+
+	status = ucsi_ccg_recv_data(uc);
+	if (status < 0)
+		return status;
+
+	/* ack interrupt to allow next command to run */
+	return ucsi_ccg_ack_interrupt(uc);
+}
+
+static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
+*ctrl) {
+	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
+
+	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
+	return ucsi_ccg_send_data(uc);
+}
+
+static irqreturn_t ccg_irq_handler(int irq, void *data) {
+	struct ucsi_ccg *uc = data;
+
+	ucsi_notify(uc->ucsi);
+
+	return IRQ_HANDLED;
+}
+
+static int ucsi_ccg_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct ucsi_ccg *uc;
+	int status;
+
+	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
+	if (!uc)
+		return -ENOMEM;
+
+	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),

GFP_KERNEL);

quoted

+	if (!uc->ppm.data)
+		return -ENOMEM;
+
+	uc->ppm.cmd = ucsi_ccg_cmd;
+	uc->ppm.sync = ucsi_ccg_sync;
+	uc->dev = dev;
+	uc->client = client;
+
+	/* reset ccg device and initialize ucsi */
+	status = ucsi_ccg_init(uc);
+	if (status < 0) {
+		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
+		return status;
+	}
+
+	uc->irq = client->irq;
+
+	status = devm_request_threaded_irq(dev, uc->irq, NULL,

ccg_irq_handler,

quoted

+					   IRQF_ONESHOT |

IRQF_TRIGGER_HIGH,

quoted

+					   dev_name(dev), uc);
+	if (status < 0) {
+		dev_err(uc->dev, "request_threaded_irq failed - %d\n",

status);

quoted

+		return status;
+	}
+
+	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
+	if (IS_ERR(uc->ucsi)) {
+		dev_err(uc->dev, "ucsi_register_ppm failed\n");
+		return PTR_ERR(uc->ucsi);
+	}
+
+	i2c_set_clientdata(client, uc);
+	return 0;
+}
+
+static int ucsi_ccg_remove(struct i2c_client *client) {
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+
+	ucsi_unregister_ppm(uc->ucsi);
+
+	return 0;
+}
+
+static const struct i2c_device_id ucsi_ccg_device_id[] = {
+	{"ccgx-ucsi", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
+
+static struct i2c_driver ucsi_ccg_driver = {
+	.driver = {
+		.name = "ucsi_ccg",
+	},
+	.probe = ucsi_ccg_probe,
+	.remove = ucsi_ccg_remove,
+	.id_table = ucsi_ccg_device_id,
+};
+
+module_i2c_driver(ucsi_ccg_driver);
+
+MODULE_AUTHOR("Ajay Gupta [off-list ref]");
+MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
+MODULE_LICENSE("GPL v2");