Hi Peter,
> > 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>
> > +static void enable_i2c_bus(struct gpu_i2c_dev *i2cd)
>
> Please prefix all your functions with gpu_, or nvidia_gpu_ or something like
> that (because gpu sounds like a something graphics, not that nvidia_gpu helps
> with that problem though...)
Ok, will prefix them.
> > +{
> > + u32 val;
> > +
> > + /* enable I2C */
> > + val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
> > + val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
> > + I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
> > + I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
> > + writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
> > +
> > + /* enable 100KHZ mode */
> > + val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
> > + val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
> > + << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
> > + val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
> > + writel(val, i2cd->regs + I2C_MST_I2C0_TIMING); }
> > +
> > +static int i2c_check_status(struct gpu_i2c_dev *i2cd) {
> > + unsigned long target = jiffies + msecs_to_jiffies(1000);
> > + u32 val;
> > +
> > + do {
> > + val = readl(i2cd->regs + I2C_MST_CNTL);
> > + if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
> > + break;
> > + if ((val & I2C_MST_CNTL_STATUS) !=
> > + I2C_MST_CNTL_STATUS_BUS_BUSY)
> > + break;
> > + usleep_range(1000, 2000);
> > + } while (time_is_after_jiffies(target));
> > + if (time_is_before_jiffies(target))
> > + return -EIO;
> > +
> > + val = readl(i2cd->regs + I2C_MST_CNTL);
> > + switch (val & I2C_MST_CNTL_STATUS) {
> > + case I2C_MST_CNTL_STATUS_OKAY:
> > + return 0;
> > + case I2C_MST_CNTL_STATUS_NO_ACK:
> > + return -EIO;
> > + case I2C_MST_CNTL_STATUS_TIMEOUT:
> > + return -ETIME;
> > + case I2C_MST_CNTL_STATUS_BUS_BUSY:
> > + return -EBUSY;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len) {
> > + int status;
> > + u32 val;
> > +
> > + val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> > + I2C_MST_CNTL_CMD_READ | (len <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > + I2C_MST_CNTL_CYCLE_TRIGGER |
> I2C_MST_CNTL_GEN_NACK;
> > + val &= ~I2C_MST_CNTL_GEN_RAB;
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + status = i2c_check_status(i2cd);
> > + if (status < 0)
> > + return status;
> > +
> > + val = readl(i2cd->regs + I2C_MST_DATA);
> > + switch (len) {
> > + case 1:
> > + data[0] = val;
> > + break;
> > + case 2:
> > + put_unaligned_be16(val, data);
> > + break;
> > + case 3:
> > + put_unaligned_be16(val >> 8, data);
> > + data[2] = val;
> > + break;
> > + case 4:
> > + put_unaligned_be32(val, data);
> > + break;
> > + default:
>
> This seems to behave rather strange for len > 4, and I do not see anything
> preventing that from happening.
Actually the check is in ccg_read() of the
client driver at https://marc.info/?l=linux-usb&m=153618608301206&w=2
> Am I missing something, or do you need to add a quirk for max_read_len?
I will add a check in this function as well.
> > + break;
> > + }
> > + return status;
> > +}
> > +
> > +static int i2c_start(struct gpu_i2c_dev *i2cd, u16 addr) {
> > + u32 val;
> > +
> > + val = addr << I2C_MST_ADDR_DAB;
> > + writel(val, i2cd->regs + I2C_MST_ADDR);
> > +
> > + val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> > + I2C_MST_CNTL_GEN_NACK;
> > + val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + return i2c_check_status(i2cd);
> > +}
> > +
> > +static int i2c_stop(struct gpu_i2c_dev *i2cd) {
> > + u32 val;
> > +
> > + val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> > + I2C_MST_CNTL_GEN_NACK;
> > + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + return i2c_check_status(i2cd);
> > +}
> > +
> > +static int i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> > + u32 val;
> > +
> > + writel(data, i2cd->regs + I2C_MST_DATA);
> > +
> > + val = I2C_MST_CNTL_CMD_WRITE | (1 <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > + I2C_MST_CNTL_GEN_NACK;
> > + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> > + I2C_MST_CNTL_GEN_RAB);
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + return i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msgs, int num) {
> > + struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > + struct device *dev = i2cd->dev;
> > + int status;
> > + int i, j;
> > +
> > + mutex_lock(&i2cd->mutex);
> > + for (i = 0; i < num; i++) {
>
> I don't get how this loop works. You do not seem to always start with a start.
> E.g., if the first message is I2C_M_RD, i2c_read() is called before i2c_start(). Is
> that correct?
That’s correct and it is because i2_read() itself does the START and STOP.
> Also, if a message has I2C_M_STOP but not I2C_M_RD, the call to i2c_stop()
> happens before the call the i2c_write(). What's up with that?
Client driver sends I2C_M_STOP along with every write message.
> I would expect an i2c_start() before the loop or first in the loop, and a
> i2c_stop() after the loop.
I2c_read() function itself takes care of it.
> As is, the stop after the loop is only called on error.
To be exact, whenever there is error in i2c_write().
> In addition, I would expect messages that have I2C_M_STOP to trigger an
> i2c_stop() call *after* the message, even if the message is not last in the loop.
Yes, its like that for all write messages.
> What am I missing?
>
> > + if (msgs[i].flags & I2C_M_RD) {
> > + status = i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> > + if (status < 0) {
> > + dev_err(dev, "i2c_read error %x", status);
> > + goto unlock;
> > + }
> > + i2cd->do_start = true;
> > + } else if (msgs[i].flags & I2C_M_STOP) {
> > + status = i2c_stop(i2cd);
> > + if (status < 0) {
> > + dev_err(dev, "i2c_stop error %x", status);
> > + goto unlock;
> > + }
> > + i2cd->do_start = true;
> > + } else {
> > + if (i2cd->do_start) {
> > + status = i2c_start(i2cd, msgs[i].addr);
> > + if (status < 0) {
> > + dev_err(dev, "i2c_start error %x",
> > + status);
> > + goto unlock;
> > + }
> > + status = i2c_write(i2cd, msgs[i].addr << 1);
> > + if (status < 0) {
> > + dev_err(dev, "i2c_write error %x",
> > + status);
> > + goto stop;
> > + }
> > + i2cd->do_start = false;
> > + }
> > + for (j = 0; j < msgs[i].len; j++) {
> > + status = i2c_write(i2cd, *(msgs[i].buf + j));
> > + if (status < 0) {
> > + dev_err(dev, "i2c_write error %x",
> > + status);
> > + goto stop;
> > + }
> > + }
> > + }
> > + }
> > + status = i;
> > + goto unlock;
<here>
> > +stop:
> > + status = i2c_stop(i2cd);
> > + if (status < 0)
> > + dev_err(dev, "i2c_stop error %x", status);
>
> Hmm, every time you call one of i2c_start, i2c_read, i2c_write or i2c_stop you
> check for error and issue a generic dev_err message. I think you should move
> these error messages into the functions instead or repeating them for every
> use.
Ok, will fix.
> > +unlock:
> > + mutex_unlock(&i2cd->mutex);
> > + return status;
>
> Here you return status, which seems to be zero when everything is fine.
> That is incorrect for sure; you should return num on success.
When everything is fine then status is written with number of messages
"i". Please see above.
Thanks
Ajay
--
nvpublic
--
> Cheers,
> Peter
>
> > +}
> > +
> > +static u32 gpu_i2c_functionality(struct i2c_adapter *adap) {
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> > +
> > +static const struct i2c_algorithm gpu_i2c_algorithm = {
> > + .master_xfer = gpu_i2c_master_xfer,
> > + .functionality = gpu_i2c_functionality,
> > +};
> > +
> > +/*
> > + * This driver is for Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code only
> > + * to avoid dependency of adding product id for any new card which
> > + * requires this driver.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN class for now and it will be updated when UCSI
> > + * over PCI gets a class code.
> > + * There is no other NVIDIA cards with UNKNOWN class code. Even if
> > +the
> > + * driver gets loaded for an undesired card then eventually
> > +i2c_read()
> > + * (initiated from UCSI i2c_client) will timeout or UCSI commands
> > +will
> > + * timeout.
> > + */
> > +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80
> > +static const struct pci_device_id gpu_i2c_ids[] = {
> > + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > + PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> > +
> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) {
> > + static struct i2c_board_info gpu_ccgx_ucsi = {
> > + I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> > + };
> > +
> > + gpu_ccgx_ucsi.irq = irq;
> > + i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> > + if (!i2cd->client) {
> > + dev_err(i2cd->dev, "i2c_new_device failed\n");
> > + return -ENODEV;
> > + }
> > + return 0;
> > +}
> > +
> > +static int gpu_i2c_probe(struct pci_dev *pdev, const struct
> > +pci_device_id *id) {
> > + struct gpu_i2c_dev *i2cd;
> > + int status;
> > +
> > + i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev),
> GFP_KERNEL);
> > + if (!i2cd)
> > + return -ENOMEM;
> > +
> > + i2cd->dev = &pdev->dev;
> > + dev_set_drvdata(&pdev->dev, i2cd);
> > +
> > + status = pcim_enable_device(pdev);
> > + if (status < 0) {
> > + dev_err(&pdev->dev, "pcim_enable_device failed %d\n",
> status);
> > + return status;
> > + }
> > +
> > + pci_set_master(pdev);
> > +
> > + i2cd->regs = pcim_iomap(pdev, 0, 0);
> > + if (!i2cd->regs) {
> > + dev_err(&pdev->dev, "pcim_iomap failed\n");
> > + return -ENOMEM;
> > + }
> > +
> > + status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > + if (status < 0) {
> > + dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n",
> status);
> > + return status;
> > + }
> > +
> > + i2cd->do_start = true;
> > + mutex_init(&i2cd->mutex);
> > + enable_i2c_bus(i2cd);
> > +
> > + i2c_set_adapdata(&i2cd->adapter, i2cd);
> > + i2cd->adapter.owner = THIS_MODULE;
> > + strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> > + sizeof(i2cd->adapter.name));
> > + i2cd->adapter.algo = &gpu_i2c_algorithm;
> > + i2cd->adapter.dev.parent = &pdev->dev;
> > + status = i2c_add_adapter(&i2cd->adapter);
> > + if (status < 0) {
> > + dev_err(&pdev->dev, "i2c_add_adapter failed %d\n", status);
> > + goto free_irq_vectors;
> > + }
> > +
> > + status = gpu_populate_client(i2cd, pdev->irq);
> > + if (status < 0) {
> > + dev_err(&pdev->dev, "gpu_populate_client failed %d\n",
> status);
> > + goto del_adapter;
> > + }
> > +
> > + return 0;
> > +
> > +del_adapter:
> > + i2c_del_adapter(&i2cd->adapter);
> > +free_irq_vectors:
> > + pci_free_irq_vectors(pdev);
> > + return status;
> > +}
> > +
> > +static void gpu_i2c_remove(struct pci_dev *pdev) {
> > + struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> > +
> > + i2c_del_adapter(&i2cd->adapter);
> > + pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static int gpu_i2c_resume(struct device *dev) {
> > + struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > + enable_i2c_bus(i2cd);
> > + return 0;
> > +}
> > +
> > +static int gpu_i2c_idle(struct device *dev) {
> > + struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > + if (!mutex_trylock(&i2cd->mutex)) {
> > + dev_info(dev, "-EBUSY\n");
> > + return -EBUSY;
> > + }
> > + mutex_unlock(&i2cd->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> > +gpu_i2c_idle);
> > +
> > +static struct pci_driver gpu_i2c_driver = {
> > + .name = "nvidia-gpu",
> > + .id_table = gpu_i2c_ids,
> > + .probe = gpu_i2c_probe,
> > + .remove = gpu_i2c_remove,
> > + .driver = {
> > + .pm = &gpu_i2c_driver_pm,
> > + },
> > +};
> > +
> > +module_pci_driver(gpu_i2c_driver);
> > +
> > +MODULE_AUTHOR("Ajay Gupta <ajayg@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> > +MODULE_LICENSE("GPL v2");
> >
