[v3,01/12] scripts/dtc: check: Add 10bit/slave i2c reg flags support

Commit Message

Serge Semin May 26, 2020, 9:55 p.m. UTC

Recently the I2C-controllers slave interface support was added to the
kernel I2C subsystem. In this case Linux can be used as, for example,
a I2C EEPROM machine. See [1] for details. Other than instantiating
the EEPROM-slave device from user-space there is a way to declare the
device in dts. In this case firstly the I2C bus controller must support
the slave interface. Secondly I2C-slave sub-node of that controller
must have "reg"-property with flag I2C_OWN_SLAVE_ADDRESS set (flag is
declared in [2]). That flag is declared as (1 << 30), which when set
makes dtc unhappy about too big address set for a I2C-slave:

Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064"
Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064"

Similar problem would have happened if we had set the 10-bit address
flag I2C_TEN_BIT_ADDRESS in the "reg"-property.

In order to fix the problem we suggest to alter the I2C-bus reg-check
algorithm, so one would be aware of the upper bits set. Normally if no
flag specified, the 7-bit address is expected in the "reg"-property.
If I2C_TEN_BIT_ADDRESS is set, then the 10-bit address check will be
performed. The I2C_OWN_SLAVE_ADDRESS flag will be just ignored.

[1] Documentation/i2c/slave-interface.rst
[2] include/dt-bindings/i2c/i2c.h

Signed-off-by: Serge Semin <[e-mail]>
Cc: Alexey Malahov <[e-mail]>
Cc: Thomas Bogendoerfer <[e-mail]>
Cc: Mika Westerberg <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 scripts/dtc/checks.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Rob Herring (Arm) May 27, 2020, 1:17 a.m. UTC | #1

On Wed, May 27, 2020 at 12:55:17AM +0300, Serge Semin wrote:
> Recently the I2C-controllers slave interface support was added to the
> kernel I2C subsystem. In this case Linux can be used as, for example,
> a I2C EEPROM machine. See [1] for details. Other than instantiating
> the EEPROM-slave device from user-space there is a way to declare the
> device in dts. In this case firstly the I2C bus controller must support
> the slave interface. Secondly I2C-slave sub-node of that controller
> must have "reg"-property with flag I2C_OWN_SLAVE_ADDRESS set (flag is
> declared in [2]). That flag is declared as (1 << 30), which when set
> makes dtc unhappy about too big address set for a I2C-slave:
> 
> Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064"
> Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064"
> 
> Similar problem would have happened if we had set the 10-bit address
> flag I2C_TEN_BIT_ADDRESS in the "reg"-property.
> 
> In order to fix the problem we suggest to alter the I2C-bus reg-check
> algorithm, so one would be aware of the upper bits set. Normally if no
> flag specified, the 7-bit address is expected in the "reg"-property.
> If I2C_TEN_BIT_ADDRESS is set, then the 10-bit address check will be
> performed. The I2C_OWN_SLAVE_ADDRESS flag will be just ignored.
> 
> [1] Documentation/i2c/slave-interface.rst
> [2] include/dt-bindings/i2c/i2c.h
> 
> Signed-off-by: Serge Semin <[e-mail]>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Thomas Bogendoerfer <[e-mail]>
> Cc: Mika Westerberg <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  scripts/dtc/checks.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

I've lost track of who all I've said this to already for this issue, but 
patches to dtc should be against upstream and a version of this has been 
sent there already. But it seems they've lost interest in addressing the 
review comments. So feel free to send another one. The same comment 
applies here.

Rob

Serge Semin May 27, 2020, 9:46 a.m. UTC | #2

On Tue, May 26, 2020 at 07:17:04PM -0600, Rob Herring wrote:
> On Wed, May 27, 2020 at 12:55:17AM +0300, Serge Semin wrote:
> > Recently the I2C-controllers slave interface support was added to the
> > kernel I2C subsystem. In this case Linux can be used as, for example,
> > a I2C EEPROM machine. See [1] for details. Other than instantiating
> > the EEPROM-slave device from user-space there is a way to declare the
> > device in dts. In this case firstly the I2C bus controller must support
> > the slave interface. Secondly I2C-slave sub-node of that controller
> > must have "reg"-property with flag I2C_OWN_SLAVE_ADDRESS set (flag is
> > declared in [2]). That flag is declared as (1 << 30), which when set
> > makes dtc unhappy about too big address set for a I2C-slave:
> > 
> > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064"
> > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064"
> > 
> > Similar problem would have happened if we had set the 10-bit address
> > flag I2C_TEN_BIT_ADDRESS in the "reg"-property.
> > 
> > In order to fix the problem we suggest to alter the I2C-bus reg-check
> > algorithm, so one would be aware of the upper bits set. Normally if no
> > flag specified, the 7-bit address is expected in the "reg"-property.
> > If I2C_TEN_BIT_ADDRESS is set, then the 10-bit address check will be
> > performed. The I2C_OWN_SLAVE_ADDRESS flag will be just ignored.
> > 
> > [1] Documentation/i2c/slave-interface.rst
> > [2] include/dt-bindings/i2c/i2c.h
> > 
> > Signed-off-by: Serge Semin <[e-mail]>
> > Cc: Alexey Malahov <[e-mail]>
> > Cc: Thomas Bogendoerfer <[e-mail]>
> > Cc: Mika Westerberg <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  scripts/dtc/checks.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> 

