On Thu, Mar 1, 2018 at 8:26 AM, Mario Six mario.six@gdsys.cc wrote:

(Adding Simon, because of DM)

Hi Martin,

On Wed, Feb 28, 2018 at 7:27 PM, Martin Fuzzey mfuzzey@parkeon.com wrote:

...

Hi,

since this commit:

commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd Author: Mario Six mario.six@gdsys.cc Date: Mon Jan 15 11:07:45 2018 +0100

gpio: pca953x_gpio: Make live-tree compatible

I am getting the error message "__of_translate_address: Bad cell count for pcal6524@1"

I don't think the patch

•   addr = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), "reg", 0);
  

•   addr = dev_read_addr(dev);
  
  

is correct since it replaces a simple lookup of the value "reg" property with an attempted translation to a CPU address (which fails for I2C)

I see the issue on one of our boards as well, but the weird thing is that the translation should not happen: the I2C bus has no ranges property, which should, according to the DeviceTree specification, cause the translation routine to not attempt a translation (because a missing ranges property implies that the device's address space is not translatable). I'll take a look at the code; maybe it's a Linux-specific exception that was imported with the code that I was not aware of.

Looks like that's it (from drivers/core/of_addr.c):

* Note: We consider that crossing any level with #size-cells == 0 to mean * that translation is impossible (that is we are not dealing with a value * that can be mapped to a cpu physical address). This is not really specified * that way, but this is traditionally the way IBM at least do things

So, of_translate_address fails by design if #size-cells == 0.

I propose the following patch, which would prevent translation in this case in ofnode_get_addr_index:

---- >8 ---- diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 98f4b539ea..b2f6ffe385 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -200,13 +200,16 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index) uint flags; u64 size; int na; + int ns;

prop_val = of_get_address(ofnode_to_np(node), index, &size, &flags); if (!prop_val) return FDT_ADDR_T_NONE;

- if (IS_ENABLED(CONFIG_OF_TRANSLATE)) { + ns = of_n_size_cells(ofnode_to_np(node)); + + if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) { return of_translate_address(ofnode_to_np(node), prop_val); } else { na = of_n_addr_cells(ofnode_to_np(node)); ---- >8 ----

In other words: #size-cells == 0 -> No translation, return the plain reg value. #size-cells != 0 -> Do proper translation.

This fixes the issue on our board.

The dev_read_addr() is the go-to function for drivers to get device addresses (and have all issues like translation and the like taken care of automatically), so it would be nice to keep it in my opinion. Hence, fixing the underlying issue is probably preferable.

...

This does not cause the driver probe to fail since dev_read_addr() returns FDT_ADDR_T_NONE (== -1) and is followed by

if (addr == 0)
    return -ENODEV;

info->addr = addr;

Although addr is stored in the driver data the only thing it is used for is to build the bank name

snprintf(name, sizeof(name), "gpio@%x_", info->addr);

I'm not even sure that using the value of the "reg" property as before is really a good idea - what happens if we have multiple chips with same address on seperate busses?

We have that exact case on some of our boards. It works, but it's pretty annoying if you want to use the gpio command: When setting values of GPIOs, it only ever affects the first device the command comes across.

I have a potential solution for that, see below.

...

Maybe it would be better to use the DT node name? But would that break existing configurations by renaming the device?

I have a patch I use on our boards that looks for a "label" string property in the DT, which is then used for the bank name if it's there, and falls back to the "gpio@%x_" in the case it's not there. If you think it's useful, I could send the patch to the mailing list?

...

Regards,

Martin

Best regards,

Mario

Hi Martin,

On Thu, Mar 1, 2018 at 11:07 AM, Martin Fuzzey mfuzzey@parkeon.com wrote:

Hi Mario,

thank you for your very quick reply.

On 01/03/18 09:01, Mario Six wrote:

...

Looks like that's it (from drivers/core/of_addr.c):

• Note: We consider that crossing any level with #size-cells == 0 to

mean

• that translation is impossible (that is we are not dealing with a

value

• that can be mapped to a cpu physical address). This is not really

specified

• that way, but this is traditionally the way IBM at least do things

So, of_translate_address fails by design if #size-cells == 0.

I propose the following patch, which would prevent translation in this case in ofnode_get_addr_index:

---- >8 ---- diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 98f4b539ea..b2f6ffe385 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -200,13 +200,16 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index) uint flags; u64 size; int na;

•           int ns;
  
             prop_val = of_get_address(ofnode_to_np(node), index,
  

&size, &flags); if (!prop_val) return FDT_ADDR_T_NONE;

•           if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
  

•           ns = of_n_size_cells(ofnode_to_np(node));
  

•           if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
                     return
  

of_translate_address(ofnode_to_np(node), prop_val); } else { na = of_n_addr_cells(ofnode_to_np(node)); ---- >8 ----

In other words: #size-cells == 0 -> No translation, return the plain reg value. #size-cells != 0 -> Do proper translation.

This fixes the issue on our board.

That isn't enough for the non live-tree case.

I've added this too:

diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c index 3847dd8..9a3b4c3 100644 --- a/drivers/core/fdtaddr.c +++ b/drivers/core/fdtaddr.c @@ -49,12 +49,17 @@ fdt_addr_t devfdt_get_addr_index(struct udevice *dev, int index)

            reg += index * (na + ns);

•           /*
  

•            * Use the full-fledged translate function for complex
  

•            * bus setups.
  

•            */
  

•           addr = fdt_translate_address((void *)gd->fdt_blob,
  

•                                        dev_of_offset(dev), reg);
  

•           if (ns) {
  

•                   /*
  

•                    * Use the full-fledged translate function for
  

complex

•                    * bus setups.
  

•                    */
  

•                   addr = fdt_translate_address((void *)gd->fdt_blob,
  

• dev_of_offset(dev), reg);

•           } else {
  

•                   /* Non translatable if #size-cells == 0 */
  

•                   addr = fdt_read_number(reg, na);
  

•           }
    } else {
            /*
             * Use the "simple" translate function for less complex
  
  

Ah, yes, indeed, thanks for catching that. The enhanced patch still works in the live-tree case, so, can I go ahead and add your Signed-off-by to the patch and send it?

Thanks for testing, and sorry for the inconvenience.

...

I have a patch I use on our boards that looks for a "label" string property in

...

the DT, which is then used for the bank name if it's there, and falls back to the "gpio@%x_" in the case it's not there. If you think it's useful, I could send the patch to the mailing list?

Yes please.

OK, I'll send it in a few minutes.

Regards,

Martin

Best regards,

Mario