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

Quoted 7 lines · 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.

Quoted 2 lines

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.

Quoted 8 lines · Stephen Warren

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 2 lines

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,

Quoted 17 lines · Stephen Warren

Quoted 8 lines · Stephen Warren

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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?

Quoted 44 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 19 lines · Stephen Warren

Quoted 11 lines · Stephen Warren

Quoted 9 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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:-)

Quoted 2 lines

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.

Quoted 11 lines

Quoted 7 lines

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.

Quoted 2 lines

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,

Quoted 55 lines · Stephen Warren

Quoted 47 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 20 lines · Stephen Warren

Quoted 12 lines · Stephen Warren

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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

Quoted 23 lines

Quoted 2 lines

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.

Quoted 10 lines

Quoted 9 lines

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

Quoted 3 lines

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.

Quoted 10 lines

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.

Quoted 2 lines

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

Quoted 118 lines · Przemyslaw Marczak

Hello,

Quoted 64 lines · Stephen Warren

Quoted 54 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 25 lines · Stephen Warren

Quoted 15 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 5 lines

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

Quoted 29 lines

Quoted 2 lines

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.

Quoted 11 lines

Quoted 9 lines

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

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

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()?

Hi Stephen,

Quoted 135 lines · Stephen Warren

Quoted 118 lines · Przemyslaw Marczak

Hello,

Quoted 64 lines · Stephen Warren

Quoted 54 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 25 lines · Stephen Warren

Quoted 15 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 5 lines

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

Quoted 29 lines

Quoted 2 lines

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.

Quoted 11 lines

Quoted 9 lines

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

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 7 lines · 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.

Quoted 2 lines

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.

Quoted 8 lines

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?

Hello Stephen,

Quoted 8 lines · Stephen Warren

Quoted 7 lines · 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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 18 lines

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:

Quoted 7 lines

         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)

Quoted 28 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 10 lines · Stephen Warren

Quoted 8 lines · 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.

Quoted 9 lines

Quoted 2 lines

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/

Quoted 4 lines

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.

Quoted 4 lines

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.

...

Quoted 38 lines

Quoted 18 lines

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:

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 9 lines

---- case 1 -----

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

This is ok!

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

Quoted 9 lines

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

Quoted 10 lines

---- case 3 -----

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

Quoted 3 lines

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.

Hi Stephen,

Quoted 36 lines · Stephen Warren

Quoted 32 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 10 lines · Stephen Warren

Quoted 8 lines · 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.

Quoted 9 lines

Quoted 2 lines

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.

Quoted 1 line

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)?

Quoted 24 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 8 lines · Stephen Warren

Quoted 7 lines · 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.

Quoted 8 lines

Quoted 2 lines

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/

Quoted 4 lines

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.

Quoted 3 lines

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.

...

Quoted 37 lines

Quoted 18 lines

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:

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 9 lines

---- case 1 -----

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

This is ok!

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

Quoted 9 lines

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

Quoted 9 lines

---- case 3 -----

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

Quoted 3 lines

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.

Hello,

Quoted 29 lines · Stephen Warren

Quoted 28 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 11 lines · Stephen Warren

Quoted 9 lines · 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.

Quoted 7 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 33 lines · Przemyslaw Marczak

Hello,

Quoted 30 lines · Stephen Warren

Quoted 29 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 12 lines · Stephen Warren

Quoted 10 lines · 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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 21 lines

Quoted 8 lines

Quoted 3 lines

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)

Quoted 1 line

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.