[U-Boot] [PATCH] fdt: Allow fdt_translate_address() to work with buses

[Przemyslaw Marczak]

Hello,

On 01/05/2016 02:00 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 4 January 2016 at 13:15, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 01/03/2016 04:04 PM, Simon Glass wrote:
>>>
>>> It is common for I2C and SPI buses to have a single-cell address and a
>>> size
>>> of 0. These produce a warning at present. For example on snow:
>>>
>>> __of_translate_address: Bad cell count for gpc4
>>> __of_translate_address: Bad cell count for gpx0
>>> __of_translate_address: Bad cell count for gpv2
>>> __of_translate_address: Bad cell count for gpv4
>>>
>>> One of the nodes in question looks like this in part:
>>>
>>>          pinctrl_2: pinctrl at 10d10000 {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>>                  gpv2: gpv2 {
>>>                          reg = <0x060>;
>>>                  };
>>>                  gpv4: gpv4 {
>>>                          reg = <0xc0>;
>>>                  };
>>>          };
>>>
>>> This is clearly valid so it looks like the conversion to use
>>> fdt_translate_address() in dev_get_addr() is not currently a good move.
>>
>>
>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the affected
>> platforms? That's precisely why that config option was introduced when the
>> call to fdt_translate_address() was added to dev_get_addr()?
>>
>> That would prevent this patch from affecting platforms that don't trigger
>> this issue, this leaving the valid check in place.
>
> But since this breaks normal behaviour we don't know what platforms
> are affected. We have made CONFIG_OF_TRANSLATE the default. So this
> approach doesn't seem (in effect) any better than Przemyslaw's newer
> series, below.
>
>>
>>> Przemyslaw Marczak sent three patches to resolve this for exynos boards:
>>>
>>> https://patchwork.ozlabs.org/patch/557008/
>>> https://patchwork.ozlabs.org/patch/557010/
>>> https://patchwork.ozlabs.org/patch/557009/
>>>
>>> But this involves creating a new function, and everyone will need to know
>>> when to use which one. Also the problem may affect other boards.
>>
>>
>> I suggest adding an extra parameter to dev_get_addr() (or whatever calls it)
>> that indicates the root of the address space. The check on #size-cells
>> should be skipped for that one node (or level of translation) but enabled
>> for all other levels. This way, there would be no need for anyone to choose
>> between functions; there'd only be one. Most cases (i.e. translation of MMIO
>> addresses) would simply pass 0 as the extra parameter (for the root node),
>> but in special cases where it's known translation is not expected to reach
>> the root MMIO space (e.g. I2C, SPI controllers), the controller node would
>> be passed in.
>
> How would the caller know this root? It sounds plausible, but I do
> want to avoid complex rules. I think you are saying that buses that
> use their own address mechanism (i.e. not MMIO) must do something
> special. The current dev_get_addr() is really simple.
>
> Regards,
> Simon
>
>

Simon, Stephen

As a summary of the issue, please tell me your opinion about this:

Since the __of_translate_address() is called always with the "ranges" as 
an argument, it looks reasonable to check it at the function beginning, 
that the "ranges" property exists in the parent node.

If not exists - then don't check the size-cells count and this should 
fix the problem with additional argument.
This is simple and correct from specification point of view - which says 
ranges and #size-cells property's - are not required [ePAPR v1.1].

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