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

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()
	
 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     | 396 ++++++++++++++++++++++++++++++++
 5 files changed, 431 insertions(+)
 create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
 create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c

On 2018-09-07 01:56, 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()
> 	
>  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     | 396 ++++++++++++++++++++++++++++++++
>  5 files changed, 431 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 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,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..4a63a4e4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,396 @@
> +// 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_NONE			(0 << 2)
> +#define I2C_MST_CNTL_CMD_READ			(1 << 2)
> +#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
> +#define I2C_MST_CNTL_GEN_RAB			BIT(4)
> +#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_ADDR_DAB			0
> +
> +#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_client *client;
> +	struct mutex mutex;	/* to sync read/write */

Hmm, what do you need to sync read/write with? The only thing I can find
is that busy-check in gpu_i2c_idle, but can runtime-pm really try to idle
an adapter that has an xfer in flight? If that's the case, what stops
a new xfer from being initiated after runtime-pm has checked if
->runtime_idle returns ok but before the device is really suspended?

PM is not an area I'm familiar with, so it's an honest question.

> +};
> +
> +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 -EIO;
> +	}
> +
> +	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_GEN_STOP |
> +		I2C_MST_CNTL_CMD_READ | (len << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> +		I2C_MST_CNTL_CYCLE_TRIGGER | I2C_MST_CNTL_GEN_NACK;
> +	val &= ~I2C_MST_CNTL_GEN_RAB;
> +	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;
> +}
> +
> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd, u16 addr)
> +{
> +	u32 val;
> +
> +	val = addr << I2C_MST_ADDR_DAB;
> +	writel(val, i2cd->regs + I2C_MST_ADDR);
> +
> +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> +		I2C_MST_CNTL_GEN_NACK;
> +	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> +		I2C_MST_CNTL_GEN_NACK;
> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> +	writel(val, 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;
> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> +		 I2C_MST_CNTL_GEN_RAB);
> +	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;
> +	int i, j;
> +
> +	mutex_lock(&i2cd->mutex);
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* gpu_i2c_read has implicit start and stop */
> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);

Per your comments on v8, the address has to be programmed before the
transfer, but you fail to do that if the first message is a read.

Is the I2C_MST_ADDR register really in the loop when you do a write? Because
I think it would simplify things if you could move the write to the
I2C_MST_ADDR register to be in gpu_i2c_read instead of in gpu_i2c_start. Or,
maybe just write to I2C_MST_ADDR before the for-loop in this function.

> +			if (status < 0)
> +				goto unlock;
> +		} else {
> +			/* start on first write message */
> +			if (i == 0) {

You do not issue a restart nor do you write the address to the bus for
the second message, if it's a write. That makes me wonder if this patch
was tested, because AFAICT ccg_write in patch 2/2 uses xfers combining
two write messages and I fail to see how that can work?

Also, have you verified if gpu_i2c_start really issues a restart instead
of a stop followed by a start if the bus is not idle on function entry? Or
is the documentation clear on this detail? Might be worth checking anyway,
as documentation is not always completely correct...

Cheers,
Peter

> +				u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> +				status = gpu_i2c_start(i2cd, msgs[i].addr);
> +				if (status < 0)
> +					goto unlock;
> +
> +				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;
> +			}
> +			/* stop if last write message */
> +			if (i == (num - 1)) {
> +				status = gpu_i2c_stop(i2cd);
> +				if (status < 0)
> +					goto unlock;
> +			}
> +		}
> +	}
> +	status = i;
> +	goto unlock;
> +stop:
> +	status = gpu_i2c_stop(i2cd);
> +unlock:
> +	mutex_unlock(&i2cd->mutex);
> +	return status;
> +}
> +
> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR,
> +	.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)
> +{
> +	static struct i2c_board_info gpu_ccgx_ucsi = {
> +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> +	};
> +
> +	gpu_ccgx_ucsi.irq = irq;
> +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> +	if (!i2cd->client) {
> +		dev_err(i2cd->dev, "i2c_new_device failed\n");
> +		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;
> +	}
> +
> +	mutex_init(&i2cd->mutex);
> +	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;
> +}
> +
> +static int gpu_i2c_idle(struct device *dev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> +
> +	if (!mutex_trylock(&i2cd->mutex)) {
> +		dev_info(dev, "-EBUSY\n");
> +		return -EBUSY;
> +	}
> +	mutex_unlock(&i2cd->mutex);
> +
> +	return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, gpu_i2c_idle);
> +
> +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");
>

