On 2018-09-07 19:28, Ajay Gupta wrote:
> Hi Peter,
>
>> -----Original Message-----
>> From: Peter Rosin <peda@xxxxxxxxxx>
>> Sent: Friday, September 7, 2018 2:13 AM
>> To: Ajay Gupta <ajayg@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx;
>> heikki.krogerus@xxxxxxxxxxxxxxx
>> Cc: linux-usb@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v9 2/2] usb: typec: ucsi: add support for Cypress CCGx
>>
>> On 2018-09-07 01:56, Ajay Gupta wrote:
>>> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller over I2C
>>> interface.
>>>
>>> This UCSI I2C driver uses I2C bus driver interface for communicating
>>> with Type-C controller.
>>>
>>> Signed-off-by: Ajay Gupta <ajayg@xxxxxxxxxx>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>> Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>>> ---
>>> Changes from v1 -> v2
>>> Fixed identation in drivers/usb/typec/ucsi/Kconfig Changes from v2 ->
>>> v3
>>> Fixed most of comments from Heikki
>>> Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
>>> Changes from v3 -> v4
>>> Fixed comments from Andy
>>> Changes from v4 -> v5
>>> Fixed comments from Andy
>>> Changes from v5 -> v6
>>> Fixed review comments from Greg
>>> Changes from v6 -> v7
>>> None
>>> Changes from v7 -> v8
>>> Fixed review comments from Peter
>>> - Removed empty STOP message
>>> - Using stack memory for i2c_transfer() Changes from v8 -> v9
>>> None
>>>
>>> drivers/usb/typec/ucsi/Kconfig | 10 ++
>>> drivers/usb/typec/ucsi/Makefile | 2 +
>>> drivers/usb/typec/ucsi/ucsi_ccg.c | 335
>>> ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 347 insertions(+)
>>> create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
>>>
>>> diff --git a/drivers/usb/typec/ucsi/Kconfig
>>> b/drivers/usb/typec/ucsi/Kconfig index e36d6c7..7811888 100644
>>> --- a/drivers/usb/typec/ucsi/Kconfig
>>> +++ b/drivers/usb/typec/ucsi/Kconfig
>>> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>>>
>>> if TYPEC_UCSI
>>>
>>> +config UCSI_CCG
>>> + tristate "UCSI Interface Driver for Cypress CCGx"
>>> + depends on I2C
>>> + help
>>> + This driver enables UCSI support on platforms that expose a
>>> + Cypress CCGx Type-C controller over I2C interface.
>>> +
>>> + To compile the driver as a module, choose M here: the module will
>> be
>>> + called ucsi_ccg.
>>> +
>>> config UCSI_ACPI
>>> tristate "UCSI ACPI Interface Driver"
>>> depends on ACPI
>>> diff --git a/drivers/usb/typec/ucsi/Makefile
>>> b/drivers/usb/typec/ucsi/Makefile index 7afbea5..2f4900b 100644
>>> --- a/drivers/usb/typec/ucsi/Makefile
>>> +++ b/drivers/usb/typec/ucsi/Makefile
>>> @@ -8,3 +8,5 @@ typec_ucsi-y := ucsi.o
>>> typec_ucsi-$(CONFIG_TRACING) += trace.o
>>>
>>> obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
>>> +
>>> +obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
>>> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> new file mode 100644
>>> index 0000000..387b6fd
>>> --- /dev/null
>>> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> @@ -0,0 +1,335 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * UCSI driver for Cypress CCGx Type-C controller
>>> + *
>>> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
>>> + * Author: Ajay Gupta <ajayg@xxxxxxxxxx>
>>> + *
>>> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
>>> + */
>>> +#include <linux/acpi.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include <asm/unaligned.h>
>>> +#include "ucsi.h"
>>> +
>>> +struct ucsi_ccg {
>>> + struct device *dev;
>>> + struct ucsi *ucsi;
>>> + struct ucsi_ppm ppm;
>>> + struct i2c_client *client;
>>> + int irq;
>>> +};
>>> +
>>> +#define CCGX_I2C_RAB_DEVICE_MODE 0x00
>>> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x2
>>> +#define CCGX_I2C_RAB_INTR_REG 0x06
>>> +#define CCGX_I2C_RAB_FW1_VERSION 0x28
>>> +#define CCGX_I2C_RAB_FW2_VERSION 0x20
>>> +#define CCGX_I2C_RAB_UCSI_CONTROL 0x39
>>> +#define CCGX_I2C_RAB_UCSI_CONTROL_START BIT(0)
>>> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP BIT(1)
>>> +#define CCGX_I2C_RAB_RESPONSE_REG 0x7E
>>> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000
>>> +
>>> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> + struct i2c_client *client = uc->client;
>>> + unsigned char buf[2];
>>> + struct i2c_msg msgs[] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .len = 0x2,
>>> + .buf = buf,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = I2C_M_RD,
>>> + .buf = data,
>>> + },
>>> + };
>>> + u32 rlen, rem_len = len;
>>> + int status;
>>> +
>>> + while (rem_len > 0) {
>>> + msgs[1].buf = &data[len - rem_len];
>>> + rlen = min_t(u16, rem_len, 4);
>>> + msgs[1].len = rlen;
>>> + put_unaligned_le16(rab, buf);
>>
>> Why not simply do whichever is correct of
>>
>> buf[0] = rab >> 8;
>> buf[1] = rab;
>>
>> and
>>
>> buf[0] = rab;
>> buf[1] = rab >> 8;
>>
>> and feed rab as a cpu-native value and get rid of the endianess crap.
> It was like that but was changed to put_unaligned_le16() in one of
> review comments from Andy at
> https://marc.info/?l=linux-usb&m=153561689418696&w=2
>
> I would rather stay with put_unaligned_le16() which looks better to me
> and is similar to your suggestion of using i2c_8bit_addr_from_msg() in 1/2
> patch of series.
Right, put_unaligned_le16 is the correct thing to do if rab is cpu-native.
I was confused by the fact that some users of ccg_read/ccg_write do
not call with a cpu-native rab. But since I also suggested that rab
should be made cpu-native for those cases, I guess I'm still to blame...
Sorry about that!
>>> + status = i2c_transfer(client->adapter, msgs,
>> ARRAY_SIZE(msgs));
>>> + if (status < 0) {
>>> + dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> + return status;
>>> + }
>>> + rab += rlen;
>>> + rem_len -= rlen;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> + struct i2c_client *client = uc->client;
>>> + unsigned char buf[2];
>>> + struct i2c_msg msgs[] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .len = 0x2,
>>> + .buf = buf,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .buf = data,
>>> + .len = len,
>>> + },
>>> + };
>>> + int status;
>>> +
>>> + put_unaligned_le16(rab, buf);
>>
>> Dito.
> See above.
>>
>>> + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>>> + if (status < 0) {
>>> + dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> + return status;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
>>> + struct device *dev = uc->dev;
>>> + unsigned int count = 10;
>>> + u8 data[64];
>>> + int status;
>>> +
>>> + /*
>>> + * Selectively issue device reset
>>> + * - if RESPONSE register is RESET_COMPLETE, do not issue device
>> reset
>>> + * (will cause usb device disconnect / reconnect)
>>> + * - if RESPONSE register is not RESET_COMPLETE, issue device reset
>>> + * (causes PPC to resync device connect state by re-issuing
>>> + * set mux command)
>>> + */
>>> + data[0] = 0x00;
>>> + data[1] = 0x00;
>>
>> Why do you need these assigments? Will not ccg_read just overwrite this
>> anyway?
> ok
>>
>>> +
>>> + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + memset(data, 0, sizeof(data));
>>
>> Dito.
> ok
>>
>>> + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
>> sizeof(data));
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + dev_dbg(dev, "Silicon id %2ph", data +
>> CCGX_I2C_RAB_READ_SILICON_ID);
>>> + dev_dbg(dev, "FW1 version %8ph\n", data +
>> CCGX_I2C_RAB_FW1_VERSION);
>>> + dev_dbg(dev, "FW2 version %8ph\n", data +
>> CCGX_I2C_RAB_FW2_VERSION);
>>> +
>>> + data[0] = 0x0;
>>> + data[1] = 0x0;
>>
>> Dito.
> ok
>>
>>> + status = ccg_read(uc, CCGX_I2C_RAB_RESPONSE_REG, data, 0x2);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
>>> + status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
>>> + status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + /*
>>> + * Flush CCGx RESPONSE queue by acking interrupts
>>> + * - above ucsi control register write will push response
>>> + * which must be flushed
>>> + * - affects f/w update which reads response register
>>> + */
>>> + data[0] = 0xff;
>>> + do {
>>> + status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + usleep_range(10000, 11000);
>>> +
>>> + status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> + } while ((data[0] != 0x00) && count--);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
>>> + int status;
>>> + unsigned char buf[4] = {
>>> + 0x20, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
>>> + 0x8, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
>>> + };
>>> + unsigned char buf1[16];
>>> + unsigned char buf2[8];
>>> +
>>> + memcpy(buf1, ((const void *)uc->ppm.data) + 0x20, sizeof(buf1));
>>> + memcpy(buf2, ((const void *)uc->ppm.data) + 0x8, sizeof(buf2));
>>> +
>>> + status = ccg_write(uc, *(u16 *)buf, buf1, sizeof(buf1));
>>
>> This seems to be endian-dependent. May I suggest that you do as suggested
>> above for ccg_read, and then somthing like
>>
>> #define CCGX_I2C_RAB_USCI_DATA_BLOCK(xxx) (0xf000 | ((xxx) & <mask>))
>>
>> where you of course use an appropriate value for <mask> (perhaps 0xff, or
>> 0xfff, what do I know) and a better name for the field than xxx (perhaps len,
>> what do I know), and then finally do
>>
>> status = ccg_write(uc, CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20), ...
>>
>> Also, the 0x20 and 0x8 are repeated and are some magic numbers that really
>> should be given a name or some explanation. They appear to be data lengths,
>> but again, what do I know?
> I will check on this.
>From the below reference, it's
0x8 is USBC_CONTROL with USBC_CONTROL_SIZE 0x8 (64/8)
0x20 is USBC_MESSAGE_OUT with USBC_MESSAGE_OUT_SIZE 0x10 (128/8)
You could do
#define USBC_MESSAGE_OUT CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20)
#define USBC_MESSAGE_OUT_SIZE (128/8)
etc, so that it becomes
unsigned char buf1[USBC_MESSAGE_OUT_SIZE];
...
status = ccg_write(uc, USBC_MESSAGE_OUT, buf1, sizeof(buf1));
Which is a whole lot more readable IMHO.
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + return ccg_write(uc, *(u16 *)(buf + 2), buf2, sizeof(buf2)); }
>>> +
>>> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
>>> + u8 *ppm = (u8 *)uc->ppm.data;
>>> + int status;
>>> + unsigned char buf[6] = {
>>> + 0x0, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
>>> + 0x4, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
>>> + 0x10, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
>>> + };
>>> +
>>> + status = ccg_read(uc, *(u16 *)buf, ppm, 0x2);
>>
>> There are plenty magic numbers, but this call does not follow the pattern.
>> Should perhaps buf[0] be 0x2, or should perhaps the last 0x2 argument be
>> 0x0? All other ...DATA_BLOCK calls seem to have the len in the other byte of
>> the rab argument. Why does this call not follow the pattern?
> We are reading message IN data from Type-C controller in response to a
> UCSI command. You can find details at
> https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/usb-type-c-ucsi-spec.pdf
So, according to table 3-1,
0x0 is UCSI_VERSION with UCSI_VERSION_SIZE 0x2 (16/8)
0x4 is USBC_CCI (connector change indication) with USBC_CCI_SIZE 0x4 (32/8)
0x10 is USBC_MESSAGE_IN with USBC_MESSAGE_IN_SIZE 0x10 (128/8)
The pattern for 0x4 and 0x10 was a accidental, but again, *please* use
defines for all these magic numbers.
Cheers,
Peter
