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

[Simon Glass]

Hi Przemyslaw,

On 12 January 2016 at 07:22, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> 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.
>

OK sounds good, thanks.

Regards,
Simon