Hi Peter,
> > 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()
> >
> >  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     | 396
> ++++++++++++++++++++++++++++++++
> >  5 files changed, 431 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 9ad052a..2d1c5a1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6797,6 +6797,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..4a63a4e4
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > @@ -0,0 +1,396 @@
> > +// 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_NONE			(0 << 2)
> > +#define I2C_MST_CNTL_CMD_READ			(1 << 2)
> > +#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
> > +#define I2C_MST_CNTL_GEN_RAB			BIT(4)
> > +#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_ADDR_DAB			0
> > +
> > +#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_client *client;
> > +	struct mutex mutex;	/* to sync read/write */
> 
> Hmm, what do you need to sync read/write with? 
What if there are multiple clients and each of them wants to use I2C bus for read/write?
Even in UCSI client, user may want to change alt mode which will result in read/write
request in a thread. So we have to synchronize between UCSI ISR and the user thread.

> The only thing I can find is
> that busy-check in gpu_i2c_idle, but can runtime-pm really try to idle an
> adapter that has an xfer in flight? If that's the case, what stops a new xfer from
> being initiated after runtime-pm has checked if
> ->runtime_idle returns ok but before the device is really suspended?
> 
> PM is not an area I'm familiar with, so it's an honest question.
> 
> > +};
> > +
> > +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 -EIO;
> > +	}
> > +
> > +	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_GEN_STOP |
> > +		I2C_MST_CNTL_CMD_READ | (len <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > +		I2C_MST_CNTL_CYCLE_TRIGGER |
> I2C_MST_CNTL_GEN_NACK;
> > +	val &= ~I2C_MST_CNTL_GEN_RAB;
> > +	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;
> > +}
> > +
> > +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd, u16 addr) {
> > +	u32 val;
> > +
> > +	val = addr << I2C_MST_ADDR_DAB;
> > +	writel(val, i2cd->regs + I2C_MST_ADDR);
> > +
> > +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> > +		I2C_MST_CNTL_GEN_NACK;
> > +	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> > +	u32 val;
> > +
> > +	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> > +		I2C_MST_CNTL_GEN_NACK;
> > +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> > +	writel(val, 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;
> > +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> > +		 I2C_MST_CNTL_GEN_RAB);
> > +	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;
> > +	int i, j;
> > +
> > +	mutex_lock(&i2cd->mutex);
> > +	for (i = 0; i < num; i++) {
> > +		if (msgs[i].flags & I2C_M_RD) {
> > +			/* gpu_i2c_read has implicit start and stop */
> > +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> 
> Per your comments on v8, the address has to be programmed before the
> transfer, but you fail to do that if the first message is a read.
This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST
 
> Is the I2C_MST_ADDR register really in the loop when you do a write?
That’s not needed. We need to write I2C_MST_ADDR once before starting
one or more transfers to a client.

> Because
> I think it would simplify things if you could move the write to the
> I2C_MST_ADDR register to be in gpu_i2c_read instead of in gpu_i2c_start. Or,
> maybe just write to I2C_MST_ADDR before the for-loop in this function.
See above. Hint: I2C_AQ_COMB_WRITE_FIRST

> > +			if (status < 0)
> > +				goto unlock;
> > +		} else {
> > +			/* start on first write message */
> > +			if (i == 0) {
> 
> You do not issue a restart nor do you write the address to the bus for the
> second message, if it's a write.
That’s not needed. We need to write I2C_MST_ADDR once before starting
one or more transfers to a client.

> That makes me wonder if this patch was
> tested, because AFAICT ccg_write in patch 2/2 uses xfers combining two write
> messages and I fail to see how that can work?
See above.
 
> Also, have you verified if gpu_i2c_start really issues a restart instead of a stop
> followed by a start if the bus is not idle on function entry?
Yes. Hint: refer gpu_i2_start() and look for I2C_MST_CNTL_GEN_STOP bit.

Thanks
Ajay
--
nvpublic
--
> Or is the
> documentation clear on this detail? Might be worth checking anyway, as
> documentation is not always completely correct...
> 
> Cheers,
> Peter
> 
> > +				u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> > +				status = gpu_i2c_start(i2cd, msgs[i].addr);
> > +				if (status < 0)
> > +					goto unlock;
> > +
> > +				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;
> > +			}
> > +			/* stop if last write message */
> > +			if (i == (num - 1)) {
> > +				status = gpu_i2c_stop(i2cd);
> > +				if (status < 0)
> > +					goto unlock;
> > +			}
> > +		}
> > +	}
> > +	status = i;
> > +	goto unlock;
> > +stop:
> > +	status = gpu_i2c_stop(i2cd);
> > +unlock:
> > +	mutex_unlock(&i2cd->mutex);
> > +	return status;
> > +}
> > +
> > +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> > +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST |
> I2C_AQ_COMB_SAME_ADDR,
> > +	.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) {
> > +	static struct i2c_board_info gpu_ccgx_ucsi = {
> > +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> > +	};
> > +
> > +	gpu_ccgx_ucsi.irq = irq;
> > +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> > +	if (!i2cd->client) {
> > +		dev_err(i2cd->dev, "i2c_new_device failed\n");
> > +		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;
> > +	}
> > +
> > +	mutex_init(&i2cd->mutex);
> > +	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;
> > +}
> > +
> > +static int gpu_i2c_idle(struct device *dev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > +	if (!mutex_trylock(&i2cd->mutex)) {
> > +		dev_info(dev, "-EBUSY\n");
> > +		return -EBUSY;
> > +	}
> > +	mutex_unlock(&i2cd->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> > +gpu_i2c_idle);
> > +
> > +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");
> >

On 2018-09-07 19:15, Ajay Gupta wrote:
> Hi Peter,
>>> 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()
>>>
>>>  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     | 396
>> ++++++++++++++++++++++++++++++++
>>>  5 files changed, 431 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 9ad052a..2d1c5a1 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6797,6 +6797,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..4a63a4e4
>>> --- /dev/null
>>> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> @@ -0,0 +1,396 @@
>>> +// 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_NONE			(0 << 2)
>>> +#define I2C_MST_CNTL_CMD_READ			(1 << 2)
>>> +#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
>>> +#define I2C_MST_CNTL_GEN_RAB			BIT(4)
>>> +#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_ADDR_DAB			0
>>> +
>>> +#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_client *client;
>>> +	struct mutex mutex;	/* to sync read/write */
>>
>> Hmm, what do you need to sync read/write with? 
> What if there are multiple clients and each of them wants to use I2C bus for read/write?
> Even in UCSI client, user may want to change alt mode which will result in read/write
> request in a thread. So we have to synchronize between UCSI ISR and the user thread.

Synchronization of reads/writes from multiple clients is handled by the I2C core and the
adapter lock. Lock in i2c_transfer() for the details.

>> The only thing I can find is
>> that busy-check in gpu_i2c_idle, but can runtime-pm really try to idle an
>> adapter that has an xfer in flight? If that's the case, what stops a new xfer from
>> being initiated after runtime-pm has checked if
>> ->runtime_idle returns ok but before the device is really suspended?
>>
>> PM is not an area I'm familiar with, so it's an honest question.
>>
>>> +};
>>> +
>>> +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 -EIO;
>>> +	}
>>> +
>>> +	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_GEN_STOP |
>>> +		I2C_MST_CNTL_CMD_READ | (len <<
>> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
>>> +		I2C_MST_CNTL_CYCLE_TRIGGER |
>> I2C_MST_CNTL_GEN_NACK;
>>> +	val &= ~I2C_MST_CNTL_GEN_RAB;
>>> +	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;
>>> +}
>>> +
>>> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd, u16 addr) {
>>> +	u32 val;
>>> +
>>> +	val = addr << I2C_MST_ADDR_DAB;
>>> +	writel(val, i2cd->regs + I2C_MST_ADDR);
>>> +
>>> +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
>>> +		I2C_MST_CNTL_GEN_NACK;
>>> +	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
>>> +	writel(val, i2cd->regs + I2C_MST_CNTL);
>>> +
>>> +	return gpu_i2c_check_status(i2cd);
>>> +}
>>> +
>>> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
>>> +	u32 val;
>>> +
>>> +	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
>>> +		I2C_MST_CNTL_GEN_NACK;
>>> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
>>> +	writel(val, 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;
>>> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
>>> +		 I2C_MST_CNTL_GEN_RAB);
>>> +	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;
>>> +	int i, j;
>>> +
>>> +	mutex_lock(&i2cd->mutex);
>>> +	for (i = 0; i < num; i++) {
>>> +		if (msgs[i].flags & I2C_M_RD) {
>>> +			/* gpu_i2c_read has implicit start and stop */
>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>
>> Per your comments on v8, the address has to be programmed before the
>> transfer, but you fail to do that if the first message is a read.
> This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST

Yes, it will. If the transfer consists of a single read, i.e. w/o
any leading write. E.g. i2c_smbus_read_byte (which is emulated in your
case and will generate this pattern).

>> Is the I2C_MST_ADDR register really in the loop when you do a write?
> That’s not needed. We need to write I2C_MST_ADDR once before starting
> one or more transfers to a client.
> 
>> Because
>> I think it would simplify things if you could move the write to the
>> I2C_MST_ADDR register to be in gpu_i2c_read instead of in gpu_i2c_start. Or,
>> maybe just write to I2C_MST_ADDR before the for-loop in this function.
> See above. Hint: I2C_AQ_COMB_WRITE_FIRST
> 
>>> +			if (status < 0)
>>> +				goto unlock;
>>> +		} else {
>>> +			/* start on first write message */
>>> +			if (i == 0) {
>>
>> You do not issue a restart nor do you write the address to the bus for the
>> second message, if it's a write.
> That’s not needed. We need to write I2C_MST_ADDR once before starting
> one or more transfers to a client.
> 
>> That makes me wonder if this patch was
>> tested, because AFAICT ccg_write in patch 2/2 uses xfers combining two write
>> messages and I fail to see how that can work?
> See above.
>  
>> Also, have you verified if gpu_i2c_start really issues a restart instead of a stop
>> followed by a start if the bus is not idle on function entry?
> Yes. Hint: refer gpu_i2_start() and look for I2C_MST_CNTL_GEN_STOP bit.

Yes, I see that you mask out that bit, but my question was if that actually
behaves as we wish and that we in fact do get a restart instead of a stop/start.

> Thanks
> Ajay
> --
> nvpublic
> --
>> Or is the
>> documentation clear on this detail? Might be worth checking anyway, as
>> documentation is not always completely correct...
>>
>> Cheers,
>> Peter
>>
>>> +				u8 addr = i2c_8bit_addr_from_msg(msgs + i);
>>> +				status = gpu_i2c_start(i2cd, msgs[i].addr);
>>> +				if (status < 0)
>>> +					goto unlock;
>>> +
>>> +				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;
>>> +			}
>>> +			/* stop if last write message */
>>> +			if (i == (num - 1)) {
>>> +				status = gpu_i2c_stop(i2cd);
>>> +				if (status < 0)
>>> +					goto unlock;
>>> +			}
>>> +		}
>>> +	}
>>> +	status = i;
>>> +	goto unlock;
>>> +stop:
>>> +	status = gpu_i2c_stop(i2cd);
>>> +unlock:
>>> +	mutex_unlock(&i2cd->mutex);
>>> +	return status;
>>> +}
>>> +
>>> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
>>> +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST |
>> I2C_AQ_COMB_SAME_ADDR,
>>> +	.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) {
>>> +	static struct i2c_board_info gpu_ccgx_ucsi = {
>>> +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
>>> +	};
>>> +
>>> +	gpu_ccgx_ucsi.irq = irq;
>>> +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
>>> +	if (!i2cd->client) {
>>> +		dev_err(i2cd->dev, "i2c_new_device failed\n");
>>> +		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;
>>> +	}
>>> +
>>> +	mutex_init(&i2cd->mutex);
>>> +	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;
>>> +}
>>> +
>>> +static int gpu_i2c_idle(struct device *dev) {
>>> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
>>> +
>>> +	if (!mutex_trylock(&i2cd->mutex)) {
>>> +		dev_info(dev, "-EBUSY\n");
>>> +		return -EBUSY;
>>> +	}
>>> +	mutex_unlock(&i2cd->mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
>>> +gpu_i2c_idle);
>>> +
>>> +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");
>>>
>

Hi Peter,
> >> Hmm, what do you need to sync read/write with?
> > What if there are multiple clients and each of them wants to use I2C bus for
> read/write?
> > Even in UCSI client, user may want to change alt mode which will
> > result in read/write request in a thread. So we have to synchronize between
> UCSI ISR and the user thread.
> 
> Synchronization of reads/writes from multiple clients is handled by the I2C
> core and the adapter lock. Lock in i2c_transfer() for the details.
Ok. I hope it will be good to drop mutex then.
 
> >> The only thing I can find is
> >> that busy-check in gpu_i2c_idle, but can runtime-pm really try to
> >> idle an adapter that has an xfer in flight? If that's the case, what
> >> stops a new xfer from being initiated after runtime-pm has checked if
> >> ->runtime_idle returns ok but before the device is really suspended?
> >>
> >> PM is not an area I'm familiar with, so it's an honest question.
> >>
> >>> +};
> >>> +
> >>> +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 -EIO;
> >>> +	}
> >>> +
> >>> +	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_GEN_STOP |
> >>> +		I2C_MST_CNTL_CMD_READ | (len <<
> >> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> >>> +		I2C_MST_CNTL_CYCLE_TRIGGER |
> >> I2C_MST_CNTL_GEN_NACK;
> >>> +	val &= ~I2C_MST_CNTL_GEN_RAB;
> >>> +	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;
> >>> +}
> >>> +
> >>> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd, u16 addr) {
> >>> +	u32 val;
> >>> +
> >>> +	val = addr << I2C_MST_ADDR_DAB;
> >>> +	writel(val, i2cd->regs + I2C_MST_ADDR);
> >>> +
> >>> +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> >>> +		I2C_MST_CNTL_GEN_NACK;
> >>> +	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
> >>> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> >>> +
> >>> +	return gpu_i2c_check_status(i2cd); }
> >>> +
> >>> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> >>> +	u32 val;
> >>> +
> >>> +	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> >>> +		I2C_MST_CNTL_GEN_NACK;
> >>> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> >>> +	writel(val, 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;
> >>> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> >>> +		 I2C_MST_CNTL_GEN_RAB);
> >>> +	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;
> >>> +	int i, j;
> >>> +
> >>> +	mutex_lock(&i2cd->mutex);
> >>> +	for (i = 0; i < num; i++) {
> >>> +		if (msgs[i].flags & I2C_M_RD) {
> >>> +			/* gpu_i2c_read has implicit start and stop */
> >>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> >>
> >> Per your comments on v8, the address has to be programmed before the
> >> transfer, but you fail to do that if the first message is a read.
> > This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST
> 
> Yes, it will. If the transfer consists of a single read, i.e. w/o any leading write.
> E.g. i2c_smbus_read_byte (which is emulated in your case and will generate
> this pattern).
I don't think we intend to support SMBUS commands and so I will drop
I2C_FUNC_SMBUS_EMUL.

> >> Is the I2C_MST_ADDR register really in the loop when you do a write?
> > That’s not needed. We need to write I2C_MST_ADDR once before starting
> > one or more transfers to a client.
> >
> >> Because
> >> I think it would simplify things if you could move the write to the
> >> I2C_MST_ADDR register to be in gpu_i2c_read instead of in
> >> gpu_i2c_start. Or, maybe just write to I2C_MST_ADDR before the for-loop
> in this function.
> > See above. Hint: I2C_AQ_COMB_WRITE_FIRST
> >
> >>> +			if (status < 0)
> >>> +				goto unlock;
> >>> +		} else {
> >>> +			/* start on first write message */
> >>> +			if (i == 0) {
> >>
> >> You do not issue a restart nor do you write the address to the bus
> >> for the second message, if it's a write.
> > That’s not needed. We need to write I2C_MST_ADDR once before starting
> > one or more transfers to a client.
> >
> >> That makes me wonder if this patch was tested, because AFAICT
> >> ccg_write in patch 2/2 uses xfers combining two write messages and I
> >> fail to see how that can work?
> > See above.
> >
> >> Also, have you verified if gpu_i2c_start really issues a restart
> >> instead of a stop followed by a start if the bus is not idle on function entry?
> > Yes. Hint: refer gpu_i2_start() and look for I2C_MST_CNTL_GEN_STOP bit.
> 
> Yes, I see that you mask out that bit, but my question was if that actually
> behaves as we wish and that we in fact do get a restart instead of a stop/start.
It does as per documentation. It forces a START bit to be sent.

> > Thanks
> > Ajay
--
nvpublic
--
> >> Or is the
> >> documentation clear on this detail? Might be worth checking anyway,
> >> as documentation is not always completely correct...
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +				u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> >>> +				status = gpu_i2c_start(i2cd, msgs[i].addr);
> >>> +				if (status < 0)
> >>> +					goto unlock;
> >>> +
> >>> +				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;
> >>> +			}
> >>> +			/* stop if last write message */
> >>> +			if (i == (num - 1)) {
> >>> +				status = gpu_i2c_stop(i2cd);
> >>> +				if (status < 0)
> >>> +					goto unlock;
> >>> +			}
> >>> +		}
> >>> +	}
> >>> +	status = i;
> >>> +	goto unlock;
> >>> +stop:
> >>> +	status = gpu_i2c_stop(i2cd);
> >>> +unlock:
> >>> +	mutex_unlock(&i2cd->mutex);
> >>> +	return status;
> >>> +}
> >>> +
> >>> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> >>> +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST |
> >> I2C_AQ_COMB_SAME_ADDR,
> >>> +	.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) {
> >>> +	static struct i2c_board_info gpu_ccgx_ucsi = {
> >>> +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> >>> +	};
> >>> +
> >>> +	gpu_ccgx_ucsi.irq = irq;
> >>> +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> >>> +	if (!i2cd->client) {
> >>> +		dev_err(i2cd->dev, "i2c_new_device failed\n");
> >>> +		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;
> >>> +	}
> >>> +
> >>> +	mutex_init(&i2cd->mutex);
> >>> +	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;
> >>> +}
> >>> +
> >>> +static int gpu_i2c_idle(struct device *dev) {
> >>> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> >>> +
> >>> +	if (!mutex_trylock(&i2cd->mutex)) {
> >>> +		dev_info(dev, "-EBUSY\n");
> >>> +		return -EBUSY;
> >>> +	}
> >>> +	mutex_unlock(&i2cd->mutex);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> >>> +gpu_i2c_idle);
> >>> +
> >>> +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");
> >>>
> >