> I've lost track of who all I've said this to already for this issue, but 
> patches to dtc should be against upstream and a version of this has been 
> sent there already. But it seems they've lost interest in addressing the 
> review comments. So feel free to send another one. The same comment 
> applies here.

Agreed. Rob, could you also take a look at the patch
[PATCH v3 03/12] dt-bindings: i2c: Discard i2c-slave flag from the DW I2C example
from this series? You must have missed that. I've created that patch in
accordance with your suggestion from v2:
https://lore.kernel.org/linux-i2c/20200511160924.GA9628@bogus/

-Sergey

> 
> Rob

Serge Semin May 27, 2020, 11:20 a.m. UTC | #3

On Tue, May 26, 2020 at 07:17:04PM -0600, Rob Herring wrote:
> On Wed, May 27, 2020 at 12:55:17AM +0300, Serge Semin wrote:
> > Recently the I2C-controllers slave interface support was added to the
> > kernel I2C subsystem. In this case Linux can be used as, for example,
> > a I2C EEPROM machine. See [1] for details. Other than instantiating
> > the EEPROM-slave device from user-space there is a way to declare the
> > device in dts. In this case firstly the I2C bus controller must support
> > the slave interface. Secondly I2C-slave sub-node of that controller
> > must have "reg"-property with flag I2C_OWN_SLAVE_ADDRESS set (flag is
> > declared in [2]). That flag is declared as (1 << 30), which when set
> > makes dtc unhappy about too big address set for a I2C-slave:
> > 
> > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064"
> > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064"
> > 
> > Similar problem would have happened if we had set the 10-bit address
> > flag I2C_TEN_BIT_ADDRESS in the "reg"-property.
> > 
> > In order to fix the problem we suggest to alter the I2C-bus reg-check
> > algorithm, so one would be aware of the upper bits set. Normally if no
> > flag specified, the 7-bit address is expected in the "reg"-property.
> > If I2C_TEN_BIT_ADDRESS is set, then the 10-bit address check will be
> > performed. The I2C_OWN_SLAVE_ADDRESS flag will be just ignored.
> > 
> > [1] Documentation/i2c/slave-interface.rst
> > [2] include/dt-bindings/i2c/i2c.h
> > 
> > Signed-off-by: Serge Semin <[e-mail]>
> > Cc: Alexey Malahov <[e-mail]>
> > Cc: Thomas Bogendoerfer <[e-mail]>
> > Cc: Mika Westerberg <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  scripts/dtc/checks.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> I've lost track of who all I've said this to already for this issue, but 
> patches to dtc should be against upstream and a version of this has been 
> sent there already. But it seems they've lost interest in addressing the 
> review comments. So feel free to send another one. The same comment 
> applies here.

There is another patch in this series:
[PATCH v3 04/12] dt-bindings: i2c: dw: Add Baikal-T1 SoC I2C controller
which is also waiting for your review. I've updated it as you requested.
Could you take a look at that too?

-Sergey

> 
> Rob

Patch

diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
index 4b3c486f1399..6321fc5b7404 100644
--- a/scripts/dtc/checks.c
+++ b/scripts/dtc/checks.c
@@ -1028,6 +1028,7 @@  static void check_i2c_bus_reg(struct check *c, struct dt_info *dti, struct node
 	const char *unitname = get_unitname(node);
 	char unit_addr[17];
 	uint32_t reg = 0;
+	uint32_t addr;
 	int len;
 	cell_t *cells = NULL;
 
@@ -1044,17 +1045,21 @@  static void check_i2c_bus_reg(struct check *c, struct dt_info *dti, struct node
 	}
 
 	reg = fdt32_to_cpu(*cells);
-	snprintf(unit_addr, sizeof(unit_addr), "%x", reg);
+	addr = reg & 0x3FFFFFFFU;
+	snprintf(unit_addr, sizeof(unit_addr), "%x", addr);
 	if (!streq(unitname, unit_addr))
 		FAIL(c, dti, node, "I2C bus unit address format error, expected \"%s\"",
 		     unit_addr);
 
 	for (len = prop->val.len; len > 0; len -= 4) {
 		reg = fdt32_to_cpu(*(cells++));
-		if (reg > 0x3ff)
+		addr = reg & 0x3FFFFFFFU;
+		if ((reg & (1 << 31)) && addr > 0x3ff)
 			FAIL_PROP(c, dti, node, prop, "I2C address must be less than 10-bits, got \"0x%x\"",
-				  reg);
-
+				  addr);
+		else if (!(reg & (1 << 31)) && addr > 0x7f)
+			FAIL_PROP(c, dti, node, prop, "I2C address must be less than 7-bits, got \"0x%x\"",
+				  addr);
 	}
 }
 WARNING(i2c_bus_reg, check_i2c_bus_reg, NULL, &reg_format, &i2c_bus_bridge);