[v12,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

Message ID	1536687902-31666-1-git-send-email-ajayg@nvidia.com
State	Superseded
Headers	show Return-Path: <linux-i2c-owner@vger.kernel.org> TLS: TLSv1.2, DES-CBC3-SHA) id <B5b97ff320000>; Tue, 11 Sep 2018 10:45:22 -0700 From: Ajay Gupta <ajayg@nvidia.com> To: <wsa@the-dreams.de>, <heikki.krogerus@linux.intel.com> CC: <linux-usb@vger.kernel.org>, <linux-i2c@vger.kernel.org>, Ajay Gupta <ajayg@nvidia.com> Subject: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU Date: Tue, 11 Sep 2018 10:45:01 -0700 Message-ID: <1536687902-31666-1-git-send-email-ajayg@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-i2c-owner@vger.kernel.org Precedence: bulk
Series	[v12,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU \| expand [v12,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU [v12,2/2] usb: typec: ucsi: add support for Cypress CCGx

Ajay Gupta Sept. 11, 2018, 5:45 p.m. UTC

Latest NVIDIA GPU card has USB Type-C interface. There is a
Type-C controller which can be accessed over I2C.

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@nvidia.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@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

Peter Rosin Sept. 11, 2018, 8:57 p.m. UTC | #1

On 2018-09-11 19:45, Ajay Gupta wrote:
> Latest NVIDIA GPU card has USB Type-C interface. There is a
> Type-C controller which can be accessed over I2C.
> 
> 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@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/i2c-core-acpi.c
>  
> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> +M:	Ajay Gupta <ajayg@nvidia.com>
> +L:	linux-i2c@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 <peda@axentia.se>
>  L:	linux-i2c@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 <ajayg@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))
> +			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;
> +}
> +

I'm out on a limb here and obviously don't have documentation, but could
you try to add a function like this

static int gpu_i2c_read_byte(struct gpu_i2c_dev *i2cd, u8 *data)
{
	int status;
	u32 val;

	val = I2C_MST_CNTL_CMD_READ | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
		I2C_MST_CNTL_GEN_NACK;
	writel(val, i2cd->regs + I2C_MST_CNTL);

	status = gpu_i2c_check_status(i2cd);
	if (status < 0)
		return status;

	*data = readl(i2cd->regs + I2C_MST_DATA);

	return status;
}

(i.e. I'm attempting to just read bytes w/o doing any starts, addresses
etc, just like the gpu_i2c_write function does for writes)

and then ...

> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
> +{
> +	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
> +{
> +	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
> +{
> +	u32 val;
> +
> +	writel(data, i2cd->regs + I2C_MST_DATA);
> +
> +	val = I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> +		I2C_MST_CNTL_GEN_NACK;
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> +	int status, status2;
> +	int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* program client address before starting read */
> +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> +			/* gpu_i2c_read has implicit start */
> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);

... replace the third arg with a hard-coded 1, like this

			status = gpu_i2c_read(i2cd, msgs[i].buf, 1);

and ...

> +			if (status < 0) {
> +				if (i == 0)
> +					return status;
> +				goto stop;
> +			}

... add a loop here, like so

			for (j = 1; j < msgs[i].len; j++) {
				status = gpu_i2c_read_byte(i2cd,
							   &msgs[i].buf[j]);
				if (status < 0)
					goto stop;
			}

then I wonder if it still works? If it does, you may remove the below
max_read_len quirk, and the message splitting in the patch 2/2 can also
be zapped.

Further, if it works, it should be possible to optimize this and read
in chunks of four instead of byte-by-byte. The reason I hard-code the
length to 1 in the gpu_i2c_read call is to force the loop to be used
for testing with the existing client which limits reads to four bytes.
So, if the read loop works, then it's time to instead call gpu_i2c_read
with a length that is the minimum of msgs[i].len and 4, and to start
the read loop with j = 4 instead of j = 1.

If it doesn't work, maybe you can tweak something in gpu_i2c_read_byte
to make it work?

Hmm, maybe the read loop should be moved into gpu_i2c_read so that this
function is kept as-is? But that's for later. If you do a test as I
suggest above, we will know if it's possible to do a read loop at all.
How that read loop code should eventually look is another matter...

Cheers,
Peter

> +		} else {
> +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> +
> +			status = gpu_i2c_start(i2cd);
> +			if (status < 0) {
> +				if (i == 0)
> +					return status;
> +				goto stop;
> +			}
> +
> +			status = gpu_i2c_write(i2cd, addr);
> +			if (status < 0)
> +				goto stop;
> +
> +			for (j = 0; j < msgs[i].len; j++) {
> +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> +				if (status < 0)
> +					goto stop;
> +			}
> +		}
> +	}
> +	status = gpu_i2c_stop(i2cd);
> +	if (status < 0)
> +		return status;
> +	return i;
> +stop:
> +	status2 = gpu_i2c_stop(i2cd);
> +	if (status2 < 0)
> +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> +	return status;
> +}
> +
> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> +	.max_read_len = 4,
> +};
> +
> +static u32 gpu_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm gpu_i2c_algorithm = {
> +	.master_xfer	= gpu_i2c_master_xfer,
> +	.functionality	= gpu_i2c_functionality,
> +};
> +
> +/*
> + * This driver is for Nvidia GPU cards with USB Type-C interface.
> + * We want to identify the cards using vendor ID and class code only
> + * to avoid dependency of adding product id for any new card which
> + * requires this driver.
> + * Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN class for now and it will be updated when UCSI
> + * over PCI gets a class code.
> + * There is no other NVIDIA cards with UNKNOWN class code. Even if the
> + * driver gets loaded for an undesired card then eventually i2c_read()
> + * (initiated from UCSI i2c_client) will timeout or UCSI commands will
> + * timeout.
> + */
> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> +static const struct pci_device_id gpu_i2c_ids[] = {
> +	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> +		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> +
> +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),
> +					   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);
> +	if (!i2cd)
> +		return -ENOMEM;
> +
> +	i2cd->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, i2cd);
> +
> +	status = pcim_enable_device(pdev);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
> +		return status;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> +	if (!i2cd->regs) {
> +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
> +		return status;
> +	}
> +
> +	gpu_enable_i2c_bus(i2cd);
> +
> +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> +	i2cd->adapter.owner = THIS_MODULE;
> +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> +		sizeof(i2cd->adapter.name));
> +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> +	i2cd->adapter.dev.parent = &pdev->dev;
> +	status = i2c_add_adapter(&i2cd->adapter);
> +	if (status < 0)
> +		goto free_irq_vectors;
> +
> +	status = gpu_populate_client(i2cd, pdev->irq);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
> +		goto del_adapter;
> +	}
> +
> +	return 0;
> +
> +del_adapter:
> +	i2c_del_adapter(&i2cd->adapter);
> +free_irq_vectors:
> +	pci_free_irq_vectors(pdev);
> +	return status;
> +}
> +
> +static void gpu_i2c_remove(struct pci_dev *pdev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> +
> +	i2c_del_adapter(&i2cd->adapter);
> +	pci_free_irq_vectors(pdev);
> +}
> +
> +static int gpu_i2c_resume(struct device *dev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> +
> +	gpu_enable_i2c_bus(i2cd);
> +	return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
> +
> +static struct pci_driver gpu_i2c_driver = {
> +	.name		= "nvidia-gpu",
> +	.id_table	= gpu_i2c_ids,
> +	.probe		= gpu_i2c_probe,
> +	.remove		= gpu_i2c_remove,
> +	.driver		= {
> +		.pm	= &gpu_i2c_driver_pm,
> +	},
> +};
> +
> +module_pci_driver(gpu_i2c_driver);
> +
> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
>

Ajay Gupta Sept. 11, 2018, 10:48 p.m. UTC | #2

Hi Peter,

> > Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C
> > controller which can be accessed over I2C.
> >
> > 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@nvidia.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/i2c/i2c-core-acpi.c
> >
> > +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> > +M:	Ajay Gupta <ajayg@nvidia.com>
> > +L:	linux-i2c@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 <peda@axentia.se>
> >  L:	linux-i2c@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 <ajayg@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))
> > +			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;
> > +}
> > +
> 
> I'm out on a limb here and obviously don't have documentation, but could you
> try to add a function like this
> 
> static int gpu_i2c_read_byte(struct gpu_i2c_dev *i2cd, u8 *data) {
> 	int status;
> 	u32 val;
> 
> 	val = I2C_MST_CNTL_CMD_READ | (1 <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> 		I2C_MST_CNTL_GEN_NACK;
> 	writel(val, i2cd->regs + I2C_MST_CNTL);
> 
> 	status = gpu_i2c_check_status(i2cd);
> 	if (status < 0)
> 		return status;
> 
> 	*data = readl(i2cd->regs + I2C_MST_DATA);
> 
> 	return status;
> }
> 
> (i.e. I'm attempting to just read bytes w/o doing any starts, addresses etc, just
> like the gpu_i2c_write function does for writes)
> 
> and then ...
> 
> > +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd) {
> > +	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> > +	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> > +	u32 val;
> > +
> > +	writel(data, i2cd->regs + I2C_MST_DATA);
> > +
> > +	val = I2C_MST_CNTL_CMD_WRITE | (1 <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > +		I2C_MST_CNTL_GEN_NACK;
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > +			       struct i2c_msg *msgs, int num) {
> > +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > +	int status, status2;
> > +	int i, j;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (msgs[i].flags & I2C_M_RD) {
> > +			/* program client address before starting read */
> > +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> > +			/* gpu_i2c_read has implicit start */
> > +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> 
> ... replace the third arg with a hard-coded 1, like this
> 
> 			status = gpu_i2c_read(i2cd, msgs[i].buf, 1);
> 
> and ...
> 
> > +			if (status < 0) {
> > +				if (i == 0)
> > +					return status;
> > +				goto stop;
> > +			}
> 
> ... add a loop here, like so
> 
> 			for (j = 1; j < msgs[i].len; j++) {
> 				status = gpu_i2c_read_byte(i2cd,
> 							   &msgs[i].buf[j]);
> 				if (status < 0)
> 					goto stop;
> 			}
> 
> then I wonder if it still works? 
It didn't work.

> If it does, you may remove the below
> max_read_len quirk, and the message splitting in the patch 2/2 can also be
> zapped.
> 
> Further, if it works, it should be possible to optimize this and read in chunks of
> four instead of byte-by-byte. The reason I hard-code the length to 1 in the
> gpu_i2c_read call is to force the loop to be used for testing with the existing
> client which limits reads to four bytes.
> So, if the read loop works, then it's time to instead call gpu_i2c_read with a
> length that is the minimum of msgs[i].len and 4, and to start the read loop
> with j = 4 instead of j = 1.
> 
> If it doesn't work, maybe you can tweak something in gpu_i2c_read_byte to
> make it work?
I tried a few tweaks but it didn't work that way. I think it may take time to get 
this part working.

Can we update this part later when we have it working and let this patch merged?

Thanks
Ajay
--nvpublic
> Hmm, maybe the read loop should be moved into gpu_i2c_read so that this
> function is kept as-is? But that's for later. 
> If you do a test as I suggest above,
> we will know if it's possible to do a read loop at all.
> How that read loop code should eventually look is another matter...
> 
> Cheers,
> Peter
> 
> > +		} else {
> > +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> > +
> > +			status = gpu_i2c_start(i2cd);
> > +			if (status < 0) {
> > +				if (i == 0)
> > +					return status;
> > +				goto stop;
> > +			}
> > +
> > +			status = gpu_i2c_write(i2cd, addr);
> > +			if (status < 0)
> > +				goto stop;
> > +
> > +			for (j = 0; j < msgs[i].len; j++) {
> > +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> > +				if (status < 0)
> > +					goto stop;
> > +			}
> > +		}
> > +	}
> > +	status = gpu_i2c_stop(i2cd);
> > +	if (status < 0)
> > +		return status;
> > +	return i;
> > +stop:
> > +	status2 = gpu_i2c_stop(i2cd);
> > +	if (status2 < 0)
> > +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> > +	return status;
> > +}
> > +
> > +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> > +	.max_read_len = 4,
> > +};
> > +
> > +static u32 gpu_i2c_functionality(struct i2c_adapter *adap) {
> > +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> > +
> > +static const struct i2c_algorithm gpu_i2c_algorithm = {
> > +	.master_xfer	= gpu_i2c_master_xfer,
> > +	.functionality	= gpu_i2c_functionality,
> > +};
> > +
> > +/*
> > + * This driver is for Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code only
> > + * to avoid dependency of adding product id for any new card which
> > + * requires this driver.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN class for now and it will be updated when UCSI
> > + * over PCI gets a class code.
> > + * There is no other NVIDIA cards with UNKNOWN class code. Even if
> > +the
> > + * driver gets loaded for an undesired card then eventually
> > +i2c_read()
> > + * (initiated from UCSI i2c_client) will timeout or UCSI commands
> > +will
> > + * timeout.
> > + */
> > +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> > +static const struct pci_device_id gpu_i2c_ids[] = {
> > +	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > +		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> > +
> > +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),
> > +					   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);
> > +	if (!i2cd)
> > +		return -ENOMEM;
> > +
> > +	i2cd->dev = &pdev->dev;
> > +	dev_set_drvdata(&pdev->dev, i2cd);
> > +
> > +	status = pcim_enable_device(pdev);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> > +	if (!i2cd->regs) {
> > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +
> > +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> > +	i2cd->adapter.owner = THIS_MODULE;
> > +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> > +		sizeof(i2cd->adapter.name));
> > +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> > +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> > +	i2cd->adapter.dev.parent = &pdev->dev;
> > +	status = i2c_add_adapter(&i2cd->adapter);
> > +	if (status < 0)
> > +		goto free_irq_vectors;
> > +
> > +	status = gpu_populate_client(i2cd, pdev->irq);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n",
> status);
> > +		goto del_adapter;
> > +	}
> > +
> > +	return 0;
> > +
> > +del_adapter:
> > +	i2c_del_adapter(&i2cd->adapter);
> > +free_irq_vectors:
> > +	pci_free_irq_vectors(pdev);
> > +	return status;
> > +}
> > +
> > +static void gpu_i2c_remove(struct pci_dev *pdev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> > +
> > +	i2c_del_adapter(&i2cd->adapter);
> > +	pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static int gpu_i2c_resume(struct device *dev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +	return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> NULL);
> > +
> > +static struct pci_driver gpu_i2c_driver = {
> > +	.name		= "nvidia-gpu",
> > +	.id_table	= gpu_i2c_ids,
> > +	.probe		= gpu_i2c_probe,
> > +	.remove		= gpu_i2c_remove,
> > +	.driver		= {
> > +		.pm	= &gpu_i2c_driver_pm,
> > +	},
> > +};
> > +
> > +module_pci_driver(gpu_i2c_driver);
> > +
> > +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> > +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> > +MODULE_LICENSE("GPL v2");
> >

Peter Rosin Sept. 12, 2018, 10:25 a.m. UTC | #3

On 2018-09-12 00:48, Ajay Gupta wrote:
>> If it doesn't work, maybe you can tweak something in gpu_i2c_read_byte to
>> make it work?
> I tried a few tweaks but it didn't work that way. I think it may take time to get 
> this part working.
> 
> Can we update this part later when we have it working and let this patch merged?

We could, but it feels like what I suggested should be *very* close. I'll write
a new response to v12 1/2 with a new attempt...

I've spent so much time looking at this, and this last limitation annoys me.

Cheers,
Peter

Peter Rosin Sept. 12, 2018, 11:02 a.m. UTC | #4

On 2018-09-11 19:45, Ajay Gupta wrote:
> Latest NVIDIA GPU card has USB Type-C interface. There is a
> Type-C controller which can be accessed over I2C.
> 
> 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@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/i2c-core-acpi.c
>  
> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> +M:	Ajay Gupta <ajayg@nvidia.com>
> +L:	linux-i2c@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 <peda@axentia.se>
>  L:	linux-i2c@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 <ajayg@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*

> +
> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
> +{
> +	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
> +{
> +	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
> +{
> +	u32 val;
> +
> +	writel(data, i2cd->regs + I2C_MST_DATA);
> +
> +	val = I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> +		I2C_MST_CNTL_GEN_NACK;

Generating NACK for a write does not make sense, which is what I think
the last flag means? It's the client device that has the power to ACK/NACK
writes.

> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> +	int status, status2;
> +	int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* program client address before starting read */
> +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> +			/* gpu_i2c_read has implicit start */
> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> +			if (status < 0) {
> +				if (i == 0)
> +					return status;

I missed this in my first reply to this message. You want an unconditional
"goto stop" here, so please drop the "if (i == 0) return status;" thing.
My reasoning is that a spurious stop is - while undesirable - much better
than a missing stop.

> +				goto stop;
> +			}
> +		} else {
> +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> +
> +			status = gpu_i2c_start(i2cd);
> +			if (status < 0) {
> +				if (i == 0)
> +					return status;
> +				goto stop;
> +			}
> +
> +			status = gpu_i2c_write(i2cd, addr);
> +			if (status < 0)
> +				goto stop;
> +
> +			for (j = 0; j < msgs[i].len; j++) {
> +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> +				if (status < 0)
> +					goto stop;
> +			}
> +		}
> +	}
> +	status = gpu_i2c_stop(i2cd);
> +	if (status < 0)
> +		return status;
> +	return i;
> +stop:
> +	status2 = gpu_i2c_stop(i2cd);
> +	if (status2 < 0)
> +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> +	return status;
> +}
> +
> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> +	.max_read_len = 4,
> +};
> +
> +static u32 gpu_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm gpu_i2c_algorithm = {
> +	.master_xfer	= gpu_i2c_master_xfer,
> +	.functionality	= gpu_i2c_functionality,
> +};
> +
> +/*
> + * This driver is for Nvidia GPU cards with USB Type-C interface.
> + * We want to identify the cards using vendor ID and class code only
> + * to avoid dependency of adding product id for any new card which
> + * requires this driver.
> + * Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN class for now and it will be updated when UCSI
> + * over PCI gets a class code.
> + * There is no other NVIDIA cards with UNKNOWN class code. Even if the
> + * driver gets loaded for an undesired card then eventually i2c_read()
> + * (initiated from UCSI i2c_client) will timeout or UCSI commands will
> + * timeout.
> + */
> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> +static const struct pci_device_id gpu_i2c_ids[] = {
> +	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> +		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> +
> +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...

> +					   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 please check
for the pattern in patch 2/2.

Cheers,
Peter

> +	if (!i2cd)
> +		return -ENOMEM;
> +
> +	i2cd->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, i2cd);
> +
> +	status = pcim_enable_device(pdev);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
> +		return status;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> +	if (!i2cd->regs) {
> +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
> +		return status;
> +	}
> +
> +	gpu_enable_i2c_bus(i2cd);
> +
> +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> +	i2cd->adapter.owner = THIS_MODULE;
> +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> +		sizeof(i2cd->adapter.name));
> +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> +	i2cd->adapter.dev.parent = &pdev->dev;
> +	status = i2c_add_adapter(&i2cd->adapter);
> +	if (status < 0)
> +		goto free_irq_vectors;
> +
> +	status = gpu_populate_client(i2cd, pdev->irq);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
> +		goto del_adapter;
> +	}
> +
> +	return 0;
> +
> +del_adapter:
> +	i2c_del_adapter(&i2cd->adapter);
> +free_irq_vectors:
> +	pci_free_irq_vectors(pdev);
> +	return status;
> +}
> +
> +static void gpu_i2c_remove(struct pci_dev *pdev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> +
> +	i2c_del_adapter(&i2cd->adapter);
> +	pci_free_irq_vectors(pdev);
> +}
> +
> +static int gpu_i2c_resume(struct device *dev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> +
> +	gpu_enable_i2c_bus(i2cd);
> +	return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
> +
> +static struct pci_driver gpu_i2c_driver = {
> +	.name		= "nvidia-gpu",
> +	.id_table	= gpu_i2c_ids,
> +	.probe		= gpu_i2c_probe,
> +	.remove		= gpu_i2c_remove,
> +	.driver		= {
> +		.pm	= &gpu_i2c_driver_pm,
> +	},
> +};
> +
> +module_pci_driver(gpu_i2c_driver);
> +
> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
>

Ajay Gupta Sept. 12, 2018, 6:02 p.m. UTC | #5

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@nvidia.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/i2c/i2c-core-acpi.c
> >
> > +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> > +M:	Ajay Gupta <ajayg@nvidia.com>
> > +L:	linux-i2c@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 <peda@axentia.se>
> >  L:	linux-i2c@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 <ajayg@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.
 
> > +
> > +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd) {
> > +	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> > +	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> > +	u32 val;
> > +
> > +	writel(data, i2cd->regs + I2C_MST_DATA);
> > +
> > +	val = I2C_MST_CNTL_CMD_WRITE | (1 <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > +		I2C_MST_CNTL_GEN_NACK;
> 
> Generating NACK for a write does not make sense, which is what I think the
> last flag means? It's the client device that has the power to ACK/NACK writes.
Ok, will check.
 
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > +			       struct i2c_msg *msgs, int num) {
> > +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > +	int status, status2;
> > +	int i, j;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (msgs[i].flags & I2C_M_RD) {
> > +			/* program client address before starting read */
> > +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> > +			/* gpu_i2c_read has implicit start */
> > +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> > +			if (status < 0) {
> > +				if (i == 0)
> > +					return status;
> 
> I missed this in my first reply to this message. You want an unconditional
> "goto stop" here, so please drop the "if (i == 0) return status;" thing.
> My reasoning is that a spurious stop is - while undesirable - much better than
> a missing stop.
Ok.
 
> > +				goto stop;
> > +			}
> > +		} else {
> > +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> > +
> > +			status = gpu_i2c_start(i2cd);
> > +			if (status < 0) {
> > +				if (i == 0)
> > +					return status;
> > +				goto stop;
> > +			}
> > +
> > +			status = gpu_i2c_write(i2cd, addr);
> > +			if (status < 0)
> > +				goto stop;
> > +
> > +			for (j = 0; j < msgs[i].len; j++) {
> > +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> > +				if (status < 0)
> > +					goto stop;
> > +			}
> > +		}
> > +	}
> > +	status = gpu_i2c_stop(i2cd);
> > +	if (status < 0)
> > +		return status;
> > +	return i;
> > +stop:
> > +	status2 = gpu_i2c_stop(i2cd);
> > +	if (status2 < 0)
> > +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> > +	return status;
> > +}
> > +
> > +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> > +	.max_read_len = 4,
> > +};
> > +
> > +static u32 gpu_i2c_functionality(struct i2c_adapter *adap) {
> > +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> > +
> > +static const struct i2c_algorithm gpu_i2c_algorithm = {
> > +	.master_xfer	= gpu_i2c_master_xfer,
> > +	.functionality	= gpu_i2c_functionality,
> > +};
> > +
> > +/*
> > + * This driver is for Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code only
> > + * to avoid dependency of adding product id for any new card which
> > + * requires this driver.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN class for now and it will be updated when UCSI
> > + * over PCI gets a class code.
> > + * There is no other NVIDIA cards with UNKNOWN class code. Even if
> > +the
> > + * driver gets loaded for an undesired card then eventually
> > +i2c_read()
> > + * (initiated from UCSI i2c_client) will timeout or UCSI commands
> > +will
> > + * timeout.
> > + */
> > +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> > +static const struct pci_device_id gpu_i2c_ids[] = {
> > +	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > +		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> > +
> > +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),

> > +					   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 

Thanks
Ajay

--nvpublic
> 
> Cheers,
> Peter
> 
> > +	if (!i2cd)
> > +		return -ENOMEM;
> > +
> > +	i2cd->dev = &pdev->dev;
> > +	dev_set_drvdata(&pdev->dev, i2cd);
> > +
> > +	status = pcim_enable_device(pdev);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> > +	if (!i2cd->regs) {
> > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +
> > +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> > +	i2cd->adapter.owner = THIS_MODULE;
> > +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> > +		sizeof(i2cd->adapter.name));
> > +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> > +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> > +	i2cd->adapter.dev.parent = &pdev->dev;
> > +	status = i2c_add_adapter(&i2cd->adapter);
> > +	if (status < 0)
> > +		goto free_irq_vectors;
> > +
> > +	status = gpu_populate_client(i2cd, pdev->irq);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n",
> status);
> > +		goto del_adapter;
> > +	}
> > +
> > +	return 0;
> > +
> > +del_adapter:
> > +	i2c_del_adapter(&i2cd->adapter);
> > +free_irq_vectors:
> > +	pci_free_irq_vectors(pdev);
> > +	return status;
> > +}
> > +
> > +static void gpu_i2c_remove(struct pci_dev *pdev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> > +
> > +	i2c_del_adapter(&i2cd->adapter);
> > +	pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static int gpu_i2c_resume(struct device *dev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +	return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> NULL);
> > +
> > +static struct pci_driver gpu_i2c_driver = {
> > +	.name		= "nvidia-gpu",
> > +	.id_table	= gpu_i2c_ids,
> > +	.probe		= gpu_i2c_probe,
> > +	.remove		= gpu_i2c_remove,
> > +	.driver		= {
> > +		.pm	= &gpu_i2c_driver_pm,
> > +	},
> > +};
> > +
> > +module_pci_driver(gpu_i2c_driver);
> > +
> > +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> > +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> > +MODULE_LICENSE("GPL v2");
> >

Peter Rosin Sept. 12, 2018, 9:57 p.m. UTC | #6

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@nvidia.com>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
>>>  S:	Maintained
>>>  F:	drivers/i2c/i2c-core-acpi.c
>>>
>>> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
>>> +M:	Ajay Gupta <ajayg@nvidia.com>
>>> +L:	linux-i2c@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 <peda@axentia.se>
>>>  L:	linux-i2c@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 <ajayg@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? 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

Ajay Gupta Sept. 12, 2018, 10:22 p.m. UTC | #7

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@nvidia.com>
> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >>> Reviewed-by: Heikki Krogerus <heikki.krogerus@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 <ajayg@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-acpi@vger.kernel.org
> >>>  S:	Maintained
> >>>  F:	drivers/i2c/i2c-core-acpi.c
> >>>
> >>> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> >>> +M:	Ajay Gupta <ajayg@nvidia.com>
> >>> +L:	linux-i2c@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 <peda@axentia.se>
> >>>  L:	linux-i2c@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 <ajayg@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

Peter Rosin Sept. 13, 2018, 7:36 a.m. UTC | #8

On 2018-09-13 00:22, Ajay Gupta wrote:
> Hi Peter,
> Can we get the working set in while the issues is being debugged?

I'm not the one making the decision, and I don't even know if this
is going through the i2c or the usb tree? If it's going through the
i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
and if it's going through the usb tree, you need a tag from Wolfram
on patch 1/2. As I said, I'm not involved with that part, I'm just
reviewing these patches because I felt like it.

The remaining issue that bothers me is the looping reads, and your
email address reveals that you should be in a very good position to
work out why they do not work, and fix it or let us know why they
don't. However, your responses indicate that you have given up. That
coupled with the fact that the datasheet is not publicly available
(but why, seems a little over-protective to think the interface to an
i2c controller is worth all that much) makes me think that perhaps
this little detail might never be fixed if it's not fixed now. Once
merged, there is no pressure on you to actually do anything about it,
and others are stuck in darkness without a spec.

> So I assume nothing more left to check on this for now.

Have you hooked up a scope to the I2C bus while trying the various
attempts to get looping reads working? What makes a difference?
Have you talked to the hardware people and described what you have
attempted? Maybe they know what is missing?

Can the datasheet be made public so that someone with more passion
can take a crack at it?

Cheers,
Peter

Heikki Krogerus Sept. 13, 2018, 11:11 a.m. UTC | #9

Hi,

On Thu, Sep 13, 2018 at 09:36:58AM +0200, Peter Rosin wrote:
> On 2018-09-13 00:22, Ajay Gupta wrote:
> > Hi Peter,
> > Can we get the working set in while the issues is being debugged?
> 
> I'm not the one making the decision, and I don't even know if this
> is going through the i2c or the usb tree? If it's going through the
> i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
> and if it's going through the usb tree, you need a tag from Wolfram
> on patch 1/2. As I said, I'm not involved with that part, I'm just
> reviewing these patches because I felt like it.
> 
> The remaining issue that bothers me is the looping reads, and your
> email address reveals that you should be in a very good position to
> work out why they do not work, and fix it or let us know why they
> don't. However, your responses indicate that you have given up. That
> coupled with the fact that the datasheet is not publicly available
> (but why, seems a little over-protective to think the interface to an
> i2c controller is worth all that much) makes me think that perhaps
> this little detail might never be fixed if it's not fixed now. Once
> merged, there is no pressure on you to actually do anything about it,
> and others are stuck in darkness without a spec.

I got similar impression. Ajay, you have really made it look like you
just want to dump the code even though it still has problems, then
lift your arms and never look back. I'm sure that is not the case, but
the fact that you are continuously making pleas, asking us to just
accept the code even though it is not ready, does give that
impression.

I can appreciate that you are in a hurry, and I know you have a lot
of other tasks that also need your attention, just like everybody, but
I'm asking you to take a deep breath, and take one more look at these
two drivers. Go over the code as a whole instead of trying to fix the
problems as fast as possible (you've sent a new version almost daily).
I'm certain you can figure out how to fix that last issue. You are
almost there.

And when you are ready, please include a cover letter in your next
version and provide some background for these patches if you can.
Ideally you could tell something about the platform that has that the
PD controller and the I2C host. There you can also mention which tree
you think these patches should go through, usb or i2c. I'm guessing
usb based on the fact that the I2C host controller driver seems to be
there just for the USB PD controller, at least for now.

Thanks,

Ajay Gupta Sept. 13, 2018, 11:32 p.m. UTC | #10

Hi Heikki and Peter,

> > > Can we get the working set in while the issues is being debugged?
> >
> > I'm not the one making the decision, and I don't even know if this is
> > going through the i2c or the usb tree? If it's going through the i2c
> > tree you need a tag from the usb people (Greg?) on patch 2/2, and if
> > it's going through the usb tree, you need a tag from Wolfram on patch
> > 1/2. As I said, I'm not involved with that part, I'm just reviewing
> > these patches because I felt like it.
> >
> > The remaining issue that bothers me is the looping reads, and your
> > email address reveals that you should be in a very good position to
> > work out why they do not work, and fix it or let us know why they
> > don't.
I am working with different teams to debug this and I think it may take
some time to know the root cause.

>> However, your responses indicate that you have given up. That
> > coupled with the fact that the datasheet is not publicly available
> > (but why, seems a little over-protective to think the interface to an
> > i2c controller is worth all that much) makes me think that perhaps
> > this little detail might never be fixed if it's not fixed now. Once
> > merged, there is no pressure on you to actually do anything about it,
> > and others are stuck in darkness without a spec.
> 
> I got similar impression. Ajay, you have really made it look like you just want
> to dump the code even though it still has problems, then lift your arms and
> never look back. I'm sure that is not the case, but the fact that you are
> continuously making pleas, asking us to just accept the code even though it is
> not ready, does give that impression.
> I can appreciate that you are in a hurry, and I know you have a lot of other
> tasks that also need your attention, just like everybody, but I'm asking you to
> take a deep breath, and take one more look at these two drivers. Go over the
> code as a whole instead of trying to fix the problems as fast as possible
> (you've sent a new version almost daily).
> I'm certain you can figure out how to fix that last issue. You are almost there.
> 
> And when you are ready, please include a cover letter in your next version and
> provide some background for these patches if you can.
Ok.

Thanks
Ajay

--nvpublic
> Ideally you could tell something about the platform that has that the PD
> controller and the I2C host. There you can also mention which tree you think
> these patches should go through, usb or i2c. I'm guessing usb based on the
> fact that the I2C host controller driver seems to be there just for the USB PD
> controller, at least for now.
> 
> 
> Thanks,
> 
> --
> heikki

Ajay Gupta Oct. 1, 2018, 7:33 p.m. UTC | #11

Hi Heikki and Peter,

> > > > Can we get the working set in while the issues is being debugged?
> > >
> > > I'm not the one making the decision, and I don't even know if this
> > > is going through the i2c or the usb tree? If it's going through the
> > > i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
> > > and if it's going through the usb tree, you need a tag from Wolfram
> > > on patch 1/2. As I said, I'm not involved with that part, I'm just
> > > reviewing these patches because I felt like it.
> > >
> > > The remaining issue that bothers me is the looping reads, and your
> > > email address reveals that you should be in a very good position to
> > > work out why they do not work, and fix it or let us know why they
> > > don't.
> I am working with different teams to debug this and I think it may take some
> time to know the root cause.
We got confirmation from HW team about 4 byte read limitation. There has to
be a STOP after every single read cycle. One read cycle supports maximum of
4 byte burst. I will update the patches with a comment on this.

Thanks
Ajay
 
--nvpublic
> >
> > --

Wolfram Sang Oct. 1, 2018, 7:37 p.m. UTC | #12

On Mon, Oct 01, 2018 at 07:33:02PM +0000, Ajay Gupta wrote:
> Hi Heikki and Peter,
> 
> > > > > Can we get the working set in while the issues is being debugged?
> > > >
> > > > I'm not the one making the decision, and I don't even know if this
> > > > is going through the i2c or the usb tree? If it's going through the
> > > > i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
> > > > and if it's going through the usb tree, you need a tag from Wolfram
> > > > on patch 1/2. As I said, I'm not involved with that part, I'm just
> > > > reviewing these patches because I felt like it.
> > > >
> > > > The remaining issue that bothers me is the looping reads, and your
> > > > email address reveals that you should be in a very good position to
> > > > work out why they do not work, and fix it or let us know why they
> > > > don't.
> > I am working with different teams to debug this and I think it may take some
> > time to know the root cause.
> We got confirmation from HW team about 4 byte read limitation. There has to
> be a STOP after every single read cycle. One read cycle supports maximum of
> 4 byte burst. I will update the patches with a comment on this.

Could it be that this is more an SMBus controller than an I2C
controller? I haven't looked at the specs but maybe populating
smbus_xfer instead of master_xfer is an option here?

Peter Rosin Oct. 1, 2018, 7:51 p.m. UTC | #13

On 2018-10-01 21:33, Ajay Gupta wrote:
> Hi Heikki and Peter,
> 
>>>>> Can we get the working set in while the issues is being debugged?
>>>>
>>>> I'm not the one making the decision, and I don't even know if this
>>>> is going through the i2c or the usb tree? If it's going through the
>>>> i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
>>>> and if it's going through the usb tree, you need a tag from Wolfram
>>>> on patch 1/2. As I said, I'm not involved with that part, I'm just
>>>> reviewing these patches because I felt like it.
>>>>
>>>> The remaining issue that bothers me is the looping reads, and your
>>>> email address reveals that you should be in a very good position to
>>>> work out why they do not work, and fix it or let us know why they
>>>> don't.
>> I am working with different teams to debug this and I think it may take some
>> time to know the root cause.
> We got confirmation from HW team about 4 byte read limitation. There has to
> be a STOP after every single read cycle. One read cycle supports maximum of
> 4 byte burst. I will update the patches with a comment on this.

Thanks for digging into this! And if the HW team says it's not possible, then
of course my objection falls flat. However, you only mention "read cycle", and
based on your defines (I2C_MST_CNTL_CYCLE_TRIGGER) that seems to be terminology
from the spec. Yet, you apparently do writes without triggering a cycle. Do
the HW team have anything to say about doing reads without triggering a "cycle"?

Cheers,
Peter

Ajay Gupta Oct. 1, 2018, 9:16 p.m. UTC | #14

Hi Peter,

> >>>> I'm not the one making the decision, and I don't even know if this
> >>>> is going through the i2c or the usb tree? If it's going through the
> >>>> i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
> >>>> and if it's going through the usb tree, you need a tag from Wolfram
> >>>> on patch 1/2. As I said, I'm not involved with that part, I'm just
> >>>> reviewing these patches because I felt like it.
> >>>>
> >>>> The remaining issue that bothers me is the looping reads, and your
> >>>> email address reveals that you should be in a very good position to
> >>>> work out why they do not work, and fix it or let us know why they
> >>>> don't.
> >> I am working with different teams to debug this and I think it may
> >> take some time to know the root cause.
> > We got confirmation from HW team about 4 byte read limitation. There
> > has to be a STOP after every single read cycle. One read cycle
> > supports maximum of
> > 4 byte burst. I will update the patches with a comment on this.
> 
> Thanks for digging into this! And if the HW team says it's not possible, then of
> course my objection falls flat. However, you only mention "read cycle", and
> based on your defines (I2C_MST_CNTL_CYCLE_TRIGGER) that seems to be
> terminology from the spec.
Comment "read cycle" and cycle in define I2C_MST_CNTL_CYCLE_TRIGGER is
not related. I should say "There has to be a STOP after every single read".

Thanks
Ajay

--nvpublic
> Yet, you apparently do writes without triggering a
> cycle. Do the HW team have anything to say about doing reads without
> triggering a "cycle"?
> 
> Cheers,
> Peter

Ajay Gupta Oct. 1, 2018, 10:04 p.m. UTC | #15

Hi Wolfram,

> > > > > > Can we get the working set in while the issues is being debugged?
> > > > >
> > > > > I'm not the one making the decision, and I don't even know if
> > > > > this is going through the i2c or the usb tree? If it's going
> > > > > through the i2c tree you need a tag from the usb people (Greg?)
> > > > > on patch 2/2, and if it's going through the usb tree, you need a
> > > > > tag from Wolfram on patch 1/2. As I said, I'm not involved with
> > > > > that part, I'm just reviewing these patches because I felt like it.
> > > > >
> > > > > The remaining issue that bothers me is the looping reads, and
> > > > > your email address reveals that you should be in a very good
> > > > > position to work out why they do not work, and fix it or let us
> > > > > know why they don't.
> > > I am working with different teams to debug this and I think it may
> > > take some time to know the root cause.
> > We got confirmation from HW team about 4 byte read limitation. There
> > has to be a STOP after every single read cycle. One read cycle
> > supports maximum of
> > 4 byte burst. I will update the patches with a comment on this.
> 
> Could it be that this is more an SMBus controller than an I2C controller?
> I haven't looked at the specs but maybe populating smbus_xfer instead of
> master_xfer is an option here?
I think its more of i2c controller and we do mention " .max_read_len = 4" in
" struct i2c_adapter_quirks". Do you still see something missing here?

Thanks
Ajay

--nvpublic

Wolfram Sang Oct. 2, 2018, 7:27 a.m. UTC | #16

Hi,

> > > We got confirmation from HW team about 4 byte read limitation. There
> > > has to be a STOP after every single read cycle. One read cycle
> > > supports maximum of
> > > 4 byte burst. I will update the patches with a comment on this.
> > 
> > Could it be that this is more an SMBus controller than an I2C controller?
> > I haven't looked at the specs but maybe populating smbus_xfer instead of
> > master_xfer is an option here?
> I think its more of i2c controller and we do mention " .max_read_len = 4" in
> " struct i2c_adapter_quirks". Do you still see something missing here?

Well, if there has to be STOP after a read, then you can't do a transfer
containing "write-read-write", or? So, I wondered if this controller is
of the type which can mainly do "write-then-read" transfers only (check
I2C_AQ_COMB* quirk flags). And for some of those controller types, it
might be even easier to drop generic I2C transfers and only handle the
SMBUS calls.

I didn't check this driver closely yet, so I can't tell if/what it needs
from the above. I wanted to give this input already, though.

Regards,

   Wolfram

Ajay Gupta Oct. 2, 2018, 4:34 p.m. UTC | #17

Hi Wolfram,

> > > > We got confirmation from HW team about 4 byte read limitation.
> > > > There has to be a STOP after every single read cycle. One read
> > > > cycle supports maximum of
> > > > 4 byte burst. I will update the patches with a comment on this.
> > >
> > > Could it be that this is more an SMBus controller than an I2C controller?
> > > I haven't looked at the specs but maybe populating smbus_xfer
> > > instead of master_xfer is an option here?
> > I think its more of i2c controller and we do mention " .max_read_len =
> > 4" in " struct i2c_adapter_quirks". Do you still see something missing here?
> 
> Well, if there has to be STOP after a read, then you can't do a transfer
> containing "write-read-write", or?
Yes, that's correct.

> So, I wondered if this controller is of the
> type which can mainly do "write-then-read" transfers only (check
> I2C_AQ_COMB* quirk flags). 

Yes it is mainly used "write-then-read" and also "write" only.
Read can be maximum of 4 bytes only and write has no size limitation.

I will add the flag I2C_AQ_COMB_WRITE_THEN_READ in the adapter quirk.

static const struct i2c_adapter_quirks gpu_i2c_quirks = {
        .max_read_len = 4,
        .flags = I2C_AQ_COMB_WRITE_THEN_READ,
};

> And for some of those controller types, it might
> be even easier to drop generic I2C transfers and only handle the SMBUS calls.
> 
> I didn't check this driver closely yet, so I can't tell if/what it needs from the
> above. I wanted to give this input already, though.
Thanks for early feedback.
Please help review v12 at [1] and provide your input.

Thanks
Ajay

[1] 
https://marc.info/?l=linux-usb&m=153668792309635&w=2 
https://marc.info/?l=linux-usb&m=153668793009637&w=2 

--nvpublic
> 
> Regards,
> 
>    Wolfram

Peter Rosin Oct. 16, 2018, 5:46 p.m. UTC | #18

On 2018-10-02 09:27, Wolfram Sang wrote:
> Hi,
> 
>>>> We got confirmation from HW team about 4 byte read limitation. There
>>>> has to be a STOP after every single read cycle. One read cycle
>>>> supports maximum of
>>>> 4 byte burst. I will update the patches with a comment on this.
>>>
>>> Could it be that this is more an SMBus controller than an I2C controller?
>>> I haven't looked at the specs but maybe populating smbus_xfer instead of
>>> master_xfer is an option here?
>> I think its more of i2c controller and we do mention " .max_read_len = 4" in
>> " struct i2c_adapter_quirks". Do you still see something missing here?
> 
> Well, if there has to be STOP after a read, then you can't do a transfer
> containing "write-read-write", or? So, I wondered if this controller is
> of the type which can mainly do "write-then-read" transfers only (check
> I2C_AQ_COMB* quirk flags). And for some of those controller types, it
> might be even easier to drop generic I2C transfers and only handle the
> SMBUS calls.
> 
> I didn't check this driver closely yet, so I can't tell if/what it needs
> from the above. I wanted to give this input already, though.

I don't think SMBus is an option in this case since the intended client (Cypress
something in patch 2/2) does xfers that would need 16-bit commands and I think
they are always 8-bit in SMBus, no?

Cheers,
Peter

Wolfram Sang Oct. 16, 2018, 11:21 p.m. UTC | #19

> I don't think SMBus is an option in this case since the intended client (Cypress
> something in patch 2/2) does xfers that would need 16-bit commands and I think
> they are always 8-bit in SMBus, no?

Yes. Command is 8 bit, data can be 16. Thanks for the heads up!

Ajay Gupta Oct. 24, 2018, 5:46 p.m. UTC | #20

Hi Wolfram,
> > I don't think SMBus is an option in this case since the intended
> > client (Cypress something in patch 2/2) does xfers that would need
> > 16-bit commands and I think they are always 8-bit in SMBus, no?
> 
> Yes. Command is 8 bit, data can be 16. Thanks for the heads up!
Please help review v13 of this patch set at
https://marc.info/?l=linux-i2c&m=153859126630601&w=2 
https://marc.info/?l=linux-i2c&m=153859127330604&w=2
https://marc.info/?l=linux-i2c&m=153859127830605&w=2 

Thanks
Ajay
--nvpublic

Greg Kroah-Hartman Oct. 24, 2018, 7:44 p.m. UTC | #21

On Wed, Oct 24, 2018 at 05:46:22PM +0000, Ajay Gupta wrote:
> Hi Wolfram,
> > > I don't think SMBus is an option in this case since the intended
> > > client (Cypress something in patch 2/2) does xfers that would need
> > > 16-bit commands and I think they are always 8-bit in SMBus, no?
> > 
> > Yes. Command is 8 bit, data can be 16. Thanks for the heads up!
> Please help review v13 of this patch set at
> https://marc.info/?l=linux-i2c&m=153859126630601&w=2 
> https://marc.info/?l=linux-i2c&m=153859127330604&w=2
> https://marc.info/?l=linux-i2c&m=153859127830605&w=2 

It is the middle of the merge window, maintainers are a "bit" busy at
the moment :)

[v12,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

Commit Message

Comments

Patch