[U-Boot] [PATCH] dm: fdtdec: Check full path for alias

[Michal Simek <michal.simek@xilinx.com>]

Hi Simon,

On 20.4.2016 16:41, Simon Glass wrote:
> Hi Michal,
> 
> On 14 April 2016 at 07:02, Michal Simek <michal.simek@xilinx.com> wrote:
>> Fix fdtdec_get_alias_seq() which is not checking full path to certain
>> node and it incorrectly provides incorrect seq number.
>> Checking full path ensure that if alias is present correct seq number is
>> return.
>> This problem was found on ZynqMP zcu102 where are several I2C muxes
>> where the first bus name is i2c at 0 and for following muxes incorrect seq
>> numbers are return.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> I am not sure if there is any macro for max alias length which I can use
>> instead of new macro.
>>
>> Before:
>> ZynqMP> i2c bus
>> Bus 0:  i2c at ff020000
>>    20: gpio at 20, offset len 1, flags 0
>>    21: gpio at 21, offset len 1, flags 0
>>    75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750:        i2c at 0
>> Bus 751:        i2c at 1
>> Bus 752:        i2c at 2
>> Bus 1:  i2c at ff030000
>>    74: i2cswitch at 74, offset len 1, flags 0
>>    75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750:        i2c at 0
> 
> Sorry I'm a bit confused by this. Do you have two i2c at 0 buses?

Yes there are two i2c IP cores. It means 2 main i2c busses.

Here is full i2c description
arch/arm/dts/zynqmp-zcu102.dts

I have pushed my hacky/testing tree here with these changes.
u-boot-microblaze.git xnext/i2c

> 
>> Bus 751:        i2c at 1
>> Bus 752:        i2c at 2
>> Bus 1743:       i2c at 3
>> Bus 1744:       i2c at 4
>> Bus 750:        i2c at 0
>> Bus 751:        i2c at 1
>> Bus 752:        i2c at 2
>> Bus 1743:       i2c at 3
>> Bus 1744:       i2c at 4
>> Bus 1755:       i2c at 5
>> Bus 1756:       i2c at 6
>> Bus 1757:       i2c at 7
>>
>> After:
>> ZynqMP> i2c bus
>> Bus 0:  i2c at ff020000
>>    20: gpio at 20, offset len 1, flags 0
>>    21: gpio at 21, offset len 1, flags 0
>>    75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750:        i2c at 0
>> Bus 751:        i2c at 1
>> Bus 752:        i2c at 2
>> Bus 1:  i2c at ff030000
>>    74: i2cswitch at 74, offset len 1, flags 0
>>    75: i2cswitch at 75, offset len 1, flags 0
>> Bus 1740:       i2c at 0
>> Bus 1741:       i2c at 1
>> Bus 1742:       i2c at 2
>> Bus 1743:       i2c at 3
>> Bus 1744:       i2c at 4
>> Bus 1750:       i2c at 0
>> Bus 1751:       i2c at 1
>> Bus 1752:       i2c at 2
>> Bus 1753:       i2c at 3
>> Bus 1754:       i2c at 4
>> Bus 1755:       i2c at 5
>> Bus 1756:       i2c at 6
>> Bus 1757:       i2c at 7
>>
>> Which reflects my aliases
>> i2c0 = &i2c0;
>> i2c750 = &i2c0_75_0;
>> i2c751 = &i2c0_75_1;
>> i2c752 = &i2c0_75_2;
>> i2c1 = &i2c1;
>> i2c1740 = &i2c1_74_0;
>> i2c1741 = &i2c1_74_1;
>> i2c1742 = &i2c1_74_2;
>> i2c1743 = &i2c1_74_3;
>> i2c1744 = &i2c1_74_4;
>> i2c1750 = &i2c1_75_0;
>> i2c1751 = &i2c1_75_1;
>> i2c1752 = &i2c1_75_2;
>> i2c1753 = &i2c1_75_3;
>> i2c1754 = &i2c1_75_4;
>> i2c1755 = &i2c1_75_5;
>> i2c1756 = &i2c1_75_6;
>> i2c1757 = &i2c1_75_7;
> 
> I only see one in you aliases above.

i2c0 and i2c1 are that main one.


> 
>>
>> ---
>>  lib/fdtdec.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index 70acc29c924d..79efcf0cd6b0 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
>>         return num_found;
>>  }
>>
>> +#define FDT_ALIAS_PATH_LEN     64
>> +
>>  int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>                          int *seqp)
>>  {
>> @@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>         int find_namelen;
>>         int prop_offset;
>>         int aliases;
>> +       char buf[FDT_ALIAS_PATH_LEN];
>> +
>> +       fdt_get_path(blob, offset, buf, sizeof(buf));
> 
> Eek! That is pretty expensive.

How to do it better?

> 
>>
>>         find_name = fdt_get_name(blob, offset, &find_namelen);
>> -       debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
>> +       debug("Looking for '%s' at %d, name %s, buf %s\n",
>> +             base, offset, find_name, buf);
>>
>>         aliases = fdt_path_offset(blob, "/aliases");
>>         for (prop_offset = fdt_first_property_offset(blob, aliases);
>> @@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>                     strncmp(name, base, base_len))
>>                         continue;
>>
>> +               if (len >= FDT_ALIAS_PATH_LEN)
>> +                       printf("Too long path in aliases node\n");
> 
> Can you return an error and make it debug() here?

Error code?

> 
>> +
>> +               /* Check full path first not to point to incorrect alias */
>> +               if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
>> +                       continue;
>> +
>>                 slash = strrchr(prop, '/');
>>                 if (strcmp(slash + 1, find_name))
>>                         continue;
>> --
>> 1.9.1
>>

Thanks,
Michal