| Message ID | 1535146416-11754-1-git-send-email-ajayg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU | expand |
On Fri, Aug 24, 2018 at 02:33:35PM -0700, 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 add 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> > --- > Changes from v1 -> v2: None > > Documentation/i2c/busses/i2c-gpu | 18 ++ > MAINTAINERS | 7 + > drivers/i2c/busses/Kconfig | 9 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-gpu.c | 493 +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 528 insertions(+) > create mode 100644 Documentation/i2c/busses/i2c-gpu > create mode 100644 drivers/i2c/busses/i2c-gpu.c Hi Ajay, I think this looks pretty good. A couple of minor, mostly nit-picky, comments below. > > diff --git a/Documentation/i2c/busses/i2c-gpu b/Documentation/i2c/busses/i2c-gpu > new file mode 100644 > index 0000000..873ba34 > --- /dev/null > +++ b/Documentation/i2c/busses/i2c-gpu I think this is too generic. Maybe use something like i2c-nvidia-gpu here and everywhere else, to make it explicit that this is for NVIDIA GPUs rather than GPUs in general. > @@ -0,0 +1,18 @@ > +Kernel driver i2c-gpu > + > +Datasheet: not publicly available. > + > +Authors: > + Ajay Gupta <ajayg@nvidia.com> > + > +Description > +----------- > + > +i2c-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 b2fcd1c..e99f8a2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6796,6 +6796,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-gpu > +F: drivers/i2c/busses/i2c-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..ff8b2d4 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_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-gpu.ko. > + > config I2C_SIS5595 > tristate "SiS 5595" > depends on PCI > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 18b26af..15d2894 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_GPU) += i2c-gpu.o > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > diff --git a/drivers/i2c/busses/i2c-gpu.c b/drivers/i2c/busses/i2c-gpu.c > new file mode 100644 > index 0000000..0fd2944 > --- /dev/null > +++ b/drivers/i2c/busses/i2c-gpu.c > @@ -0,0 +1,493 @@ > +// 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> > + > +/* STATUS definitions */ > +#define STATUS_SUCCESS 0 > +#define STATUS_UNSUCCESSFUL 0x80000000UL > +#define STATUS_TIMEOUT 0x80000001UL > +#define STATUS_IO_DEVICE_ERROR 0x80000002UL > +#define STATUS_IO_TIMEOUT 0x80000004UL > +#define STATUS_IO_PREEMPTED 0x80000008UL This seems a little odd. Can these not be converted to standard errno codes? It's more idiomatic to return 0 on success (like you define in the above) and a negative error code on failure. If you use that scheme, there's no need to have a symbolic name for success because it is used pretty much everywhere. See include/uapi/asm-generic/errno{,-base}.h for a list of standard error codes. I think you should be able to find a match for each of the above. > +/* Cypress Type-C controllers (CCGx) device */ > +#define CCGX_I2C_DEV_ADDRESS 0x08 I don't think there's a need for the define here. You only use the address once, and that's where the client is defined, so you can just use 0x08 literally in that one location. > +/* I2C definitions */ > +#define I2C_MST_CNTL 0x00 > +#define I2C_MST_CNTL_GEN_START (1 << 0) > +#define I2C_MST_CNTL_GEN_STOP (1 << 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_CMD_RESET (3 << 2) > +#define I2C_MST_CNTL_GEN_RAB (1 << 4) > +#define I2C_MST_CNTL_BURST_SIZE_SHIFT (6) > +#define I2C_MST_CNTL_GEN_NACK (1 << 28) > +#define I2C_MST_CNTL_STATUS (3 << 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 (1 << 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 << 0) > +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_200KHZ (0x087 << 0) > +#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 (1 << 24) > + > +#define I2C_MST_DATA 0x0c > + > +#define I2C_MST_HYBRID_PADCTL 0x20 > +#define I2C_MST_HYBRID_PADCTL_MODE_I2C (1 << 0) > +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV (1 << 14) > +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV (1 << 15) > + > +/* PCI driver data */ > +struct gpu_i2c_dev { > + struct pci_dev *pci_dev; > + void __iomem *regs; > + struct i2c_adapter adapter; > + struct i2c_client *client; > + struct mutex mutex; > + bool do_start; > +}; > + > +static void enable_i2c_bus(struct gpu_i2c_dev *gdev) > +{ > + struct device *dev = &gdev->pci_dev->dev; > + u32 val; > + > + /* enable I2C */ > + val = readl(gdev->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; > + > + dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", __func__, > + (gdev->regs + I2C_MST_HYBRID_PADCTL), val); Do we really need these debug messages? None of the configuration above is in any way parameterized, so the values that are written are pretty deterministic (unless maybe the initial value of the register differs). If you really want to keep the debug messages, I think you should remove at least the register address (you already print the name, which is much more useful). Alternatively, have you considered using ftrace to allow these traces to be dynamically enabled? > + > + writel(val, gdev->regs + I2C_MST_HYBRID_PADCTL); > + > + /* enable 100KHZ mode */ > + val = 0; > + 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; > + > + dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__, > + gdev->regs + I2C_MST_I2C0_TIMING, val); > + writel(val, gdev->regs + I2C_MST_I2C0_TIMING); > +} > + > +static u32 i2c_check_status(struct gpu_i2c_dev *gdev) > +{ > + struct device *dev = &gdev->pci_dev->dev; > + unsigned long target = jiffies + msecs_to_jiffies(1000); > + u32 status = STATUS_UNSUCCESSFUL; > + u32 val; > + > + while (time_is_after_jiffies(target)) { > + val = readl(gdev->regs + I2C_MST_CNTL); > + if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) != > + I2C_MST_CNTL_CYCLE_TRIGGER) > + break; > + if ((val & I2C_MST_CNTL_STATUS) != > + I2C_MST_CNTL_STATUS_BUS_BUSY) > + break; > + usleep_range(1000, 2000); > + } > + > + if (time_is_before_jiffies(target)) { > + dev_err(dev, "%si2c timeout", __func__); > + return status; The error message looks suspicious here. There should be some sort of separator between the function name and the rest of the message. Also, do we really need to print the function name? That's not very useful to users (it might actually be confusing because the function name is pretty generic, so could be mistaken for a core function) and this is the only place where you check for timeouts anyway, so no need to resolve any ambiguities by adding the function name. > + } > + > + val = readl(gdev->regs + I2C_MST_CNTL); > + switch (val & I2C_MST_CNTL_STATUS) { > + case I2C_MST_CNTL_STATUS_OKAY: > + status = STATUS_SUCCESS; > + break; > + case I2C_MST_CNTL_STATUS_NO_ACK: > + status = STATUS_IO_DEVICE_ERROR; > + break; > + case I2C_MST_CNTL_STATUS_TIMEOUT: > + status = STATUS_IO_TIMEOUT; > + break; > + case I2C_MST_CNTL_STATUS_BUS_BUSY: > + status = STATUS_IO_PREEMPTED; > + break; > + default: > + break; > + } > + return status; > +} > + > +static u32 i2c_read(struct gpu_i2c_dev *gdev, u8 *data, u16 len) > +{ > + struct device *dev = &gdev->pci_dev->dev; > + u32 status; > + u32 val = 0; > + > + 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, gdev->regs + I2C_MST_CNTL); > + > + status = i2c_check_status(gdev); > + if (status == STATUS_UNSUCCESSFUL) { > + dev_err(dev, "%s failed\n", __func__); > + return status; > + } There's no need for this error message, since the caller already provides one. > + > + val = readl(gdev->regs + I2C_MST_DATA); > + switch (len) { > + case 1: > + data[0] = (val >> 0) & 0xff; > + break; > + case 2: > + data[0] = (val >> 8) & 0xff; > + data[1] = (val >> 0) & 0xff; > + break; > + case 3: > + data[0] = (val >> 16) & 0xff; > + data[1] = (val >> 8) & 0xff; > + data[2] = (val >> 0) & 0xff; > + break; > + case 4: > + data[0] = (val >> 24) & 0xff; > + data[1] = (val >> 16) & 0xff; > + data[2] = (val >> 8) & 0xff; > + data[3] = (val >> 0) & 0xff; > + break; > + default: > + break; > + } > + return status; > +} > + > +static u32 i2c_manual_start(struct gpu_i2c_dev *gdev, u16 addr) > +{ > + u32 val = 0; > + > + val = addr << I2C_MST_ADDR_DAB; > + writel(val, gdev->regs + I2C_MST_ADDR); > + > + val = 0; > + 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, gdev->regs + I2C_MST_CNTL); > + > + return i2c_check_status(gdev); > +} > + > +static u32 i2c_manual_stop(struct gpu_i2c_dev *gdev) > +{ > + u32 val = 0; > + > + 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, gdev->regs + I2C_MST_CNTL); > + > + return i2c_check_status(gdev); > +} > + > +static u32 i2c_manual_write(struct gpu_i2c_dev *gdev, u8 data) > +{ > + u32 val = 0; > + > + writel(data, gdev->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, gdev->regs + I2C_MST_CNTL); > + > + return i2c_check_status(gdev); > +} > + > +/* gdev i2c adapter */ > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, int num) > +{ > + struct gpu_i2c_dev *gdev = i2c_get_adapdata(adap); > + struct device *dev = &gdev->pci_dev->dev; > + int retry1b = 10; > + u32 status; > + int i, j; > + > + dev_dbg(dev, "%s: adap %p msgs %p num %d\n", __func__, adap, msgs, num); There's already code in drivers/i2c/i2c-core-base.c that will print mostly the same information when DEBUG is enabled. > + mutex_lock(&gdev->mutex); > + > + for (i = 0; i < num; i++) { > + if (msgs[i].flags & I2C_M_RD) { > +retry2: > + status = i2c_read(gdev, msgs[i].buf, msgs[i].len); > + if (status != STATUS_SUCCESS) { > + dev_err(dev, > + "%s:%d i2c_read failed %08lx", __func__, > + __LINE__, (unsigned long)status); Again, I'm not sure filename and line number will provide useful information. I'd simply refer to the operation that failed, along the lines of: err = i2c_read(...); if (err < 0) { dev_err(dev, "I2C read failed: %d\n", err); ... } > + > + if (--retry1b > 0) { > + usleep_range(10000, 11000); > + goto retry2; > + } > + break; > + } Also, perhaps you want to only output the error right before the break so that you don't potentially get 10 identical error messages? > + gdev->do_start = true; > + } else if (msgs[i].flags & I2C_M_STOP) { > + status = i2c_manual_stop(gdev); > + if (status != STATUS_SUCCESS) { > + dev_err(dev, > + "%s:%d i2c_manual_stop failed %08lx", __func__, > + __LINE__, (unsigned long)status); > + goto exit; > + } > + gdev->do_start = true; > + } else { > + dev_dbg(dev, "!I2C_M_RD start %d len %d\n", > + gdev->do_start, msgs[i].len); > + if (gdev->do_start) { > + status = i2c_manual_start(gdev, msgs[i].addr); > + if (status != STATUS_SUCCESS) { > + dev_err(dev, > + "%s:%d i2c_manual_start failed %08lx", > + __func__, __LINE__, > + (unsigned long)status); > + goto exit; > + } > + status = i2c_manual_write(gdev, > + msgs[i].addr << 1); > + if (status != STATUS_SUCCESS) { > + dev_err(dev, > + "%s:%d i2c_manual_write failed %08lx", > + __func__, __LINE__, > + (unsigned long)status); > + goto exit_stop; > + } > + gdev->do_start = false; > + } > + for (j = 0; j < msgs[i].len; j++) { > + status = i2c_manual_write(gdev, > + *(msgs[i].buf + j)); > + if (status != STATUS_SUCCESS) { > + dev_err(dev, > + "%s:%d i2c_manual_write failed %08lx", > + __func__, __LINE__, > + (unsigned long)status); > + goto exit_stop; > + } > + } > + } > + } > + goto exit; > +exit_stop: > + status = i2c_manual_stop(gdev); > + if (status != STATUS_SUCCESS) > + dev_err(dev, "i2c_manual_stop failed %x", status); > +exit: > + mutex_unlock(&gdev->mutex); > + return i; > +} You might want to consider using more descriptive names for the labels to make the code more readable. Something like: ... goto stop; } goto unlock; stop: ... unlock: ... Is more readable I think. Also, I think the high level of indentation makes this function hard to read. You may want to split it up into smaller chunks. > + > +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, > +}; > + > +static int gpu_i2c_dev_init(struct gpu_i2c_dev *gdev) > +{ > + gdev->do_start = true; > + > + /* initialize mutex */ > + mutex_init(&gdev->mutex); > + > + /* initialize i2c */ > + enable_i2c_bus(gdev); > + > + return 0; > +} You might want to consider moving this directly into the ->probe() implementation. You never reuse this elsewhere and it's not big enough to warrant the separate function. Inlining it into ->probe() will make the code easier to follow, in my opinion. > + > +struct i2c_board_info gpu_i2c_ucsi_board_info = { > + I2C_BOARD_INFO("i2c-gpu-ucsi", CCGX_I2C_DEV_ADDRESS), > +}; > + > +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80 > +/* pci driver */ > +static const struct pci_device_id gpu_i2c_ids[] = { The comment there looks out of place. > + { 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_i2c_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + struct gpu_i2c_dev *gdev; > + int status; > + > + dev_info(&dev->dev, > + "dev %p id %08x %08x sub %08x %08x class %08x %08x\n", > + dev, id->vendor, id->device, id->subvendor, id->subdevice, > + id->class, id->class_mask); If at all, this should be dev_dbg(). Generally I don't think there's any reason to mention this at all. ->probe() should assume that it will succeed, which is the expected case, and that should not produce any messages. Instead it's better to let the user know if something failed using an error message. > + > + gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL); > + if (!gdev) > + return -ENOMEM; > + > + gdev->pci_dev = dev; > + pci_set_drvdata(dev, gdev); > + > + status = pci_enable_device(dev); > + if (status < 0) { > + dev_err(&dev->dev, "pci_enable_device failed - %d\n", status); > + return status; > + } > + > + pci_set_master(dev); > + > + gdev->regs = pci_iomap(dev, 0, 0); > + if (!gdev->regs) { > + dev_err(&dev->dev, "pci_iomap failed\n"); > + status = -ENOMEM; > + goto iomap_err; > + } > + > + status = pci_enable_msi(dev); > + if (status < 0) { > + dev_err(&dev->dev, "pci_enable_msi failed - %d\n", status); > + goto enable_msi_err; > + } > + > + status = gpu_i2c_dev_init(gdev); > + if (status < 0) { > + dev_err(&dev->dev, "gpu_i2c_dev_init failed - %d\n", status); > + goto i2c_init_err; > + } > + > + i2c_set_adapdata(&gdev->adapter, gdev); > + gdev->adapter.owner = THIS_MODULE; > + strlcpy(gdev->adapter.name, "NVIDIA GPU I2C adapter", > + sizeof(gdev->adapter.name)); > + gdev->adapter.algo = &gpu_i2c_algorithm; > + gdev->adapter.dev.parent = &dev->dev; > + status = i2c_add_adapter(&gdev->adapter); > + if (status < 0) { > + dev_err(&dev->dev, "i2c_add_adapter failed - %d\n", status); > + goto add_adapter_err; > + } > + > + gpu_i2c_ucsi_board_info.irq = dev->irq; > + gdev->client = i2c_new_device(&gdev->adapter, > + &gpu_i2c_ucsi_board_info); I think it's technically possible that the above would race between multiple instances of this I2C controller. The safer way would be to make gpu_i2c_ucsi_board_info variable local to this function. > + > + if (!gdev->client) { > + dev_err(&dev->dev, "i2c_new_device failed - %d\n", status); > + status = -ENODEV; > + goto add_adapter_err; > + } > + > + dev_set_drvdata(&dev->dev, gdev); I don't think you need this here since you've already called pci_set_drvdata() above. > + pm_runtime_put_noidle(&dev->dev); > + pm_runtime_allow(&dev->dev); > + > + return 0; > + > +add_adapter_err: > + i2c_del_adapter(&gdev->adapter); > +i2c_init_err: > + pci_disable_msi(dev); > +enable_msi_err: > + pci_iounmap(dev, gdev->regs); > +iomap_err: > + pci_disable_device(dev); > + return status; > +} It's generally preferred to have names that describe the actions of the label, instead of describing where they failed. I think that makes code easier to read, and it also has the benefit of not being confusing when you jump to a label, say add_adapter_err, from a location that doesn't have anything to do with adding an adapter. Something like this would be better in my opinion: del_adapter: ... disable_msi: ... unmap: ... disable: ... > + > +static void gpu_i2c_remove(struct pci_dev *dev) > +{ > + struct gpu_i2c_dev *gdev = pci_get_drvdata(dev); > + > + i2c_del_adapter(&gdev->adapter); > + pci_disable_msi(dev); > + pci_iounmap(dev, gdev->regs); > +} > + > +static int gpu_i2c_suspend(struct device *dev) > +{ > + dev_dbg(dev, "%s\n", __func__); > + > + return 0; > +} Do we need this if it doesn't do anything? > + > +static int gpu_i2c_resume(struct device *dev) > +{ > + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + enable_i2c_bus(gdev); > + > + return 0; > +} > + > +static int gpu_i2c_idle(struct device *dev) > +{ > + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); > + > + if (!mutex_trylock(&gdev->mutex)) { > + dev_info(dev, "%s: -EBUSY\n", __func__); > + return -EBUSY; > + } > + mutex_unlock(&gdev->mutex); > + > + return 0; > +} > + > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, gpu_i2c_resume, > + gpu_i2c_idle); > + > +static struct pci_driver gpu_i2c_driver = { > + .name = "gpu_i2c_driver", This name looks unusual for a driver. Perhaps "i2c-nvidia-gpu" to match the module name? > + .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 Andy, > > Latest NVIDIA GPU card has USB Type-C interface. There is a > > Type-C controller which can be accessed over I2C. > > > > This driver add I2C bus driver to communicate with Type-C controller. > > I2C client driver will be part of USB Type-C UCSI driver. > > > drivers/i2c/busses/i2c-gpu.c | 493 > +++++++++++++++++++++++++++++++++++++++ > > Can we got more better name, which includes vendor and/or model of the I2C > host? Sure will change to i2c-nvidia-gpu.c > > +/* STATUS definitions */ > > +#define STATUS_SUCCESS 0 > > +#define STATUS_UNSUCCESSFUL 0x80000000UL > > +#define STATUS_TIMEOUT 0x80000001UL > > +#define STATUS_IO_DEVICE_ERROR 0x80000002UL > > +#define STATUS_IO_TIMEOUT 0x80000004UL > > +#define STATUS_IO_PREEMPTED 0x80000008UL > > Looks slightly different from my point of view, something like > > /* Bit 31 shows error condition while LSB encodes the error code */ > STATUS_TIMEOUT BIT(0) > ... > STATUS_ERROR BIT(31) Will fix. > > + dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", __func__, > > + (gdev->regs + I2C_MST_HYBRID_PADCTL), val); > > Parens are redundant, __func__ is redundant. Will fix. > > + dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__, > > + gdev->regs + I2C_MST_I2C0_TIMING, val); > > Ditto. Check your debug messages, and perheps even drop some. Will fix. > > +static u32 i2c_check_status(struct gpu_i2c_dev *gdev) > > +{ > > > + while (time_is_after_jiffies(target)) { > > + } > > For functions like this better to get in a form > do { > } while(). Ok, will fix. > There is no guarantee that it runs even once in your case. > > > + dev_err(dev, "%si2c timeout", __func__); > > No space? Ok, will fix. > > > + val = readl(gdev->regs + I2C_MST_DATA); > > + switch (len) { > > + case 1: > > + data[0] = (val >> 0) & 0xff; > > + break; > > + case 2: > > + data[0] = (val >> 8) & 0xff; > > + data[1] = (val >> 0) & 0xff; > > + break; > > + case 3: > > + data[0] = (val >> 16) & 0xff; > > + data[1] = (val >> 8) & 0xff; > > + data[2] = (val >> 0) & 0xff; > > + break; > > + case 4: > > + data[0] = (val >> 24) & 0xff; > > + data[1] = (val >> 16) & 0xff; > > + data[2] = (val >> 8) & 0xff; > > + data[3] = (val >> 0) & 0xff; > > + break; > > Redundant & 0xff. > We have get_unaligned*(), put_unaligned*() and many variations of > cpu_to_Xe*() and Xe*_to_cpu(). Ok, will fix. > > > + u32 val = 0; > > Redundant assignment. Ok, will fix. > > > + val = addr << I2C_MST_ADDR_DAB; > > > + val = 0; > > Ditto. What's wrong with assign value below directly? > > > + val |= I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE | > > + I2C_MST_CNTL_GEN_NACK; > > > + u32 val = 0; > > Check your code for these kind of style mistakes. > > > +/* gdev i2c adapter */ > > Pointless. > > > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap, > > + struct i2c_msg *msgs, int num) > > +{ > > + struct gpu_i2c_dev *gdev = i2c_get_adapdata(adap); > > + struct device *dev = &gdev->pci_dev->dev; > > + int retry1b = 10; > > + u32 status; > > + int i, j; > > > + goto exit; > > +exit_stop: > > + status = i2c_manual_stop(gdev); > > + if (status != STATUS_SUCCESS) > > + dev_err(dev, "i2c_manual_stop failed %x", status); > > +exit: > > + mutex_unlock(&gdev->mutex); > > + return i; > > +} > > Ouch! Besides many small style issues and redundancy (like __LINE__), > this function needs to be refactored to few smaller and readable ones. Ok, will fix. > > > +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80 > > > +/* pci driver */ > > Pointless. Ok, will fix. > > > +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}, > > Are you sure?! Yes, we want to identify using vendor ID and class code. Currently there is no class code defined for UCSI device over PCI so using UNKNOWN. > > > + { }, > > Terminator line better w/o comma. Ok, will fix. > > > +}; > > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids); > > > +static int gpu_i2c_probe(struct pci_dev *dev, const struct pci_device_id > *id) > > +{ > > + struct gpu_i2c_dev *gdev; > > + int status; > > + > > > + dev_info(&dev->dev, > > + "dev %p id %08x %08x sub %08x %08x class %08x %08x\n", > > + dev, id->vendor, id->device, id->subvendor, id->subdevice, > > + id->class, id->class_mask); > > Useless. We have PCI core printed this information out several times > during the boot or, if card is hotpluggable, when it's plugged in. Ok, will fix. > > > + gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev), > GFP_KERNEL); > > + if (!gdev) > > + return -ENOMEM; > > > + status = pci_enable_device(dev); > > Using devm_ without pcim_ sound slightly inconsistent. Ok, will fix. > > > + status = pci_enable_msi(dev); > > This done in the other way. Check pci_alloc_irq_vectors(), IIRC. sure, will fix. > > > +i2c_init_err: > > + pci_disable_msi(dev); > > +enable_msi_err: > > + pci_iounmap(dev, gdev->regs); > > +iomap_err: > > + pci_disable_device(dev); > > At least above will gone after switching to pcim_ > > > + pci_disable_msi(dev); > > + pci_iounmap(dev, gdev->regs); > > Same. > > > + dev_dbg(dev, "%s\n", __func__); > > Pointless. We have ftrace, for example to see this. > > > + dev_dbg(dev, "%s\n", __func__); > > Ditto. > > Thanks Ajay -- nvpublic -- > -- > With Best Regards, > Andy Shevchenko
Hi Thierry, > > Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C > > controller which can be accessed over I2C. > > > > This driver add 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> > > --- > > Changes from v1 -> v2: None > > > > Documentation/i2c/busses/i2c-gpu | 18 ++ > > MAINTAINERS | 7 + > > drivers/i2c/busses/Kconfig | 9 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-gpu.c | 493 > +++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 528 insertions(+) > > create mode 100644 Documentation/i2c/busses/i2c-gpu create mode > > 100644 drivers/i2c/busses/i2c-gpu.c > > Hi Ajay, > > I think this looks pretty good. A couple of minor, mostly nit-picky, comments > below. > > > > > diff --git a/Documentation/i2c/busses/i2c-gpu > > b/Documentation/i2c/busses/i2c-gpu > > new file mode 100644 > > index 0000000..873ba34 > > --- /dev/null > > +++ b/Documentation/i2c/busses/i2c-gpu > > I think this is too generic. Maybe use something like i2c-nvidia-gpu here and > everywhere else, to make it explicit that this is for NVIDIA GPUs rather than > GPUs in general. ok > > @@ -0,0 +1,18 @@ > > +Kernel driver i2c-gpu > > + > > +Datasheet: not publicly available. > > + > > +Authors: > > + Ajay Gupta <ajayg@nvidia.com> > > + > > +Description > > +----------- > > + > > +i2c-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 b2fcd1c..e99f8a2 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6796,6 +6796,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-gpu > > +F: drivers/i2c/busses/i2c-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..ff8b2d4 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_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-gpu.ko. > > + > > config I2C_SIS5595 > > tristate "SiS 5595" > > depends on PCI > > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > > index 18b26af..15d2894 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_GPU) += i2c-gpu.o > > > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git > > a/drivers/i2c/busses/i2c-gpu.c b/drivers/i2c/busses/i2c-gpu.c new file > > mode 100644 index 0000000..0fd2944 > > --- /dev/null > > +++ b/drivers/i2c/busses/i2c-gpu.c > > @@ -0,0 +1,493 @@ > > +// 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> > > + > > +/* STATUS definitions */ > > +#define STATUS_SUCCESS 0 > > +#define STATUS_UNSUCCESSFUL 0x80000000UL > > +#define STATUS_TIMEOUT 0x80000001UL > > +#define STATUS_IO_DEVICE_ERROR 0x80000002UL > > +#define STATUS_IO_TIMEOUT 0x80000004UL > > +#define STATUS_IO_PREEMPTED 0x80000008UL > > This seems a little odd. Can these not be converted to standard errno codes? > It's more idiomatic to return 0 on success (like you define in the above) and a > negative error code on failure. If you use that scheme, there's no need to have > a symbolic name for success because it is used pretty much everywhere. > > See include/uapi/asm-generic/errno{,-base}.h for a list of standard error > codes. I think you should be able to find a match for each of the above. Will fix. > > +/* Cypress Type-C controllers (CCGx) device */ > > +#define CCGX_I2C_DEV_ADDRESS 0x08 > > I don't think there's a need for the define here. You only use the address once, > and that's where the client is defined, so you can just use 0x08 literally in that > one location. ok > > +/* I2C definitions */ > > +#define I2C_MST_CNTL 0x00 > > +#define I2C_MST_CNTL_GEN_START (1 << 0) > > +#define I2C_MST_CNTL_GEN_STOP (1 << 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_CMD_RESET (3 << 2) > > +#define I2C_MST_CNTL_GEN_RAB (1 << 4) > > +#define I2C_MST_CNTL_BURST_SIZE_SHIFT (6) > > +#define I2C_MST_CNTL_GEN_NACK (1 << 28) > > +#define I2C_MST_CNTL_STATUS (3 << 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 (1 << 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 << 0) > > +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_200KHZ > (0x087 << 0) > > +#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 (1 << 24) > > + > > +#define I2C_MST_DATA 0x0c > > + > > +#define I2C_MST_HYBRID_PADCTL 0x20 > > +#define I2C_MST_HYBRID_PADCTL_MODE_I2C (1 << > 0) > > +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV (1 << > 14) > > +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV (1 << > 15) > > + > > +/* PCI driver data */ > > +struct gpu_i2c_dev { > > + struct pci_dev *pci_dev; > > + void __iomem *regs; > > + struct i2c_adapter adapter; > > + struct i2c_client *client; > > + struct mutex mutex; > > + bool do_start; > > +}; > > + > > +static void enable_i2c_bus(struct gpu_i2c_dev *gdev) { > > + struct device *dev = &gdev->pci_dev->dev; > > + u32 val; > > + > > + /* enable I2C */ > > + val = readl(gdev->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; > > + > > + dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", > __func__, > > + (gdev->regs + I2C_MST_HYBRID_PADCTL), val); > > Do we really need these debug messages? None of the configuration above is > in any way parameterized, so the values that are written are pretty > deterministic (unless maybe the initial value of the register differs). > > If you really want to keep the debug messages, I think you should remove at > least the register address (you already print the name, which is much more > useful). Alternatively, have you considered using ftrace to allow these traces > to be dynamically enabled? Ok, will fix. > > > + > > + writel(val, gdev->regs + I2C_MST_HYBRID_PADCTL); > > + > > + /* enable 100KHZ mode */ > > + val = 0; > > + 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; > > + > > + dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__, > > + gdev->regs + I2C_MST_I2C0_TIMING, val); > > + writel(val, gdev->regs + I2C_MST_I2C0_TIMING); } > > + > > +static u32 i2c_check_status(struct gpu_i2c_dev *gdev) { > > + struct device *dev = &gdev->pci_dev->dev; > > + unsigned long target = jiffies + msecs_to_jiffies(1000); > > + u32 status = STATUS_UNSUCCESSFUL; > > + u32 val; > > + > > + while (time_is_after_jiffies(target)) { > > + val = readl(gdev->regs + I2C_MST_CNTL); > > + if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) != > > + I2C_MST_CNTL_CYCLE_TRIGGER) > > + break; > > + if ((val & I2C_MST_CNTL_STATUS) != > > + I2C_MST_CNTL_STATUS_BUS_BUSY) > > + break; > > + usleep_range(1000, 2000); > > + } > > + > > + if (time_is_before_jiffies(target)) { > > + dev_err(dev, "%si2c timeout", __func__); > > + return status; > > The error message looks suspicious here. There should be some sort of > separator between the function name and the rest of the message. Also, do > we really need to print the function name? That's not very useful to users (it > might actually be confusing because the function name is pretty generic, so > could be mistaken for a core function) and this is the only place where you > check for timeouts anyway, so no need to resolve any ambiguities by adding > the function name. Ok, will fix. > > > + } > > + > > + val = readl(gdev->regs + I2C_MST_CNTL); > > + switch (val & I2C_MST_CNTL_STATUS) { > > + case I2C_MST_CNTL_STATUS_OKAY: > > + status = STATUS_SUCCESS; > > + break; > > + case I2C_MST_CNTL_STATUS_NO_ACK: > > + status = STATUS_IO_DEVICE_ERROR; > > + break; > > + case I2C_MST_CNTL_STATUS_TIMEOUT: > > + status = STATUS_IO_TIMEOUT; > > + break; > > + case I2C_MST_CNTL_STATUS_BUS_BUSY: > > + status = STATUS_IO_PREEMPTED; > > + break; > > + default: > > + break; > > + } > > + return status; > > +} > > + > > +static u32 i2c_read(struct gpu_i2c_dev *gdev, u8 *data, u16 len) { > > + struct device *dev = &gdev->pci_dev->dev; > > + u32 status; > > + u32 val = 0; > > + > > + 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, gdev->regs + I2C_MST_CNTL); > > + > > + status = i2c_check_status(gdev); > > + if (status == STATUS_UNSUCCESSFUL) { > > + dev_err(dev, "%s failed\n", __func__); > > + return status; > > + } > > There's no need for this error message, since the caller already provides one. Sure. > > > + > > + val = readl(gdev->regs + I2C_MST_DATA); > > + switch (len) { > > + case 1: > > + data[0] = (val >> 0) & 0xff; > > + break; > > + case 2: > > + data[0] = (val >> 8) & 0xff; > > + data[1] = (val >> 0) & 0xff; > > + break; > > + case 3: > > + data[0] = (val >> 16) & 0xff; > > + data[1] = (val >> 8) & 0xff; > > + data[2] = (val >> 0) & 0xff; > > + break; > > + case 4: > > + data[0] = (val >> 24) & 0xff; > > + data[1] = (val >> 16) & 0xff; > > + data[2] = (val >> 8) & 0xff; > > + data[3] = (val >> 0) & 0xff; > > + break; > > + default: > > + break; > > + } > > + return status; > > +} > > + > > +static u32 i2c_manual_start(struct gpu_i2c_dev *gdev, u16 addr) { > > + u32 val = 0; > > + > > + val = addr << I2C_MST_ADDR_DAB; > > + writel(val, gdev->regs + I2C_MST_ADDR); > > + > > + val = 0; > > + 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, gdev->regs + I2C_MST_CNTL); > > + > > + return i2c_check_status(gdev); > > +} > > + > > +static u32 i2c_manual_stop(struct gpu_i2c_dev *gdev) { > > + u32 val = 0; > > + > > + 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, gdev->regs + I2C_MST_CNTL); > > + > > + return i2c_check_status(gdev); > > +} > > + > > +static u32 i2c_manual_write(struct gpu_i2c_dev *gdev, u8 data) { > > + u32 val = 0; > > + > > + writel(data, gdev->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, gdev->regs + I2C_MST_CNTL); > > + > > + return i2c_check_status(gdev); > > +} > > + > > +/* gdev i2c adapter */ > > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap, > > + struct i2c_msg *msgs, int num) > > +{ > > + struct gpu_i2c_dev *gdev = i2c_get_adapdata(adap); > > + struct device *dev = &gdev->pci_dev->dev; > > + int retry1b = 10; > > + u32 status; > > + int i, j; > > + > > + dev_dbg(dev, "%s: adap %p msgs %p num %d\n", __func__, adap, > msgs, > > +num); > > There's already code in drivers/i2c/i2c-core-base.c that will print mostly the > same information when DEBUG is enabled. Ok, will fix. > > > + mutex_lock(&gdev->mutex); > > + > > + for (i = 0; i < num; i++) { > > + if (msgs[i].flags & I2C_M_RD) { > > +retry2: > > + status = i2c_read(gdev, msgs[i].buf, msgs[i].len); > > + if (status != STATUS_SUCCESS) { > > + dev_err(dev, > > + "%s:%d i2c_read failed %08lx", __func__, > > + __LINE__, (unsigned long)status); > > Again, I'm not sure filename and line number will provide useful information. > I'd simply refer to the operation that failed, along the lines of: > > err = i2c_read(...); > if (err < 0) { > dev_err(dev, "I2C read failed: %d\n", err); > ... > } ok > > > + > > + if (--retry1b > 0) { > > + usleep_range(10000, 11000); > > + goto retry2; > > + } > > + break; > > + } > > Also, perhaps you want to only output the error right before the break so that > you don't potentially get 10 identical error messages? > > > + gdev->do_start = true; > > + } else if (msgs[i].flags & I2C_M_STOP) { > > + status = i2c_manual_stop(gdev); > > + if (status != STATUS_SUCCESS) { > > + dev_err(dev, > > + "%s:%d i2c_manual_stop failed %08lx", > __func__, > > + __LINE__, (unsigned long)status); > > + goto exit; > > + } > > + gdev->do_start = true; > > + } else { > > + dev_dbg(dev, "!I2C_M_RD start %d len %d\n", > > + gdev->do_start, msgs[i].len); > > + if (gdev->do_start) { > > + status = i2c_manual_start(gdev, msgs[i].addr); > > + if (status != STATUS_SUCCESS) { > > + dev_err(dev, > > + "%s:%d i2c_manual_start failed > %08lx", > > + __func__, __LINE__, > > + (unsigned long)status); > > + goto exit; > > + } > > + status = i2c_manual_write(gdev, > > + msgs[i].addr << 1); > > + if (status != STATUS_SUCCESS) { > > + dev_err(dev, > > + "%s:%d i2c_manual_write failed > %08lx", > > + __func__, __LINE__, > > + (unsigned long)status); > > + goto exit_stop; > > + } > > + gdev->do_start = false; > > + } > > + for (j = 0; j < msgs[i].len; j++) { > > + status = i2c_manual_write(gdev, > > + *(msgs[i].buf + j)); > > + if (status != STATUS_SUCCESS) { > > + dev_err(dev, > > + "%s:%d i2c_manual_write failed > %08lx", > > + __func__, __LINE__, > > + (unsigned long)status); > > + goto exit_stop; > > + } > > + } > > + } > > + } > > + goto exit; > > +exit_stop: > > + status = i2c_manual_stop(gdev); > > + if (status != STATUS_SUCCESS) > > + dev_err(dev, "i2c_manual_stop failed %x", status); > > +exit: > > + mutex_unlock(&gdev->mutex); > > + return i; > > +} > > You might want to consider using more descriptive names for the labels to > make the code more readable. Something like: > > ... > goto stop; > } > > goto unlock; > > stop: > ... > unlock: > ... > > Is more readable I think. Also, I think the high level of indentation makes this > function hard to read. You may want to split it up into smaller chunks. ok > > > + > > +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, > > +}; > > + > > +static int gpu_i2c_dev_init(struct gpu_i2c_dev *gdev) { > > + gdev->do_start = true; > > + > > + /* initialize mutex */ > > + mutex_init(&gdev->mutex); > > + > > + /* initialize i2c */ > > + enable_i2c_bus(gdev); > > + > > + return 0; > > +} > > You might want to consider moving this directly into the ->probe() > implementation. You never reuse this elsewhere and it's not big enough to > warrant the separate function. Inlining it into ->probe() will make the code > easier to follow, in my opinion. ok > > > + > > +struct i2c_board_info gpu_i2c_ucsi_board_info = { > > + I2C_BOARD_INFO("i2c-gpu-ucsi", CCGX_I2C_DEV_ADDRESS), }; > > + > > +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80 > > +/* pci driver */ > > +static const struct pci_device_id gpu_i2c_ids[] = { > > The comment there looks out of place. > > > + { 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_i2c_probe(struct pci_dev *dev, const struct > > +pci_device_id *id) { > > + struct gpu_i2c_dev *gdev; > > + int status; > > + > > + dev_info(&dev->dev, > > + "dev %p id %08x %08x sub %08x %08x class %08x %08x\n", > > + dev, id->vendor, id->device, id->subvendor, id->subdevice, > > + id->class, id->class_mask); > > If at all, this should be dev_dbg(). Generally I don't think there's any reason to > mention this at all. ->probe() should assume that it will succeed, which is the > expected case, and that should not produce any messages. Instead it's better > to let the user know if something failed using an error message. sure > > > + > > + gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev), > GFP_KERNEL); > > + if (!gdev) > > + return -ENOMEM; > > + > > + gdev->pci_dev = dev; > > + pci_set_drvdata(dev, gdev); > > + > > + status = pci_enable_device(dev); > > + if (status < 0) { > > + dev_err(&dev->dev, "pci_enable_device failed - %d\n", > status); > > + return status; > > + } > > + > > + pci_set_master(dev); > > + > > + gdev->regs = pci_iomap(dev, 0, 0); > > + if (!gdev->regs) { > > + dev_err(&dev->dev, "pci_iomap failed\n"); > > + status = -ENOMEM; > > + goto iomap_err; > > + } > > + > > + status = pci_enable_msi(dev); > > + if (status < 0) { > > + dev_err(&dev->dev, "pci_enable_msi failed - %d\n", status); > > + goto enable_msi_err; > > + } > > + > > + status = gpu_i2c_dev_init(gdev); > > + if (status < 0) { > > + dev_err(&dev->dev, "gpu_i2c_dev_init failed - %d\n", status); > > + goto i2c_init_err; > > + } > > + > > + i2c_set_adapdata(&gdev->adapter, gdev); > > + gdev->adapter.owner = THIS_MODULE; > > + strlcpy(gdev->adapter.name, "NVIDIA GPU I2C adapter", > > + sizeof(gdev->adapter.name)); > > + gdev->adapter.algo = &gpu_i2c_algorithm; > > + gdev->adapter.dev.parent = &dev->dev; > > + status = i2c_add_adapter(&gdev->adapter); > > + if (status < 0) { > > + dev_err(&dev->dev, "i2c_add_adapter failed - %d\n", status); > > + goto add_adapter_err; > > + } > > + > > + gpu_i2c_ucsi_board_info.irq = dev->irq; > > + gdev->client = i2c_new_device(&gdev->adapter, > > + &gpu_i2c_ucsi_board_info); > > I think it's technically possible that the above would race between multiple > instances of this I2C controller. The safer way would be to make > gpu_i2c_ucsi_board_info variable local to this function. sure > > > + > > + if (!gdev->client) { > > + dev_err(&dev->dev, "i2c_new_device failed - %d\n", status); > > + status = -ENODEV; > > + goto add_adapter_err; > > + } > > + > > + dev_set_drvdata(&dev->dev, gdev); > > I don't think you need this here since you've already called > pci_set_drvdata() above. ok > > > + pm_runtime_put_noidle(&dev->dev); > > + pm_runtime_allow(&dev->dev); > > + > > + return 0; > > + > > +add_adapter_err: > > + i2c_del_adapter(&gdev->adapter); > > +i2c_init_err: > > + pci_disable_msi(dev); > > +enable_msi_err: > > + pci_iounmap(dev, gdev->regs); > > +iomap_err: > > + pci_disable_device(dev); > > + return status; > > +} > > It's generally preferred to have names that describe the actions of the label, > instead of describing where they failed. I think that makes code easier to read, > and it also has the benefit of not being confusing when you jump to a label, > say add_adapter_err, from a location that doesn't have anything to do with > adding an adapter. > > Something like this would be better in my opinion: > > del_adapter: > ... > disable_msi: > ... > unmap: > ... > disable: > ... > ok > > + > > +static void gpu_i2c_remove(struct pci_dev *dev) { > > + struct gpu_i2c_dev *gdev = pci_get_drvdata(dev); > > + > > + i2c_del_adapter(&gdev->adapter); > > + pci_disable_msi(dev); > > + pci_iounmap(dev, gdev->regs); > > +} > > + > > +static int gpu_i2c_suspend(struct device *dev) { > > + dev_dbg(dev, "%s\n", __func__); > > + > > + return 0; > > +} > > Do we need this if it doesn't do anything? Will remove. > > > + > > +static int gpu_i2c_resume(struct device *dev) { > > + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); > > + > > + dev_dbg(dev, "%s\n", __func__); > > + > > + enable_i2c_bus(gdev); > > + > > + return 0; > > +} > > + > > +static int gpu_i2c_idle(struct device *dev) { > > + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); > > + > > + if (!mutex_trylock(&gdev->mutex)) { > > + dev_info(dev, "%s: -EBUSY\n", __func__); > > + return -EBUSY; > > + } > > + mutex_unlock(&gdev->mutex); > > + > > + return 0; > > +} > > + > > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, > gpu_i2c_resume, > > + gpu_i2c_idle); > > + > > +static struct pci_driver gpu_i2c_driver = { > > + .name = "gpu_i2c_driver", > > This name looks unusual for a driver. Perhaps "i2c-nvidia-gpu" to match the > module name? Ok Thanks Ajay -- nvpublic -- > > > + .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");
diff --git a/Documentation/i2c/busses/i2c-gpu b/Documentation/i2c/busses/i2c-gpu new file mode 100644 index 0000000..873ba34 --- /dev/null +++ b/Documentation/i2c/busses/i2c-gpu @@ -0,0 +1,18 @@ +Kernel driver i2c-gpu + +Datasheet: not publicly available. + +Authors: + Ajay Gupta <ajayg@nvidia.com> + +Description +----------- + +i2c-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 b2fcd1c..e99f8a2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6796,6 +6796,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-gpu +F: drivers/i2c/busses/i2c-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..ff8b2d4 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_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-gpu.ko. + config I2C_SIS5595 tristate "SiS 5595" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 18b26af..15d2894 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_GPU) += i2c-gpu.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-gpu.c b/drivers/i2c/busses/i2c-gpu.c new file mode 100644 index 0000000..0fd2944 --- /dev/null +++ b/drivers/i2c/busses/i2c-gpu.c @@ -0,0 +1,493 @@ +// 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> + +/* STATUS definitions */ +#define STATUS_SUCCESS 0 +#define STATUS_UNSUCCESSFUL 0x80000000UL +#define STATUS_TIMEOUT 0x80000001UL +#define STATUS_IO_DEVICE_ERROR 0x80000002UL +#define STATUS_IO_TIMEOUT 0x80000004UL +#define STATUS_IO_PREEMPTED 0x80000008UL + +/* Cypress Type-C controllers (CCGx) device */ +#define CCGX_I2C_DEV_ADDRESS 0x08 + +/* I2C definitions */ +#define I2C_MST_CNTL 0x00 +#define I2C_MST_CNTL_GEN_START (1 << 0) +#define I2C_MST_CNTL_GEN_STOP (1 << 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_CMD_RESET (3 << 2) +#define I2C_MST_CNTL_GEN_RAB (1 << 4) +#define I2C_MST_CNTL_BURST_SIZE_SHIFT (6) +#define I2C_MST_CNTL_GEN_NACK (1 << 28) +#define I2C_MST_CNTL_STATUS (3 << 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 (1 << 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 << 0) +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_200KHZ (0x087 << 0) +#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 (1 << 24) + +#define I2C_MST_DATA 0x0c + +#define I2C_MST_HYBRID_PADCTL 0x20 +#define I2C_MST_HYBRID_PADCTL_MODE_I2C (1 << 0) +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV (1 << 14) +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV (1 << 15) + +/* PCI driver data */ +struct gpu_i2c_dev { + struct pci_dev *pci_dev; + void __iomem *regs; + struct i2c_adapter adapter; + struct i2c_client *client; + struct mutex mutex; + bool do_start; +}; + +static void enable_i2c_bus(struct gpu_i2c_dev *gdev) +{ + struct device *dev = &gdev->pci_dev->dev; + u32 val; + + /* enable I2C */ + val = readl(gdev->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; + + dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", __func__, + (gdev->regs + I2C_MST_HYBRID_PADCTL), val); + + writel(val, gdev->regs + I2C_MST_HYBRID_PADCTL); + + /* enable 100KHZ mode */ + val = 0; + 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; + + dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__, + gdev->regs + I2C_MST_I2C0_TIMING, val); + writel(val, gdev->regs + I2C_MST_I2C0_TIMING); +} + +static u32 i2c_check_status(struct gpu_i2c_dev *gdev) +{ + struct device *dev = &gdev->pci_dev->dev; + unsigned long target = jiffies + msecs_to_jiffies(1000); + u32 status = STATUS_UNSUCCESSFUL; + u32 val; + + while (time_is_after_jiffies(target)) { + val = readl(gdev->regs + I2C_MST_CNTL); + if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) != + I2C_MST_CNTL_CYCLE_TRIGGER) + break; + if ((val & I2C_MST_CNTL_STATUS) != + I2C_MST_CNTL_STATUS_BUS_BUSY) + break; + usleep_range(1000, 2000); + } + + if (time_is_before_jiffies(target)) { + dev_err(dev, "%si2c timeout", __func__); + return status; + } + + val = readl(gdev->regs + I2C_MST_CNTL); + switch (val & I2C_MST_CNTL_STATUS) { + case I2C_MST_CNTL_STATUS_OKAY: + status = STATUS_SUCCESS; + break; + case I2C_MST_CNTL_STATUS_NO_ACK: + status = STATUS_IO_DEVICE_ERROR; + break; + case I2C_MST_CNTL_STATUS_TIMEOUT: + status = STATUS_IO_TIMEOUT; + break; + case I2C_MST_CNTL_STATUS_BUS_BUSY: + status = STATUS_IO_PREEMPTED; + break; + default: + break; + } + return status; +} + +static u32 i2c_read(struct gpu_i2c_dev *gdev, u8 *data, u16 len) +{ + struct device *dev = &gdev->pci_dev->dev; + u32 status; + u32 val = 0; + + 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, gdev->regs + I2C_MST_CNTL); + + status = i2c_check_status(gdev); + if (status == STATUS_UNSUCCESSFUL) { + dev_err(dev, "%s failed\n", __func__); + return status; + } + + val = readl(gdev->regs + I2C_MST_DATA); + switch (len) { + case 1: + data[0] = (val >> 0) & 0xff; + break; + case 2: + data[0] = (val >> 8) & 0xff; + data[1] = (val >> 0) & 0xff; + break; + case 3: + data[0] = (val >> 16) & 0xff; + data[1] = (val >> 8) & 0xff; + data[2] = (val >> 0) & 0xff; + break; + case 4: + data[0] = (val >> 24) & 0xff; + data[1] = (val >> 16) & 0xff; + data[2] = (val >> 8) & 0xff; + data[3] = (val >> 0) & 0xff; + break; + default: + break; + } + return status; +} + +static u32 i2c_manual_start(struct gpu_i2c_dev *gdev, u16 addr) +{ + u32 val = 0; + + val = addr << I2C_MST_ADDR_DAB; + writel(val, gdev->regs + I2C_MST_ADDR); + + val = 0; + 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, gdev->regs + I2C_MST_CNTL); + + return i2c_check_status(gdev); +} + +static u32 i2c_manual_stop(struct gpu_i2c_dev *gdev) +{ + u32 val = 0; + + 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, gdev->regs + I2C_MST_CNTL); + + return i2c_check_status(gdev); +} + +static u32 i2c_manual_write(struct gpu_i2c_dev *gdev, u8 data) +{ + u32 val = 0; + + writel(data, gdev->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, gdev->regs + I2C_MST_CNTL); + + return i2c_check_status(gdev); +} + +/* gdev i2c adapter */ +static int gpu_i2c_master_xfer(struct i2c_adapter *adap, + struct i2c_msg *msgs, int num) +{ + struct gpu_i2c_dev *gdev = i2c_get_adapdata(adap); + struct device *dev = &gdev->pci_dev->dev; + int retry1b = 10; + u32 status; + int i, j; + + dev_dbg(dev, "%s: adap %p msgs %p num %d\n", __func__, adap, msgs, num); + + mutex_lock(&gdev->mutex); + + for (i = 0; i < num; i++) { + if (msgs[i].flags & I2C_M_RD) { +retry2: + status = i2c_read(gdev, msgs[i].buf, msgs[i].len); + if (status != STATUS_SUCCESS) { + dev_err(dev, + "%s:%d i2c_read failed %08lx", __func__, + __LINE__, (unsigned long)status); + + if (--retry1b > 0) { + usleep_range(10000, 11000); + goto retry2; + } + break; + } + gdev->do_start = true; + } else if (msgs[i].flags & I2C_M_STOP) { + status = i2c_manual_stop(gdev); + if (status != STATUS_SUCCESS) { + dev_err(dev, + "%s:%d i2c_manual_stop failed %08lx", __func__, + __LINE__, (unsigned long)status); + goto exit; + } + gdev->do_start = true; + } else { + dev_dbg(dev, "!I2C_M_RD start %d len %d\n", + gdev->do_start, msgs[i].len); + if (gdev->do_start) { + status = i2c_manual_start(gdev, msgs[i].addr); + if (status != STATUS_SUCCESS) { + dev_err(dev, + "%s:%d i2c_manual_start failed %08lx", + __func__, __LINE__, + (unsigned long)status); + goto exit; + } + status = i2c_manual_write(gdev, + msgs[i].addr << 1); + if (status != STATUS_SUCCESS) { + dev_err(dev, + "%s:%d i2c_manual_write failed %08lx", + __func__, __LINE__, + (unsigned long)status); + goto exit_stop; + } + gdev->do_start = false; + } + for (j = 0; j < msgs[i].len; j++) { + status = i2c_manual_write(gdev, + *(msgs[i].buf + j)); + if (status != STATUS_SUCCESS) { + dev_err(dev, + "%s:%d i2c_manual_write failed %08lx", + __func__, __LINE__, + (unsigned long)status); + goto exit_stop; + } + } + } + } + goto exit; +exit_stop: + status = i2c_manual_stop(gdev); + if (status != STATUS_SUCCESS) + dev_err(dev, "i2c_manual_stop failed %x", status); +exit: + mutex_unlock(&gdev->mutex); + return i; +} + +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, +}; + +static int gpu_i2c_dev_init(struct gpu_i2c_dev *gdev) +{ + gdev->do_start = true; + + /* initialize mutex */ + mutex_init(&gdev->mutex); + + /* initialize i2c */ + enable_i2c_bus(gdev); + + return 0; +} + +struct i2c_board_info gpu_i2c_ucsi_board_info = { + I2C_BOARD_INFO("i2c-gpu-ucsi", CCGX_I2C_DEV_ADDRESS), +}; + +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80 +/* pci driver */ +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_i2c_probe(struct pci_dev *dev, const struct pci_device_id *id) +{ + struct gpu_i2c_dev *gdev; + int status; + + dev_info(&dev->dev, + "dev %p id %08x %08x sub %08x %08x class %08x %08x\n", + dev, id->vendor, id->device, id->subvendor, id->subdevice, + id->class, id->class_mask); + + gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL); + if (!gdev) + return -ENOMEM; + + gdev->pci_dev = dev; + pci_set_drvdata(dev, gdev); + + status = pci_enable_device(dev); + if (status < 0) { + dev_err(&dev->dev, "pci_enable_device failed - %d\n", status); + return status; + } + + pci_set_master(dev); + + gdev->regs = pci_iomap(dev, 0, 0); + if (!gdev->regs) { + dev_err(&dev->dev, "pci_iomap failed\n"); + status = -ENOMEM; + goto iomap_err; + } + + status = pci_enable_msi(dev); + if (status < 0) { + dev_err(&dev->dev, "pci_enable_msi failed - %d\n", status); + goto enable_msi_err; + } + + status = gpu_i2c_dev_init(gdev); + if (status < 0) { + dev_err(&dev->dev, "gpu_i2c_dev_init failed - %d\n", status); + goto i2c_init_err; + } + + i2c_set_adapdata(&gdev->adapter, gdev); + gdev->adapter.owner = THIS_MODULE; + strlcpy(gdev->adapter.name, "NVIDIA GPU I2C adapter", + sizeof(gdev->adapter.name)); + gdev->adapter.algo = &gpu_i2c_algorithm; + gdev->adapter.dev.parent = &dev->dev; + status = i2c_add_adapter(&gdev->adapter); + if (status < 0) { + dev_err(&dev->dev, "i2c_add_adapter failed - %d\n", status); + goto add_adapter_err; + } + + gpu_i2c_ucsi_board_info.irq = dev->irq; + gdev->client = i2c_new_device(&gdev->adapter, + &gpu_i2c_ucsi_board_info); + + if (!gdev->client) { + dev_err(&dev->dev, "i2c_new_device failed - %d\n", status); + status = -ENODEV; + goto add_adapter_err; + } + + dev_set_drvdata(&dev->dev, gdev); + pm_runtime_put_noidle(&dev->dev); + pm_runtime_allow(&dev->dev); + + return 0; + +add_adapter_err: + i2c_del_adapter(&gdev->adapter); +i2c_init_err: + pci_disable_msi(dev); +enable_msi_err: + pci_iounmap(dev, gdev->regs); +iomap_err: + pci_disable_device(dev); + return status; +} + +static void gpu_i2c_remove(struct pci_dev *dev) +{ + struct gpu_i2c_dev *gdev = pci_get_drvdata(dev); + + i2c_del_adapter(&gdev->adapter); + pci_disable_msi(dev); + pci_iounmap(dev, gdev->regs); +} + +static int gpu_i2c_suspend(struct device *dev) +{ + dev_dbg(dev, "%s\n", __func__); + + return 0; +} + +static int gpu_i2c_resume(struct device *dev) +{ + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); + + dev_dbg(dev, "%s\n", __func__); + + enable_i2c_bus(gdev); + + return 0; +} + +static int gpu_i2c_idle(struct device *dev) +{ + struct gpu_i2c_dev *gdev = dev_get_drvdata(dev); + + if (!mutex_trylock(&gdev->mutex)) { + dev_info(dev, "%s: -EBUSY\n", __func__); + return -EBUSY; + } + mutex_unlock(&gdev->mutex); + + return 0; +} + +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, gpu_i2c_resume, + gpu_i2c_idle); + +static struct pci_driver gpu_i2c_driver = { + .name = "gpu_i2c_driver", + .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");
Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C controller which can be accessed over I2C. This driver add 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> --- Changes from v1 -> v2: None Documentation/i2c/busses/i2c-gpu | 18 ++ MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-gpu.c | 493 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 528 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-gpu create mode 100644 drivers/i2c/busses/i2c-gpu.c