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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello,

On 01/29/2016 07:23 PM, Simon Glass wrote:
> Hi Przymyslaw,
>
> On 15 January 2016 at 09:35, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 01/15/2016 03:41 AM, Przemyslaw Marczak wrote:
>>>
>>> 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@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.
>>
>>
>> There should only be one definition of DT bindings. That is, both U-Boot and
>> Linux must use the same bindings and hence interpret the DT in the same way.
>> That's the entire point of DT.
>>
>> Preferably both Linux and U-Boot will use the exact same DT content. There
>> may be some differences, e.g. if U-Boot doesn't support a particular
>> driver/feature, then the nodes/properties that enable that feature can be
>> omitted from the U-Boot DT since they won't be used. However, where the same
>> node/property exists in both places, it should be identical between both.
>>
>> Prior to proposing any DT changes for U-Boot, the best approach is to get
>> them into the Linux kernel DTs so that they get widespread review against
>> the binding definitions and so that everyone using DT approves the changes.
>
> What would you like to do here?
>
> Regards,
> Simon
>
>

I will send a proper patch for the Kernel and probably U-Boot, before 
the end of this week.

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