usb: typec: ucsi: add support for Cypress CCGx
PATCH v13 2/2 usb: typec: ucsi: add support for Cypress CCGx
Hi Ajay,
I still have a few more comments below..
Quoted 100 lines · 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 <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>Acked-by Heikki Krogerus <[email protected]>---
Changes from v1 -> v2
Fixed identation indrivers/usb/typec/ucsi/Kconfig
Changes from v2 -> v3Fixed most of comments from Heikki Rename ucsi_i2c_ccg.c -> ucsi_ccg.cChanges 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 fori2c_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
- Usedoffsetof()forucsi_datastruct
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.cdrivers/usb/typec/ucsi/Kconfig +10
-0@@ -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 ACPIdrivers/usb/typec/ucsi/Makefile +2
-0@@ -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.odrivers/usb/typec/ucsi/ucsi_ccg.c new file +15
-0@@ -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 <[email protected]> +* +* 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>
drivers/usb/typec/ucsi/ucsi_ccg.c line 25
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +10
-0+ +#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.
Quoted 19 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +19
-0+}; + +#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, + },
drivers/usb/typec/ucsi/ucsi_ccg.c line 54
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +10
-0+ { + .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?
drivers/usb/typec/ucsi/ucsi_ccg.c line 57
Quoted 3 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +3
-0+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.
Quoted 71 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +71
-0+ 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 {
drivers/usb/typec/ucsi/ucsi_ccg.c line 138
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +10
-0+ 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?
Quoted 155 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +155
-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" , + },
drivers/usb/typec/ucsi/ucsi_ccg.c line 303
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi_ccg.c new file +10
-0+ .probe = ucsi_ccg_probe, + .remove = ucsi_ccg_remove, + .id_table = ucsi_ccg_device_id, +}; + +module_i2c_driver (ucsi_ccg_driver); + +MODULE_AUTHOR ("Ajay Gupta <[email protected]>" ); +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,