[U-Boot] [PATCH] fdt: __of_translate_address(): check parent's 'ranges' before translate

[Przemyslaw Marczak]

Hello Stephen,

On 01/07/2016 07:25 PM, Stephen Warren wrote:
> On 01/07/2016 04:40 AM, Przemyslaw Marczak wrote:
>> The present implementation of __of_translate_address() taken
>> from the Linux, is designed for translate bus/child address
>> mappings by using 'ranges' property - and it doesn't allow
>> for checking an address for a device's node with zero size-cells.
>>
>> The 'size-cells > 0' is required for bus/child address mapping,
>> but is not required for non-memory mapped address, e.g.: I2C chip.
>> Then when we need only raw 'reg' property's value.
>>
>> Since the I2C device address goes to a single-cell reg property,
>> support for that case is welcome, but currently calling dev_get_addr()
>> for I2C device will return 'FDT_ADDR_T_NONE', and print the warning:
>>
>> warning:
>> __of_translate_address: Bad cell count for 'some-dev'
>
> This patch takes the wrong approach.
>
> It simply doesn't make sense to /attempt/ to translate an I2C address
> into an MMIO address space. It's a nonsensical operation; no such
> translation is possible under any circumstances because I2C and MMIO
> addresses mean completely different things and simply can't be
> translated to each-other.
>
> Rather than making this nonsensical operation succeed in a way that
> gives the desired no-op result, the nonsensical operation simply
> shouldn't be performed in the first place.
>
>

Okay, the example with I2C may be little confusing - I could use some 
general naming convention. However, this patch updates FDT-related code 
only.

In one of your previous e-mails, you well argued that we shouldn't use 
dev_get_reg() for some buses, since they have a different 'reg' meaning.

You are right, using dev_get_addr() as universal function may be 
nonsensical.

Please note, that the present implementation of function: 
'__of_translate_address()' - allows for 1:1 translation, but only if 
'#size-cells' exists. So the below case is possible:

----------------------
parent {
     address-cells = <1>;
     size-cells = <1>;
     reg = <0x10000000 0x1000>;

     child {
         reg = <0xa00 0x100>;
     };
};

dev_get_reg(child) - will return '0xa00'
----------------------

If we don't need the address length, we can define:
----------------------
parent {
     address-cells = <1>;
     size-cells = <0>;
     reg = <0x10000000 0x1000>;

     child {
         reg = <0xa00>;
     };
};

code:

dev_get_reg(child) - returns '0xa00'
----------------------

I would like to distinguish few things:

1. This patch just adds the support for the above second case, which is 
sensible and possible from specification point of view.

2. How it will be used by the code - is another thing.

3. If some driver's code can just use of dev_get_addr() for it's case,
then why shouldn't it? Since it, knows how to interpret the returned 
value in its own specified way - and also it's described by the proper 
binding file, what the reg represents - then I don't threat this as 
issue. And here the example may be:
- s5p_gpio.c - exynos GPIO driver

4. If I update the commit message with a general naming convention 
(parent/children) instead of using I2C suggestion - will be that patch 
acceptable for you?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com