[U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible

[Mario Six]

On Thu, Mar 1, 2018 at 8:26 AM, Mario Six <mario.six at gdsys.cc> wrote:
> (Adding Simon, because of DM)
>
> Hi Martin,
>
> On Wed, Feb 28, 2018 at 7:27 PM, Martin Fuzzey <mfuzzey at parkeon.com> wrote:
>> Hi,
>>
>> since this commit:
>>
>> commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd
>> Author: Mario Six <mario.six at 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 at 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