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

[Przemyslaw Marczak]

Hello Simon,

On 01/14/2016 06:17 PM, Simon Glass wrote:
> Hi Przemyslaw, Stephen,
>
> On 13 January 2016 at 04:10, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
>> Hello Stephen,
>>
>>
>> On 01/12/2016 05:43 PM, Stephen Warren wrote:
>>>
>>> On 01/12/2016 03:25 AM, Przemyslaw Marczak wrote:
>>>>
>>>> Hello Stephen,
>>>>
>>>> On 01/11/2016 05:47 PM, Stephen Warren wrote:
>>>>>
>>>>> On 01/11/2016 04:21 AM, Przemyslaw Marczak wrote:
>>>>>>
>>>>>> 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>;
>>>>>>       };
>>>>>> };
>>>>>
>>>>>
>>>>> This case won't ever appear in a correctly written DT where reg
>>>>> represents an MMIO address; MMIO addresses always have sizes, and hence
>>>>> can't have size-cells=0. Hence, translating through a DT structures like
>>>>> that is an error case, and shouldn't work.
>>>>
>>>>
>>>> As we found out, the 'reg' property can represent not only MMIO, but may
>>>> have other meaning,
>>>
>>>
>>> Of course.
>>>
>>>> so the above case is possible.
>>>
>>>
>>> Yes and no.
>>>
>>> That DT snippet is certainly possible.
>>>
>>> However, that's irrelevant to whether address translation should be
>>> attempted across that boundary. *That* is not legal and should not be
>>> attempted.
>>>
>>
>> Going through your suggestions I took your side.
>> You are on Cc in the new patchset.
>>
>>>   > The 'reg' for the
>>>>
>>>> parent bus can represent MMIO (depends on what its parent defines) and
>>>> the child is non-MMIO.
>>>
>>>
>>> Correct.
>>>
>>>> You won't allow to use dev_get_addr() for other than MMIO addresses.
>>>> Ok, I have no more arguments and no more time.
>>>
>>>
>>> "You" is incorrect. This has absolutely nothing to do with me, but
>>> rather the rule is imposed by the semantics of device tree.
>>>
>>> Also, I never said that dev_get_addr() must not be used for non-MMIO
>>> addresses. In fact, I offered a suggestion to make it work correctly.
>>> What I actually stated is that address translation must not be attempted
>>> across boundaries between address spaces, since it is semantically
>>> non-sensical.
>>>
>>
>> Ok, please don't take it personally:), it was just how I understood your
>> opinion.
>>
>> As you know the specification is not so clean, I thought, that checking the
>> existence of "ranges" in parent node - is enough to provide proper
>> "translation" (or rather choosing the root address space), when size-cells
>> == 0. However, checking this condition is probably not enough, but you
>> didn't provide a device-tree example to give it some light.
>>
>> Also maybe the translation is a bad word here, since we know that it's not
>> MMIO translatable address.
>>
>> For me, this patch is okay.
>> If I call it for I2C chip and it returns the chip address in I2C address
>> space - then I can assume, that this is correct.
>>
>> Since, at present I2C subsystem takes the 'reg' as property's value, it
>> looks that there should be no difference when using modified dev_get_reg().
>>
>> However the main reason for this change was not I2C code update, but fixing
>> Exynos GPIO driver which uses DTB in a quite different way than the others.
>>
>> So, I don't need to put the pressure for applying an improvement like this
>> one - because it can be fixed in a more proper way.
>>
>>>> My issue can be also fixed by removing dev_get_addr() call from Exynos
>>>> GPIO driver - so I will do this and within this change, will also revert
>>>> the commit:
>>>> "fdt: fix address cell count checking in fdt_translate_address()"
>>>
>>>
>>> That sounds fine. It'd be better to introduce some code into the I2C
>>> subsystem to handle this, but the approach you mention should work in
>>> practice.
>>>
>>>
>>
>> So finally, as you can see at the new patches:
>>
>> http://patchwork.ozlabs.org/patch/566584/
>> http://patchwork.ozlabs.org/patch/566587/
>>
>> I made other quick fix. This should be extended by ranges to be proper in
>> 100%, but Linux don't use it for this platform and I don't see the reason
>> for adding it to U-Boot.
>
> You could presumably add it to Linux also.
>
> Thank you both for figuring this out.
>
> Regards,
> Simon
>
>

The commit updates files, which exists in U-Boot only.

Moreover, the problematic reg properties are not used by Linux's Exynos 
GPIO driver - because all required addresses are hardcoded in the 
driver. So I don't see the reason for doing it there.

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