On September 7, 2018 11:48:37 PM GMT+02:00, Ajay Gupta <ajayg@nvidia.com> wrote:

>> >> Per your comments on v8, the address has to be programmed before
>the
>> >> transfer, but you fail to do that if the first message is a read.
>> > This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST
>> 
>> Yes, it will. If the transfer consists of a single read, i.e. w/o any
>leading write.
>> E.g. i2c_smbus_read_byte (which is emulated in your case and will
>generate
>> this pattern).
>I don't think we intend to support SMBUS commands and so I will drop
>I2C_FUNC_SMBUS_EMUL.

SMBUS has nothing to do with the problem, that was just an example. An I2C client driver can issue such I2C xfers all by itself without going through emulation, so just dropping the _EMUL flag is not the answer. And I'd be surprised if the hardware doesn't support single message reads.

There is no quirk flag for this abnormality, so you will have to open code the check in your master_xfer if you can't make such xfers work, but the best fix is certainly to just make them work...

Cheers,
Peter

Hi Peter,

>>>> .

>SMBUS has nothing to do with the >problem, that was just an example. An I2C >client driver can issue such I2C xfers all by >itself without going through emulation, so >just dropping the _EMUL flag is not the >answer. And I'd be surprised if the >hardware doesn't support single message >reads.

>There is no quirk flag for this abnormality, >so you will have to open code the check in >your master_xfer if you can't make such >xfers work, but the best fix is certainly to >just make them work...

I have fixed that in v10. Please check

>Cheers,
>Peter

—
nvpublic
—

On 2018-09-08 16:13, Ajay Gupta wrote:
> 
> Hi Peter,
> 
>>>>> .
> 
>> SMBUS has nothing to do with the >problem, that was just an example. An I2C >client driver can issue such I2C xfers all by >itself without going through emulation, so >just dropping the _EMUL flag is not the >answer. And I'd be surprised if the >hardware doesn't support single message >reads.
> 
>> There is no quirk flag for this abnormality, >so you will have to open code the check in >your master_xfer if you can't make such >xfers work, but the best fix is certainly to >just make them work...
> 
> I have fixed that in v10. Please check

Ah, I didn't even bother to look earlier. I have now taken a peek, and
there are still issues. I'll comment inline for the v10 1/2 patch as
usual, but I'm short on time at the moment so it will probably be a few
hours. It's an option to hold on to v11 until that happens...

Cheers,
Peter