| Message ID | 1536278168-18711-2-git-send-email-ajayg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU | expand |
On 2018-09-07 01:56, Ajay Gupta wrote: > 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 <andy.shevchenko@gmail.com> > 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 > + 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. > + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + 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. > + 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 > + * (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? > + > + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2); > + if (status < 0) > + return status; > + > + memset(data, 0, sizeof(data)); Dito. > + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data)); > + if (status < 0) > + return status; > + > + dev_dbg(dev, "Silicon id %2ph", data + CCGX_I2C_RAB_READ_SILICON_ID); > + dev_dbg(dev, "FW1 version %8ph\n", data + CCGX_I2C_RAB_FW1_VERSION); > + dev_dbg(dev, "FW2 version %8ph\n", data + CCGX_I2C_RAB_FW2_VERSION); > + > + data[0] = 0x0; > + data[1] = 0x0; Dito. > + 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? > + 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? > + 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. Cheers, Peter > + 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); > + 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, > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > + dev_name(dev), uc); > + if (status < 0) { > + dev_err(uc->dev, "request_threaded_irq failed - %d\n", status); > + 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 <ajayg@nvidia.com>"); > +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller"); > +MODULE_LICENSE("GPL v2"); >
Hi Peter, > -----Original Message----- > From: Peter Rosin <peda@axentia.se> > 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: > > 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 <andy.shevchenko@gmail.com> > > 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 > > + 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. > > + status = i2c_transfer(client->adapter, msgs, > ARRAY_SIZE(msgs)); > > + 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. > > > + 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 > > + * (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 > > > + > > + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2); > > + if (status < 0) > > + return status; > > + > > + memset(data, 0, sizeof(data)); > > Dito. ok > > > + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, > sizeof(data)); > > + if (status < 0) > > + return status; > > + > > + dev_dbg(dev, "Silicon id %2ph", data + > CCGX_I2C_RAB_READ_SILICON_ID); > > + dev_dbg(dev, "FW1 version %8ph\n", data + > CCGX_I2C_RAB_FW1_VERSION); > > + dev_dbg(dev, "FW2 version %8ph\n", data + > CCGX_I2C_RAB_FW2_VERSION); > > + > > + data[0] = 0x0; > > + data[1] = 0x0; > > Dito. ok > > > + 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. > > + 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 > > + 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 > > > + 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); > > + 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, > > + IRQF_ONESHOT | > IRQF_TRIGGER_HIGH, > > + dev_name(dev), uc); > > + if (status < 0) { > > + dev_err(uc->dev, "request_threaded_irq failed - %d\n", > status); > > + 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 <ajayg@nvidia.com>"); > > +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller"); > > +MODULE_LICENSE("GPL v2"); > >
On 2018-09-07 19:28, Ajay Gupta wrote: > Hi Peter, > >> -----Original Message----- >> From: Peter Rosin <peda@axentia.se> >> 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: >>> 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 <andy.shevchenko@gmail.com> >>> 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 >>> + 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. Right, put_unaligned_le16 is the correct thing to do if rab is cpu-native. I was confused by the fact that some users of ccg_read/ccg_write do not call with a cpu-native rab. But since I also suggested that rab should be made cpu-native for those cases, I guess I'm still to blame... Sorry about that! >>> + status = i2c_transfer(client->adapter, msgs, >> ARRAY_SIZE(msgs)); >>> + 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. >> >>> + 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 >>> + * (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 >> >>> + >>> + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2); >>> + if (status < 0) >>> + return status; >>> + >>> + memset(data, 0, sizeof(data)); >> >> Dito. > ok >> >>> + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, >> sizeof(data)); >>> + if (status < 0) >>> + return status; >>> + >>> + dev_dbg(dev, "Silicon id %2ph", data + >> CCGX_I2C_RAB_READ_SILICON_ID); >>> + dev_dbg(dev, "FW1 version %8ph\n", data + >> CCGX_I2C_RAB_FW1_VERSION); >>> + dev_dbg(dev, "FW2 version %8ph\n", data + >> CCGX_I2C_RAB_FW2_VERSION); >>> + >>> + data[0] = 0x0; >>> + data[1] = 0x0; >> >> Dito. > ok >> >>> + 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. From the below reference, it's 0x8 is USBC_CONTROL with USBC_CONTROL_SIZE 0x8 (64/8) 0x20 is USBC_MESSAGE_OUT with USBC_MESSAGE_OUT_SIZE 0x10 (128/8) You could do #define USBC_MESSAGE_OUT CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20) #define USBC_MESSAGE_OUT_SIZE (128/8) etc, so that it becomes unsigned char buf1[USBC_MESSAGE_OUT_SIZE]; ... status = ccg_write(uc, USBC_MESSAGE_OUT, buf1, sizeof(buf1)); Which is a whole lot more readable IMHO. >>> + 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 So, according to table 3-1, 0x0 is UCSI_VERSION with UCSI_VERSION_SIZE 0x2 (16/8) 0x4 is USBC_CCI (connector change indication) with USBC_CCI_SIZE 0x4 (32/8) 0x10 is USBC_MESSAGE_IN with USBC_MESSAGE_IN_SIZE 0x10 (128/8) The pattern for 0x4 and 0x10 was a accidental, but again, *please* use defines for all these magic numbers. Cheers, Peter
Hi Peter, > >>> + 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. > > From the below reference, it's > > 0x8 is USBC_CONTROL with USBC_CONTROL_SIZE 0x8 (64/8) > 0x20 is USBC_MESSAGE_OUT with USBC_MESSAGE_OUT_SIZE 0x10 (128/8) > > You could do > #define USBC_MESSAGE_OUT CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20) > #define USBC_MESSAGE_OUT_SIZE (128/8) > etc, so that it becomes > > unsigned char buf1[USBC_MESSAGE_OUT_SIZE]; ... > status = ccg_write(uc, USBC_MESSAGE_OUT, buf1, sizeof(buf1)); > > Which is a whole lot more readable IMHO. Sure. > >>> + 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/technica > l > > -specifications/usb-type-c-ucsi-spec.pdf > > So, according to table 3-1, > 0x0 is UCSI_VERSION with UCSI_VERSION_SIZE 0x2 (16/8) > 0x4 is USBC_CCI (connector change indication) with USBC_CCI_SIZE 0x4 (32/8) > 0x10 is USBC_MESSAGE_IN with USBC_MESSAGE_IN_SIZE 0x10 (128/8) > > The pattern for 0x4 and 0x10 was a accidental, but again, *please* use defines > for all these magic numbers. Will fix in next version. Thanks Ajay -- nvpublic --
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 + 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); + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + 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); + 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 + * (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; + + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2); + if (status < 0) + return status; + + memset(data, 0, sizeof(data)); + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data)); + if (status < 0) + return status; + + dev_dbg(dev, "Silicon id %2ph", data + CCGX_I2C_RAB_READ_SILICON_ID); + dev_dbg(dev, "FW1 version %8ph\n", data + CCGX_I2C_RAB_FW1_VERSION); + dev_dbg(dev, "FW2 version %8ph\n", data + CCGX_I2C_RAB_FW2_VERSION); + + data[0] = 0x0; + data[1] = 0x0; + 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)); + 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); + 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); + 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); + 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, + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, + dev_name(dev), uc); + if (status < 0) { + dev_err(uc->dev, "request_threaded_irq failed - %d\n", status); + 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 <ajayg@nvidia.com>"); +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller"); +MODULE_LICENSE("GPL v2");