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

[Przemyslaw Marczak]

Hello Simon,

On 01/12/2016 02:57 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 12 January 2016 at 03:25, Przemyslaw Marczak <p.marczak at samsung.com> 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, so the above case is possible. The 'reg' for the parent bus can represent MMIO (depends on what its parent defines) and the child is non-MMIO.
>>
>> You won't allow to use dev_get_addr() for other than MMIO addresses.
>> Ok, I have no more arguments and no more time.
>>
>> 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()"
>
> I'm sorry this has been so difficult. Thank you for digging into it.
>
> I'm going to take this patch as is unless there is an alternative
> patch on the table. Stephen please let me know if you'd like to write
> something. One idea seems to be a new function (like
> dev_get_addr_local()) which avoids the address translation. But
> Przemyslaw has put enough energy into this I think.
>
> Regards,
> Simon
>
>

I think, that we don't need such function.

Stephen has right with the universal dev_get_addr() - it should be used 
only for MMIO addresses.

And also any universal function for getting the reg value is useless, 
for some specific reasons, which Stephen mentioned.

I'm going to send another patch soon, which I think (again) should close 
the issue at all. Changing GPIO driver is not required, it will be 
enough when I fix the device-tree files (SoCxxx-pinctrl-uboot.dts).

We don't need to look at kernel, since we have two different drivers and 
also the kernel doesn't use the GPIO's regs (addresses are hardcoded).
So fixing device-tree is a good choose. It's really only few lines per file.

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