Re: [PATCH v6 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

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

 



On 2018-09-06 19:39, Ajay Gupta wrote:
> 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.

No, you should add a pointer to a struct i2c_adapter_quirks, with
max_read_len set I think. At least that's how e.g. i2c-qup.c does it.

>>> +		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.

Well, in that case, you don't fully support combined I2C transactions.
You cannot e.g. generate this from Documentation/i2c/i2c-protocol

	S Addr Rd [A] [Data] NA S Addr Wr [A] Data [A] P

By your description, all reads are terminated by a stop, and that
is a quirk. I think you need to add some of the I2C_AQ_COMB* flags
to the above mentioned struct i2c_adapter_quirks.

>> 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.

Why is it certain that the client driver in 2/2 is the only client of
this adapter? If that's really fundamentally the case, and it can't
change for whatever reason, then I think these things should be
mentioned somewhere.

>> 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.

Right you are, I missed that assignment. Sorry...

Cheers,
Peter

> 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");
>>>
> 




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux