* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-29 23:19 Ajay Gupta
0 siblings, 0 replies; 6+ messages in thread
From: Ajay Gupta @ 2018-08-29 23:19 UTC (permalink / raw)
To: Heikki Krogerus; +Cc: linux-usb, linux-i2c, wsa
Hi Heikki,
> On Fri, Aug 24, 2018 at 02:33:36PM -0700, 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.
>
> Cool. The patch looks fairly good to me, but I put a few comments
> below.
Thanks for review.
>
> > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > ---
> > Changes from v1 -> v2:
> > Fixed identation in drivers/usb/typec/ucsi/Kconfig
> >
> > drivers/usb/typec/ucsi/Kconfig | 10 +
> > drivers/usb/typec/ucsi/Makefile | 2 +
> > drivers/usb/typec/ucsi/ucsi_i2c_ccg.c | 591
> ++++++++++++++++++++++++++++++++++
>
> CCGx controllers support also SPI and UART AFAIK. Though, I'm not sure
> how commonly they are used (I would expect I2C to be the most common
> with these controllers), the driver should ultimately work with both
> of those busses as well.
>
> To avoid confusion, and potential driver duplicates in the future,
> just name the driver ucsi_ccg.c for now, and also s/i2c_ccg/ccg/
> everything in the driver (except the i2c_driver structure of course).
Sure.
> > 3 files changed, 603 insertions(+)
> > create mode 100644 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> >
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index e36d6c7..0ce9d48 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_I2C_CCG
> > + tristate "UCSI I2C Interface Driver for Cypress CCGx"
> > + depends on I2C_GPU
>
> Why does it need to depend only on your I2C controller?
>
> I think that should be just "depends on I2C".
ok
> > + help
> > + This driver enables UCSI support on NVIDIA GPUs 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_i2c_ccg.ko.
> > +
> > 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..4439482 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_I2C_CCG) += ucsi_i2c_ccg.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> > new file mode 100644
> > index 0000000..587e3f8
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> > @@ -0,0 +1,591 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * UCSI I2C 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/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +#include <linux/pci.h>
> > +#include "ucsi.h"
> > +
> > +struct ucsi_i2c_ccg {
> > + struct device *dev;
> > + struct ucsi *ucsi;
> > + struct ucsi_ppm ppm;
> > + struct i2c_client *client;
> > + int irq;
> > + bool wake_enabled;
> > + unsigned char ver;
> > +};
> > +
> > +#define CCGX_I2C_RAB_DEVICE_MODE 0x0000U
> > +#define CCGX_I2C_RAB_BOOT_MODE_REASON
> 0x0001U
> > +#define CCGX_I2C_RAB_READ_SILICON_ID 0x0002U
> > +#define CCGX_I2C_RAB_INTR_REG 0x0006U
> > +#define CCGX_I2C_RAB_RESET 0x0008U
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION 0x0010U
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x00)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_BASE \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER
> + 0)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_FW \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER
> + 4)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x08)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_BASE \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 0)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_FW \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 4)
> > +#define CCGX_I2C_RAB_FW2_VERSION 0x0020U
> > +#define CCGX_I2C_RAB_PDPORT_ENABLE 0x002CU
> > +#define CCGX_I2C_RAB_UCSI_STATUS 0x0038U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL 0x0039U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP 0x2U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_START 0x1U
> > +#define CCGX_I2C_RAB_HPI_VERSION 0x003CU
> > +#define CCGX_I2C_RAB_RESPONSE_REG 0x007EU
> > +#define CCGX_I2C_RAB_DM_CONTROL_1 0x1000U
> > +#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_1 0x1800U
> > +#define CCGX_I2C_RAB_DM_CONTROL_2 0x2000U
> > +#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_2 0x2800U
> > +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000U
> > +
> > +#define CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE 0x80
> > +
> > +static int ccg_read(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
> > +{
> > + struct device *dev = ui->dev;
> > + struct i2c_client *client = ui->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 err;
> > +
> > + while (rem_len > 0) {
> > + msgs[1].buf = &data[len - rem_len];
> > + rlen = min_t(u16, rem_len, 4);
> > + msgs[1].len = rlen;
> > + buf[0] = (rab >> 0) & 0xff;
> > + buf[1] = (rab >> 8) & 0xff;
> > + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (err == ARRAY_SIZE(msgs)) {
> > + err = 0;
> > + } else if (err >= 0) {
> > + dev_err(dev, "%s i2c_transfer failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > + rab += rlen;
> > + rem_len -= rlen;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int ccg_write(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
> > +{
> > + struct device *dev = ui->dev;
> > + struct i2c_client *client = ui->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,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_STOP,
> > + },
> > + };
> > + int err;
> > +
> > + buf[0] = (rab >> 0) & 0xff;
> > + buf[1] = (rab >> 8) & 0xff;
> > + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (err == ARRAY_SIZE(msgs)) {
> > + err = 0;
> > + } else if (err >= 0) {
> > + dev_err(dev, "%s i2c_transfer failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + return err;
> > +}
>
> Ideally we could use regmap. Is there something preventing you from
> using i2c regmap?
I will check it out and update in few days.
It seems I have to change i2c bus driver.
> > +static int ucsi_i2c_ccg_init(struct ucsi_i2c_ccg *ui)
> > +{
> > + struct device *dev = ui->dev;
> > + u8 data[64];
> > + int i, err;
> > +
> > + /* 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;
> > +
> > + err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + dev_info(dev, "CCGX_I2C_RAB_RESPONSE_REG %02x", data[0]);
> > +
> > + /* read device mode */
> > + memset(data, 0, sizeof(data));
> > +
> > + err = ccg_read(ui, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data));
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + dev_info(dev, "[DEVICE_MODE] %02x (HPIv%c) (Flash row size %d)\n",
> > + data[CCGX_I2C_RAB_DEVICE_MODE],
> > + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 7) & 0x01) ? '2' : '1',
> > + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 4) & 0x03) ? 256 :
> 128);
> > +
> > + dev_info(dev, "(PD ports %d) (Firmware mode %d)\n",
> > + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 2) & 0x03) ? 2 : 1,
> > + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 0) & 0x03));
> > +
> > + dev_info(dev, "[BOOT_MODE_REASON] %02x (Boot mode requested
> %d)\n",
> > + data[CCGX_I2C_RAB_BOOT_MODE_REASON],
> > + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 0) & 0x01) ?
> 1 : 0);
> > +
> > + dev_info(dev, "(FW1 valid %d) (FW2 valid %d)\n",
> > + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 2) & 0x01) ?
> 1 : 0,
> > + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 3) & 0x01) ?
> 1 : 0);
> > +
> > + dev_info(dev, "[READ_SILICON_ID] %02x %02x",
> > + data[CCGX_I2C_RAB_READ_SILICON_ID+0],
> > + data[CCGX_I2C_RAB_READ_SILICON_ID+1]);
> > +
> > + dev_info(dev, "[READ_ALL_VERSION][BOOTLOADER]\n");
> > + dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+0],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+1],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+2],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+3],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+4],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+5],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+6],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+7]);
> > +
> > + dev_info(dev, "[READ_ALL_VERSION][FW1]\n");
> > + dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+0],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+1],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+2],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+3],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+4],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+5],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+6],
> > + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+7]);
> > +
> > + dev_info(dev, "[FW2_VERSION] %02x %02x %02x %02x %02x %02x
> %02x %02x\n",
> > + data[CCGX_I2C_RAB_FW2_VERSION+0],
> > + data[CCGX_I2C_RAB_FW2_VERSION+1],
> > + data[CCGX_I2C_RAB_FW2_VERSION+2],
> > + data[CCGX_I2C_RAB_FW2_VERSION+3],
> > + data[CCGX_I2C_RAB_FW2_VERSION+4],
> > + data[CCGX_I2C_RAB_FW2_VERSION+5],
> > + data[CCGX_I2C_RAB_FW2_VERSION+6],
> > + data[CCGX_I2C_RAB_FW2_VERSION+7]);
>
> That is a lot of noise. It's not clear to me what all that means. Is
> all that information useful, even for debug purposes?
Will cleanup.
>
> > + /* read response register */
> > + data[0] = 0x0;
> > + data[1] = 0x0;
> > +
> > + err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + if (data[0] != CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE) {
> > + dev_info(dev, "response (%02x %02x) != reset_complete",
> > + data[0], data[1]);
> > + }
> > +
> > + /* stop UCSI */
> > + err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + msleep(500);
> > +
> > + /* start UCSI */
> > + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
> > + err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + msleep(500);
>
> So in total you are waiting for one second to reset that "UCSI
> control"? Are you sure you really need to wait that long?
Will remove as they are not needed.
>
> One second is a bit too long time to wait for a driver to probe.
>
> > + /* test read-1 */
> > + err = ccg_read(ui, CCGX_I2C_RAB_UCSI_DATA_BLOCK, data, 0x2);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + /* test read-2 */
> > + err = ccg_read(ui, 0xf004, data, 0x4);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + /* test read-3 */
> > + err = ccg_read(ui, 0xf010, data, 0x10);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + /* 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;
> > + for (i = 0; (i < 10) && (data[0] != 0x00); i++) {
> > + dev_dbg(dev, "flushing response %02x\n", data[0]);
>
> I'm not sure if that is useful information to anybody.
Will remove
>
> > + err = ccg_write(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_write failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + usleep_range(10000, 11000);
> > +
> > + err = ccg_read(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > + }
> > +
> > + /* get i2c slave firmware version
> > + * - [0..1] = Application name (ASCII "nb" for notebook)
> > + * - [2] = external circuil specific version
> > + * - [3] bit 0...3 = minor version
> > + * - [3] bit 4...7 = major version
> > + */
> > + err = ccg_read(ui, 0x0, data, 0x4);
> > + if (err < 0) {
> > + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> > + return -EIO;
> > + }
> > +
> > + ui->ver = data[3];
> > +
> > + dev_info(dev, "version %d.%d\n", (ui->ver >> 4) & 0x0f,
> > + (ui->ver >> 0) & 0x0f);
>
> More noise. Please avoid printing that kind of stuff that is only
> useful for debugging, if even for that.
>
> We should probable think about showing details like firmware
> version(s) to the user for example via sysfs, but we can do that
> later.
sure
>
> > + return 0;
> > +}
> > +
> > +static int ucsi_i2c_ccg_send_data(struct ucsi_i2c_ccg *ui)
> > +{
> > + int err;
> > + 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[0], ((const void *) ui->ppm.data) + 0x20, sizeof(buf1));
> > + memcpy(&buf2[0], ((const void *) ui->ppm.data) + 0x8, sizeof(buf2));
> > +
> > + err = ccg_write(ui, *(u16 *)buf, buf1, sizeof(buf1));
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_write failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + err = ccg_write(ui, *(u16 *)(buf+2), buf2, sizeof(buf2));
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_write failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int ucsi_i2c_ccg_recv_data(struct ucsi_i2c_ccg *ui)
> > +{
> > + static int first = 1;
> > + int err;
> > + 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),
> > + };
> > + u8 *ppm = (u8 *)ui->ppm.data;
> > +
> > + if (first) {
> > + err = ccg_read(ui, *(u16 *)buf, ppm, 0x2);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > + first = 1;
> > + }
> > +
> > + err = ccg_read(ui, *(u16 *)(buf + 2), ppm + 0x4, 0x4);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + err = ccg_read(ui, *(u16 *)(buf + 4), ppm + 0x10, 0x10);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int ucsi_i2c_ccg_ack_interrupt(struct ucsi_i2c_ccg *ui)
> > +{
> > + int err;
> > + unsigned char buf[3] = {(CCGX_I2C_RAB_INTR_REG & 0xff),
> > + (CCGX_I2C_RAB_INTR_REG >> 8), 0x0};
> > +
> > + err = ccg_read(ui, *(u16 *)buf, &buf[2], 0x1);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + err = ccg_write(ui, *(u16 *)buf, &buf[2], 0x1);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> > + __func__, err);
> > + return -EIO;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int ucsi_i2c_ccg_sync(struct ucsi_ppm *ppm)
> > +{
> > + struct ucsi_i2c_ccg *ui = container_of(ppm,
> > + struct ucsi_i2c_ccg, ppm);
> > + int err;
> > +
> > + err = ucsi_i2c_ccg_recv_data(ui);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s: ucsi_i2c_ccg_recv_data() err %d\n",
> > + __func__, err);
> > + goto exit;
> > + }
> > +
> > + /* ack interrupt to allow next command to run */
> > + err = ucsi_i2c_ccg_ack_interrupt(ui);
> > + if (err < 0) {
> > + dev_err(ui->dev, "%s: ucsi_i2c_ccg_ack_interrupt() err %d\n",
> > + __func__, err);
> > + }
> > +exit:
> > + return 0;
> > +}
> > +
> > +static int ucsi_i2c_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
> *ctrl)
> > +{
> > + struct ucsi_i2c_ccg *ui = container_of(ppm,
> > + struct ucsi_i2c_ccg, ppm);
> > + int err = 0;
> > +
> > + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> > + err = ucsi_i2c_ccg_send_data(ui);
> > +
> > + return err;
> > +}
> > +
> > +static irqreturn_t i2c_ccg_irq_handler(int irq, void *data)
> > +{
> > + struct ucsi_i2c_ccg *ui = data;
> > +
> > + dev_dbg(ui->dev, "%s irq %d data %p ui %p\n",
> > + __func__, irq, data, ui);
>
> I'm not sure that's useful information.
>
> You should probable try to avoid dev_dbg() in general, especially
> since the ucsi driver already uses trace points.
Ok, will fix.
>
> > + if (!ui) {
> > + dev_err(ui->dev, "%s: !ui\n", __func__);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + ucsi_notify(ui->ucsi);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int ucsi_i2c_ccg_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct ucsi_i2c_ccg *ui;
> > + int status;
> > +
> > + ui = devm_kzalloc(dev, sizeof(*ui), GFP_KERNEL);
> > + if (!ui)
> > + return -ENOMEM;
> > +
> > + ui->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),
> GFP_KERNEL);
> > + if (!ui->ppm.data)
> > + return -ENOMEM;
> > +
> > + ui->ppm.cmd = ucsi_i2c_ccg_cmd;
> > + ui->ppm.sync = ucsi_i2c_ccg_sync;
> > + ui->dev = dev;
> > + ui->client = client;
> > +
> > + /* reset i2c device and initialize ucsi */
> > + status = ucsi_i2c_ccg_init(ui);
> > + if (status < 0) {
> > + dev_err(ui->dev, "%s: ucsi_i2c_ccg_init failed - %d\n",
> > + __func__, status);
> > + return status;
> > + }
> > +
> > + ui->irq = client->irq;
> > +
> > + status = devm_request_threaded_irq(dev, ui->irq, NULL,
> > + i2c_ccg_irq_handler, IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > + dev_name(dev), ui);
>
> There are a lot of those alignment issues in this patch. You can run
> "./scripts/checkpatch.pl --strict" to find all of them.
sure
>
> > + if (status < 0) {
> > + dev_err(ui->dev, "%s: request_irq failed - %d\n",
> > + __func__, status);
> > + return status;
> > + }
> > +
> > + ui->ucsi = ucsi_register_ppm(dev, &ui->ppm);
> > + if (IS_ERR(ui->ucsi)) {
> > + dev_err(ui->dev, "ucsi_register_ppm failed\n");
> > + return PTR_ERR(ui->ucsi);
> > + }
> > +
> > + i2c_set_clientdata(client, ui);
> > + return 0;
> > +}
> > +
> > +static int ucsi_i2c_ccg_remove(struct i2c_client *client)
> > +{
> > + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> > +
> > + ucsi_unregister_ppm(ui->ucsi);
> > +
> > + return 0;
> > +}
> > +
> > +static int ucsi_i2c_ccg_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> > + int err;
> > +
> > + if (device_may_wakeup(dev)) {
> > + err = enable_irq_wake(ui->irq);
> > + if (!err)
> > + ui->wake_enabled = true;
> > + }
> > + retrn 0;
> > +}
> > +
> > +static int ucsi_i2c_ccg_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> > + struct ucsi_control c;
> > + int ret;
> > +
> > + if (device_may_wakeup(dev) && ui->wake_enabled) {
> > + disable_irq_wake(ui->irq);
> > + ui->wake_enabled = false;
> > + }
> > +
> > + /* restore UCSI notification enable mask */
> > + UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
> > + ret = ucsi_run_command(ui->ucsi, &c, NULL, 0);
> > + if (ret) {
> > + dev_err(ui->dev, "%s: failed to set notification enable - %d\n",
> > + __func__, ret);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(ucsi_i2c_ccg_driver_pm, ucsi_i2c_ccg_suspend,
> > + ucsi_i2c_ccg_resume, NULL);
> > +
> > +static const struct i2c_device_id ucsi_i2c_ccg_device_id[] = {
> > + {"i2c-gpu-ucsi", 0},
>
> I think the name should be "i2c-ccgx-ucsi", or maybe just "ccgx-ucsi".
Sure.
>
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ucsi_i2c_ccg_device_id);
> > +
> > +static struct i2c_driver ucsi_i2c_ccg_driver = {
> > + .driver = {
> > + .name = "ucsi_i2c_ccg",
> > + .pm = &ucsi_i2c_ccg_driver_pm,
> > + },
> > + .probe = ucsi_i2c_ccg_probe,
> > + .remove = ucsi_i2c_ccg_remove,
> > + .id_table = ucsi_i2c_ccg_device_id,
> > +};
> > +
> > +module_i2c_driver(ucsi_i2c_ccg_driver);
> > +
> > +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> > +MODULE_DESCRIPTION("UCSI I2C driver for Cypress CCGx Type-C
> controller");
> > +MODULE_LICENSE("GPL v2");
> > --
Thanks
Ajay
---
nvpublic
--
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-27 12:47 kbuild test robot
0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2018-08-27 12:47 UTC (permalink / raw)
To: Ajay Gupta; +Cc: kbuild-all, linux-usb, linux-i2c, wsa, heikki.krogerus
Hi Ajay,
I love your patch! Yet something to improve:
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.19-rc1 next-20180827]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ajay-Gupta/i2c-buses-add-i2c-bus-driver-for-NVIDIA-GPU/20180827-093218
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/usb/typec/ucsi/ucsi_i2c_ccg.c: In function 'ucsi_i2c_ccg_resume':
>> drivers/usb/typec/ucsi/ucsi_i2c_ccg.c:559:8: error: implicit declaration of function 'ucsi_run_command'; did you mean 'ucsi_i2c_ccg_cmd'? [-Werror=implicit-function-declaration]
ret = ucsi_run_command(ui->ucsi, &c, NULL, 0);
^~~~~~~~~~~~~~~~
ucsi_i2c_ccg_cmd
cc1: some warnings being treated as errors
vim +559 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
544
545 static int ucsi_i2c_ccg_resume(struct device *dev)
546 {
547 struct i2c_client *client = to_i2c_client(dev);
548 struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
549 struct ucsi_control c;
550 int ret;
551
552 if (device_may_wakeup(dev) && ui->wake_enabled) {
553 disable_irq_wake(ui->irq);
554 ui->wake_enabled = false;
555 }
556
557 /* restore UCSI notification enable mask */
558 UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
> 559 ret = ucsi_run_command(ui->ucsi, &c, NULL, 0);
560 if (ret) {
561 dev_err(ui->dev, "%s: failed to set notification enable - %d\n",
562 __func__, ret);
563 }
564
565 return 0;
566 }
567
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-27 10:59 Heikki Krogerus
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Krogerus @ 2018-08-27 10:59 UTC (permalink / raw)
To: Ajay Gupta; +Cc: linux-usb, linux-i2c, wsa
Hi Ajay,
On Fri, Aug 24, 2018 at 02:33:36PM -0700, 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.
Cool. The patch looks fairly good to me, but I put a few comments
below.
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
> Changes from v1 -> v2:
> Fixed identation in drivers/usb/typec/ucsi/Kconfig
>
> drivers/usb/typec/ucsi/Kconfig | 10 +
> drivers/usb/typec/ucsi/Makefile | 2 +
> drivers/usb/typec/ucsi/ucsi_i2c_ccg.c | 591 ++++++++++++++++++++++++++++++++++
CCGx controllers support also SPI and UART AFAIK. Though, I'm not sure
how commonly they are used (I would expect I2C to be the most common
with these controllers), the driver should ultimately work with both
of those busses as well.
To avoid confusion, and potential driver duplicates in the future,
just name the driver ucsi_ccg.c for now, and also s/i2c_ccg/ccg/
everything in the driver (except the i2c_driver structure of course).
> 3 files changed, 603 insertions(+)
> create mode 100644 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
>
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..0ce9d48 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_I2C_CCG
> + tristate "UCSI I2C Interface Driver for Cypress CCGx"
> + depends on I2C_GPU
Why does it need to depend only on your I2C controller?
I think that should be just "depends on I2C".
> + help
> + This driver enables UCSI support on NVIDIA GPUs 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_i2c_ccg.ko.
> +
> 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..4439482 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_I2C_CCG) += ucsi_i2c_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> new file mode 100644
> index 0000000..587e3f8
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> @@ -0,0 +1,591 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI I2C 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/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/pci.h>
> +#include "ucsi.h"
> +
> +struct ucsi_i2c_ccg {
> + struct device *dev;
> + struct ucsi *ucsi;
> + struct ucsi_ppm ppm;
> + struct i2c_client *client;
> + int irq;
> + bool wake_enabled;
> + unsigned char ver;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE 0x0000U
> +#define CCGX_I2C_RAB_BOOT_MODE_REASON 0x0001U
> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x0002U
> +#define CCGX_I2C_RAB_INTR_REG 0x0006U
> +#define CCGX_I2C_RAB_RESET 0x0008U
> +#define CCGX_I2C_RAB_READ_ALL_VERSION 0x0010U
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER \
> + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x00)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_BASE \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 0)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_FW \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 4)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP \
> + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x08)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_BASE \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 0)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_FW \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 4)
> +#define CCGX_I2C_RAB_FW2_VERSION 0x0020U
> +#define CCGX_I2C_RAB_PDPORT_ENABLE 0x002CU
> +#define CCGX_I2C_RAB_UCSI_STATUS 0x0038U
> +#define CCGX_I2C_RAB_UCSI_CONTROL 0x0039U
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP 0x2U
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START 0x1U
> +#define CCGX_I2C_RAB_HPI_VERSION 0x003CU
> +#define CCGX_I2C_RAB_RESPONSE_REG 0x007EU
> +#define CCGX_I2C_RAB_DM_CONTROL_1 0x1000U
> +#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_1 0x1800U
> +#define CCGX_I2C_RAB_DM_CONTROL_2 0x2000U
> +#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_2 0x2800U
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000U
> +
> +#define CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE 0x80
> +
> +static int ccg_read(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
> +{
> + struct device *dev = ui->dev;
> + struct i2c_client *client = ui->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 err;
> +
> + while (rem_len > 0) {
> + msgs[1].buf = &data[len - rem_len];
> + rlen = min_t(u16, rem_len, 4);
> + msgs[1].len = rlen;
> + buf[0] = (rab >> 0) & 0xff;
> + buf[1] = (rab >> 8) & 0xff;
> + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "%s i2c_transfer failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> + rab += rlen;
> + rem_len -= rlen;
> + }
> +
> + return err;
> +}
> +
> +static int ccg_write(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
> +{
> + struct device *dev = ui->dev;
> + struct i2c_client *client = ui->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,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_STOP,
> + },
> + };
> + int err;
> +
> + buf[0] = (rab >> 0) & 0xff;
> + buf[1] = (rab >> 8) & 0xff;
> + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "%s i2c_transfer failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + return err;
> +}
Ideally we could use regmap. Is there something preventing you from
using i2c regmap?
> +static int ucsi_i2c_ccg_init(struct ucsi_i2c_ccg *ui)
> +{
> + struct device *dev = ui->dev;
> + u8 data[64];
> + int i, err;
> +
> + /* 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;
> +
> + err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + dev_info(dev, "CCGX_I2C_RAB_RESPONSE_REG %02x", data[0]);
> +
> + /* read device mode */
> + memset(data, 0, sizeof(data));
> +
> + err = ccg_read(ui, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data));
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + dev_info(dev, "[DEVICE_MODE] %02x (HPIv%c) (Flash row size %d)\n",
> + data[CCGX_I2C_RAB_DEVICE_MODE],
> + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 7) & 0x01) ? '2' : '1',
> + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 4) & 0x03) ? 256 : 128);
> +
> + dev_info(dev, "(PD ports %d) (Firmware mode %d)\n",
> + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 2) & 0x03) ? 2 : 1,
> + ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 0) & 0x03));
> +
> + dev_info(dev, "[BOOT_MODE_REASON] %02x (Boot mode requested %d)\n",
> + data[CCGX_I2C_RAB_BOOT_MODE_REASON],
> + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 0) & 0x01) ? 1 : 0);
> +
> + dev_info(dev, "(FW1 valid %d) (FW2 valid %d)\n",
> + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 2) & 0x01) ? 1 : 0,
> + ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 3) & 0x01) ? 1 : 0);
> +
> + dev_info(dev, "[READ_SILICON_ID] %02x %02x",
> + data[CCGX_I2C_RAB_READ_SILICON_ID+0],
> + data[CCGX_I2C_RAB_READ_SILICON_ID+1]);
> +
> + dev_info(dev, "[READ_ALL_VERSION][BOOTLOADER]\n");
> + dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+0],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+1],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+2],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+3],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+4],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+5],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+6],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+7]);
> +
> + dev_info(dev, "[READ_ALL_VERSION][FW1]\n");
> + dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+0],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+1],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+2],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+3],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+4],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+5],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+6],
> + data[CCGX_I2C_RAB_READ_ALL_VERSION+8+7]);
> +
> + dev_info(dev, "[FW2_VERSION] %02x %02x %02x %02x %02x %02x %02x %02x\n",
> + data[CCGX_I2C_RAB_FW2_VERSION+0],
> + data[CCGX_I2C_RAB_FW2_VERSION+1],
> + data[CCGX_I2C_RAB_FW2_VERSION+2],
> + data[CCGX_I2C_RAB_FW2_VERSION+3],
> + data[CCGX_I2C_RAB_FW2_VERSION+4],
> + data[CCGX_I2C_RAB_FW2_VERSION+5],
> + data[CCGX_I2C_RAB_FW2_VERSION+6],
> + data[CCGX_I2C_RAB_FW2_VERSION+7]);
That is a lot of noise. It's not clear to me what all that means. Is
all that information useful, even for debug purposes?
> + /* read response register */
> + data[0] = 0x0;
> + data[1] = 0x0;
> +
> + err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + if (data[0] != CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE) {
> + dev_info(dev, "response (%02x %02x) != reset_complete",
> + data[0], data[1]);
> + }
> +
> + /* stop UCSI */
> + err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + msleep(500);
> +
> + /* start UCSI */
> + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
> + err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + msleep(500);
So in total you are waiting for one second to reset that "UCSI
control"? Are you sure you really need to wait that long?
One second is a bit too long time to wait for a driver to probe.
> + /* test read-1 */
> + err = ccg_read(ui, CCGX_I2C_RAB_UCSI_DATA_BLOCK, data, 0x2);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + /* test read-2 */
> + err = ccg_read(ui, 0xf004, data, 0x4);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + /* test read-3 */
> + err = ccg_read(ui, 0xf010, data, 0x10);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + /* 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;
> + for (i = 0; (i < 10) && (data[0] != 0x00); i++) {
> + dev_dbg(dev, "flushing response %02x\n", data[0]);
I'm not sure if that is useful information to anybody.
> + err = ccg_write(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_write failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + usleep_range(10000, 11000);
> +
> + err = ccg_read(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> + }
> +
> + /* get i2c slave firmware version
> + * - [0..1] = Application name (ASCII "nb" for notebook)
> + * - [2] = external circuil specific version
> + * - [3] bit 0...3 = minor version
> + * - [3] bit 4...7 = major version
> + */
> + err = ccg_read(ui, 0x0, data, 0x4);
> + if (err < 0) {
> + dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
> + return -EIO;
> + }
> +
> + ui->ver = data[3];
> +
> + dev_info(dev, "version %d.%d\n", (ui->ver >> 4) & 0x0f,
> + (ui->ver >> 0) & 0x0f);
More noise. Please avoid printing that kind of stuff that is only
useful for debugging, if even for that.
We should probable think about showing details like firmware
version(s) to the user for example via sysfs, but we can do that
later.
> + return 0;
> +}
> +
> +static int ucsi_i2c_ccg_send_data(struct ucsi_i2c_ccg *ui)
> +{
> + int err;
> + 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[0], ((const void *) ui->ppm.data) + 0x20, sizeof(buf1));
> + memcpy(&buf2[0], ((const void *) ui->ppm.data) + 0x8, sizeof(buf2));
> +
> + err = ccg_write(ui, *(u16 *)buf, buf1, sizeof(buf1));
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_write failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + err = ccg_write(ui, *(u16 *)(buf+2), buf2, sizeof(buf2));
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_write failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + return err;
> +}
> +
> +static int ucsi_i2c_ccg_recv_data(struct ucsi_i2c_ccg *ui)
> +{
> + static int first = 1;
> + int err;
> + 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),
> + };
> + u8 *ppm = (u8 *)ui->ppm.data;
> +
> + if (first) {
> + err = ccg_read(ui, *(u16 *)buf, ppm, 0x2);
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> + first = 1;
> + }
> +
> + err = ccg_read(ui, *(u16 *)(buf + 2), ppm + 0x4, 0x4);
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + err = ccg_read(ui, *(u16 *)(buf + 4), ppm + 0x10, 0x10);
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + return err;
> +}
> +
> +static int ucsi_i2c_ccg_ack_interrupt(struct ucsi_i2c_ccg *ui)
> +{
> + int err;
> + unsigned char buf[3] = {(CCGX_I2C_RAB_INTR_REG & 0xff),
> + (CCGX_I2C_RAB_INTR_REG >> 8), 0x0};
> +
> + err = ccg_read(ui, *(u16 *)buf, &buf[2], 0x1);
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + err = ccg_write(ui, *(u16 *)buf, &buf[2], 0x1);
> + if (err < 0) {
> + dev_err(ui->dev, "%s ccg_read failed, err %d\n",
> + __func__, err);
> + return -EIO;
> + }
> +
> + return err;
> +}
> +
> +static int ucsi_i2c_ccg_sync(struct ucsi_ppm *ppm)
> +{
> + struct ucsi_i2c_ccg *ui = container_of(ppm,
> + struct ucsi_i2c_ccg, ppm);
> + int err;
> +
> + err = ucsi_i2c_ccg_recv_data(ui);
> + if (err < 0) {
> + dev_err(ui->dev, "%s: ucsi_i2c_ccg_recv_data() err %d\n",
> + __func__, err);
> + goto exit;
> + }
> +
> + /* ack interrupt to allow next command to run */
> + err = ucsi_i2c_ccg_ack_interrupt(ui);
> + if (err < 0) {
> + dev_err(ui->dev, "%s: ucsi_i2c_ccg_ack_interrupt() err %d\n",
> + __func__, err);
> + }
> +exit:
> + return 0;
> +}
> +
> +static int ucsi_i2c_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
> +{
> + struct ucsi_i2c_ccg *ui = container_of(ppm,
> + struct ucsi_i2c_ccg, ppm);
> + int err = 0;
> +
> + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> + err = ucsi_i2c_ccg_send_data(ui);
> +
> + return err;
> +}
> +
> +static irqreturn_t i2c_ccg_irq_handler(int irq, void *data)
> +{
> + struct ucsi_i2c_ccg *ui = data;
> +
> + dev_dbg(ui->dev, "%s irq %d data %p ui %p\n",
> + __func__, irq, data, ui);
I'm not sure that's useful information.
You should probable try to avoid dev_dbg() in general, especially
since the ucsi driver already uses trace points.
> + if (!ui) {
> + dev_err(ui->dev, "%s: !ui\n", __func__);
> + return IRQ_HANDLED;
> + }
> +
> + ucsi_notify(ui->ucsi);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ucsi_i2c_ccg_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct ucsi_i2c_ccg *ui;
> + int status;
> +
> + ui = devm_kzalloc(dev, sizeof(*ui), GFP_KERNEL);
> + if (!ui)
> + return -ENOMEM;
> +
> + ui->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data), GFP_KERNEL);
> + if (!ui->ppm.data)
> + return -ENOMEM;
> +
> + ui->ppm.cmd = ucsi_i2c_ccg_cmd;
> + ui->ppm.sync = ucsi_i2c_ccg_sync;
> + ui->dev = dev;
> + ui->client = client;
> +
> + /* reset i2c device and initialize ucsi */
> + status = ucsi_i2c_ccg_init(ui);
> + if (status < 0) {
> + dev_err(ui->dev, "%s: ucsi_i2c_ccg_init failed - %d\n",
> + __func__, status);
> + return status;
> + }
> +
> + ui->irq = client->irq;
> +
> + status = devm_request_threaded_irq(dev, ui->irq, NULL,
> + i2c_ccg_irq_handler, IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> + dev_name(dev), ui);
There are a lot of those alignment issues in this patch. You can run
"./scripts/checkpatch.pl --strict" to find all of them.
> + if (status < 0) {
> + dev_err(ui->dev, "%s: request_irq failed - %d\n",
> + __func__, status);
> + return status;
> + }
> +
> + ui->ucsi = ucsi_register_ppm(dev, &ui->ppm);
> + if (IS_ERR(ui->ucsi)) {
> + dev_err(ui->dev, "ucsi_register_ppm failed\n");
> + return PTR_ERR(ui->ucsi);
> + }
> +
> + i2c_set_clientdata(client, ui);
> + return 0;
> +}
> +
> +static int ucsi_i2c_ccg_remove(struct i2c_client *client)
> +{
> + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> +
> + ucsi_unregister_ppm(ui->ucsi);
> +
> + return 0;
> +}
> +
> +static int ucsi_i2c_ccg_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> + int err;
> +
> + if (device_may_wakeup(dev)) {
> + err = enable_irq_wake(ui->irq);
> + if (!err)
> + ui->wake_enabled = true;
> + }
> + retrn 0;
> +}
> +
> +static int ucsi_i2c_ccg_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
> + struct ucsi_control c;
> + int ret;
> +
> + if (device_may_wakeup(dev) && ui->wake_enabled) {
> + disable_irq_wake(ui->irq);
> + ui->wake_enabled = false;
> + }
> +
> + /* restore UCSI notification enable mask */
> + UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
> + ret = ucsi_run_command(ui->ucsi, &c, NULL, 0);
> + if (ret) {
> + dev_err(ui->dev, "%s: failed to set notification enable - %d\n",
> + __func__, ret);
> + }
> +
> + return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(ucsi_i2c_ccg_driver_pm, ucsi_i2c_ccg_suspend,
> + ucsi_i2c_ccg_resume, NULL);
> +
> +static const struct i2c_device_id ucsi_i2c_ccg_device_id[] = {
> + {"i2c-gpu-ucsi", 0},
I think the name should be "i2c-ccgx-ucsi", or maybe just "ccgx-ucsi".
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, ucsi_i2c_ccg_device_id);
> +
> +static struct i2c_driver ucsi_i2c_ccg_driver = {
> + .driver = {
> + .name = "ucsi_i2c_ccg",
> + .pm = &ucsi_i2c_ccg_driver_pm,
> + },
> + .probe = ucsi_i2c_ccg_probe,
> + .remove = ucsi_i2c_ccg_remove,
> + .id_table = ucsi_i2c_ccg_device_id,
> +};
> +
> +module_i2c_driver(ucsi_i2c_ccg_driver);
> +
> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("UCSI I2C driver for Cypress CCGx Type-C controller");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
Thanks,
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-25 16:35 Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2018-08-25 16:35 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: ajayg, USB, linux-i2c, Wolfram Sang, Krogerus, Heikki
On Sat, Aug 25, 2018 at 07:29:24PM +0300, Andy Shevchenko wrote:
> > -----------------------------------------------------------------------------------
> > This email message is for the sole use of the intended recipient(s) and may contain
> > confidential information. Any unauthorized review, use, disclosure or distribution
> > is prohibited. If you are not the intended recipient, please contact the sender by
> > reply email and destroy all copies of the original message.
> > -----------------------------------------------------------------------------------
>
> You have an issue here.
Not an issue if you want your emails to just be deleted automatically,
like I do :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-25 16:29 Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2018-08-25 16:29 UTC (permalink / raw)
To: ajayg; +Cc: USB, linux-i2c, Wolfram Sang, Krogerus, Heikki
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information. Any unauthorized review, use, disclosure or distribution
> is prohibited. If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
You have an issue here.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx
@ 2018-08-24 21:33 Ajay Gupta
0 siblings, 0 replies; 6+ messages in thread
From: Ajay Gupta @ 2018-08-24 21:33 UTC (permalink / raw)
To: linux-usb, linux-i2c; +Cc: wsa, heikki.krogerus, Ajay Gupta
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>
---
Changes from v1 -> v2:
Fixed identation in drivers/usb/typec/ucsi/Kconfig
drivers/usb/typec/ucsi/Kconfig | 10 +
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_i2c_ccg.c | 591 ++++++++++++++++++++++++++++++++++
3 files changed, 603 insertions(+)
create mode 100644 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index e36d6c7..0ce9d48 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_I2C_CCG
+ tristate "UCSI I2C Interface Driver for Cypress CCGx"
+ depends on I2C_GPU
+ help
+ This driver enables UCSI support on NVIDIA GPUs 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_i2c_ccg.ko.
+
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..4439482 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_I2C_CCG) += ucsi_i2c_ccg.o
diff --git a/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
new file mode 100644
index 0000000..587e3f8
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
@@ -0,0 +1,591 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI I2C 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/platform_device.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/pci.h>
+#include "ucsi.h"
+
+struct ucsi_i2c_ccg {
+ struct device *dev;
+ struct ucsi *ucsi;
+ struct ucsi_ppm ppm;
+ struct i2c_client *client;
+ int irq;
+ bool wake_enabled;
+ unsigned char ver;
+};
+
+#define CCGX_I2C_RAB_DEVICE_MODE 0x0000U
+#define CCGX_I2C_RAB_BOOT_MODE_REASON 0x0001U
+#define CCGX_I2C_RAB_READ_SILICON_ID 0x0002U
+#define CCGX_I2C_RAB_INTR_REG 0x0006U
+#define CCGX_I2C_RAB_RESET 0x0008U
+#define CCGX_I2C_RAB_READ_ALL_VERSION 0x0010U
+#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER \
+ (CCGX_I2C_RAB_READ_ALL_VERSION + 0x00)
+#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_BASE \
+ (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 0)
+#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_FW \
+ (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 4)
+#define CCGX_I2C_RAB_READ_ALL_VERSION_APP \
+ (CCGX_I2C_RAB_READ_ALL_VERSION + 0x08)
+#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_BASE \
+ (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 0)
+#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_FW \
+ (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 4)
+#define CCGX_I2C_RAB_FW2_VERSION 0x0020U
+#define CCGX_I2C_RAB_PDPORT_ENABLE 0x002CU
+#define CCGX_I2C_RAB_UCSI_STATUS 0x0038U
+#define CCGX_I2C_RAB_UCSI_CONTROL 0x0039U
+#define CCGX_I2C_RAB_UCSI_CONTROL_STOP 0x2U
+#define CCGX_I2C_RAB_UCSI_CONTROL_START 0x1U
+#define CCGX_I2C_RAB_HPI_VERSION 0x003CU
+#define CCGX_I2C_RAB_RESPONSE_REG 0x007EU
+#define CCGX_I2C_RAB_DM_CONTROL_1 0x1000U
+#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_1 0x1800U
+#define CCGX_I2C_RAB_DM_CONTROL_2 0x2000U
+#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_2 0x2800U
+#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000U
+
+#define CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE 0x80
+
+static int ccg_read(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
+{
+ struct device *dev = ui->dev;
+ struct i2c_client *client = ui->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 err;
+
+ while (rem_len > 0) {
+ msgs[1].buf = &data[len - rem_len];
+ rlen = min_t(u16, rem_len, 4);
+ msgs[1].len = rlen;
+ buf[0] = (rab >> 0) & 0xff;
+ buf[1] = (rab >> 8) & 0xff;
+ err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (err == ARRAY_SIZE(msgs)) {
+ err = 0;
+ } else if (err >= 0) {
+ dev_err(dev, "%s i2c_transfer failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+ rab += rlen;
+ rem_len -= rlen;
+ }
+
+ return err;
+}
+
+static int ccg_write(struct ucsi_i2c_ccg *ui, u16 rab, u8 *data, u32 len)
+{
+ struct device *dev = ui->dev;
+ struct i2c_client *client = ui->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,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_STOP,
+ },
+ };
+ int err;
+
+ buf[0] = (rab >> 0) & 0xff;
+ buf[1] = (rab >> 8) & 0xff;
+ err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (err == ARRAY_SIZE(msgs)) {
+ err = 0;
+ } else if (err >= 0) {
+ dev_err(dev, "%s i2c_transfer failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ return err;
+}
+
+static int ucsi_i2c_ccg_init(struct ucsi_i2c_ccg *ui)
+{
+ struct device *dev = ui->dev;
+ u8 data[64];
+ int i, err;
+
+ /* 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;
+
+ err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ dev_info(dev, "CCGX_I2C_RAB_RESPONSE_REG %02x", data[0]);
+
+ /* read device mode */
+ memset(data, 0, sizeof(data));
+
+ err = ccg_read(ui, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data));
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ dev_info(dev, "[DEVICE_MODE] %02x (HPIv%c) (Flash row size %d)\n",
+ data[CCGX_I2C_RAB_DEVICE_MODE],
+ ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 7) & 0x01) ? '2' : '1',
+ ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 4) & 0x03) ? 256 : 128);
+
+ dev_info(dev, "(PD ports %d) (Firmware mode %d)\n",
+ ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 2) & 0x03) ? 2 : 1,
+ ((data[CCGX_I2C_RAB_DEVICE_MODE] >> 0) & 0x03));
+
+ dev_info(dev, "[BOOT_MODE_REASON] %02x (Boot mode requested %d)\n",
+ data[CCGX_I2C_RAB_BOOT_MODE_REASON],
+ ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 0) & 0x01) ? 1 : 0);
+
+ dev_info(dev, "(FW1 valid %d) (FW2 valid %d)\n",
+ ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 2) & 0x01) ? 1 : 0,
+ ((data[CCGX_I2C_RAB_BOOT_MODE_REASON] >> 3) & 0x01) ? 1 : 0);
+
+ dev_info(dev, "[READ_SILICON_ID] %02x %02x",
+ data[CCGX_I2C_RAB_READ_SILICON_ID+0],
+ data[CCGX_I2C_RAB_READ_SILICON_ID+1]);
+
+ dev_info(dev, "[READ_ALL_VERSION][BOOTLOADER]\n");
+ dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+0],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+1],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+2],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+3],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+4],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+5],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+6],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+7]);
+
+ dev_info(dev, "[READ_ALL_VERSION][FW1]\n");
+ dev_info(dev, "%02x %02x %02x %02x %02x %02x %02x %02x\n",
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+0],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+1],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+2],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+3],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+4],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+5],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+6],
+ data[CCGX_I2C_RAB_READ_ALL_VERSION+8+7]);
+
+ dev_info(dev, "[FW2_VERSION] %02x %02x %02x %02x %02x %02x %02x %02x\n",
+ data[CCGX_I2C_RAB_FW2_VERSION+0],
+ data[CCGX_I2C_RAB_FW2_VERSION+1],
+ data[CCGX_I2C_RAB_FW2_VERSION+2],
+ data[CCGX_I2C_RAB_FW2_VERSION+3],
+ data[CCGX_I2C_RAB_FW2_VERSION+4],
+ data[CCGX_I2C_RAB_FW2_VERSION+5],
+ data[CCGX_I2C_RAB_FW2_VERSION+6],
+ data[CCGX_I2C_RAB_FW2_VERSION+7]);
+
+ /* read response register */
+ data[0] = 0x0;
+ data[1] = 0x0;
+
+ err = ccg_read(ui, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ if (data[0] != CCGX_I2C_RAB_RESPONSE_REG_RESET_COMPLETE) {
+ dev_info(dev, "response (%02x %02x) != reset_complete",
+ data[0], data[1]);
+ }
+
+ /* stop UCSI */
+ err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ msleep(500);
+
+ /* start UCSI */
+ data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
+ err = ccg_write(ui, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_write failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ msleep(500);
+
+ /* test read-1 */
+ err = ccg_read(ui, CCGX_I2C_RAB_UCSI_DATA_BLOCK, data, 0x2);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ /* test read-2 */
+ err = ccg_read(ui, 0xf004, data, 0x4);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ /* test read-3 */
+ err = ccg_read(ui, 0xf010, data, 0x10);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ /* 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;
+ for (i = 0; (i < 10) && (data[0] != 0x00); i++) {
+ dev_dbg(dev, "flushing response %02x\n", data[0]);
+
+ err = ccg_write(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_write failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ usleep_range(10000, 11000);
+
+ err = ccg_read(ui, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+ }
+
+ /* get i2c slave firmware version
+ * - [0..1] = Application name (ASCII "nb" for notebook)
+ * - [2] = external circuil specific version
+ * - [3] bit 0...3 = minor version
+ * - [3] bit 4...7 = major version
+ */
+ err = ccg_read(ui, 0x0, data, 0x4);
+ if (err < 0) {
+ dev_err(dev, "%s ccg_read failed, err %d\n", __func__, err);
+ return -EIO;
+ }
+
+ ui->ver = data[3];
+
+ dev_info(dev, "version %d.%d\n", (ui->ver >> 4) & 0x0f,
+ (ui->ver >> 0) & 0x0f);
+
+ return 0;
+}
+
+static int ucsi_i2c_ccg_send_data(struct ucsi_i2c_ccg *ui)
+{
+ int err;
+ 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[0], ((const void *) ui->ppm.data) + 0x20, sizeof(buf1));
+ memcpy(&buf2[0], ((const void *) ui->ppm.data) + 0x8, sizeof(buf2));
+
+ err = ccg_write(ui, *(u16 *)buf, buf1, sizeof(buf1));
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_write failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ err = ccg_write(ui, *(u16 *)(buf+2), buf2, sizeof(buf2));
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_write failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ return err;
+}
+
+static int ucsi_i2c_ccg_recv_data(struct ucsi_i2c_ccg *ui)
+{
+ static int first = 1;
+ int err;
+ 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),
+ };
+ u8 *ppm = (u8 *)ui->ppm.data;
+
+ if (first) {
+ err = ccg_read(ui, *(u16 *)buf, ppm, 0x2);
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+ first = 1;
+ }
+
+ err = ccg_read(ui, *(u16 *)(buf + 2), ppm + 0x4, 0x4);
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ err = ccg_read(ui, *(u16 *)(buf + 4), ppm + 0x10, 0x10);
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ return err;
+}
+
+static int ucsi_i2c_ccg_ack_interrupt(struct ucsi_i2c_ccg *ui)
+{
+ int err;
+ unsigned char buf[3] = {(CCGX_I2C_RAB_INTR_REG & 0xff),
+ (CCGX_I2C_RAB_INTR_REG >> 8), 0x0};
+
+ err = ccg_read(ui, *(u16 *)buf, &buf[2], 0x1);
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ err = ccg_write(ui, *(u16 *)buf, &buf[2], 0x1);
+ if (err < 0) {
+ dev_err(ui->dev, "%s ccg_read failed, err %d\n",
+ __func__, err);
+ return -EIO;
+ }
+
+ return err;
+}
+
+static int ucsi_i2c_ccg_sync(struct ucsi_ppm *ppm)
+{
+ struct ucsi_i2c_ccg *ui = container_of(ppm,
+ struct ucsi_i2c_ccg, ppm);
+ int err;
+
+ err = ucsi_i2c_ccg_recv_data(ui);
+ if (err < 0) {
+ dev_err(ui->dev, "%s: ucsi_i2c_ccg_recv_data() err %d\n",
+ __func__, err);
+ goto exit;
+ }
+
+ /* ack interrupt to allow next command to run */
+ err = ucsi_i2c_ccg_ack_interrupt(ui);
+ if (err < 0) {
+ dev_err(ui->dev, "%s: ucsi_i2c_ccg_ack_interrupt() err %d\n",
+ __func__, err);
+ }
+exit:
+ return 0;
+}
+
+static int ucsi_i2c_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
+{
+ struct ucsi_i2c_ccg *ui = container_of(ppm,
+ struct ucsi_i2c_ccg, ppm);
+ int err = 0;
+
+ ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
+ err = ucsi_i2c_ccg_send_data(ui);
+
+ return err;
+}
+
+static irqreturn_t i2c_ccg_irq_handler(int irq, void *data)
+{
+ struct ucsi_i2c_ccg *ui = data;
+
+ dev_dbg(ui->dev, "%s irq %d data %p ui %p\n",
+ __func__, irq, data, ui);
+
+ if (!ui) {
+ dev_err(ui->dev, "%s: !ui\n", __func__);
+ return IRQ_HANDLED;
+ }
+
+ ucsi_notify(ui->ucsi);
+
+ return IRQ_HANDLED;
+}
+
+static int ucsi_i2c_ccg_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct ucsi_i2c_ccg *ui;
+ int status;
+
+ ui = devm_kzalloc(dev, sizeof(*ui), GFP_KERNEL);
+ if (!ui)
+ return -ENOMEM;
+
+ ui->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data), GFP_KERNEL);
+ if (!ui->ppm.data)
+ return -ENOMEM;
+
+ ui->ppm.cmd = ucsi_i2c_ccg_cmd;
+ ui->ppm.sync = ucsi_i2c_ccg_sync;
+ ui->dev = dev;
+ ui->client = client;
+
+ /* reset i2c device and initialize ucsi */
+ status = ucsi_i2c_ccg_init(ui);
+ if (status < 0) {
+ dev_err(ui->dev, "%s: ucsi_i2c_ccg_init failed - %d\n",
+ __func__, status);
+ return status;
+ }
+
+ ui->irq = client->irq;
+
+ status = devm_request_threaded_irq(dev, ui->irq, NULL,
+ i2c_ccg_irq_handler, IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+ dev_name(dev), ui);
+ if (status < 0) {
+ dev_err(ui->dev, "%s: request_irq failed - %d\n",
+ __func__, status);
+ return status;
+ }
+
+ ui->ucsi = ucsi_register_ppm(dev, &ui->ppm);
+ if (IS_ERR(ui->ucsi)) {
+ dev_err(ui->dev, "ucsi_register_ppm failed\n");
+ return PTR_ERR(ui->ucsi);
+ }
+
+ i2c_set_clientdata(client, ui);
+ return 0;
+}
+
+static int ucsi_i2c_ccg_remove(struct i2c_client *client)
+{
+ struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
+
+ ucsi_unregister_ppm(ui->ucsi);
+
+ return 0;
+}
+
+static int ucsi_i2c_ccg_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
+ int err;
+
+ if (device_may_wakeup(dev)) {
+ err = enable_irq_wake(ui->irq);
+ if (!err)
+ ui->wake_enabled = true;
+ }
+ return 0;
+}
+
+static int ucsi_i2c_ccg_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
+ struct ucsi_control c;
+ int ret;
+
+ if (device_may_wakeup(dev) && ui->wake_enabled) {
+ disable_irq_wake(ui->irq);
+ ui->wake_enabled = false;
+ }
+
+ /* restore UCSI notification enable mask */
+ UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
+ ret = ucsi_run_command(ui->ucsi, &c, NULL, 0);
+ if (ret) {
+ dev_err(ui->dev, "%s: failed to set notification enable - %d\n",
+ __func__, ret);
+ }
+
+ return 0;
+}
+
+UNIVERSAL_DEV_PM_OPS(ucsi_i2c_ccg_driver_pm, ucsi_i2c_ccg_suspend,
+ ucsi_i2c_ccg_resume, NULL);
+
+static const struct i2c_device_id ucsi_i2c_ccg_device_id[] = {
+ {"i2c-gpu-ucsi", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, ucsi_i2c_ccg_device_id);
+
+static struct i2c_driver ucsi_i2c_ccg_driver = {
+ .driver = {
+ .name = "ucsi_i2c_ccg",
+ .pm = &ucsi_i2c_ccg_driver_pm,
+ },
+ .probe = ucsi_i2c_ccg_probe,
+ .remove = ucsi_i2c_ccg_remove,
+ .id_table = ucsi_i2c_ccg_device_id,
+};
+
+module_i2c_driver(ucsi_i2c_ccg_driver);
+
+MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
+MODULE_DESCRIPTION("UCSI I2C driver for Cypress CCGx Type-C controller");
+MODULE_LICENSE("GPL v2");
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-29 23:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-29 23:19 [v2,2/2] usb: typec: ucsi: add support for Cypress CCGx Ajay Gupta
-- strict thread matches above, loose matches on Subject: below --
2018-08-27 12:47 kbuild test robot
2018-08-27 10:59 Heikki Krogerus
2018-08-25 16:35 Greg Kroah-Hartman
2018-08-25 16:29 Andy Shevchenko
2018-08-24 21:33 Ajay Gupta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.