RE: [PATCH v3 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

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

 



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 adds I2C bus driver to communicate with Type-C controller.
> > I2C client driver will be part of USB Type-C UCSI driver.
> >
> 
> Some small comments below, after addressing them
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> 
> > 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
> >
> >  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     | 389
> ++++++++++++++++++++++++++++++++
> >  5 files changed, 424 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..fa01187
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > @@ -0,0 +1,389 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nvidia GPU I2C controller Driver
> > + *
> > + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
> > + * Author: Ajay Gupta <ajayg@xxxxxxxxxx>
> 
> > + *
> 
> Redundant line.
> 
> > + */
ok
> 
> > +#include <asm/unaligned.h>
> 
> This should go after linux/* ones...
> 
> > +#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>
> > +
> 
> ...here.
will fix
> 
> > +/* I2C definitions */
> > +#define I2C_MST_CNTL                           0x0
> > +#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)
> 
> Redundant parens
> 
> > +#define I2C_MST_CNTL_GEN_NACK                  BIT(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             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 pci_dev *pci_dev;
> 
> This should be a good argument why you are keeping struct pci_dev instead of
> struct device.
sure
> 
> > +       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) {
> > +       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);
> 
> > +       int status = -EIO;
> 
> You can get rid of this variable by returning in place.
Sounds good
> 
> > +       u32 val;
> > +
> > +       do {
> > +               val = readl(i2cd->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);
> > +       } while (time_is_after_jiffies(target));
> > +
> > +       if (time_is_before_jiffies(target))
> > +               return status;
> > +
> > +       val = readl(i2cd->regs + I2C_MST_CNTL);
> > +       switch (val & I2C_MST_CNTL_STATUS) {
> > +       case I2C_MST_CNTL_STATUS_OKAY:
> > +               status = 0;
> > +               break;
> > +       case I2C_MST_CNTL_STATUS_NO_ACK:
> > +               status = -EIO;
> > +               break;
> > +       case I2C_MST_CNTL_STATUS_TIMEOUT:
> > +               status = -ETIME;
> > +               break;
> > +       case I2C_MST_CNTL_STATUS_BUS_BUSY:
> > +               status = -EBUSY;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +       return status;
> > +}
> > +
> > +static int i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len) {
> > +       int 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, 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:
> > +               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->pci_dev->dev;
> 
> > +       int sts;
> 
> Btw, be consistent with names, status looks slightly better.
ok
> 
> > +       int i, j;
> > +
> > +       mutex_lock(&i2cd->mutex);
> > +       for (i = 0; i < num; i++) {
> > +               if (msgs[i].flags & I2C_M_RD) {
> > +                       sts = i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> > +                       if (sts < 0) {
> > +                               dev_err(dev, "i2c_read error %x", sts);
> > +                               break;
> > +                       }
> > +                       i2cd->do_start = true;
> > +               } else if (msgs[i].flags & I2C_M_STOP) {
> > +                       sts = i2c_stop(i2cd);
> > +                       if (sts < 0) {
> > +                               dev_err(dev, "i2c_stop error %x", sts);
> > +                               goto unlock;
> > +                       }
> > +                       i2cd->do_start = true;
> > +               } else {
> > +                       if (i2cd->do_start) {
> > +                               sts = i2c_start(i2cd, msgs[i].addr);
> > +                               if (sts < 0) {
> > +                                       dev_err(dev, "i2c_start error %x", sts);
> > +                                       goto unlock;
> > +                               }
> > +                               sts = i2c_write(i2cd, msgs[i].addr << 1);
> > +                               if (sts < 0) {
> > +                                       dev_err(dev, "i2c_write error %x", sts);
> > +                                       goto stop;
> > +                               }
> > +                               i2cd->do_start = false;
> > +                       }
> > +                       for (j = 0; j < msgs[i].len; j++) {
> > +                               sts = i2c_write(i2cd, *(msgs[i].buf + j));
> > +                               if (sts < 0) {
> > +                                       dev_err(dev, "i2c_write error %x", sts);
> > +                                       goto stop;
> > +                               }
> > +                       }
> > +               }
> > +       }
> > +       goto unlock;
> > +stop:
> > +       sts = i2c_stop(i2cd);
> > +       if (sts < 0)
> > +               dev_err(dev, "i2c_stop error %x", sts);
> > +unlock:
> > +       mutex_unlock(&i2cd->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, };
> > +
> 
> > +#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},
> > +       { }
> > +};
> 
> I'm really in doubt how it's supposed to be enumerated without conflicts or
> shadowing other hardware.
> Explain in the comments, please.
Will add comments.

> 
> > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> > +
> > +static int gpu_i2c_probe(struct pci_dev *dev, const struct
> > +pci_device_id *id) {
> 
> > +       struct i2c_board_info gpu_i2c_ucsi_board_info = {
> > +               I2C_BOARD_INFO("ccgx-ucsi", 0x8)
> > +       };
> 
> Can't you rather put this as static const which is provided in driver_data above
Can't add as "const" since I am saving "irq" field from pci_dev->irq 
but will add new function gpu_populate_client()

Thanks
Ajay
--
nvpublic
--




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux