arm: tegra: implement NVEC driver using tegra i2c.

Quoted 4 lines

drivers/i2c/busses/i2c-tegra.c +4 -0

+static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
+{
+	i2c_writel(i2c_dev, val, I2C_SL_RCVD);
+

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) · line 486

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c +6 -0

+	/*
+	 * TODO: A correct fix needs to be found for this.
+	 *
+	 * We experience less incomplete messages with this delay than without
+	 * it, but we don't know why. Help is appreciated.
+	 */

Uh oh. So I assume this is another reason for staging?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+		return -EINVAL;

The core checks for slave_cb being valid.

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) · line 512

Quoted 1 line

drivers/i2c/busses/i2c-tegra.c +1 -0

+		i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);

You could use value here, too, or?

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) · line 547

Quoted 2 lines

drivers/i2c/busses/i2c-tegra.c +2 -0

+	if (!tegra_i2c_slave_isr(irq, i2c_dev))
+		return IRQ_HANDLED;

Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return handled".

drivers/i2c/busses/i2c-tegra.c static u32 tegra_i2c_func(struct i2c_adapter *adap) · line 762

Quoted 2 lines

drivers/i2c/busses/i2c-tegra.c +2 -0

+	if (slave->addr > 0x7F)
+		addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;

Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you need to check for the flag (slave->flags & I2C_CLIENT_TEN).

drivers/staging/nvec/nvec.c +15 -0

+/**
+ * nvec_slave_cb - I2C slave callback
+ *
+ * This callback fills our RX buffers and empties our TX
+ * buffers. This uses a finite state machine.
+ */
+static int nvec_slave_cb(struct i2c_client *client,
+		enum i2c_slave_event event, u8 *val)
+{
+	struct nvec_chip *nvec = i2c_get_clientdata(client);
+
+	switch (event) {
+	case I2C_SLAVE_WRITE_REQUESTED:
+		/* Alloc new msg only if prev transaction finished */
+		if (nvec->state == ST_NONE)

drivers/staging/nvec/nvec.c +8 -0

+			dev_err(&client->dev,
+				"received address 0x%02x, expected 0x%02x\n",
+				((*val) >> 1), client->addr);
+			return -1;
+		}
+		nvec->state = ST_TRANS_START;
+		break;
+

drivers/staging/nvec/nvec.c +11 -0

+	case I2C_SLAVE_READ_PROCESSED:
+		if (nvec->state != ST_RX &&
+		    nvec->state != ST_TX) {
+			dev_err(&client->dev,
+				"unexpected read: state %d\n",
+				nvec->state);
+			return -1;
+		}
+
+		if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
+			dev_err(nvec->dev,

(quoted diff) +2 -0

+		break;
+

NVEC driver contains code to manage tegra i2c controller in slave mode. I2C slave support was implemented in linux kernel. The goal of this patch serie is to implement I2C slave mode in tegra drived and rework NVEC driver to use it.

Quoted 4 lines

NVEC driver contains code to manage tegra i2c controller in slave mode. I2C slave support was implemented in linux kernel. The goal of this patch serie is to implement I2C slave mode in tegra drived and rework NVEC driver to use it.

Thanks. Looks pretty good already. If being I2C slave was the reason for this driver being in staging, we soon need to think of a proper place outside staging.

One thing where we need your help as a I2C maintainer is how to represent an i2c slave device using device-tree. You may remember our discussion in the past from here [1] where you suggested to just make a slave client by its compatible name. Stephen Warren from NVIDIA raised some concerns about this solution because it may not be appropriate in all possible future cases (which is what a proper device-tree representation should take care off). He instead suggested to mark a slave client by adding some flag to the reg property, to be able to handle a situation where both master client and slave client have the same i2c bus address forming a loopback (e.g. for testing purpose) on the same bus. More details here [2].

I hope with this post I can join the different discussions somehow so we are able to find a common sense which is acceptable for all.

One thing where we need your help as a I2C maintainer is how to represent an i2c slave device using device-tree. You may remember our discussion in the past from here [1] where you suggested to just make a slave client by its compatible name. Stephen Warren from NVIDIA raised some concerns about this solution because it may not be appropriate in all possible future cases (which is what a proper device-tree representation should take care off). He instead suggested to mark a slave client by adding some flag to the reg property, to be able to handle a situation where both master client and slave client have the same i2c bus address forming a loopback (e.g. for testing purpose) on the same bus. More details here [2].

I hope with this post I can join the different discussions somehow so we are able to find a common sense which is acceptable for all.

I'm back in town. Sorry for the delay...

Quoted 10 lines

One thing where we need your help as a I2C maintainer is how to represent an i2c slave device using device-tree. You may remember our discussion in the past from here [1] where you suggested to just make a slave client by its compatible name. Stephen Warren from NVIDIA raised some concerns about this solution because it may not be appropriate in all possible future cases (which is what a proper device-tree representation should take care off). He instead suggested to mark a slave client by adding some flag to the reg property, to be able to handle a situation where both master client and slave client have the same i2c bus address forming a loopback (e.g. for testing purpose) on the same bus. More details here [2].

Well... I can agree that we shouldn't prevent a loopback from a DT point of view. (Despite the fact that it is really for development and I wonder how many I2C IP cores can do this flawlessly)

However, I am still against putting that information into the reg property. I see devices coming which have multiple addresses, so people somewhen want to encode this in DT as well. I'd like to have that a simple array of addresses. Adding flags, rarely used, will create a mess IMO.

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 2 lines

I hope with this post I can join the different discussions somehow so we are able to find a common sense which is acceptable for all.

Thanks for doing this! I changed the subject to maybe raise interest a bit more.

The container node has a #address-cells property for this very reason. It's perfectly well-defined how to split up a property containing a large number of cells into separate values, by using the value of #address-cells. Plus, the canonical formatting (albeit not enforced by the DT compiler) for a property that contains an array of entries, each 2 cells in size, would be:

reg = <0 0x1a>, <0 0x40>, <0 0x48>;

rather than:

reg = <0 0x1a 0 0x40 0 0x48>;

... so it's quite simple to make it very human-readable too.

Quoted 7 lines

The container node has a #address-cells property for this very reason. It's perfectly well-defined how to split up a property containing a large number of cells into separate values, by using the value of #address-cells. Plus, the canonical formatting (albeit not enforced by the DT compiler) for a property that contains an array of entries, each 2 cells in size, would be:

Quoted 7 lines

reg = <0 0x1a>, <0 0x40>, <0 0x48>;

rather than:

reg = <0 0x1a 0 0x40 0 0x48>;

... so it's quite simple to make it very human-readable too.

I give in to the flag idea. I also noticed that we'd need another flag anyhow to mark 10 bit addresses. I am still thinking between using two address-cells in that case (clean seperation between address and flags) or to encode the flags as MSB in the current address (all busses will have same address-cells and child description, less code paths and no overhead in dtbs).

That being said, for the loopback testcase, the I2C slave framework will need updates as well. I think I can cook up something. Will be interesting to see if my hardware can do this. Has the loopback already been tested on Tegra?

Quoted 13 lines

The container node has a #address-cells property for this very reason. It's perfectly well-defined how to split up a property containing a large number of cells into separate values, by using the value of #address-cells. Plus, the canonical formatting (albeit not enforced by the DT compiler) for a property that contains an array of entries, each 2 cells in size, would be:

reg = <0 0x1a>, <0 0x40>, <0 0x48>;

rather than:

reg = <0 0x1a 0 0x40 0 0x48>;

Quoted 1 line

... so it's quite simple to make it very human-readable too.

I give in to the flag idea. I also noticed that we'd need another flag anyhow to mark 10 bit addresses. I am still thinking between using two address-cells in that case (clean seperation between address and flags) or to encode the flags as MSB in the current address (all busses will have same address-cells and child description, less code paths and no overhead in dtbs).

Quoted 7 lines

I give in to the flag idea. I also noticed that we'd need another flag anyhow to mark 10 bit addresses. I am still thinking between using two address-cells in that case (clean seperation between address and flags) or to encode the flags as MSB in the current address (all busses will have same address-cells and child description, less code paths and no overhead in dtbs).

Reading thru the thread, this seems good to me. I would go with adding flags in the MSB of the reg cell rather than adding a cell.

Quoted 8 lines

Quoted 7 lines

I give in to the flag idea. I also noticed that we'd need another flag anyhow to mark 10 bit addresses. I am still thinking between using two address-cells in that case (clean seperation between address and flags) or to encode the flags as MSB in the current address (all busses will have same address-cells and child description, less code paths and no overhead in dtbs).

Quoted 2 lines

Reading thru the thread, this seems good to me. I would go with adding flags in the MSB of the reg cell rather than adding a cell.

Thanks. I was leaning to the MSB idea, too, and am currently checking how the code would look like. Feels much better to do knowing that it is basically upstream compatible ;)

Did you have enough time to check this ? (sorry for the noise, if I missed patch(es) in the ml)

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 2 lines

I hope with this post I can join the different discussions somehow so we are able to find a common sense which is acceptable for all.

Thanks for doing this! I changed the subject to maybe raise interest a bit more.

Hello,

Quoted 5 lines · Wolfram Sang

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

I admit I didn't follow the discussions referenced in the footnotes, but I wonder if the slave part should be added to the device tree at all. AFAICT it could (and so should) be completely userspace-defined which slave driver is used on which address. I imagine that for most controllers the bus addresses to use can be chosen more or less freely. So what am I missing?

Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:

Quoted 7 lines · Wolfram Sang

Quoted 5 lines

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 6 lines

I admit I didn't follow the discussions referenced in the footnotes, but I wonder if the slave part should be added to the device tree at all. AFAICT it could (and so should) be completely userspace-defined which slave driver is used on which address. I imagine that for most controllers the bus addresses to use can be chosen more or less freely. So what am I missing?

if you had read the footnotes you would know :-) Our usecase is connect an embeedded controller via i2c to the host soc, similar to cros-ec, but here the ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other stuff, for which the drivers are best implemented in kernel code AFAIK.

Hello Marc,

Quoted 15 lines · Marc Dietrich

Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:

Quoted 7 lines · Wolfram Sang

Quoted 5 lines

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 6 lines

I admit I didn't follow the discussions referenced in the footnotes, but I wonder if the slave part should be added to the device tree at all. AFAICT it could (and so should) be completely userspace-defined which slave driver is used on which address. I imagine that for most controllers the bus addresses to use can be chosen more or less freely. So what am I missing?

Quoted 4 lines

if you had read the footnotes you would know :-) Our usecase is connect an embeedded controller via i2c to the host soc, similar to cros-ec, but here the ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other stuff, for which the drivers are best implemented in kernel code AFAIK.

Right, the driver might sensibly be implemented in kernel space. But I'd vote that you still need to do the binding of these drivers to your slave controller from userspace. Then there is no need to specify anything in your dtb.

Quoted 21 lines · Uwe Kleine-König

Quoted 15 lines · Marc Dietrich

Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:

Quoted 7 lines · Wolfram Sang

Quoted 5 lines

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 6 lines

I admit I didn't follow the discussions referenced in the footnotes, but I wonder if the slave part should be added to the device tree at all. AFAICT it could (and so should) be completely userspace-defined which slave driver is used on which address. I imagine that for most controllers the bus addresses to use can be chosen more or less freely. So what am I missing?

Quoted 4 lines

if you had read the footnotes you would know :-) Our usecase is connect an embeedded controller via i2c to the host soc, similar to cros-ec, but here the ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other stuff, for which the drivers are best implemented in kernel code AFAIK.

Quoted 4 lines

Right, the driver might sensibly be implemented in kernel space. But I'd vote that you still need to do the binding of these drivers to your slave controller from userspace. Then there is no need to specify anything in your dtb.

I think the set of I2C slave devices that are implemented by the Linux system can reasonably be considered part of the HW definition. Most DT content to date has been a definition of the HW that's available to SW, but at least in this case, this I2C slave device is something that must be implemented (admittedly in conjunction with SW) using the I2C slave HW on the main SoC, in order for the overall HW to work as intended.

BTW, I believe [email protected] was created to address subsystem-level DT schema questions like this. It's much
lower volume, so perhaps the thread would get noticed by the DT maintainers if posted there (or perhaps just CC them)?

Hello Stephen,

Quoted 27 lines · Stephen Warren

Quoted 21 lines · Uwe Kleine-König

Quoted 15 lines · Marc Dietrich

Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:

Quoted 7 lines · Wolfram Sang

Quoted 5 lines

So what about adding a new property "i2c-slave-reg"? This does not only prevent the confusion above, but also makes it very clear that this node is an I2C slave without the need to encode that somehow in the compatible property (although it probably should be described there as well, still).

Quoted 6 lines

I admit I didn't follow the discussions referenced in the footnotes, but I wonder if the slave part should be added to the device tree at all. AFAICT it could (and so should) be completely userspace-defined which slave driver is used on which address. I imagine that for most controllers the bus addresses to use can be chosen more or less freely. So what am I missing?

Quoted 4 lines

if you had read the footnotes you would know :-) Our usecase is connect an embeedded controller via i2c to the host soc, similar to cros-ec, but here the ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other stuff, for which the drivers are best implemented in kernel code AFAIK.

Quoted 4 lines

Right, the driver might sensibly be implemented in kernel space. But I'd vote that you still need to do the binding of these drivers to your slave controller from userspace. Then there is no need to specify anything in your dtb.

Quoted 7 lines

I think the set of I2C slave devices that are implemented by the Linux system can reasonably be considered part of the HW definition. Most DT content to date has been a definition of the HW that's available to SW, but at least in this case, this I2C slave device is something that must be implemented (admittedly in conjunction with SW) using the I2C slave HW on the main SoC, in order for the overall HW to work as intended.

I'm not convinced. Why must it be implemented?

And is it too late if the setup is done by userspace?

The devicetree description of a flash chip doesn't include the rootfs content either, although a working rootfs is critical for most operating systems. The devicetree description of an ethernet adapter doesn't include its network setup although an ethernet adapter hardly makes any sense without an IP.

Quoted 4 lines

BTW, I believe [email protected] was created to address subsystem-level DT schema questions like this. It's much
lower volume, so perhaps the thread would get noticed by the DT maintainers if posted there (or perhaps just CC them)?

I added them to Cc.