| Message ID | 1538591248-15814-3-git-send-email-ajayg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for USB Type-C interface on latest NVIDIA GPU | expand |
Hi Ajay, I still have a few more comments below.. On Wed, Oct 03, 2018 at 11:27:28AM -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. > > 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 > Changes from v9 -> v10 > Fixed review comments from Peter > - Use UCSI macros > - Cleanups > Changes from v10 -> v11 > Fixed review comments from Peter > - Combined two writes into one > - Used offsetof() for ucsi_data struct > Changes from v11 -> v12 > - some cleanup > Changes from v12 -> v13 > - changed "u8 buf[1]" -> "u8 data" > > drivers/usb/typec/ucsi/Kconfig | 10 ++ > drivers/usb/typec/ucsi/Makefile | 2 + > drivers/usb/typec/ucsi/ucsi_ccg.c | 305 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 317 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..0e0bac1 > --- /dev/null > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c > @@ -0,0 +1,305 @@ > +// 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; That member is actually useless. > +}; > + > +#define CCGX_RAB_INTR_REG 0x06 > +#define CCGX_RAB_UCSI_CONTROL 0x39 > +#define CCGX_RAB_UCSI_CONTROL_START BIT(0) > +#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1) > +#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff)) > + > +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 = sizeof(buf), > + .buf = buf, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .buf = data, > + }, > + }; > + u32 rlen, rem_len = len; > + int status; > + > + /* i2c adapter (ccgx-ucsi) can read 4 byte max */ By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C host adapter? > + while (rem_len > 0) { > + msgs[1].buf = &data[len - rem_len]; > + rlen = min_t(u16, rem_len, 4); I guess this is where you check for that 4 bytes. I'm guessing this limitation is for the NVIDIA I2C host adapter. If that is the case than this driver really should not care about it. We most likely need to use this driver on other platforms as well where the I2C host is something else. > + 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; > + struct i2c_msg msgs[] = { > + { > + .addr = client->addr, > + .flags = 0x0, > + } > + }; > + int status; > + > + buf = kzalloc(len + sizeof(rab), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + put_unaligned_le16(rab, buf); > + memcpy(buf + sizeof(rab), data, len); > + > + msgs[0].len = len + sizeof(rab); > + msgs[0].buf = buf; > + > + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + if (status < 0) { > + dev_err(uc->dev, "i2c_transfer failed %d\n", status); > + kfree(buf); > + return status; > + } > + > + kfree(buf); > + return 0; > +} > + > +static int ucsi_ccg_init(struct ucsi_ccg *uc) > +{ > + unsigned int count = 10; > + u8 data; > + int status; > + > + data = CCGX_RAB_UCSI_CONTROL_STOP; > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, sizeof(data)); > + if (status < 0) > + return status; > + > + data = CCGX_RAB_UCSI_CONTROL_START; > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, sizeof(data)); > + if (status < 0) > + return status; > + > + /* > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi control > + * register write will push response which must be cleared. > + */ > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > + if (status < 0) > + return status; > + do { > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > + if (status < 0) > + return status; > + > + usleep_range(10000, 11000); > + > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > + if (status < 0) > + return status; > + } while ((data != 0x00) && count--); What's the significance of that count? Shouldn't you return -ETIMEDOUT if count == 0? Something like: ... while (count--) status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); if (status < 0) return status; if (!data) return 0; } return -ETIMEDOUT; Or does the count of 10 have some specific meaning? > +} > + > +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) > +{ > + u8 *ppm = (u8 *)uc->ppm.data; > + int status; > + u16 rab; > + > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_out)); > + status = ccg_write(uc, rab, ppm + > + offsetof(struct ucsi_data, message_out), > + sizeof(uc->ppm.data->message_out)); > + if (status < 0) > + return status; > + > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); > + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), > + sizeof(uc->ppm.data->ctrl)); > +} > + > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) > +{ > + u8 *ppm = (u8 *)uc->ppm.data; > + int status; > + u16 rab; > + > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); > + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), > + sizeof(uc->ppm.data->cci)); > + if (status < 0) > + return status; > + > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_in)); > + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), > + sizeof(uc->ppm.data->message_in)); > +} > + > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) > +{ > + int status; > + unsigned char data; > + > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > + if (status < 0) > + return status; > + > + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > +} > + > +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; > + u16 rab; > + > + 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); > + } > + > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, version)); > + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + > + offsetof(struct ucsi_data, version), > + sizeof(uc->ppm.data->version)); > + if (status < 0) { > + ucsi_unregister_ppm(uc->ucsi); > + return status; > + } > + > + 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"); I'm still worried about how this driver works on other platforms. It just looks like you have written ccg_read/write() functions for only your I2C host. I would feel much more comfortable with this if you for example used those i2c_smbus_read/write*() helpers instead of raw i2c_transfer(). I would expect them to force you to write your i2c host driver, as well as this driver, in a more generic fashion. Thanks,
Hi Heikki, > I still have a few more comments below.. Thanks for reviewing. > On Wed, Oct 03, 2018 at 11:27:28AM -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. > > > > 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 > > Changes from v9 -> v10 > > Fixed review comments from Peter > > - Use UCSI macros > > - Cleanups > > Changes from v10 -> v11 > > Fixed review comments from Peter > > - Combined two writes into one > > - Used offsetof() for ucsi_data struct Changes from v11 -> v12 > > - some cleanup > > Changes from v12 -> v13 > > - changed "u8 buf[1]" -> "u8 data" > > > > drivers/usb/typec/ucsi/Kconfig | 10 ++ > > drivers/usb/typec/ucsi/Makefile | 2 + > > drivers/usb/typec/ucsi/ucsi_ccg.c | 305 > > ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 317 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..0e0bac1 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c > > @@ -0,0 +1,305 @@ > > +// 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; > > That member is actually useless. I will drop it in next revision. > > +}; > > + > > +#define CCGX_RAB_INTR_REG 0x06 > > +#define CCGX_RAB_UCSI_CONTROL 0x39 > > +#define CCGX_RAB_UCSI_CONTROL_START BIT(0) > > +#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1) > > +#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & > 0xff)) > > + > > +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 = sizeof(buf), > > + .buf = buf, > > + }, > > + { > > + .addr = client->addr, > > + .flags = I2C_M_RD, > > + .buf = data, > > + }, > > + }; > > + u32 rlen, rem_len = len; > > + int status; > > + > > + /* i2c adapter (ccgx-ucsi) can read 4 byte max */ > > By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C > host adapter? It mean NVIDIA I2C host adapter with name "ccgx-ucsi" > > + while (rem_len > 0) { > > + msgs[1].buf = &data[len - rem_len]; > > + rlen = min_t(u16, rem_len, 4); > > I guess this is where you check for that 4 bytes. > > I'm guessing this limitation is for the NVIDIA I2C host adapter. Correct > If that is the case than this driver really should not care about it. I got your point but need to handle this case gracefully. I think best way to handle this is to add a runtime check to find I2C adapter's quirk and use quirks->max_read_len of the adapter. How does below look? @@ -247,6 +247,7 @@ struct ucsi_ccg { static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) { struct i2c_client *client = uc->client; + const struct i2c_adapter_quirks *quirks = client->adapter->quirks; unsigned char buf[2]; struct i2c_msg msgs[] = { { @@ -261,13 +262,16 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) .buf = data, }, }; - u32 rlen, rem_len = len; + u32 rlen, rem_len = len, max_read_len = len; int status; - /* i2c adapter (ccgx-ucsi) can read 4 byte max */ + /* check any max_read_len limitation on i2c adapter */ + if (quirks && quirks->max_read_len) + max_read_len = quirks->max_read_len; + while (rem_len > 0) { msgs[1].buf = &data[len - rem_len]; - rlen = min_t(u16, rem_len, 4); + rlen = min_t(u16, rem_len, max_read_len); msgs[1].len = rlen; put_unaligned_le16(rab, buf); status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > We most likely need to use this driver on other platforms as well > where the I2C host is something else. Correct and above solution would not impact other I2C host. > > > + 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; > > + struct i2c_msg msgs[] = { > > + { > > + .addr = client->addr, > > + .flags = 0x0, > > + } > > + }; > > + int status; > > + > > + buf = kzalloc(len + sizeof(rab), GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + put_unaligned_le16(rab, buf); > > + memcpy(buf + sizeof(rab), data, len); > > + > > + msgs[0].len = len + sizeof(rab); > > + msgs[0].buf = buf; > > + > > + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > + if (status < 0) { > > + dev_err(uc->dev, "i2c_transfer failed %d\n", status); > > + kfree(buf); > > + return status; > > + } > > + > > + kfree(buf); > > + return 0; > > +} > > + > > +static int ucsi_ccg_init(struct ucsi_ccg *uc) { > > + unsigned int count = 10; > > + u8 data; > > + int status; > > + > > + data = CCGX_RAB_UCSI_CONTROL_STOP; > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, > sizeof(data)); > > + if (status < 0) > > + return status; > > + > > + data = CCGX_RAB_UCSI_CONTROL_START; > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, > sizeof(data)); > > + if (status < 0) > > + return status; > > + > > + /* > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi > control > > + * register write will push response which must be cleared. > > + */ > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > + if (status < 0) > > + return status; > > + do { > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > > + if (status < 0) > > + return status; > > + > > + usleep_range(10000, 11000); > > + > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > > + if (status < 0) > > + return status; > > + } while ((data != 0x00) && count--); > > What's the significance of that count? It is like a retry count to clear interrupt status. > Shouldn't you return -ETIMEDOUT if count == 0? Yes. Good catch. Does the below fix looks ok? do { status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); if (status < 0) return status; usleep_range(10000, 11000); status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); if (status < 0) return status; if (!data) return 0; } while (data && count--); return -ETIMEDOUT; > Something like: > > ... > while (count--) > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > if (status < 0) > return status; > if (!data) > return 0; > } > > return -ETIMEDOUT; > > Or does the count of 10 have some specific meaning? > > > +} > > + > > +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) { > > + u8 *ppm = (u8 *)uc->ppm.data; > > + int status; > > + u16 rab; > > + > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > message_out)); > > + status = ccg_write(uc, rab, ppm + > > + offsetof(struct ucsi_data, message_out), > > + sizeof(uc->ppm.data->message_out)); > > + if (status < 0) > > + return status; > > + > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); > > + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), > > + sizeof(uc->ppm.data->ctrl)); > > +} > > + > > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) { > > + u8 *ppm = (u8 *)uc->ppm.data; > > + int status; > > + u16 rab; > > + > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); > > + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), > > + sizeof(uc->ppm.data->cci)); > > + if (status < 0) > > + return status; > > + > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > message_in)); > > + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), > > + sizeof(uc->ppm.data->message_in)); > > +} > > + > > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) { > > + int status; > > + unsigned char data; > > + > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > + if (status < 0) > > + return status; > > + > > + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); } > > + > > +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; > > + u16 rab; > > + > > + 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); > > + } > > + > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > version)); > > + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + > > + offsetof(struct ucsi_data, version), > > + sizeof(uc->ppm.data->version)); > > + if (status < 0) { > > + ucsi_unregister_ppm(uc->ucsi); > > + return status; > > + } > > + > > + 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"); > > I'm still worried about how this driver works on other platforms. It just looks > like you have written ccg_read/write() functions for only your I2C host. > > I would feel much more comfortable with this if you for example used those > i2c_smbus_read/write*() helpers instead of raw i2c_transfer(). > I would expect them to force you to write your i2c host driver, as well as this > driver, in a more generic fashion. I2c_smbus_read/write*() will not work with Cypress CCGx controller since CCGx requires 2 byte of command for any read/write transaction. I2c_smbus_read/write*() APIs support only 1 byte of command. Thanks Ajay --nvpublic > > > Thanks, > > -- > heikki
On Tue, Oct 23, 2018 at 06:56:59PM +0000, Ajay Gupta wrote: > > On Wed, Oct 03, 2018 at 11:27:28AM -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. > > > + /* > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi > > control > > > + * register write will push response which must be cleared. > > > + */ > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > + if (status < 0) > > > + return status; (1) > > > + do { > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; (2) > > > + > > > + usleep_range(10000, 11000); > > > + > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + } while ((data != 0x00) && count--); > > > > What's the significance of that count? > It is like a retry count to clear interrupt status. > > > Shouldn't you return -ETIMEDOUT if count == 0? > Yes. Good catch. Does the below fix looks ok? At least for me looks OK (I dunno why I missed that what Heikki found recently). Nevertheless, I have one more question about (1) and (2) above. Is it necessary to do one more read before do-while loop? > > do { > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > if (status < 0) > return status; > > usleep_range(10000, 11000); > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > if (status < 0) > return status; > > if (!data) > return 0; > } while (data && count--); > > return -ETIMEDOUT;
Hi Andy > > > > 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. > > > > > + /* > > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi > > > control > > > > + * register write will push response which must be cleared. > > > > + */ > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > > + if (status < 0) > > > > + return status; > > (1) > > > > > + do { > > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > > > sizeof(data)); > > > > + if (status < 0) > > > > + return status; > > (2) > > > > > + > > > > + usleep_range(10000, 11000); > > > > + > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > > sizeof(data)); > > > > + if (status < 0) > > > > + return status; > > > > + } while ((data != 0x00) && count--); > > > > > > What's the significance of that count? > > It is like a retry count to clear interrupt status. > > > > > Shouldn't you return -ETIMEDOUT if count == 0? > > Yes. Good catch. Does the below fix looks ok? > > At least for me looks OK (I dunno why I missed that what Heikki found > recently). > Nevertheless, I have one more question about (1) and (2) above. > > Is it necessary to do one more read before do-while loop? Yes, we need to read to get interrupt status bits and then write them back to clear the status. Thanks Ajay --nvpublic > > > > > do { > > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > if (status < 0) > > return status; > > > > usleep_range(10000, 11000); > > > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > if (status < 0) > > return status; > > > > if (!data) > > return 0; > > } while (data && count--); > > > > return -ETIMEDOUT; > > -- > With Best Regards, > Andy Shevchenko >
Hi, On Tue, Oct 23, 2018 at 06:56:59PM +0000, Ajay Gupta wrote: > > > + /* i2c adapter (ccgx-ucsi) can read 4 byte max */ > > > > By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C > > host adapter? > It mean NVIDIA I2C host adapter with name "ccgx-ucsi" > > > > + while (rem_len > 0) { > > > + msgs[1].buf = &data[len - rem_len]; > > > + rlen = min_t(u16, rem_len, 4); > > > > I guess this is where you check for that 4 bytes. > > > > I'm guessing this limitation is for the NVIDIA I2C host adapter. > Correct > > > If that is the case than this driver really should not care about it. > I got your point but need to handle this case gracefully. > > I think best way to handle this is to add a runtime check to find > I2C adapter's quirk and use quirks->max_read_len of the adapter. > How does below look? > > @@ -247,6 +247,7 @@ struct ucsi_ccg { > static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) > { > struct i2c_client *client = uc->client; > + const struct i2c_adapter_quirks *quirks = client->adapter->quirks; > unsigned char buf[2]; > struct i2c_msg msgs[] = { > { > @@ -261,13 +262,16 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) > .buf = data, > }, > }; > - u32 rlen, rem_len = len; > + u32 rlen, rem_len = len, max_read_len = len; > int status; > > - /* i2c adapter (ccgx-ucsi) can read 4 byte max */ > + /* check any max_read_len limitation on i2c adapter */ > + if (quirks && quirks->max_read_len) > + max_read_len = quirks->max_read_len; > + > while (rem_len > 0) { > msgs[1].buf = &data[len - rem_len]; > - rlen = min_t(u16, rem_len, 4); > + rlen = min_t(u16, rem_len, max_read_len); > msgs[1].len = rlen; > put_unaligned_le16(rab, buf); > status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > > We most likely need to use this driver on other platforms as well > > where the I2C host is something else. > Correct and above solution would not impact other I2C host. I still didn't understand why can't this just be taken care of in your I2C host driver? Why can't you just read 4 bytes at a time in your master_xfer hook until you have received as much as the message is asking, and only after that return? > > > + 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; > > > + struct i2c_msg msgs[] = { > > > + { > > > + .addr = client->addr, > > > + .flags = 0x0, > > > + } > > > + }; > > > + int status; > > > + > > > + buf = kzalloc(len + sizeof(rab), GFP_KERNEL); > > > + if (!buf) > > > + return -ENOMEM; > > > + > > > + put_unaligned_le16(rab, buf); > > > + memcpy(buf + sizeof(rab), data, len); > > > + > > > + msgs[0].len = len + sizeof(rab); > > > + msgs[0].buf = buf; > > > + > > > + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > > + if (status < 0) { > > > + dev_err(uc->dev, "i2c_transfer failed %d\n", status); > > > + kfree(buf); > > > + return status; > > > + } > > > + > > > + kfree(buf); > > > + return 0; > > > +} > > > + > > > +static int ucsi_ccg_init(struct ucsi_ccg *uc) { > > > + unsigned int count = 10; > > > + u8 data; > > > + int status; > > > + > > > + data = CCGX_RAB_UCSI_CONTROL_STOP; > > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + > > > + data = CCGX_RAB_UCSI_CONTROL_START; > > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + > > > + /* > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi > > control > > > + * register write will push response which must be cleared. > > > + */ > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + do { > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + > > > + usleep_range(10000, 11000); > > > + > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + } while ((data != 0x00) && count--); > > > > What's the significance of that count? > It is like a retry count to clear interrupt status. > > > Shouldn't you return -ETIMEDOUT if count == 0? > Yes. Good catch. Does the below fix looks ok? > > do { > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > if (status < 0) > return status; > > usleep_range(10000, 11000); > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > if (status < 0) > return status; > > if (!data) > return 0; > } while (data && count--); Doesn't that condition break out of the loop immediately? > return -ETIMEDOUT; > > > Something like: > > > > ... > > while (count--) > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > sizeof(data)); > > if (status < 0) > > return status; > > if (!data) > > return 0; > > } > > > > return -ETIMEDOUT; > > > > Or does the count of 10 have some specific meaning? > > > > > +} > > > + > > > +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) { > > > + u8 *ppm = (u8 *)uc->ppm.data; > > > + int status; > > > + u16 rab; > > > + > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > message_out)); > > > + status = ccg_write(uc, rab, ppm + > > > + offsetof(struct ucsi_data, message_out), > > > + sizeof(uc->ppm.data->message_out)); > > > + if (status < 0) > > > + return status; > > > + > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); > > > + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), > > > + sizeof(uc->ppm.data->ctrl)); > > > +} > > > + > > > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) { > > > + u8 *ppm = (u8 *)uc->ppm.data; > > > + int status; > > > + u16 rab; > > > + > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); > > > + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), > > > + sizeof(uc->ppm.data->cci)); > > > + if (status < 0) > > > + return status; > > > + > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > message_in)); > > > + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), > > > + sizeof(uc->ppm.data->message_in)); > > > +} > > > + > > > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) { > > > + int status; > > > + unsigned char data; > > > + > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > + if (status < 0) > > > + return status; > > > + > > > + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); } > > > + > > > +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; > > > + u16 rab; > > > + > > > + 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); > > > + } > > > + > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > version)); > > > + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + > > > + offsetof(struct ucsi_data, version), > > > + sizeof(uc->ppm.data->version)); > > > + if (status < 0) { > > > + ucsi_unregister_ppm(uc->ucsi); > > > + return status; > > > + } > > > + > > > + 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"); > > > > I'm still worried about how this driver works on other platforms. It just looks > > like you have written ccg_read/write() functions for only your I2C host. > > > > I would feel much more comfortable with this if you for example used those > > i2c_smbus_read/write*() helpers instead of raw i2c_transfer(). > > I would expect them to force you to write your i2c host driver, as well as this > > driver, in a more generic fashion. > > I2c_smbus_read/write*() will not work with Cypress CCGx controller since CCGx > requires 2 byte of command for any read/write transaction. > I2c_smbus_read/write*() APIs support only 1 byte of command. OK, got it. thanks,
On 2018-10-25 10:17, Heikki Krogerus wrote: > Hi, > > On Tue, Oct 23, 2018 at 06:56:59PM +0000, Ajay Gupta wrote: >>>> + /* i2c adapter (ccgx-ucsi) can read 4 byte max */ >>> >>> By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C >>> host adapter? >> It mean NVIDIA I2C host adapter with name "ccgx-ucsi" >> >>>> + while (rem_len > 0) { >>>> + msgs[1].buf = &data[len - rem_len]; >>>> + rlen = min_t(u16, rem_len, 4); >>> >>> I guess this is where you check for that 4 bytes. >>> >>> I'm guessing this limitation is for the NVIDIA I2C host adapter. >> Correct >> >>> If that is the case than this driver really should not care about it. >> I got your point but need to handle this case gracefully. >> >> I think best way to handle this is to add a runtime check to find >> I2C adapter's quirk and use quirks->max_read_len of the adapter. >> How does below look? >> >> @@ -247,6 +247,7 @@ struct ucsi_ccg { >> static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) >> { >> struct i2c_client *client = uc->client; >> + const struct i2c_adapter_quirks *quirks = client->adapter->quirks; >> unsigned char buf[2]; >> struct i2c_msg msgs[] = { >> { >> @@ -261,13 +262,16 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) >> .buf = data, >> }, >> }; >> - u32 rlen, rem_len = len; >> + u32 rlen, rem_len = len, max_read_len = len; >> int status; >> >> - /* i2c adapter (ccgx-ucsi) can read 4 byte max */ >> + /* check any max_read_len limitation on i2c adapter */ >> + if (quirks && quirks->max_read_len) >> + max_read_len = quirks->max_read_len; >> + >> while (rem_len > 0) { >> msgs[1].buf = &data[len - rem_len]; >> - rlen = min_t(u16, rem_len, 4); >> + rlen = min_t(u16, rem_len, max_read_len); >> msgs[1].len = rlen; >> put_unaligned_le16(rab, buf); >> status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); >> >>> We most likely need to use this driver on other platforms as well >>> where the I2C host is something else. >> Correct and above solution would not impact other I2C host. > > I still didn't understand why can't this just be taken care of in your > I2C host driver? Why can't you just read 4 bytes at a time in your > master_xfer hook until you have received as much as the message is > asking, and only after that return? The I2C host hardware *cannot* read more than 4 bytes in any one xfer according to the HW designers. Seriously broken crap, that piece of hardware... (If that assertion from the HW designers is indeed true? I suspect that it can be made to work, but the docs are closed and I don't have HW to experiment with, so it remains a suspicion...) And the I2C host driver *cannot* be expected to know exactly how any one client device needs to split xfers into many when the 4 byte limit is getting in the way, and neither can the I2C core. Because I bet there are devices where it's not even possible to split xfers while keeping semantics equivalent... So, every client driver will need to adjust to this quirk if they are to operate with this worthless I2C host (or others with similar limitations, if there are any?). Fortunately, most client drivers don't read in bulk. Unfortunately, many do... Cheers, Peter
Hi Peter, > > I still didn't understand why can't this just be taken care of in your > > I2C host driver? Why can't you just read 4 bytes at a time in your > > master_xfer hook until you have received as much as the message is > > asking, and only after that return? > > The I2C host hardware *cannot* read more than 4 bytes in any one xfer > according to the HW designers. Seriously broken crap, that piece of > hardware... (If that assertion from the HW designers is indeed true? > I suspect that it can be made to work, but the docs are closed and I > don't have HW to experiment with, so it remains a suspicion...) > > And the I2C host driver *cannot* be expected to know exactly how any > one client device needs to split xfers into many when the 4 byte limit > is getting in the way, and neither can the I2C core. Because I bet > there are devices where it's not even possible to split xfers while > keeping semantics equivalent... > > So, every client driver will need to adjust to this quirk if they are > to operate with this worthless I2C host (or others with similar > limitations, if there are any?). Fortunately, most client drivers don't > read in bulk. Unfortunately, many do... OK, thanks for the explanation. I think I'm repeating these questions. You guys already went through this, so sorry for the noise. Thanks,
On Wed, Oct 24, 2018 at 05:43:27PM +0000, Ajay Gupta wrote: > Hi Andy > > > > > > 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. > > > > > > > + /* > > > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi > > > > control > > > > > + * register write will push response which must be cleared. > > > > > + */ > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > > > + if (status < 0) > > > > > + return status; > > > > (1) > > > > > > > + do { > > > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > > > > sizeof(data)); > > > > > + if (status < 0) > > > > > + return status; > > > > (2) > > > > > > > + > > > > > + usleep_range(10000, 11000); > > > > > + > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > > > sizeof(data)); > > > > > + if (status < 0) > > > > > + return status; > > > > > + } while ((data != 0x00) && count--); > > > > > > > > What's the significance of that count? > > > It is like a retry count to clear interrupt status. > > > > > > > Shouldn't you return -ETIMEDOUT if count == 0? > > > Yes. Good catch. Does the below fix looks ok? > > > > At least for me looks OK (I dunno why I missed that what Heikki found > > recently). > > Nevertheless, I have one more question about (1) and (2) above. > > > > Is it necessary to do one more read before do-while loop? > Yes, we need to read to get interrupt status bits and then write > them back to clear the status. Ah, I see, but why you not reorganize it to put this into do-while loop? do { read write check for data sleep } while (--count); Also note predecrement.
Hi Andy, > > > > > > 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. > > > > > > > > > + /* > > > > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above > > > > > > +ucsi > > > > > control > > > > > > + * register write will push response which must be cleared. > > > > > > + */ > > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > > > > > > + if (status < 0) > > > > > > + return status; > > > > > > (1) > > > > > > > > > + do { > > > > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > > > > > sizeof(data)); > > > > > > + if (status < 0) > > > > > > + return status; > > > > > > (2) > > > > > > > > > + > > > > > > + usleep_range(10000, 11000); > > > > > > + > > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > > > > > sizeof(data)); > > > > > > + if (status < 0) > > > > > > + return status; > > > > > > + } while ((data != 0x00) && count--); > > > > > > > > > > What's the significance of that count? > > > > It is like a retry count to clear interrupt status. > > > > > > > > > Shouldn't you return -ETIMEDOUT if count == 0? > > > > Yes. Good catch. Does the below fix looks ok? > > > > > > At least for me looks OK (I dunno why I missed that what Heikki > > > found recently). > > > Nevertheless, I have one more question about (1) and (2) above. > > > > > > Is it necessary to do one more read before do-while loop? > > Yes, we need to read to get interrupt status bits and then write them > > back to clear the status. > > Ah, I see, but why you not reorganize it to put this into do-while loop? > > do { > read > write > check for data > sleep > } while (--count); > > Also note predecrement. Sure, will do that way! Thanks Ajay --nvpublic > > > > -- > With Best Regards, > Andy Shevchenko >
Hi Heikki, > > > I still didn't understand why can't this just be taken care of in > > > your I2C host driver? Why can't you just read 4 bytes at a time in > > > your master_xfer hook until you have received as much as the message > > > is asking, and only after that return? > > > > The I2C host hardware *cannot* read more than 4 bytes in any one xfer > > according to the HW designers. Seriously broken crap, that piece of > > hardware... (If that assertion from the HW designers is indeed true? > > I suspect that it can be made to work, but the docs are closed and I > > don't have HW to experiment with, so it remains a suspicion...) > > > > And the I2C host driver *cannot* be expected to know exactly how any > > one client device needs to split xfers into many when the 4 byte limit > > is getting in the way, and neither can the I2C core. Because I bet > > there are devices where it's not even possible to split xfers while > > keeping semantics equivalent... > > > > So, every client driver will need to adjust to this quirk if they are > > to operate with this worthless I2C host (or others with similar > > limitations, if there are any?). Fortunately, most client drivers > > don't read in bulk. Unfortunately, many do... > > OK, thanks for the explanation. > > I think I'm repeating these questions. You guys already went through this, so > sorry for the noise. I will go ahead and post new set. Thank you Peter for explanation! Thanks Ajay --nvpublic > > > Thanks, > > -- > heikki
Hi Heikki and Andy [...] > > > Shouldn't you return -ETIMEDOUT if count == 0? > > Yes. Good catch. Does the below fix looks ok? > > > > do { > > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > if (status < 0) > > return status; > > > > usleep_range(10000, 11000); > > > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > if (status < 0) > > return status; > > > > if (!data) > > return 0; > > } while (data && count--); > > Doesn't that condition break out of the loop immediately? How? I didn't get your point? We want to break out when data is zero (interrupt status cleared). > > Ah, I see, but why you not reorganize it to put this into do-while loop? We actually need to check data after reading it so will reorganize accordingly. do { read check for data and break out if (!data) write sleep } while (--count); Thanks Ajay -nvpublic > > return -ETIMEDOUT; > > > > > Something like: > > > > > > ... > > > while (count--) > > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > > > if (status < 0) > > > return status; > > > if (!data) > > > return 0; > > > } > > > > > > return -ETIMEDOUT; > > > > > > Or does the count of 10 have some specific meaning? > > > > > > > +} > > > > + > > > > +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) { > > > > + u8 *ppm = (u8 *)uc->ppm.data; > > > > + int status; > > > > + u16 rab; > > > > + > > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > > message_out)); > > > > + status = ccg_write(uc, rab, ppm + > > > > + offsetof(struct ucsi_data, message_out), > > > > + sizeof(uc->ppm.data->message_out)); > > > > + if (status < 0) > > > > + return status; > > > > + > > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); > > > > + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), > > > > + sizeof(uc->ppm.data->ctrl)); > > > > +} > > > > + > > > > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) { > > > > + u8 *ppm = (u8 *)uc->ppm.data; > > > > + int status; > > > > + u16 rab; > > > > + > > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); > > > > + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), > > > > + sizeof(uc->ppm.data->cci)); > > > > + if (status < 0) > > > > + return status; > > > > + > > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > > message_in)); > > > > + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), > > > > + sizeof(uc->ppm.data->message_in)); > > > > +} > > > > + > > > > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) { > > > > + int status; > > > > + unsigned char data; > > > > + > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > > + if (status < 0) > > > > + return status; > > > > + > > > > + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); } > > > > + > > > > +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; > > > > + u16 rab; > > > > + > > > > + 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); > > > > + } > > > > + > > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, > > > version)); > > > > + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + > > > > + offsetof(struct ucsi_data, version), > > > > + sizeof(uc->ppm.data->version)); > > > > + if (status < 0) { > > > > + ucsi_unregister_ppm(uc->ucsi); > > > > + return status; > > > > + } > > > > + > > > > + 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"); > > > > > > I'm still worried about how this driver works on other platforms. It > > > just looks like you have written ccg_read/write() functions for only your > I2C host. > > > > > > I would feel much more comfortable with this if you for example used > > > those > > > i2c_smbus_read/write*() helpers instead of raw i2c_transfer(). > > > I would expect them to force you to write your i2c host driver, as > > > well as this driver, in a more generic fashion. > > > > I2c_smbus_read/write*() will not work with Cypress CCGx controller > > since CCGx requires 2 byte of command for any read/write transaction. > > I2c_smbus_read/write*() APIs support only 1 byte of command. > > OK, got it. > > > thanks, > > -- > heikki
On 2018-10-25 23:55, Ajay Gupta wrote: > Hi Heikki and Andy > [...] >>>> Shouldn't you return -ETIMEDOUT if count == 0? >>> Yes. Good catch. Does the below fix looks ok? >>> >>> do { >>> status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); >>> if (status < 0) >>> return status; >>> >>> usleep_range(10000, 11000); >>> >>> status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); >>> if (status < 0) >>> return status; >>> >>> if (!data) >>> return 0; >>> } while (data && count--); >> >> Doesn't that condition break out of the loop immediately? > How? I didn't get your point? We want to break out when data is > zero (interrupt status cleared). The statement if (!data) return 0; ensures that 'data' is non-zero when the loop continues, so checking that 'data' is non-zero in the while loop test is pointless. >>> Ah, I see, but why you not reorganize it to put this into do-while loop? > We actually need to check data after reading it so will reorganize accordingly. > do { > read > check for data and break out if (!data) > write > sleep > } while (--count); Here, you have fixed the "issue" (but it doesn't match v14). Cheers, Peter
On Thu, Oct 25, 2018 at 09:55:47PM +0000, Ajay Gupta wrote: > Hi Heikki and Andy > [...] > > > > Shouldn't you return -ETIMEDOUT if count == 0? > > > Yes. Good catch. Does the below fix looks ok? > > > > > > do { > > > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > if (status < 0) > > > return status; > > > > > > usleep_range(10000, 11000); > > > > > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > > > if (status < 0) > > > return status; > > > > > > if (!data) > > > return 0; > > > } while (data && count--); > > > > Doesn't that condition break out of the loop immediately? > How? I didn't get your point? We want to break out when data is > zero (interrupt status cleared). Sorry Ajay. My brain interpreted that "while" as an "if" statement :-) thanks,
Hi Peter and Andy, > >>>> Shouldn't you return -ETIMEDOUT if count == 0? > >>> Yes. Good catch. Does the below fix looks ok? > >>> > >>> do { > >>> status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, > sizeof(data)); > >>> if (status < 0) > >>> return status; > >>> > >>> usleep_range(10000, 11000); > >>> > >>> status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); > >>> if (status < 0) > >>> return status; > >>> > >>> if (!data) > >>> return 0; > >>> } while (data && count--); > >> > >> Doesn't that condition break out of the loop immediately? > > How? I didn't get your point? We want to break out when data is zero > > (interrupt status cleared). > > The statement > > if (!data) > return 0; > > ensures that 'data' is non-zero when the loop continues, so checking that > 'data' is non-zero in the while loop test is pointless. > > >>> Ah, I see, but why you not reorganize it to put this into do-while loop? > > We actually need to check data after reading it so will reorganize > accordingly. Sorry, my bad, will revise. Thanks Ajay --nvpublic > > do { > > read > > check for data and break out if (!data) > > write > > sleep > > } while (--count); > > Here, you have fixed the "issue" (but it doesn't match v14). > > Cheers, > Peter
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..0e0bac1 --- /dev/null +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c @@ -0,0 +1,305 @@ +// 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_RAB_INTR_REG 0x06 +#define CCGX_RAB_UCSI_CONTROL 0x39 +#define CCGX_RAB_UCSI_CONTROL_START BIT(0) +#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1) +#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff)) + +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 = sizeof(buf), + .buf = buf, + }, + { + .addr = client->addr, + .flags = I2C_M_RD, + .buf = data, + }, + }; + u32 rlen, rem_len = len; + int status; + + /* i2c adapter (ccgx-ucsi) can read 4 byte max */ + 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; + struct i2c_msg msgs[] = { + { + .addr = client->addr, + .flags = 0x0, + } + }; + int status; + + buf = kzalloc(len + sizeof(rab), GFP_KERNEL); + if (!buf) + return -ENOMEM; + + put_unaligned_le16(rab, buf); + memcpy(buf + sizeof(rab), data, len); + + msgs[0].len = len + sizeof(rab); + msgs[0].buf = buf; + + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + if (status < 0) { + dev_err(uc->dev, "i2c_transfer failed %d\n", status); + kfree(buf); + return status; + } + + kfree(buf); + return 0; +} + +static int ucsi_ccg_init(struct ucsi_ccg *uc) +{ + unsigned int count = 10; + u8 data; + int status; + + data = CCGX_RAB_UCSI_CONTROL_STOP; + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, sizeof(data)); + if (status < 0) + return status; + + data = CCGX_RAB_UCSI_CONTROL_START; + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, sizeof(data)); + if (status < 0) + return status; + + /* + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi control + * register write will push response which must be cleared. + */ + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + do { + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + + usleep_range(10000, 11000); + + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + } while ((data != 0x00) && count--); + + return 0; +} + +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) +{ + u8 *ppm = (u8 *)uc->ppm.data; + int status; + u16 rab; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_out)); + status = ccg_write(uc, rab, ppm + + offsetof(struct ucsi_data, message_out), + sizeof(uc->ppm.data->message_out)); + if (status < 0) + return status; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), + sizeof(uc->ppm.data->ctrl)); +} + +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) +{ + u8 *ppm = (u8 *)uc->ppm.data; + int status; + u16 rab; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), + sizeof(uc->ppm.data->cci)); + if (status < 0) + return status; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_in)); + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), + sizeof(uc->ppm.data->message_in)); +} + +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) +{ + int status; + unsigned char data; + + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); +} + +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; + u16 rab; + + 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); + } + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, version)); + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + + offsetof(struct ucsi_data, version), + sizeof(uc->ppm.data->version)); + if (status < 0) { + ucsi_unregister_ppm(uc->ucsi); + return status; + } + + 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");