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 <aj...@nvidia.com>
> >>> Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> >>> Reviewed-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> >>> ---
> >>> 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 <aj...@nvidia.com>
> >>> +
> >>> +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-a...@vger.kernel.org
> >>> S: Maintained
> >>> F: drivers/i2c/i2c-core-acpi.c
> >>>
> >>> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> >>> +M: Ajay Gupta <aj...@nvidia.com>
> >>> +L: linux-...@vger.kernel.org
> >>> +S: Maintained
> >>> +F: Documentation/i2c/busses/i2c-nvidia-gpu
> >>> +F: drivers/i2c/busses/i2c-nvidia-gpu.c
> >>> +
> >>> I2C MUXES
> >>> M: Peter Rosin <p...@axentia.se>
> >>> L: linux-...@vger.kernel.org
> >>> 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 <aj...@nvidia.com> */ #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?
Yes.
> I assume so and
> that there simply is something about the chip that is not understood.
Can we get the working set in while the issues is being debugged?
> >>> +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...
So I assume nothing more left to check on this for now.
Thanks
Ajay
--nvpublic
> Cheers,
> Peter
