Re: [PATCH v6 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2018-09-06 00:20, 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@xxxxxxxxxx>
> ---
> 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 
> 
>  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     | 405 ++++++++++++++++++++++++++++++++
>  5 files changed, 440 insertions(+)
>  create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
>  create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c
> 
> diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu
> new file mode 100644
> index 0000000..31884d2
> --- /dev/null
> +++ b/Documentation/i2c/busses/i2c-nvidia-gpu
> @@ -0,0 +1,18 @@
> +Kernel driver i2c-nvidia-gpu
> +
> +Datasheet: not publicly available.
> +
> +Authors:
> +	Ajay Gupta <ajayg@xxxxxxxxxx>
> +
> +Description
> +-----------
> +
> +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing
> +and later GPUs and it is used to communicate with Type-C controller on GPUs.
> +
> +If your 'lspci -v' listing shows something like the following,
> +
> +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1)
> +
> +then this driver should support the I2C controller of your GPU.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,13 @@ L:	linux-acpi@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	drivers/i2c/i2c-core-acpi.c
>  
> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> +M:	Ajay Gupta <ajayg@xxxxxxxxxx>
> +L:	linux-i2c@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	Documentation/i2c/busses/i2c-nvidia-gpu
> +F:	drivers/i2c/busses/i2c-nvidia-gpu.c
> +
>  I2C MUXES
>  M:	Peter Rosin <peda@xxxxxxxxxx>
>  L:	linux-i2c@xxxxxxxxxxxxxxx
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 451d4ae..eed827b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-nforce2-s4985.
>  
> +config I2C_NVIDIA_GPU
> +	tristate "NVIDIA GPU I2C controller"
> +	depends on PCI
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  NVIDIA GPU I2C controller which is used to communicate with the GPU's
> +	  Type-C controller. This driver can also be built as a module called
> +	  i2c-nvidia-gpu.
> +
>  config I2C_SIS5595
>  	tristate "SiS 5595"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 18b26af..d499813 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
>  obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
> +obj-$(CONFIG_I2C_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
>  
>  ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> new file mode 100644
> index 0000000..2ce6706
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nvidia GPU I2C controller Driver
> + *
> + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta <ajayg@xxxxxxxxxx>
> + */
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* I2C definitions */
> +#define I2C_MST_CNTL				0x00
> +#define I2C_MST_CNTL_GEN_START			BIT(0)
> +#define I2C_MST_CNTL_GEN_STOP			BIT(1)
> +#define I2C_MST_CNTL_CMD_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 */
> +	bool do_start;
> +};
> +
> +static void enable_i2c_bus(struct gpu_i2c_dev *i2cd)

Please prefix all your functions with gpu_, or nvidia_gpu_ or something
like that (because gpu sounds like a something graphics, not that
nvidia_gpu helps with that problem though...)

> +{
> +	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 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))
> +		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 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 = 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:

This seems to behave rather strange for len > 4, and I do not see anything
preventing that from happening.

Am I missing something, or do you need to add a quirk for max_read_len?

> +		break;
> +	}
> +	return status;
> +}
> +
> +static int 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 i2c_check_status(i2cd);
> +}
> +
> +static int 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 i2c_check_status(i2cd);
> +}
> +
> +static int 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 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);
> +	struct device *dev = i2cd->dev;
> +	int status;
> +	int i, j;
> +
> +	mutex_lock(&i2cd->mutex);
> +	for (i = 0; i < num; i++) {

I don't get how this loop works. You do not seem to always start with
a start. E.g., if the first message is I2C_M_RD, i2c_read() is called
before i2c_start(). Is that correct?

Also, if a message has I2C_M_STOP but not I2C_M_RD, the call to i2c_stop()
happens before the call the i2c_write(). What's up with that?

I would expect an i2c_start() before the loop or first in the loop, and a
i2c_stop() after the loop. As is, the stop after the loop is only called
on error. In addition, I would expect messages that have I2C_M_STOP to
trigger an i2c_stop() call *after* the message, even if the message is not
last in the loop.

What am I missing?

> +		if (msgs[i].flags & I2C_M_RD) {
> +			status = i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> +			if (status < 0) {
> +				dev_err(dev, "i2c_read error %x", status);
> +				goto unlock;
> +			}
> +			i2cd->do_start = true;
> +		} else if (msgs[i].flags & I2C_M_STOP) {
> +			status = i2c_stop(i2cd);
> +			if (status < 0) {
> +				dev_err(dev, "i2c_stop error %x", status);
> +				goto unlock;
> +			}
> +			i2cd->do_start = true;
> +		} else {
> +			if (i2cd->do_start) {
> +				status = i2c_start(i2cd, msgs[i].addr);
> +				if (status < 0) {
> +					dev_err(dev, "i2c_start error %x",
> +						status);
> +					goto unlock;
> +				}
> +				status = i2c_write(i2cd, msgs[i].addr << 1);
> +				if (status < 0) {
> +					dev_err(dev, "i2c_write error %x",
> +						status);
> +					goto stop;
> +				}
> +				i2cd->do_start = false;
> +			}
> +			for (j = 0; j < msgs[i].len; j++) {
> +				status = i2c_write(i2cd, *(msgs[i].buf + j));
> +				if (status < 0) {
> +					dev_err(dev, "i2c_write error %x",
> +						status);
> +					goto stop;
> +				}
> +			}
> +		}
> +	}
> +	status = i;
> +	goto unlock;
> +stop:
> +	status = i2c_stop(i2cd);
> +	if (status < 0)
> +		dev_err(dev, "i2c_stop error %x", status);

Hmm, every time you call one of i2c_start, i2c_read, i2c_write or i2c_stop you
check for error and issue a generic dev_err message. I think you should move
these error messages into the functions instead or repeating them for every
use.

> +unlock:
> +	mutex_unlock(&i2cd->mutex);
> +	return status;

Here you return status, which seems to be zero when everything is fine.
That is incorrect for sure; you should return num on success.

Cheers,
Peter

> +}
> +
> +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;
> +	}
> +
> +	i2cd->do_start = true;
> +	mutex_init(&i2cd->mutex);
> +	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.dev.parent = &pdev->dev;
> +	status = i2c_add_adapter(&i2cd->adapter);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "i2c_add_adapter failed %d\n", status);
> +		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);
> +
> +	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@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
> 




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux