Andy, there's a question for you below.
On 2018-09-08 02:09, 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>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> Changes from v1 -> v2
> None
> Changes from v2 -> v3
> Fixed review comments from Andy and Thierry
> Rename i2c-gpu.c -> i2c-nvidia-gpu.c
> Changes from v3 -> v4
> Fixed review comments from Andy
> Changes from v4 -> v5
> Fixed review comments from Andy
> Changes from v5 -> v6
> None
> Changes from v6 -> v7 -> v8
> Fixed review comments from Peter
> - Add implicit STOP for last write message
> - Add i2c_adapter_quirks with max_read_len and
> I2C_AQ_COMB flags
> Changes from v8 -> v9
> Fixed review comments from Peter
> - Drop do_start flag
> - Use i2c_8bit_addr_from_msg()
> Changes from v9 -> v10
> Fixed review comments from Peter
> - Dropped I2C_FUNC_SMBUS_EMUL
> - Dropped local mutex
>
> 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 | 372 ++++++++++++++++++++++++++++++++
> 5 files changed, 407 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..c231121
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,372 @@
> +// 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_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;
> +};
> +
> +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));
Andy, if you're still following this thread I'm curious, why did you claim
that the empty line here was redundant[1]? I'm personally with Ajay on this
one and think it looks cramped as is. Or do Ajay and I misinterpret you?
[1] https://marc.info/?l=linux-i2c&m=153569761011089
> + 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)
> +{
> + u32 val;
> +
> + 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;
> +
> + for (i = 0; i < num; i++) {
> + /* Write the client address before processing any message */
> + writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
Since you have I2C_AQ_COMB_SAME_ADDR, you know that the .addr of all messages
are the same, so you should be able to hoist this to before the loop and
use msgs[0].addr, which is safe since the I2C core never calls .master_xfer
with num <= 0.
I think this is so, since elsewhere you have claimed that I2C_MST_ADDR has
to be written once before transfers begins. And that's before the loop.
Inside the loop is once for every message, not for every transfer.
> +
> + 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);
> + if (status < 0)
> + return status;
> + } else {
> + /* start on first write message */
> + if (i == 0) {
This "if (i == 0)" test is completely bogus to me. I fail to see why the
meat of the block should not happen for both writes in a double-write
transfer.
If the second message is a write, you do not issue any start nor do you
write out the address for the second message. You want to generate the
following for a transfer consisting of 2 write-messages:
S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
=============
(where "..." denotes further optional "Data [A]" instances)
As is, the stuff underlined by equal signs are not generated, at least as
I read the code.
This is what I meant in my comment around this area for the v9 patch.
> + u8 addr = i2c_8bit_addr_from_msg(msgs + i);
Missing blank line here between declarations and code.
> + status = gpu_i2c_start(i2cd);
> + if (status < 0)
> + return status;
> +
> + status = gpu_i2c_write(i2cd, addr);
> + if (status < 0)
> + return gpu_i2c_stop(i2cd);
You should issue the stop and then return the original error (i.e. status)
instead of risking a return of zero (or some unrelated error).
> + }
> + for (j = 0; j < msgs[i].len; j++) {
> + status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> + if (status < 0)
> + return gpu_i2c_stop(i2cd);
Dito.
> + }
> + /* stop if last write message */
> + if (i == (num - 1)) {
> + status = gpu_i2c_stop(i2cd);
> + if (status < 0)
> + return status;
> + }
> + }
> + }
> + return i;
> +}
> +
> +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;
I think you should add back the I2C_FUNC_SMBUS_EMUL flags (possibly
with I2C_FUNC_SMBUS_QUICK excluded if the hardware does not support
zero-length reads). But all other SMBUS emulations should work just
fine, methinks.
> +}
> +
> +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)
> + 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@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
>
