[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

commit: dm: core: Enable optional use of fdt_translate_address()

enables device's bus/child address translation method, depending
on bus 'ranges' property and including child 'reg' property.
This change makes impossible to decode the 'reg' for node with
'#size-cells' equal to 0.

Such case is possible by the specification and is also used in U-Boot,
e.g. by I2C uclass or S5P GPIO - the last one is broken at present.

For this purpose this patch set introduces new core function:
 fdt_addr_t dev_get_reg(struct udevice *dev)
which returns the 'reg' value in the same way as previously dev_get_addr().

This fixes s5p gpio driver and booting issue on few Exynos based boards:
- Trats2
- Odroid U3/X2

As an example of use, this patch set also modifies i2c uclass driver
by using the new function for getting chip address.

Przemyslaw Marczak (3):
  dm: core: extend API by new function: dev_get_reg()
  gpio: s5p: use dev_get_reg() instead of dev_get_addr()
  dm: i2c: get chip address with dev_get_reg()

 drivers/core/device.c    | 17 +++++++++++++----
 drivers/gpio/s5p_gpio.c  |  2 +-
 drivers/i2c/i2c-uclass.c | 17 ++++++++---------
 include/dm/device.h      | 23 +++++++++++++++++++++++
 include/i2c.h            | 11 ++++-------
 5 files changed, 49 insertions(+), 21 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/3] dm: core: extend API by new function: dev_get_reg()

[Przemyslaw Marczak]

After changes introduced to dev_get_addr() by:

commit: dm: core: Enable optional use of fdt_translate_address()

the mentioned function is not allowed to parse the 'reg' property
of child node for which the '#size-cells == 0'.

To fill the gap, this commit introduces new core function dev_get_reg(),
which makes it possible to get the 'reg' property's value for that use case.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
---
 drivers/core/device.c | 17 +++++++++++++----
 include/dm/device.h   | 23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 758f390..1131175 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -581,6 +581,18 @@ const char *dev_get_uclass_name(struct udevice *dev)
 	return dev->uclass->uc_drv->name;
 }
 
+fdt_addr_t dev_get_reg(struct udevice *dev)
+{
+#if CONFIG_IS_ENABLED(OF_CONTROL)
+	return fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
+						dev->parent->of_offset,
+						dev->of_offset, "reg",
+						0, NULL);
+#else
+	return FDT_ADDR_T_NONE;
+#endif
+}
+
 fdt_addr_t dev_get_addr(struct udevice *dev)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
@@ -601,14 +613,11 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
 					     dev->of_offset, reg);
 	}
 
+	addr = dev_get_reg(dev);
 	/*
 	 * Use the "simple" translate function for less complex
 	 * bus setups.
 	 */
-	addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
-						dev->parent->of_offset,
-						dev->of_offset, "reg",
-						0, NULL);
 	if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
 		if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
 			addr = simple_bus_translate(dev->parent, addr);
diff --git a/include/dm/device.h b/include/dm/device.h
index 7fb9935..08bcb02 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -445,8 +445,31 @@ int device_find_first_child(struct udevice *parent, struct udevice **devp);
 int device_find_next_child(struct udevice **devp);
 
 /**
+ * dev_get_reg() - Get the reg property of a device
+ *
+ * This returns the address without bus/child address space translation.
+ *
+ * @dev: Pointer to a device
+ *
+ * @return addr
+ */
+fdt_addr_t dev_get_reg(struct udevice *dev);
+
+/**
  * dev_get_addr() - Get the reg property of a device
  *
+ * The returned address value depends on a config options:
+ * Case 1: CONFIG_OF_TRANSLATE=y (default)
+ * Result: Make bus/child address space translation, that dependents on "ranges"
+ *         property.
+ *
+ * Case 2: CONFIG_OF_TRANSLATE is not set
+ * Result: Decode only device node's 'reg' property, without translation.
+ *
+ * Case 3: CONFIG_OF_TRANSLATE is not set; CONFIG_SIMPLE_BUS=y
+ * Result: When 'dev->parent' is a simple bus - then do the same as in case 1,
+ *         otherwise do the same as in case 2.
+ *
  * @dev: Pointer to a device
  *
  * @return addr
-- 
1.9.1

[U-Boot] [PATCH 2/3] gpio: s5p: use dev_get_reg() instead of dev_get_addr()

[Przemyslaw Marczak]

In a result of enabling the CONFIG_OF_TRANSLAE, function dev_get_addr(),
doesn't support device tree nodes with 'size-cells == 0'.

But this is the way how the s5p gpio driver's compatible device tree
nodes are defined. Switching the driver to use function dev_get_reg(),
fixes this issue.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/gpio/s5p_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
index 0f22b23..383c6ce 100644
--- a/drivers/gpio/s5p_gpio.c
+++ b/drivers/gpio/s5p_gpio.c
@@ -350,7 +350,7 @@ static int gpio_exynos_bind(struct udevice *parent)
 
 		dev->of_offset = node;
 
-		reg = dev_get_addr(dev);
+		reg = dev_get_reg(dev);
 		if (reg != FDT_ADDR_T_NONE)
 			bank = (struct s5p_gpio_bank *)((ulong)base + reg);
 
-- 
1.9.1

[U-Boot] [PATCH 3/3] dm: i2c: get chip address with dev_get_reg()

[Przemyslaw Marczak]

This commit cleanups the I2C uclass driver by:
- simplify i2c_child_post_bind() method
- cleanups i2c_chip_ofdata_to_platdata(), by calling dev_get_reg()
  for getting chip address

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefan Roese <sr@denx.de>
---
 drivers/i2c/i2c-uclass.c | 17 ++++++++---------
 include/i2c.h            | 11 ++++-------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index 50b99ea..3cfbd22 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -467,16 +467,17 @@ int i2c_deblock(struct udevice *bus)
 	return ops->deblock(bus);
 }
 
-int i2c_chip_ofdata_to_platdata(const void *blob, int node,
-				struct dm_i2c_chip *chip)
+int i2c_chip_ofdata_to_platdata(struct udevice *dev)
 {
-	chip->offset_len = fdtdec_get_int(gd->fdt_blob, node,
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+
+	chip->offset_len = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
 					  "u-boot,i2c-offset-len", 1);
 	chip->flags = 0;
-	chip->chip_addr = fdtdec_get_int(gd->fdt_blob, node, "reg", -1);
-	if (chip->chip_addr == -1) {
+	chip->chip_addr = dev_get_reg(dev);
+	if (chip->chip_addr == FDT_ADDR_T_NONE) {
 		debug("%s: I2C Node '%s' has no 'reg' property\n", __func__,
-		      fdt_get_name(blob, node, NULL));
+		      fdt_get_name(gd->fdt_blob, dev->of_offset, NULL));
 		return -EINVAL;
 	}
 
@@ -501,12 +502,10 @@ static int i2c_post_bind(struct udevice *dev)
 
 static int i2c_child_post_bind(struct udevice *dev)
 {
-	struct dm_i2c_chip *plat = dev_get_parent_platdata(dev);
-
 	if (dev->of_offset == -1)
 		return 0;
 
-	return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset, plat);
+	return i2c_chip_ofdata_to_platdata(dev);
 }
 
 UCLASS_DRIVER(i2c) = {
diff --git a/include/i2c.h b/include/i2c.h
index 1f5ae45..c5bb39c 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -518,15 +518,12 @@ int i2c_get_chip_for_busnum(int busnum, int chip_addr, uint offset_len,
  * i2c_chip_ofdata_to_platdata() - Decode standard I2C platform data
  *
  * This decodes the chip address from a device tree node and puts it into
- * its dm_i2c_chip structure. This should be called in your driver's
- * ofdata_to_platdata() method.
+ * its dm_i2c_chip structure. This is called after device's bind inside
+ * uclass driver's i2c_child_post_bind() method.
  *
- * @blob:	Device tree blob
- * @node:	Node offset to read from
- * @spi:	Place to put the decoded information
+ * @dev:	pointer to a chip device
  */
-int i2c_chip_ofdata_to_platdata(const void *blob, int node,
-				struct dm_i2c_chip *chip);
+int i2c_chip_ofdata_to_platdata(struct udevice *dev);
 
 /**
  * i2c_dump_msgs() - Dump a list of I2C messages
-- 
1.9.1

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
> commit: dm: core: Enable optional use of fdt_translate_address()
>
> enables device's bus/child address translation method, depending
> on bus 'ranges' property and including child 'reg' property.
> This change makes impossible to decode the 'reg' for node with
> '#size-cells' equal to 0.
>
> Such case is possible by the specification and is also used in U-Boot,
> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.

Can you please explain the problem you're seeing in more detail? Without 
any context, my initial reaction is that this is simply a bug somewhere. 
That bug should be fixed, rather than introducing new APIs to hide the 
problem.

> For this purpose this patch set introduces new core function:
>   fdt_addr_t dev_get_reg(struct udevice *dev)
> which returns the 'reg' value in the same way as previously dev_get_addr().
>
> This fixes s5p gpio driver and booting issue on few Exynos based boards:
> - Trats2
> - Odroid U3/X2

Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following:


        i2c at 138d0000 {
                 samsung,i2c-sda-delay = <100>;
                 samsung,i2c-slave-addr = <0x10>;
                 samsung,i2c-max-bus-freq = <100000>;
                 status = "okay";

                 max77686_pmic at 09 {
                         compatible = "maxim,max77686";
                         interrupts = <7 0>;
                         reg = <0x09 0 0>;

Is that the node you're having problems with? If so, I believe this may 
simply be due to invalid DT content. In exynos4.dtsi, that i2c node is 
defined as:

         i2c at 138d0000 {
                 #address-cells = <1>;
                 #size-cells = <0>;

Thus, any reg property in a child of that node must only contain a 
single cell (the sum of #address-cells and #size-cells in the parent). 
Does fixing the DT so it's valid solve your issue at all?

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 12/16/2015 11:53 AM, Stephen Warren wrote:
> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>> commit: dm: core: Enable optional use of fdt_translate_address()
>>
>> enables device's bus/child address translation method, depending
>> on bus 'ranges' property and including child 'reg' property.
>> This change makes impossible to decode the 'reg' for node with
>> '#size-cells' equal to 0.
>>
>> Such case is possible by the specification and is also used in U-Boot,
>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>
> Can you please explain the problem you're seeing in more detail? Without
> any context, my initial reaction is that this is simply a bug somewhere.
> That bug should be fixed, rather than introducing new APIs to hide the
> problem.

Ah, I guess the problem is caused by the following code in 
__of_translate_address():

	bus->count_cells(blob, parent, &na, &ns);
	if (!OF_CHECK_COUNTS(na, ns)) {
		printf("%s: Bad cell count for %s\n", __FUNCTION__,

That's because the function assumes it's called for MMIO addresses. 
However, reg values for I2C devices aren't MMIO addresses, so those 
assumptions don't apply. So, there is an argument for introducing some 
new functionality. I'm not sure that a whole new function is the correct 
way to go though. Rather, the existing translation functions should be 
enhanced to know the location of root of the address space that contains 
the address that's being translated. Then, translation can stop there.

Something like skipping the check on ns in the above code if parent == 
addr_space_root_offset, and also terminating the for (;;) loop in that 
function under a similar condition.

This would allow for translation to occur for buses other than the CPU's 
root MMIO space, yet not attempt to translate across known address space 
boundaries (i.e. where address translation is known to be impossible).

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello Stephen,

On 12/16/2015 08:07 PM, Stephen Warren wrote:
> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>
>>> enables device's bus/child address translation method, depending
>>> on bus 'ranges' property and including child 'reg' property.
>>> This change makes impossible to decode the 'reg' for node with
>>> '#size-cells' equal to 0.
>>>
>>> Such case is possible by the specification and is also used in U-Boot,
>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>
>> Can you please explain the problem you're seeing in more detail? Without
>> any context, my initial reaction is that this is simply a bug somewhere.
>> That bug should be fixed, rather than introducing new APIs to hide the
>> problem.
>
> Ah, I guess the problem is caused by the following code in
> __of_translate_address():
>
>      bus->count_cells(blob, parent, &na, &ns);
>      if (!OF_CHECK_COUNTS(na, ns)) {
>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>

Yes, and this is what my previous patch 'fixes'.

[1] https://patchwork.ozlabs.org/patch/537372/

However Linux makes the translate in the same way.

> That's because the function assumes it's called for MMIO addresses.
> However, reg values for I2C devices aren't MMIO addresses, so those
> assumptions don't apply. So, there is an argument for introducing some
> new functionality. I'm not sure that a whole new function is the correct
> way to go though. Rather, the existing translation functions should be
> enhanced to know the location of root of the address space that contains
> the address that's being translated. Then, translation can stop there.

This is okay but then, all device tree blobs should be defined in a 
proper way.
The problem is, that there are some additions and various assumptions in 
the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the 
reg's property value for each bank. But the driver in Linux hardcodes 
those values, however for both cases this is wrong, because the gpio 
regs could be mapped with ranges.

Even that issues above, I would prefer introduce a function or modify 
the existing one to allow keeping this as it is.

>
> Something like skipping the check on ns in the above code if parent ==
> addr_space_root_offset, and also terminating the for (;;) loop in that
> function under a similar condition.
>
> This would allow for translation to occur for buses other than the CPU's
> root MMIO space, yet not attempt to translate across known address space
> boundaries (i.e. where address translation is known to be impossible).
>
>

To achieve this functionality, it should be enough to take my first 
patch [1]. And then if no "ranges" is defined, then we have 1:1 translation.

I think, that it is safe, but then we will have a different assumptions, 
than in the Linux - is it acceptable?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
> Hello Stephen,
>
> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>
>>>> enables device's bus/child address translation method, depending
>>>> on bus 'ranges' property and including child 'reg' property.
>>>> This change makes impossible to decode the 'reg' for node with
>>>> '#size-cells' equal to 0.
>>>>
>>>> Such case is possible by the specification and is also used in U-Boot,
>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>
>>> Can you please explain the problem you're seeing in more detail? Without
>>> any context, my initial reaction is that this is simply a bug somewhere.
>>> That bug should be fixed, rather than introducing new APIs to hide the
>>> problem.
>>
>> Ah, I guess the problem is caused by the following code in
>> __of_translate_address():
>>
>>      bus->count_cells(blob, parent, &na, &ns);
>>      if (!OF_CHECK_COUNTS(na, ns)) {
>>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>
>
> Yes, and this is what my previous patch 'fixes'.
>
> [1] https://patchwork.ozlabs.org/patch/537372/
>
> However Linux makes the translate in the same way.
>
>> That's because the function assumes it's called for MMIO addresses.
>> However, reg values for I2C devices aren't MMIO addresses, so those
>> assumptions don't apply. So, there is an argument for introducing some
>> new functionality. I'm not sure that a whole new function is the correct
>> way to go though. Rather, the existing translation functions should be
>> enhanced to know the location of root of the address space that contains
>> the address that's being translated. Then, translation can stop there.
>
> This is okay but then, all device tree blobs should be defined in a
> proper way.

Well, why shouldn't that be true? There are rules for how DTs must be 
constructed. Nobody should expect DTs that violate those rules to work 
in any particular way.

> The problem is, that there are some additions and various assumptions in
> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
> reg's property value for each bank. But the driver in Linux hardcodes
> those values, however for both cases this is wrong, because the gpio
> regs could be mapped with ranges.

It sounds like there are many bugs to fix:-)

> Even that issues above, I would prefer introduce a function or modify
> the existing one to allow keeping this as it is.

Adding an extra function sounds OK, although I stand by my comment that 
the caller should pass in a parameter indicating the root of the address 
space, so that both #address-cells and #size-cells can be checked all 
the way up the chain, and #size-cells should only be allowed to be 0 at 
the root of the translation, not at any intermediate point.

>> Something like skipping the check on ns in the above code if parent ==
>> addr_space_root_offset, and also terminating the for (;;) loop in that
>> function under a similar condition.
>>
>> This would allow for translation to occur for buses other than the CPU's
>> root MMIO space, yet not attempt to translate across known address space
>> boundaries (i.e. where address translation is known to be impossible).
>
> To achieve this functionality, it should be enough to take my first
> patch [1]. And then if no "ranges" is defined, then we have 1:1
> translation.

I don't think so; that patch removes all checks on #size-cells rather 
than only removing/ignoring the check at the root of the address space.

> I think, that it is safe, but then we will have a different assumptions,
> than in the Linux - is it acceptable?

Both Linux and U-Boot should conform to the DT specification. So, if 
there's a difference between the two, there's likely a bug.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello,

On 01/04/2016 09:06 PM, Stephen Warren wrote:
> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>> Hello Stephen,
>>
>> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>>> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>
>>>>> enables device's bus/child address translation method, depending
>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>> This change makes impossible to decode the 'reg' for node with
>>>>> '#size-cells' equal to 0.
>>>>>
>>>>> Such case is possible by the specification and is also used in U-Boot,
>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>
>>>> Can you please explain the problem you're seeing in more detail?
>>>> Without
>>>> any context, my initial reaction is that this is simply a bug
>>>> somewhere.
>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>> problem.
>>>
>>> Ah, I guess the problem is caused by the following code in
>>> __of_translate_address():
>>>
>>>      bus->count_cells(blob, parent, &na, &ns);
>>>      if (!OF_CHECK_COUNTS(na, ns)) {
>>>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>
>>
>> Yes, and this is what my previous patch 'fixes'.
>>
>> [1] https://patchwork.ozlabs.org/patch/537372/
>>
>> However Linux makes the translate in the same way.
>>
>>> That's because the function assumes it's called for MMIO addresses.
>>> However, reg values for I2C devices aren't MMIO addresses, so those
>>> assumptions don't apply. So, there is an argument for introducing some
>>> new functionality. I'm not sure that a whole new function is the correct
>>> way to go though. Rather, the existing translation functions should be
>>> enhanced to know the location of root of the address space that contains
>>> the address that's being translated. Then, translation can stop there.
>>
>> This is okay but then, all device tree blobs should be defined in a
>> proper way.
>
> Well, why shouldn't that be true? There are rules for how DTs must be
> constructed. Nobody should expect DTs that violate those rules to work
> in any particular way.
>
>> The problem is, that there are some additions and various assumptions in
>> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
>> reg's property value for each bank. But the driver in Linux hardcodes
>> those values, however for both cases this is wrong, because the gpio
>> regs could be mapped with ranges.
>
> It sounds like there are many bugs to fix:-)
>

Unfortunately... :(

>> Even that issues above, I would prefer introduce a function or modify
>> the existing one to allow keeping this as it is.
>
> Adding an extra function sounds OK, although I stand by my comment that
> the caller should pass in a parameter indicating the root of the address
> space, so that both #address-cells and #size-cells can be checked all
> the way up the chain, and #size-cells should only be allowed to be 0 at
> the root of the translation, not at any intermediate point.
>
>>> Something like skipping the check on ns in the above code if parent ==
>>> addr_space_root_offset, and also terminating the for (;;) loop in that
>>> function under a similar condition.
>>>
>>> This would allow for translation to occur for buses other than the CPU's
>>> root MMIO space, yet not attempt to translate across known address space
>>> boundaries (i.e. where address translation is known to be impossible).
>>
>> To achieve this functionality, it should be enough to take my first
>> patch [1]. And then if no "ranges" is defined, then we have 1:1
>> translation.
>
> I don't think so; that patch removes all checks on #size-cells rather
> than only removing/ignoring the check at the root of the address space.
>
>> I think, that it is safe, but then we will have a different assumptions,
>> than in the Linux - is it acceptable?
>
> Both Linux and U-Boot should conform to the DT specification. So, if
> there's a difference between the two, there's likely a bug.
>
>

According to your comments with the new parameter, I think that we don't 
need this. As Simon wrote in one of his reply:

  "How would the caller know this root?".

What about adding a check that the parent defines "ranges" property and 
if not - then don't check the #size-cells?

So with this simple check, we could fix all present issues, beside the 
later fdt/drivers fixing.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 01/05/2016 08:38 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 01/04/2016 09:06 PM, Stephen Warren wrote:
>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>> Hello Stephen,
>>>
>>> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>>>> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>>
>>>>>> enables device's bus/child address translation method, depending
>>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>>> This change makes impossible to decode the 'reg' for node with
>>>>>> '#size-cells' equal to 0.
>>>>>>
>>>>>> Such case is possible by the specification and is also used in
>>>>>> U-Boot,
>>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>>
>>>>> Can you please explain the problem you're seeing in more detail?
>>>>> Without
>>>>> any context, my initial reaction is that this is simply a bug
>>>>> somewhere.
>>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>>> problem.
>>>>
>>>> Ah, I guess the problem is caused by the following code in
>>>> __of_translate_address():
>>>>
>>>>      bus->count_cells(blob, parent, &na, &ns);
>>>>      if (!OF_CHECK_COUNTS(na, ns)) {
>>>>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>>
>>>
>>> Yes, and this is what my previous patch 'fixes'.
>>>
>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>
>>> However Linux makes the translate in the same way.
>>>
>>>> That's because the function assumes it's called for MMIO addresses.
>>>> However, reg values for I2C devices aren't MMIO addresses, so those
>>>> assumptions don't apply. So, there is an argument for introducing some
>>>> new functionality. I'm not sure that a whole new function is the
>>>> correct
>>>> way to go though. Rather, the existing translation functions should be
>>>> enhanced to know the location of root of the address space that
>>>> contains
>>>> the address that's being translated. Then, translation can stop there.
>>>
>>> This is okay but then, all device tree blobs should be defined in a
>>> proper way.
>>
>> Well, why shouldn't that be true? There are rules for how DTs must be
>> constructed. Nobody should expect DTs that violate those rules to work
>> in any particular way.
>>
>>> The problem is, that there are some additions and various assumptions in
>>> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
>>> reg's property value for each bank. But the driver in Linux hardcodes
>>> those values, however for both cases this is wrong, because the gpio
>>> regs could be mapped with ranges.
>>
>> It sounds like there are many bugs to fix:-)
>>
>
> Unfortunately... :(
>
>>> Even that issues above, I would prefer introduce a function or modify
>>> the existing one to allow keeping this as it is.
>>
>> Adding an extra function sounds OK, although I stand by my comment that
>> the caller should pass in a parameter indicating the root of the address
>> space, so that both #address-cells and #size-cells can be checked all
>> the way up the chain, and #size-cells should only be allowed to be 0 at
>> the root of the translation, not at any intermediate point.
>>
>>>> Something like skipping the check on ns in the above code if parent ==
>>>> addr_space_root_offset, and also terminating the for (;;) loop in that
>>>> function under a similar condition.
>>>>
>>>> This would allow for translation to occur for buses other than the
>>>> CPU's
>>>> root MMIO space, yet not attempt to translate across known address
>>>> space
>>>> boundaries (i.e. where address translation is known to be impossible).
>>>
>>> To achieve this functionality, it should be enough to take my first
>>> patch [1]. And then if no "ranges" is defined, then we have 1:1
>>> translation.
>>
>> I don't think so; that patch removes all checks on #size-cells rather
>> than only removing/ignoring the check at the root of the address space.
>>
>>> I think, that it is safe, but then we will have a different assumptions,
>>> than in the Linux - is it acceptable?
>>
>> Both Linux and U-Boot should conform to the DT specification. So, if
>> there's a difference between the two, there's likely a bug.
>>
>>
>
> According to your comments with the new parameter, I think that we don't
> need this. As Simon wrote in one of his reply:
>
>   "How would the caller know this root?".

This is a facet of the hardware.

The root of the MMIO address space is the root of the DT.

The root of any other kind of address space is the IO controller that 
"hosts" the address space, i.e. the I2C or SPI controller.

Every device knows semantically what its address represents, and hence 
can trivially determine the root node of the address space.

Device driver writers shouldn't have to care about this, so likely some 
form of helper function should be provided by I2C/SPI/... subsystems to 
hide these details. IIRC (although I haven't looked in a while) this is 
exactly how/why the Linux kernel avoids this kind of issue; the 
I2C/SPI/... subsystem, handles parsing of reg properties before any 
device-specific driver is invoked.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Simon Glass]

Hi Stephen,

On 5 January 2016 at 10:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 01/05/2016 08:38 AM, Przemyslaw Marczak wrote:
>>
>> Hello,
>>
>> On 01/04/2016 09:06 PM, Stephen Warren wrote:
>>>
>>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>>>
>>>> Hello Stephen,
>>>>
>>>> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>>>>>
>>>>> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>>>>>>
>>>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>>>>
>>>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>>>
>>>>>>> enables device's bus/child address translation method, depending
>>>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>>>> This change makes impossible to decode the 'reg' for node with
>>>>>>> '#size-cells' equal to 0.
>>>>>>>
>>>>>>> Such case is possible by the specification and is also used in
>>>>>>> U-Boot,
>>>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>>>
>>>>>>
>>>>>> Can you please explain the problem you're seeing in more detail?
>>>>>> Without
>>>>>> any context, my initial reaction is that this is simply a bug
>>>>>> somewhere.
>>>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>>>> problem.
>>>>>
>>>>>
>>>>> Ah, I guess the problem is caused by the following code in
>>>>> __of_translate_address():
>>>>>
>>>>>      bus->count_cells(blob, parent, &na, &ns);
>>>>>      if (!OF_CHECK_COUNTS(na, ns)) {
>>>>>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>>>
>>>>
>>>> Yes, and this is what my previous patch 'fixes'.
>>>>
>>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>>
>>>> However Linux makes the translate in the same way.
>>>>
>>>>> That's because the function assumes it's called for MMIO addresses.
>>>>> However, reg values for I2C devices aren't MMIO addresses, so those
>>>>> assumptions don't apply. So, there is an argument for introducing some
>>>>> new functionality. I'm not sure that a whole new function is the
>>>>> correct
>>>>> way to go though. Rather, the existing translation functions should be
>>>>> enhanced to know the location of root of the address space that
>>>>> contains
>>>>> the address that's being translated. Then, translation can stop there.
>>>>
>>>>
>>>> This is okay but then, all device tree blobs should be defined in a
>>>> proper way.
>>>
>>>
>>> Well, why shouldn't that be true? There are rules for how DTs must be
>>> constructed. Nobody should expect DTs that violate those rules to work
>>> in any particular way.
>>>
>>>> The problem is, that there are some additions and various assumptions in
>>>> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
>>>> reg's property value for each bank. But the driver in Linux hardcodes
>>>> those values, however for both cases this is wrong, because the gpio
>>>> regs could be mapped with ranges.
>>>
>>>
>>> It sounds like there are many bugs to fix:-)
>>>
>>
>> Unfortunately... :(
>>
>>>> Even that issues above, I would prefer introduce a function or modify
>>>> the existing one to allow keeping this as it is.
>>>
>>>
>>> Adding an extra function sounds OK, although I stand by my comment that
>>> the caller should pass in a parameter indicating the root of the address
>>> space, so that both #address-cells and #size-cells can be checked all
>>> the way up the chain, and #size-cells should only be allowed to be 0 at
>>> the root of the translation, not at any intermediate point.
>>>
>>>>> Something like skipping the check on ns in the above code if parent ==
>>>>> addr_space_root_offset, and also terminating the for (;;) loop in that
>>>>> function under a similar condition.
>>>>>
>>>>> This would allow for translation to occur for buses other than the
>>>>> CPU's
>>>>> root MMIO space, yet not attempt to translate across known address
>>>>> space
>>>>> boundaries (i.e. where address translation is known to be impossible).
>>>>
>>>>
>>>> To achieve this functionality, it should be enough to take my first
>>>> patch [1]. And then if no "ranges" is defined, then we have 1:1
>>>> translation.
>>>
>>>
>>> I don't think so; that patch removes all checks on #size-cells rather
>>> than only removing/ignoring the check at the root of the address space.
>>>
>>>> I think, that it is safe, but then we will have a different assumptions,
>>>> than in the Linux - is it acceptable?
>>>
>>>
>>> Both Linux and U-Boot should conform to the DT specification. So, if
>>> there's a difference between the two, there's likely a bug.
>>>
>>>
>>
>> According to your comments with the new parameter, I think that we don't
>> need this. As Simon wrote in one of his reply:
>>
>>   "How would the caller know this root?".
>
>
> This is a facet of the hardware.
>
> The root of the MMIO address space is the root of the DT.
>
> The root of any other kind of address space is the IO controller that
> "hosts" the address space, i.e. the I2C or SPI controller.
>
> Every device knows semantically what its address represents, and hence can
> trivially determine the root node of the address space.
>
> Device driver writers shouldn't have to care about this, so likely some form
> of helper function should be provided by I2C/SPI/... subsystems to hide
> these details. IIRC (although I haven't looked in a while) this is exactly
> how/why the Linux kernel avoids this kind of issue; the I2C/SPI/...
> subsystem, handles parsing of reg properties before any device-specific
> driver is invoked.

OK, then I suppose we could do this in a uclass pre_probe() method.
But this would be different from how every current driver works (it is
the driver's responsibility to call dev_get_addr()).

I'm not super-keen on dev_get_reg() as it adds confusion - why is one
reg dealth with differently from another. Perhaps just a different
name, like dev_get_bus_addr()?

Re Linux, can someone trace through the of_address_to_resource() call
and see what it actually does? It seems to call of_translate_address()
but presumably does not suffer from this problem. So maybe I am
missing something. The S3C I2C driver calls platform_get_resource()
which is presumably set up by a call to of_address_to_resource()?

There is way too much code there to bring into U-Boot, but we can try
to do similar things.

Let's solve this in a general way for the next release.

Regards,
Simon

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 01/05/2016 05:24 PM, Simon Glass wrote:...
 > I'm not super-keen on dev_get_reg() as it adds confusion - why is one
> reg dealt with differently from another. Perhaps just a different
> name, like dev_get_bus_addr()?
>
> Re Linux, can someone trace through the of_address_to_resource() call
> and see what it actually does? It seems to call of_translate_address()
> but presumably does not suffer from this problem. So maybe I am
> missing something. The S3C I2C driver calls platform_get_resource()
> which is presumably set up by a call to of_address_to_resource()?

At least in ARM DT land, most MMIO devices are "platform devices". There 
are also e.g. PCIe devices, but they're enumerated via HW protocols, not 
by parsing DT.

Construction of a platform device works as follows in the Linux source:

For each node that's found by scanning the DT, a struct platform_device 
is created to represent it, in drivers/of/platform.c of_device_alloc(). 
Note that this scanning process is only performed on DT nodes known to 
represent MMIO buses; recursion into child nodes is explicitly performed 
by drivers for particular nodes, and takes a different path for nodes 
representing non-MMIO buses, as described later, since the node's driver 
will be different. This scanning process is core code that executes 
before the kernel has any idea re: which type of device the child nodes 
represent, or which driver will manage them. This struct contains the 
values of a number of core resource types that are represented in DT, 
such as addresses, IRQs, etc. The address values are filled in via the 
following call stack:

drivers/of/platform.c of_device_alloc()

->

drivers/of/address.c of_address_to_resource()

->

drivers/of/address.c of_get_address()

[reads the reg property, picks out a single index in it, performs BE->LE 
conversion]

then unconditionally calls the following on the address:

drivers/of/address.c __of_address_to_resource()

[translates the address to the CPU address space, then writes the result 
into the supplied pointer, which the caller has set up to point into a 
struct resource associated with the struct platform_device]

->

drivers/of/address.c of_translate_address()

->

drivers/of/address.c __of_translate_address()

This is essentially exactly identical to U-Boot's 
__of_translate_address(), modulo a few printf()/debug() differences, and 
a few more of_node_get/put() calls in order to support dynamic 
modifications to the DT.


The driver for the a DT node is responsible for enumerating (or 
triggering enumeration of) any child nodes. For a node known to 
represent an MMIO bus (e.g. simple-bus), the algorithm above will simply 
be applied recursively. For a node that represents some kind of non-MMIO 
bus controller/host, custom (bus-type-specific) code is typically 
applied. In many cases, this custom code will parse the reg property of 
child nodes according to the semantics of that bus's definition of the 
reg property, and provide that value to the device driver for the child 
node. Examples of reg parsing in Linux are:

drivers/i2c/i2c-core.c of_i2c_register_device() about 20 lines in. Note 
that I2C addresses aren't simple integer values, but also encode some 
flags too, such as 7-vs-10-bit addresses, and whether the DT node 
represents a slave device that Linux should implement, or a regular device.

drivers/spi/spi.c of_register_spi_device() about 20 lines in. Here, the 
address is the identify of the chip select wire, which may be driven by 
the SPI controller, or via GPIOs, depending on the controller HW.

drivers/of/of_mdio.c of_mdio_parse_addr(). Here, the address is a simple 
integer, with a range check.

drivers/mmc/core/core.c mmc_of_find_child_device(). At least SDIO 
buses/devices (as opposed to eMMC/SD devices) appear to be able to host 
multiple functions, and DT appears to be able to represent each function 
as a separate child node of the SDIO controller.

drivers/spmi/spmi.c of_spmi_register_devices() about 10 lines in. I 
don't know what SPMI is, but the interpretation of the reg property in 
this code seems to have a few bus-specific rules.

drivers/nvmem/core.c of_nvmem_cell_get() about 25 lines in. I don't know 
what nvmem is, but reg here seems to be a pretty simple base/size pair.

All of this discussion implies to me that drivers (for buses) really 
should be calling different functions (or executing different local 
custom code) to parse the reg property. The property is interpreted 
quite differently depending on bus type.

Whether that means creating dev_get_addr_mmio() and dev_get_addr_i2c() 
such that dev_get_addr() doesn't unconditionally translate addresses vs. 
moving reg parsing into subsystem core code rather than drivers vs. any 
other solution seems like an implementation detail. The main point is 
that we certainly can't rely on a single dev_get_addr() that just works 
for all reg properties.

All file and line count references above are relative to Linux kernel 
version v4.4-rc8.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello,

On 01/06/2016 01:24 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 January 2016 at 10:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 01/05/2016 08:38 AM, Przemyslaw Marczak wrote:
>>>
>>> Hello,
>>>
>>> On 01/04/2016 09:06 PM, Stephen Warren wrote:
>>>>
>>>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>>>>
>>>>> Hello Stephen,
>>>>>
>>>>> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>>>>>>
>>>>>> On 12/16/2015 11:53 AM, Stephen Warren wrote:
>>>>>>>
>>>>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>>>>>
>>>>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>>>>
>>>>>>>> enables device's bus/child address translation method, depending
>>>>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>>>>> This change makes impossible to decode the 'reg' for node with
>>>>>>>> '#size-cells' equal to 0.
>>>>>>>>
>>>>>>>> Such case is possible by the specification and is also used in
>>>>>>>> U-Boot,
>>>>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>>>>
>>>>>>>
>>>>>>> Can you please explain the problem you're seeing in more detail?
>>>>>>> Without
>>>>>>> any context, my initial reaction is that this is simply a bug
>>>>>>> somewhere.
>>>>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>>>>> problem.
>>>>>>
>>>>>>
>>>>>> Ah, I guess the problem is caused by the following code in
>>>>>> __of_translate_address():
>>>>>>
>>>>>>       bus->count_cells(blob, parent, &na, &ns);
>>>>>>       if (!OF_CHECK_COUNTS(na, ns)) {
>>>>>>           printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>>>>
>>>>>
>>>>> Yes, and this is what my previous patch 'fixes'.
>>>>>
>>>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>>>
>>>>> However Linux makes the translate in the same way.
>>>>>
>>>>>> That's because the function assumes it's called for MMIO addresses.
>>>>>> However, reg values for I2C devices aren't MMIO addresses, so those
>>>>>> assumptions don't apply. So, there is an argument for introducing some
>>>>>> new functionality. I'm not sure that a whole new function is the
>>>>>> correct
>>>>>> way to go though. Rather, the existing translation functions should be
>>>>>> enhanced to know the location of root of the address space that
>>>>>> contains
>>>>>> the address that's being translated. Then, translation can stop there.
>>>>>
>>>>>
>>>>> This is okay but then, all device tree blobs should be defined in a
>>>>> proper way.
>>>>
>>>>
>>>> Well, why shouldn't that be true? There are rules for how DTs must be
>>>> constructed. Nobody should expect DTs that violate those rules to work
>>>> in any particular way.
>>>>
>>>>> The problem is, that there are some additions and various assumptions in
>>>>> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
>>>>> reg's property value for each bank. But the driver in Linux hardcodes
>>>>> those values, however for both cases this is wrong, because the gpio
>>>>> regs could be mapped with ranges.
>>>>
>>>>
>>>> It sounds like there are many bugs to fix:-)
>>>>
>>>
>>> Unfortunately... :(
>>>
>>>>> Even that issues above, I would prefer introduce a function or modify
>>>>> the existing one to allow keeping this as it is.
>>>>
>>>>
>>>> Adding an extra function sounds OK, although I stand by my comment that
>>>> the caller should pass in a parameter indicating the root of the address
>>>> space, so that both #address-cells and #size-cells can be checked all
>>>> the way up the chain, and #size-cells should only be allowed to be 0 at
>>>> the root of the translation, not at any intermediate point.
>>>>
>>>>>> Something like skipping the check on ns in the above code if parent ==
>>>>>> addr_space_root_offset, and also terminating the for (;;) loop in that
>>>>>> function under a similar condition.
>>>>>>
>>>>>> This would allow for translation to occur for buses other than the
>>>>>> CPU's
>>>>>> root MMIO space, yet not attempt to translate across known address
>>>>>> space
>>>>>> boundaries (i.e. where address translation is known to be impossible).
>>>>>
>>>>>
>>>>> To achieve this functionality, it should be enough to take my first
>>>>> patch [1]. And then if no "ranges" is defined, then we have 1:1
>>>>> translation.
>>>>
>>>>
>>>> I don't think so; that patch removes all checks on #size-cells rather
>>>> than only removing/ignoring the check at the root of the address space.
>>>>
>>>>> I think, that it is safe, but then we will have a different assumptions,
>>>>> than in the Linux - is it acceptable?
>>>>
>>>>
>>>> Both Linux and U-Boot should conform to the DT specification. So, if
>>>> there's a difference between the two, there's likely a bug.
>>>>
>>>>
>>>
>>> According to your comments with the new parameter, I think that we don't
>>> need this. As Simon wrote in one of his reply:
>>>
>>>    "How would the caller know this root?".
>>
>>
>> This is a facet of the hardware.
>>
>> The root of the MMIO address space is the root of the DT.
>>
>> The root of any other kind of address space is the IO controller that
>> "hosts" the address space, i.e. the I2C or SPI controller.
>>
>> Every device knows semantically what its address represents, and hence can
>> trivially determine the root node of the address space.
>>
>> Device driver writers shouldn't have to care about this, so likely some form
>> of helper function should be provided by I2C/SPI/... subsystems to hide
>> these details. IIRC (although I haven't looked in a while) this is exactly
>> how/why the Linux kernel avoids this kind of issue; the I2C/SPI/...
>> subsystem, handles parsing of reg properties before any device-specific
>> driver is invoked.
>
> OK, then I suppose we could do this in a uclass pre_probe() method.
> But this would be different from how every current driver works (it is
> the driver's responsibility to call dev_get_addr()).
>
> I'm not super-keen on dev_get_reg() as it adds confusion - why is one
> reg dealth with differently from another. Perhaps just a different
> name, like dev_get_bus_addr()?
>

But there is only one "reg" property per device and I think, that its 
use-case is actually clearly defined in device-tree specification:
- if parent provides 'ranges' - try to map the address
- if no ranges - then return the reg value only

> Re Linux, can someone trace through the of_address_to_resource() call
> and see what it actually does? It seems to call of_translate_address()
> but presumably does not suffer from this problem. So maybe I am
> missing something. The S3C I2C driver calls platform_get_resource()
> which is presumably set up by a call to of_address_to_resource()?
>
> There is way too much code there to bring into U-Boot, but we can try
> to do similar things.
>
> Let's solve this in a general way for the next release.
>
> Regards,
> Simon
>
>

Please review my new patch:

https://patchwork.ozlabs.org/patch/564246/

If the dev_get_reg() will return what we want with the above patch,
then it could be used in each uclass pre_probe() method.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello Stephen,

On 12/16/2015 07:53 PM, Stephen Warren wrote:
> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>> commit: dm: core: Enable optional use of fdt_translate_address()
>>
>> enables device's bus/child address translation method, depending
>> on bus 'ranges' property and including child 'reg' property.
>> This change makes impossible to decode the 'reg' for node with
>> '#size-cells' equal to 0.
>>
>> Such case is possible by the specification and is also used in U-Boot,
>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>
> Can you please explain the problem you're seeing in more detail? Without
> any context, my initial reaction is that this is simply a bug somewhere.
> That bug should be fixed, rather than introducing new APIs to hide the
> problem.
>

Some time ago I send a patch with such fix:

[1] https://patchwork.ozlabs.org/patch/537372/

Sorry, I didn't add you to the 'CC' list.

However. I checked this in linux, and the code is the same, the 
size-cells == 0 is not supported also in Linux.

So to prevent breaking some consistency in parsing fdt between U-boot 
and Linux, I sent the patch which adds dev_get_reg(). And it seem to be 
useful at least for I2C and Exynos GPIO driver.

>> For this purpose this patch set introduces new core function:
>>   fdt_addr_t dev_get_reg(struct udevice *dev)
>> which returns the 'reg' value in the same way as previously
>> dev_get_addr().
>>
>> This fixes s5p gpio driver and booting issue on few Exynos based boards:
>> - Trats2
>> - Odroid U3/X2
>
> Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following:
>
>
>         i2c at 138d0000 {
>                  samsung,i2c-sda-delay = <100>;
>                  samsung,i2c-slave-addr = <0x10>;
>                  samsung,i2c-max-bus-freq = <100000>;
>                  status = "okay";
>
>                  max77686_pmic at 09 {
>                          compatible = "maxim,max77686";
>                          interrupts = <7 0>;
>                          reg = <0x09 0 0>;
>
> Is that the node you're having problems with? If so, I believe this may
> simply be due to invalid DT content. In exynos4.dtsi, that i2c node is
> defined as:
>
>          i2c at 138d0000 {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
>
> Thus, any reg property in a child of that node must only contain a
> single cell (the sum of #address-cells and #size-cells in the parent).
> Does fixing the DT so it's valid solve your issue at all?
>
>

Nice hit above! However we don't use DM API yet for the above example, 
so probably this is why it is still working - currently, the driver uses 
fdtdec_get_int(), for getting this value.

But for test, after switching it to use of sequence: fdt_getprop() -> 
fdt_translate_address(), then I can see the warning:

---- cut ----
_of_translate_address: Bad cell count for max77686_pmic at 09
---- cut ----

And for the above issue - applying patch [1] - allows return the right 
device address: 0x9 - without FDT modifying.

Now, I checked, why the above example compiles by dtc with no warning.
It looks, that dtc ignores some child's reg cells-count combination:

---- case 1 -----
parent {
	#address-cells = <1>;
	#size-cells = <0>;
	child {
		reg = <0x9>;
	};
};
This is ok!

---- case 2 -----
parent {
	#address-cells = <1>;
	#size-cells = <0>;
	child {
		reg = <0x9 0 0>;
	};
};
This is ok: (the 2nd and 3rd child's cells are ignored by dtc)

---- case 3 -----
parent {
	#address-cells = <1>;
	#size-cells = <1>;
	child {
		reg = <0x9 0 0>;
	};
};

This is wrong! dtc warning:
Warning (reg_format): "reg" property in /i2c at 138d0000/max77686_pmic has 
invalid length (12 bytes) (#address-cells == 1, #size-cells == 1)



Now I don't have a time for checking it in dtc code, however we can 
check in U-Boot, that the dtb is not coherent?

So I can compile my default dts - no dtc warnings, and next in U-Boot I 
can see:

------------ check i2c node --------------------
Trats2 # fdt list /i2c at 138d0000
i2c at 138d0000 {
         #address-cells = <0x00000001>;
         #size-cells = <0x00000000>;
         compatible = "samsung,s3c2440-i2c";
         reg = <0x138d0000 0x00000100>;
         interrupts = <0x00000007 0x0000003f 0x00000000>;
         samsung,i2c-sda-delay = <0x00000064>;
         samsung,i2c-slave-addr = <0x00000010>;
         samsung,i2c-max-bus-freq = <0x000186a0>;
         status = "okay";
         max77686_pmic at 09 {
         };
};

------------ check pmic node --------------------
Trats2 # fdt list /i2c at 138d0000/max77686_pmic at 09
max77686_pmic at 09 {
         compatible = "maxim,max77686";
         interrupts = <0x00000007 0x00000000>;
         reg = <0x00000009 0x00000000 0x00000000>;
         #clock-cells = <0x00000001>;
         voltage-regulators {
         };
};

So, the parent defines the summarized cells count as 1, and the child 
node can exceed it, because it provides 3 cells.

However it looks that we are still safe, since the code doesn't try 
exceed the cells count, defined by the parent.

What do you think about this?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
> Hello Stephen,
>
> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>
>>> enables device's bus/child address translation method, depending
>>> on bus 'ranges' property and including child 'reg' property.
>>> This change makes impossible to decode the 'reg' for node with
>>> '#size-cells' equal to 0.
>>>
>>> Such case is possible by the specification and is also used in U-Boot,
>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>
>> Can you please explain the problem you're seeing in more detail? Without
>> any context, my initial reaction is that this is simply a bug somewhere.
>> That bug should be fixed, rather than introducing new APIs to hide the
>> problem.
>>
>
> Some time ago I send a patch with such fix:
>
> [1] https://patchwork.ozlabs.org/patch/537372/
>
> Sorry, I didn't add you to the 'CC' list.
>
> However. I checked this in linux, and the code is the same, the
> size-cells == 0 is not supported also in Linux.

The discussion there does indicate that removing the check on 
#size-cells would be incorrect.

> So to prevent breaking some consistency in parsing fdt between U-boot
> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be
> useful at least for I2C and Exynos GPIO driver.

OK; as I mentioned in my other reply, some form of new function or new 
parameter does seem reasonable here.

...
>> Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following:
>>
>>
>>         i2c at 138d0000 {
>>                  samsung,i2c-sda-delay = <100>;
>>                  samsung,i2c-slave-addr = <0x10>;
>>                  samsung,i2c-max-bus-freq = <100000>;
>>                  status = "okay";
>>
>>                  max77686_pmic at 09 {
>>                          compatible = "maxim,max77686";
>>                          interrupts = <7 0>;
>>                          reg = <0x09 0 0>;
>>
>> Is that the node you're having problems with? If so, I believe this may
>> simply be due to invalid DT content. In exynos4.dtsi, that i2c node is
>> defined as:
>>
>>          i2c at 138d0000 {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>> Thus, any reg property in a child of that node must only contain a
>> single cell (the sum of #address-cells and #size-cells in the parent).
>> Does fixing the DT so it's valid solve your issue at all?
>
> Nice hit above! However we don't use DM API yet for the above example,
> so probably this is why it is still working - currently, the driver uses
> fdtdec_get_int(), for getting this value.
>
> But for test, after switching it to use of sequence: fdt_getprop() ->
> fdt_translate_address(), then I can see the warning:
>
> ---- cut ----
> _of_translate_address: Bad cell count for max77686_pmic at 09
> ---- cut ----
>
> And for the above issue - applying patch [1] - allows return the right
> device address: 0x9 - without FDT modifying.
>
> Now, I checked, why the above example compiles by dtc with no warning.
> It looks, that dtc ignores some child's reg cells-count combination:

dtc doesn't check that the length of the reg property is *equal* to the 
sum of #address-cells and #size-cells, but rather that the length is a 
*multiple* of that value. This is because the reg property can contain 
multiple addresses.

> ---- case 1 -----
> parent {
>      #address-cells = <1>;
>      #size-cells = <0>;
>      child {
>          reg = <0x9>;
>      };
> };
> This is ok!

This is "1 * (1 + 0)".

> ---- case 2 -----
> parent {
>      #address-cells = <1>;
>      #size-cells = <0>;
>      child {
>          reg = <0x9 0 0>;
>      };
> };
> This is ok: (the 2nd and 3rd child's cells are ignored by dtc)

The extra cells aren't ignored; the length is "3 * (1 + 0)".
> ---- case 3 -----
> parent {
>      #address-cells = <1>;
>      #size-cells = <1>;
>      child {
>          reg = <0x9 0 0>;
>      };
> };
>
> This is wrong! dtc warning:
> Warning (reg_format): "reg" property in /i2c at 138d0000/max77686_pmic has
> invalid length (12 bytes) (#address-cells == 1, #size-cells == 1)

Yes, this is "1.5 * (1 + 1)", yet the "1.5" isn't an integer, hence the 
warning is triggered.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Simon Glass]

Hi Stephen,

On 4 January 2016 at 13:02, Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>
>> Hello Stephen,
>>
>> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>>>
>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>
>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>
>>>> enables device's bus/child address translation method, depending
>>>> on bus 'ranges' property and including child 'reg' property.
>>>> This change makes impossible to decode the 'reg' for node with
>>>> '#size-cells' equal to 0.
>>>>
>>>> Such case is possible by the specification and is also used in U-Boot,
>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>
>>>
>>> Can you please explain the problem you're seeing in more detail? Without
>>> any context, my initial reaction is that this is simply a bug somewhere.
>>> That bug should be fixed, rather than introducing new APIs to hide the
>>> problem.
>>>
>>
>> Some time ago I send a patch with such fix:
>>
>> [1] https://patchwork.ozlabs.org/patch/537372/
>>
>> Sorry, I didn't add you to the 'CC' list.
>>
>> However. I checked this in linux, and the code is the same, the
>> size-cells == 0 is not supported also in Linux.
>
>
> The discussion there does indicate that removing the check on #size-cells would be incorrect.

OK, but since we have a breakage and a release in a week, I'm planning
on picking up Przemyslaw's patch. We can revert it immediately
afterwards.

Who is going to work out a proper solution (post-release)?

>
>> So to prevent breaking some consistency in parsing fdt between U-boot
>> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be
>> useful at least for I2C and Exynos GPIO driver.
>
>
> OK; as I mentioned in my other reply, some form of new function or new parameter does seem reasonable here.
>
> ...
>
>>> Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following:
>>>
>>>
>>>         i2c at 138d0000 {
>>>                  samsung,i2c-sda-delay = <100>;
>>>                  samsung,i2c-slave-addr = <0x10>;
>>>                  samsung,i2c-max-bus-freq = <100000>;
>>>                  status = "okay";
>>>
>>>                  max77686_pmic at 09 {
>>>                          compatible = "maxim,max77686";
>>>                          interrupts = <7 0>;
>>>                          reg = <0x09 0 0>;
>>>
>>> Is that the node you're having problems with? If so, I believe this may
>>> simply be due to invalid DT content. In exynos4.dtsi, that i2c node is
>>> defined as:
>>>
>>>          i2c at 138d0000 {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>>
>>> Thus, any reg property in a child of that node must only contain a
>>> single cell (the sum of #address-cells and #size-cells in the parent).
>>> Does fixing the DT so it's valid solve your issue at all?
>>
>>
>> Nice hit above! However we don't use DM API yet for the above example,
>> so probably this is why it is still working - currently, the driver uses
>> fdtdec_get_int(), for getting this value.
>>
>> But for test, after switching it to use of sequence: fdt_getprop() ->
>> fdt_translate_address(), then I can see the warning:
>>
>> ---- cut ----
>> _of_translate_address: Bad cell count for max77686_pmic at 09
>> ---- cut ----
>>
>> And for the above issue - applying patch [1] - allows return the right
>> device address: 0x9 - without FDT modifying.
>>
>> Now, I checked, why the above example compiles by dtc with no warning.
>> It looks, that dtc ignores some child's reg cells-count combination:
>
>
> dtc doesn't check that the length of the reg property is *equal* to the sum of #address-cells and #size-cells, but rather that the length is a *multiple* of that value. This is because the reg property can contain multiple addresses.
>
>> ---- case 1 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>      child {
>>          reg = <0x9>;
>>      };
>> };
>> This is ok!
>
>
> This is "1 * (1 + 0)".
>
>> ---- case 2 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>      child {
>>          reg = <0x9 0 0>;
>>      };
>> };
>> This is ok: (the 2nd and 3rd child's cells are ignored by dtc)
>
>
> The extra cells aren't ignored; the length is "3 * (1 + 0)".
>>
>> ---- case 3 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <1>;
>>      child {
>>          reg = <0x9 0 0>;
>>      };
>> };
>>
>> This is wrong! dtc warning:
>> Warning (reg_format): "reg" property in /i2c at 138d0000/max77686_pmic has
>> invalid length (12 bytes) (#address-cells == 1, #size-cells == 1)
>
>
> Yes, this is "1.5 * (1 + 1)", yet the "1.5" isn't an integer, hence the warning is triggered.

Great that you can explain this, thanks.

Regards,
Simon

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 01/04/2016 05:58 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 4 January 2016 at 13:02, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>>
>>> Hello Stephen,
>>>
>>> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>>>>
>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>>
>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>
>>>>> enables device's bus/child address translation method, depending
>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>> This change makes impossible to decode the 'reg' for node with
>>>>> '#size-cells' equal to 0.
>>>>>
>>>>> Such case is possible by the specification and is also used in U-Boot,
>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>
>>>>
>>>> Can you please explain the problem you're seeing in more detail? Without
>>>> any context, my initial reaction is that this is simply a bug somewhere.
>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>> problem.
>>>>
>>>
>>> Some time ago I send a patch with such fix:
>>>
>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>
>>> Sorry, I didn't add you to the 'CC' list.
>>>
>>> However. I checked this in linux, and the code is the same, the
>>> size-cells == 0 is not supported also in Linux.
>>
>>
>> The discussion there does indicate that removing the check on #size-cells would be incorrect.
>
> OK, but since we have a breakage and a release in a week, I'm planning
> on picking up Przemyslaw's patch. We can revert it immediately
> afterwards.
>
> Who is going to work out a proper solution (post-release)?

I would assume the owner of the affected I2C/SPI controllers, or if the 
issue is triggered by core/subsystem code, then the owner of that subsystem.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello,

On 01/04/2016 09:02 PM, Stephen Warren wrote:
> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>> Hello Stephen,
>>
>> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>
>>>> enables device's bus/child address translation method, depending
>>>> on bus 'ranges' property and including child 'reg' property.
>>>> This change makes impossible to decode the 'reg' for node with
>>>> '#size-cells' equal to 0.
>>>>
>>>> Such case is possible by the specification and is also used in U-Boot,
>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>
>>> Can you please explain the problem you're seeing in more detail? Without
>>> any context, my initial reaction is that this is simply a bug somewhere.
>>> That bug should be fixed, rather than introducing new APIs to hide the
>>> problem.
>>>
>>
>> Some time ago I send a patch with such fix:
>>
>> [1] https://patchwork.ozlabs.org/patch/537372/
>>
>> Sorry, I didn't add you to the 'CC' list.
>>
>> However. I checked this in linux, and the code is the same, the
>> size-cells == 0 is not supported also in Linux.
>
> The discussion there does indicate that removing the check on
> #size-cells would be incorrect.
>

Ok, this probably would be good if we assume that dts is always well 
written, so this is not acceptable.

>> So to prevent breaking some consistency in parsing fdt between U-boot
>> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be
>> useful at least for I2C and Exynos GPIO driver.
>
> OK; as I mentioned in my other reply, some form of new function or new
> parameter does seem reasonable here.
>
> ...

At this point I can say, that the device-tree files and some compatible 
drivers are using wrong assumptions.

I think, that adding the new function is not needed, and also that we 
don't need any new parameter to the function dev_get_reg(),
because the right way is to fix the fdt.

For the Exynos GPIO issue, we can use two cases:
- define proper ranges
- move  #size-cells=0 to #size-cells=1 and extend the reg property by 
it's size (actually not too much to do)

This will fix the Exynos boot issue.

>>> Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following:
>>>
>>>
>>>         i2c at 138d0000 {
>>>                  samsung,i2c-sda-delay = <100>;
>>>                  samsung,i2c-slave-addr = <0x10>;
>>>                  samsung,i2c-max-bus-freq = <100000>;
>>>                  status = "okay";
>>>
>>>                  max77686_pmic at 09 {
>>>                          compatible = "maxim,max77686";
>>>                          interrupts = <7 0>;
>>>                          reg = <0x09 0 0>;
>>>
>>> Is that the node you're having problems with? If so, I believe this may
>>> simply be due to invalid DT content. In exynos4.dtsi, that i2c node is
>>> defined as:
>>>
>>>          i2c at 138d0000 {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>>
>>> Thus, any reg property in a child of that node must only contain a
>>> single cell (the sum of #address-cells and #size-cells in the parent).
>>> Does fixing the DT so it's valid solve your issue at all?
>>
>> Nice hit above! However we don't use DM API yet for the above example,
>> so probably this is why it is still working - currently, the driver uses
>> fdtdec_get_int(), for getting this value.
>>
>> But for test, after switching it to use of sequence: fdt_getprop() ->
>> fdt_translate_address(), then I can see the warning:
>>
>> ---- cut ----
>> _of_translate_address: Bad cell count for max77686_pmic at 09
>> ---- cut ----
>>
>> And for the above issue - applying patch [1] - allows return the right
>> device address: 0x9 - without FDT modifying.
>>
>> Now, I checked, why the above example compiles by dtc with no warning.
>> It looks, that dtc ignores some child's reg cells-count combination:
>
> dtc doesn't check that the length of the reg property is *equal* to the
> sum of #address-cells and #size-cells, but rather that the length is a
> *multiple* of that value. This is because the reg property can contain
> multiple addresses.
>
>> ---- case 1 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>      child {
>>          reg = <0x9>;
>>      };
>> };
>> This is ok!
>
> This is "1 * (1 + 0)".
>
>> ---- case 2 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>      child {
>>          reg = <0x9 0 0>;
>>      };
>> };
>> This is ok: (the 2nd and 3rd child's cells are ignored by dtc)
>
> The extra cells aren't ignored; the length is "3 * (1 + 0)".
>> ---- case 3 -----
>> parent {
>>      #address-cells = <1>;
>>      #size-cells = <1>;
>>      child {
>>          reg = <0x9 0 0>;
>>      };
>> };
>>
>> This is wrong! dtc warning:
>> Warning (reg_format): "reg" property in /i2c at 138d0000/max77686_pmic has
>> invalid length (12 bytes) (#address-cells == 1, #size-cells == 1)
>
> Yes, this is "1.5 * (1 + 1)", yet the "1.5" isn't an integer, hence the
> warning is triggered.
>
>

Also thank you for the explanation.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Stephen Warren]

On 01/05/2016 08:37 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 01/04/2016 09:02 PM, Stephen Warren wrote:
>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>> Hello Stephen,
>>>
>>> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>
>>>>> enables device's bus/child address translation method, depending
>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>> This change makes impossible to decode the 'reg' for node with
>>>>> '#size-cells' equal to 0.
>>>>>
>>>>> Such case is possible by the specification and is also used in U-Boot,
>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>
>>>> Can you please explain the problem you're seeing in more detail?
>>>> Without
>>>> any context, my initial reaction is that this is simply a bug
>>>> somewhere.
>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>> problem.
>>>>
>>>
>>> Some time ago I send a patch with such fix:
>>>
>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>
>>> Sorry, I didn't add you to the 'CC' list.
>>>
>>> However. I checked this in linux, and the code is the same, the
>>> size-cells == 0 is not supported also in Linux.
>>
>> The discussion there does indicate that removing the check on
>> #size-cells would be incorrect.
>
> Ok, this probably would be good if we assume that dts is always well
> written, so this is not acceptable.

Why not?

Certainly it's reasonable to expect code not to blow up in crashy ways 
in the face of bad DTs. However, I don't think it's reasonable to 
require that code perform exactly the desired function in the face of 
arbitrary bad input data.

Besides, the problem here isn't a bad DT. It's perfectly legal for a DT 
to contain #size-cells=<0>. The problem is bad code, which attempts to 
translate an address beyond the root of the address space that the 
address is within. This is a bug in the code, pure and simple.

>>> So to prevent breaking some consistency in parsing fdt between U-boot
>>> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be
>>> useful at least for I2C and Exynos GPIO driver.
>>
>> OK; as I mentioned in my other reply, some form of new function or new
>> parameter does seem reasonable here.
>>
>> ...
>
> At this point I can say, that the device-tree files and some compatible
> drivers are using wrong assumptions.
>
> I think, that adding the new function is not needed, and also that we
> don't need any new parameter to the function dev_get_reg(),
> because the right way is to fix the fdt.
>
> For the Exynos GPIO issue, we can use two cases:
> - define proper ranges
> - move  #size-cells=0 to #size-cells=1 and extend the reg property by
> it's size (actually not too much to do)
>
> This will fix the Exynos boot issue.

That may fix/hide the issue, but semantically seems entirely incorrect. 
I2C addresses cannot logically be mapped into the CPU's MMIO address 
space, so adding a ranges property that attempts to do that doesn't make 
sense. I2C addresses don't have a size, so adding one to the DT doesn't 
make sense.

[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

Hello,

On 01/05/2016 06:08 PM, Stephen Warren wrote:
> On 01/05/2016 08:37 AM, Przemyslaw Marczak wrote:
>> Hello,
>>
>> On 01/04/2016 09:02 PM, Stephen Warren wrote:
>>> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>>>> Hello Stephen,
>>>>
>>>> On 12/16/2015 07:53 PM, Stephen Warren wrote:
>>>>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote:
>>>>>> commit: dm: core: Enable optional use of fdt_translate_address()
>>>>>>
>>>>>> enables device's bus/child address translation method, depending
>>>>>> on bus 'ranges' property and including child 'reg' property.
>>>>>> This change makes impossible to decode the 'reg' for node with
>>>>>> '#size-cells' equal to 0.
>>>>>>
>>>>>> Such case is possible by the specification and is also used in
>>>>>> U-Boot,
>>>>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present.
>>>>>
>>>>> Can you please explain the problem you're seeing in more detail?
>>>>> Without
>>>>> any context, my initial reaction is that this is simply a bug
>>>>> somewhere.
>>>>> That bug should be fixed, rather than introducing new APIs to hide the
>>>>> problem.
>>>>>
>>>>
>>>> Some time ago I send a patch with such fix:
>>>>
>>>> [1] https://patchwork.ozlabs.org/patch/537372/
>>>>
>>>> Sorry, I didn't add you to the 'CC' list.
>>>>
>>>> However. I checked this in linux, and the code is the same, the
>>>> size-cells == 0 is not supported also in Linux.
>>>
>>> The discussion there does indicate that removing the check on
>>> #size-cells would be incorrect.
>>
>> Ok, this probably would be good if we assume that dts is always well
>> written, so this is not acceptable.
>
> Why not?
>
> Certainly it's reasonable to expect code not to blow up in crashy ways
> in the face of bad DTs. However, I don't think it's reasonable to
> require that code perform exactly the desired function in the face of
> arbitrary bad input data.
>
> Besides, the problem here isn't a bad DT. It's perfectly legal for a DT
> to contain #size-cells=<0>. The problem is bad code, which attempts to
> translate an address beyond the root of the address space that the
> address is within. This is a bug in the code, pure and simple.
>
>>>> So to prevent breaking some consistency in parsing fdt between U-boot
>>>> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be
>>>> useful at least for I2C and Exynos GPIO driver.
>>>
>>> OK; as I mentioned in my other reply, some form of new function or new
>>> parameter does seem reasonable here.
>>>
>>> ...
>>
>> At this point I can say, that the device-tree files and some compatible
>> drivers are using wrong assumptions.
>>
>> I think, that adding the new function is not needed, and also that we
>> don't need any new parameter to the function dev_get_reg(),
>> because the right way is to fix the fdt.
>>
>> For the Exynos GPIO issue, we can use two cases:
>> - define proper ranges
>> - move  #size-cells=0 to #size-cells=1 and extend the reg property by
>> it's size (actually not too much to do)
>>
>> This will fix the Exynos boot issue.
>
> That may fix/hide the issue, but semantically seems entirely incorrect.
> I2C addresses cannot logically be mapped into the CPU's MMIO address
> space, so adding a ranges property that attempts to do that doesn't make
> sense. I2C addresses don't have a size, so adding one to the DT doesn't
> make sense.
>
>

Agree, then I think my new patch should be suitable for the both cases:

https://patchwork.ozlabs.org/patch/564246/

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com