Hi Peter,
> >> Hmm, what do you need to sync read/write with?
> > What if there are multiple clients and each of them wants to use I2C bus for
> read/write?
> > Even in UCSI client, user may want to change alt mode which will
> > result in read/write request in a thread. So we have to synchronize between
> UCSI ISR and the user thread.
>
> Synchronization of reads/writes from multiple clients is handled by the I2C
> core and the adapter lock. Lock in i2c_transfer() for the details.
Ok. I hope it will be good to drop mutex then.
> >> The only thing I can find is
> >> that busy-check in gpu_i2c_idle, but can runtime-pm really try to
> >> idle an adapter that has an xfer in flight? If that's the case, what
> >> stops a new xfer from being initiated after runtime-pm has checked if
> >> ->runtime_idle returns ok but before the device is really suspended?
> >>
> >> PM is not an area I'm familiar with, so it's an honest question.
> >>
> >>> +};
> >>> +
> >>> +static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd) {
> >>> + u32 val;
> >>> +
> >>> + /* enable I2C */
> >>> + val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
> >>> + val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
> >>> + I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
> >>> + I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
> >>> + writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
> >>> +
> >>> + /* enable 100KHZ mode */
> >>> + val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
> >>> + val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
> >>> + << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
> >>> + val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
> >>> + writel(val, i2cd->regs + I2C_MST_I2C0_TIMING); }
> >>> +
> >>> +static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd) {
> >>> + unsigned long target = jiffies + msecs_to_jiffies(1000);
> >>> + u32 val;
> >>> +
> >>> + do {
> >>> + val = readl(i2cd->regs + I2C_MST_CNTL);
> >>> + if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
> >>> + break;
> >>> + if ((val & I2C_MST_CNTL_STATUS) !=
> >>> + I2C_MST_CNTL_STATUS_BUS_BUSY)
> >>> + break;
> >>> + usleep_range(1000, 2000);
> >>> + } while (time_is_after_jiffies(target));
> >>> + if (time_is_before_jiffies(target)) {
> >>> + dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> >>> + return -EIO;
> >>> + }
> >>> +
> >>> + val = readl(i2cd->regs + I2C_MST_CNTL);
> >>> + switch (val & I2C_MST_CNTL_STATUS) {
> >>> + case I2C_MST_CNTL_STATUS_OKAY:
> >>> + return 0;
> >>> + case I2C_MST_CNTL_STATUS_NO_ACK:
> >>> + return -EIO;
> >>> + case I2C_MST_CNTL_STATUS_TIMEOUT:
> >>> + return -ETIME;
> >>> + case I2C_MST_CNTL_STATUS_BUS_BUSY:
> >>> + return -EBUSY;
> >>> + default:
> >>> + return 0;
> >>> + }
> >>> +}
> >>> +
> >>> +static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16
> >>> +len) {
> >>> + int status;
> >>> + u32 val;
> >>> +
> >>> + val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> >>> + I2C_MST_CNTL_CMD_READ | (len <<
> >> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> >>> + I2C_MST_CNTL_CYCLE_TRIGGER |
> >> I2C_MST_CNTL_GEN_NACK;
> >>> + val &= ~I2C_MST_CNTL_GEN_RAB;
> >>> + writel(val, i2cd->regs + I2C_MST_CNTL);
> >>> +
> >>> + status = gpu_i2c_check_status(i2cd);
> >>> + if (status < 0)
> >>> + return status;
> >>> +
> >>> + val = readl(i2cd->regs + I2C_MST_DATA);
> >>> + switch (len) {
> >>> + case 1:
> >>> + data[0] = val;
> >>> + break;
> >>> + case 2:
> >>> + put_unaligned_be16(val, data);
> >>> + break;
> >>> + case 3:
> >>> + put_unaligned_be16(val >> 8, data);
> >>> + data[2] = val;
> >>> + break;
> >>> + case 4:
> >>> + put_unaligned_be32(val, data);
> >>> + break;
> >>> + default:
> >>> + break;
> >>> + }
> >>> + return status;
> >>> +}
> >>> +
> >>> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd, 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 gpu_i2c_check_status(i2cd); }
> >>> +
> >>> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> >>> + u32 val;
> >>> +
> >>> + val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> >>> + I2C_MST_CNTL_GEN_NACK;
> >>> + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> >>> + writel(val, i2cd->regs + I2C_MST_CNTL);
> >>> +
> >>> + return gpu_i2c_check_status(i2cd); }
> >>> +
> >>> +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> >>> + u32 val;
> >>> +
> >>> + writel(data, i2cd->regs + I2C_MST_DATA);
> >>> +
> >>> + val = I2C_MST_CNTL_CMD_WRITE | (1 <<
> >> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> >>> + I2C_MST_CNTL_GEN_NACK;
> >>> + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> >>> + I2C_MST_CNTL_GEN_RAB);
> >>> + writel(val, i2cd->regs + I2C_MST_CNTL);
> >>> +
> >>> + return gpu_i2c_check_status(i2cd); }
> >>> +
> >>> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> >>> + struct i2c_msg *msgs, int num) {
> >>> + struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> >>> + int status;
> >>> + int i, j;
> >>> +
> >>> + mutex_lock(&i2cd->mutex);
> >>> + for (i = 0; i < num; i++) {
> >>> + if (msgs[i].flags & I2C_M_RD) {
> >>> + /* gpu_i2c_read has implicit start and stop */
> >>> + status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> >>
> >> Per your comments on v8, the address has to be programmed before the
> >> transfer, but you fail to do that if the first message is a read.
> > This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST
>
> Yes, it will. If the transfer consists of a single read, i.e. w/o any leading write.
> E.g. i2c_smbus_read_byte (which is emulated in your case and will generate
> this pattern).
I don't think we intend to support SMBUS commands and so I will drop
I2C_FUNC_SMBUS_EMUL.
> >> Is the I2C_MST_ADDR register really in the loop when you do a write?
> > That’s not needed. We need to write I2C_MST_ADDR once before starting
> > one or more transfers to a client.
> >
> >> Because
> >> I think it would simplify things if you could move the write to the
> >> I2C_MST_ADDR register to be in gpu_i2c_read instead of in
> >> gpu_i2c_start. Or, maybe just write to I2C_MST_ADDR before the for-loop
> in this function.
> > See above. Hint: I2C_AQ_COMB_WRITE_FIRST
> >
> >>> + if (status < 0)
> >>> + goto unlock;
> >>> + } else {
> >>> + /* start on first write message */
> >>> + if (i == 0) {
> >>
> >> You do not issue a restart nor do you write the address to the bus
> >> for the second message, if it's a write.
> > That’s not needed. We need to write I2C_MST_ADDR once before starting
> > one or more transfers to a client.
> >
> >> That makes me wonder if this patch was tested, because AFAICT
> >> ccg_write in patch 2/2 uses xfers combining two write messages and I
> >> fail to see how that can work?
> > See above.
> >
> >> Also, have you verified if gpu_i2c_start really issues a restart
> >> instead of a stop followed by a start if the bus is not idle on function entry?
> > Yes. Hint: refer gpu_i2_start() and look for I2C_MST_CNTL_GEN_STOP bit.
>
> Yes, I see that you mask out that bit, but my question was if that actually
> behaves as we wish and that we in fact do get a restart instead of a stop/start.
It does as per documentation. It forces a START bit to be sent.
> > Thanks
> > Ajay
--
nvpublic
--
> >> Or is the
> >> documentation clear on this detail? Might be worth checking anyway,
> >> as documentation is not always completely correct...
> >>
> >> Cheers,
> >> Peter
> >>
> >>> + u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> >>> + status = gpu_i2c_start(i2cd, msgs[i].addr);
> >>> + if (status < 0)
> >>> + goto unlock;
> >>> +
> >>> + status = gpu_i2c_write(i2cd, addr);
> >>> + if (status < 0)
> >>> + goto stop;
> >>> + }
> >>> + for (j = 0; j < msgs[i].len; j++) {
> >>> + status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> >>> + if (status < 0)
> >>> + goto stop;
> >>> + }
> >>> + /* stop if last write message */
> >>> + if (i == (num - 1)) {
> >>> + status = gpu_i2c_stop(i2cd);
> >>> + if (status < 0)
> >>> + goto unlock;
> >>> + }
> >>> + }
> >>> + }
> >>> + status = i;
> >>> + goto unlock;
> >>> +stop:
> >>> + status = gpu_i2c_stop(i2cd);
> >>> +unlock:
> >>> + mutex_unlock(&i2cd->mutex);
> >>> + return status;
> >>> +}
> >>> +
> >>> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> >>> + .flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST |
> >> I2C_AQ_COMB_SAME_ADDR,
> >>> + .max_read_len = 4,
> >>> +};
> >>> +
> >>> +static u32 gpu_i2c_functionality(struct i2c_adapter *adap) {
> >>> + return I2C_FUNC_I2C | 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;
> >>> + }
> >>> +
> >>> + mutex_init(&i2cd->mutex);
> >>> + gpu_enable_i2c_bus(i2cd);
> >>> +
> >>> + i2c_set_adapdata(&i2cd->adapter, i2cd);
> >>> + i2cd->adapter.owner = THIS_MODULE;
> >>> + strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> >>> + sizeof(i2cd->adapter.name));
> >>> + i2cd->adapter.algo = &gpu_i2c_algorithm;
> >>> + i2cd->adapter.quirks = &gpu_i2c_quirks;
> >>> + i2cd->adapter.dev.parent = &pdev->dev;
> >>> + status = i2c_add_adapter(&i2cd->adapter);
> >>> + if (status < 0)
> >>> + goto free_irq_vectors;
> >>> +
> >>> + status = gpu_populate_client(i2cd, pdev->irq);
> >>> + if (status < 0) {
> >>> + dev_err(&pdev->dev, "gpu_populate_client failed %d\n",
> >> status);
> >>> + goto del_adapter;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +
> >>> +del_adapter:
> >>> + i2c_del_adapter(&i2cd->adapter);
> >>> +free_irq_vectors:
> >>> + pci_free_irq_vectors(pdev);
> >>> + return status;
> >>> +}
> >>> +
> >>> +static void gpu_i2c_remove(struct pci_dev *pdev) {
> >>> + struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> >>> +
> >>> + i2c_del_adapter(&i2cd->adapter);
> >>> + pci_free_irq_vectors(pdev);
> >>> +}
> >>> +
> >>> +static int gpu_i2c_resume(struct device *dev) {
> >>> + struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> >>> +
> >>> + gpu_enable_i2c_bus(i2cd);
> >>> + return 0;
> >>> +}
> >>> +
> >>> +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");
> >>>
> >
