[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg
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(-)
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
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);
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
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@138d0000 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; samsung,i2c-max-bus-freq = <100000>; status = "okay";
max77686_pmic@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@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?
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).
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,
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.
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,
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.
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
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.
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,
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@138d0000 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; samsung,i2c-max-bus-freq = <100000>; status = "okay"; max77686_pmic@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@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@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@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@138d0000 i2c@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@09 { }; };
------------ check pmic node -------------------- Trats2 # fdt list /i2c@138d0000/max77686_pmic@09 max77686_pmic@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,
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@138d0000 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; samsung,i2c-max-bus-freq = <100000>; status = "okay"; max77686_pmic@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@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@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@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.
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@138d0000 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; samsung,i2c-max-bus-freq = <100000>; status = "okay"; max77686_pmic@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@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@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@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
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.
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@138d0000 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; samsung,i2c-max-bus-freq = <100000>; status = "okay"; max77686_pmic@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@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@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@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,
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.
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,
participants (3)
-
Przemyslaw Marczak -
Simon Glass -
Stephen Warren