On 2018-09-12 20:02, Ajay Gupta wrote:
> Hi Peter,
>
>>> This driver adds I2C bus driver to communicate with Type-C controller.
>>> I2C client driver will be part of USB Type-C UCSI driver.
>>>
>>> Signed-off-by: Ajay Gupta <ajayg@xxxxxxxxxx>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>>> ---
>>> Changes from v1 -> v2
>>> None
>>> Changes from v2 -> v3
>>> Fixed review comments from Andy and Thierry
>>> Rename i2c-gpu.c -> i2c-nvidia-gpu.c
>>> Changes from v3 -> v4
>>> Fixed review comments from Andy
>>> Changes from v4 -> v5
>>> Fixed review comments from Andy
>>> Changes from v5 -> v6
>>> None
>>> Changes from v6 -> v7 -> v8
>>> Fixed review comments from Peter
>>> - Add implicit STOP for last write message
>>> - Add i2c_adapter_quirks with max_read_len and
>>> I2C_AQ_COMB flags
>>> Changes from v8 -> v9
>>> Fixed review comments from Peter
>>> - Drop do_start flag
>>> - Use i2c_8bit_addr_from_msg()
>>> Changes from v9 -> v10
>>> Fixed review comments from Peter
>>> - Dropped I2C_FUNC_SMBUS_EMUL
>>> - Dropped local mutex
>>> Changes from v10 -> v11
>>> Fixed review comments from Peter
>>> - Moved stop in master_xfer at end of message
>>> - Change i2c_read without STOP
>>> - Dropped I2C_AC_COMB* flags
>>> Changes from v11 -> v12
>>> Fixed review comments from Peter
>>> - Removed clearing of empty bits
>>> - Fix master_xfer for correct stop use
>>>
>>> Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++
>>> MAINTAINERS | 7 +
>>> drivers/i2c/busses/Kconfig | 9 +
>>> drivers/i2c/busses/Makefile | 1 +
>>> drivers/i2c/busses/i2c-nvidia-gpu.c | 368
>> ++++++++++++++++++++++++++++++++
>>> 5 files changed, 403 insertions(+)
>>> create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
>>> create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c
>>>
>>> diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu
>>> b/Documentation/i2c/busses/i2c-nvidia-gpu
>>> new file mode 100644
>>> index 0000000..31884d2
>>> --- /dev/null
>>> +++ b/Documentation/i2c/busses/i2c-nvidia-gpu
>>> @@ -0,0 +1,18 @@
>>> +Kernel driver i2c-nvidia-gpu
>>> +
>>> +Datasheet: not publicly available.
>>> +
>>> +Authors:
>>> + Ajay Gupta <ajayg@xxxxxxxxxx>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA
>>> +Turing and later GPUs and it is used to communicate with Type-C controller
>> on GPUs.
>>> +
>>> +If your 'lspci -v' listing shows something like the following,
>>> +
>>> +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9
>>> +(rev a1)
>>> +
>>> +then this driver should support the I2C controller of your GPU.
>>> diff --git a/MAINTAINERS b/MAINTAINERS index d870cb5..b71b0b4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6798,6 +6798,13 @@ L: linux-acpi@xxxxxxxxxxxxxxx
>>> S: Maintained
>>> F: drivers/i2c/i2c-core-acpi.c
>>>
>>> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
>>> +M: Ajay Gupta <ajayg@xxxxxxxxxx>
>>> +L: linux-i2c@xxxxxxxxxxxxxxx
>>> +S: Maintained
>>> +F: Documentation/i2c/busses/i2c-nvidia-gpu
>>> +F: drivers/i2c/busses/i2c-nvidia-gpu.c
>>> +
>>> I2C MUXES
>>> M: Peter Rosin <peda@xxxxxxxxxx>
>>> L: linux-i2c@xxxxxxxxxxxxxxx
>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>> index 451d4ae..eed827b 100644
>>> --- a/drivers/i2c/busses/Kconfig
>>> +++ b/drivers/i2c/busses/Kconfig
>>> @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985
>>> This driver can also be built as a module. If so, the module
>>> will be called i2c-nforce2-s4985.
>>>
>>> +config I2C_NVIDIA_GPU
>>> + tristate "NVIDIA GPU I2C controller"
>>> + depends on PCI
>>> + help
>>> + If you say yes to this option, support will be included for the
>>> + NVIDIA GPU I2C controller which is used to communicate with the
>> GPU's
>>> + Type-C controller. This driver can also be built as a module called
>>> + i2c-nvidia-gpu.
>>> +
>>> config I2C_SIS5595
>>> tristate "SiS 5595"
>>> depends on PCI
>>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>>> index 18b26af..d499813 100644
>>> --- a/drivers/i2c/busses/Makefile
>>> +++ b/drivers/i2c/busses/Makefile
>>> @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
>>> obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
>>> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
>>> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>>> +obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o
>>>
>>> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git
>>> a/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> b/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> new file mode 100644
>>> index 0000000..0c16b0a
>>> --- /dev/null
>>> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> @@ -0,0 +1,368 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Nvidia GPU I2C controller Driver
>>> + *
>>> + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
>>> + * Author: Ajay Gupta <ajayg@xxxxxxxxxx> */ #include <linux/delay.h>
>>> +#include <linux/i2c.h> #include <linux/interrupt.h> #include
>>> +<linux/module.h> #include <linux/pci.h> #include
>>> +<linux/platform_device.h> #include <linux/pm.h> #include
>>> +<linux/pm_runtime.h>
>>> +
>>> +#include <asm/unaligned.h>
>>> +
>>> +/* I2C definitions */
>>> +#define I2C_MST_CNTL 0x00
>>> +#define I2C_MST_CNTL_GEN_START BIT(0)
>>> +#define I2C_MST_CNTL_GEN_STOP BIT(1)
>>> +#define I2C_MST_CNTL_CMD_READ (1 << 2)
>>> +#define I2C_MST_CNTL_CMD_WRITE (2 << 2)
>>> +#define I2C_MST_CNTL_BURST_SIZE_SHIFT 6
>>> +#define I2C_MST_CNTL_GEN_NACK BIT(28)
>>> +#define I2C_MST_CNTL_STATUS GENMASK(30, 29)
>>> +#define I2C_MST_CNTL_STATUS_OKAY (0 << 29)
>>> +#define I2C_MST_CNTL_STATUS_NO_ACK (1 << 29)
>>> +#define I2C_MST_CNTL_STATUS_TIMEOUT (2 << 29)
>>> +#define I2C_MST_CNTL_STATUS_BUS_BUSY (3 << 29)
>>> +#define I2C_MST_CNTL_CYCLE_TRIGGER BIT(31)
>>> +
>>> +#define I2C_MST_ADDR 0x04
>>> +
>>> +#define I2C_MST_I2C0_TIMING 0x08
>>> +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ 0x10e
>>> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT 16
>>> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX 255
>>> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CHECK BIT(24)
>>> +
>>> +#define I2C_MST_DATA 0x0c
>>> +
>>> +#define I2C_MST_HYBRID_PADCTL 0x20
>>> +#define I2C_MST_HYBRID_PADCTL_MODE_I2C BIT(0)
>>> +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV
>> BIT(14)
>>> +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV
>> BIT(15)
>>> +
>>> +struct gpu_i2c_dev {
>>> + struct device *dev;
>>> + void __iomem *regs;
>>> + struct i2c_adapter adapter;
>>> + struct i2c_board_info *gpu_ccgx_ucsi; };
>>> +
>>> +static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd) {
>>> + u32 val;
>>> +
>>> + /* enable I2C */
>>> + val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
>>> + val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
>>> + I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
>>> + I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
>>> + writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
>>> +
>>> + /* enable 100KHZ mode */
>>> + val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
>>> + val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
>>> + << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
>>> + val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
>>> + writel(val, i2cd->regs + I2C_MST_I2C0_TIMING); }
>>> +
>>> +static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd) {
>>> + unsigned long target = jiffies + msecs_to_jiffies(1000);
>>> + u32 val;
>>> +
>>> + do {
>>> + val = readl(i2cd->regs + I2C_MST_CNTL);
>>> + if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
>>
>> It occurred to me that generating NACK only makes sense for reads, and that
>> you only want to have a NACK after the final byte in a read messages. So,
>> here's a new attempt.
>>
>> What if you replace the above test with
>>
>> if (!(val & I2C_MST_CNTL_READ))
>>
>> (since all cycle-triggers are also reads, at least currently, and we also want to
>> wait for the tail reads to complete before we try to get the received data from
>> the register)
>>
>> and then...
>>
>>> + break;
>>> + if ((val & I2C_MST_CNTL_STATUS) !=
>>> + I2C_MST_CNTL_STATUS_BUS_BUSY)
>>> + break;
>>> + usleep_range(1000, 2000);
>>> + } while (time_is_after_jiffies(target));
>>> +
>>> + if (time_is_before_jiffies(target)) {
>>> + dev_err(i2cd->dev, "i2c timeout error %x\n", val);
>>> + return -ETIME;
>>> + }
>>> +
>>> + val = readl(i2cd->regs + I2C_MST_CNTL);
>>> + switch (val & I2C_MST_CNTL_STATUS) {
>>> + case I2C_MST_CNTL_STATUS_OKAY:
>>> + return 0;
>>> + case I2C_MST_CNTL_STATUS_NO_ACK:
>>> + return -EIO;
>>> + case I2C_MST_CNTL_STATUS_TIMEOUT:
>>> + return -ETIME;
>>> + case I2C_MST_CNTL_STATUS_BUS_BUSY:
>>> + return -EBUSY;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
>>> +{
>>> + int status;
>>> + u32 val;
>>> +
>>> + val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_READ |
>>> + (len << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
>>> + I2C_MST_CNTL_CYCLE_TRIGGER |
>> I2C_MST_CNTL_GEN_NACK;
>>> + writel(val, i2cd->regs + I2C_MST_CNTL);
>>> +
>>> + status = gpu_i2c_check_status(i2cd);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + val = readl(i2cd->regs + I2C_MST_DATA);
>>> + switch (len) {
>>> + case 1:
>>> + data[0] = val;
>>> + break;
>>> + case 2:
>>> + put_unaligned_be16(val, data);
>>> + break;
>>> + case 3:
>>> + put_unaligned_be16(val >> 8, data);
>>> + data[2] = val;
>>> + break;
>>> + case 4:
>>> + put_unaligned_be32(val, data);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> + return status;
>>> +}
>>
>> ...replace your gpu_i2c_read with this:
>>
>> #define GPU_MAX_BURST 1
>> static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len) {
>> u16 burst = min(len, GPU_MAX_BURST);
>> int status;
>> u32 val;
>>
>> val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_READ |
>> (burst << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
>> I2C_MST_CNTL_CYCLE_TRIGGER;
>>
>> for (;;) {
>> if (len <= GPU_MAX_BURST)
>> val |= I2C_MST_CNTL_GEN_NACK;
>> writel(val, i2cd->regs + I2C_MST_CNTL);
>>
>> status = gpu_i2c_check_status(i2cd);
>> if (status < 0)
>> return status;
>>
>> val = readl(i2cd->regs + I2C_MST_DATA);
>> switch (burst) {
>> case 1:
>> data[0] = val;
>> break;
>> case 2:
>> put_unaligned_be16(val, data);
>> break;
>> case 3:
>> put_unaligned_be16(val >> 8, data);
>> data[2] = val;
>> break;
>> case 4:
>> put_unaligned_be32(val, data);
>> break;
>> default:
>> break;
>> }
>>
>> if (len <= GPU_MAX_BURST)
>> break;
>>
>> data += GPU_MAX_BURST;
>> len -= GPU_MAX_BURST;
>>
>> burst = min(len, GPU_MAX_BURST);
>> val = I2C_MST_CNTL_CMD_READ |
>> (burst << I2C_MST_CNTL_BURST_SIZE_SHIFT);
>> }
>>
>> return status;
>> }
>>
>> If that works,
>> then change GPU_MAX_BURST to 4, drop the quirk and the
>> splitting of the reads in patch 2/2 and check that too...
>>
>> *fingers crossed*
> Unfortunately, that also didn't work. I tried some tweaks with _TRIGGER
> and _START but that also didn't help.
Did you notice the above change to gpu_i2c_check_status? I assume so
and that there simply is something about the chip that is not understood.
Crap.
>>> +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) {
>>> + struct i2c_client *ccgx_client;
>>> +
>>> + i2cd->gpu_ccgx_ucsi = devm_kzalloc(i2cd->dev,
>>> + sizeof(struct i2c_board_info),
>>
>> I prefer sizeof(*i2cd->gpu_ccgx_ucsi). Especially when the type is far away as
>> it is here...
> First one looks more readable and cleaner to me but will change.
>
> sizeof(struct i2c_board_info),
> sizeof(*i2cd->gpu_ccgx_ucsi),
In my experience, the latter variant is easier to keep correct if/when the
code is changed. But yes, it is a matter of taste...
>>> + GFP_KERNEL);
>>> + if (!i2cd->gpu_ccgx_ucsi)
>>> + return -ENOMEM;
>>> +
>>> + strlcpy(i2cd->gpu_ccgx_ucsi->type, "ccgx-ucsi",
>>> + sizeof(i2cd->gpu_ccgx_ucsi->type));
>>> + i2cd->gpu_ccgx_ucsi->addr = 0x8;
>>> + i2cd->gpu_ccgx_ucsi->irq = irq;
>>> + ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
>>> + if (!ccgx_client)
>>> + return -ENODEV;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int gpu_i2c_probe(struct pci_dev *pdev, const struct
>>> +pci_device_id *id) {
>>> + struct gpu_i2c_dev *i2cd;
>>> + int status;
>>> +
>>> + i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev),
>>> +GFP_KERNEL);
>>
>> While at it, you might as well change this to sizeof(*i2cd), and
> Ok.
>
>> please check for the pattern in patch 2/2.
> I hope you saw the latest response at [1]. The change works on multiple
> systems and no error has been reported yet.
> What else (and how) to check ?
>
> [1] https://marc.info/?l=linux-usb&m=153667127502959&w=2
Yes, I saw that. I generally assume that patches work for the sender,
but if I see something that I can't understand how it can work, it tend
to not hold on to the assumption and explicitly ask if the code has
in fact been tested...
Cheers,
Peter
