[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Bin,

On 04.12.2015 07:17, Bin Meng wrote:
> Hi,
>
> On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Stefan,
>>
>> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <sr at denx.de> wrote:
>>> Hi Bin,
>>>
>>>
>>> On 03.12.2015 14:34, Bin Meng wrote:
>>>>
>>>> Hi Stefan, Simon,
>>>>
>>>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> On 29 September 2015 at 23:00, Stefan Roese <sr at denx.de> wrote:
>>>>>>
>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>> which handles this translation correctly.
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
>>>>>> Cc: Stephen Warren <swarren at nvidia.com>
>>>>>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>>>>>> ---
>>>>>> v3:
>>>>>> - Rebased on current U-Boot version
>>>>>> - Added Stephen and Lukasz to Cc
>>>>>>
>>>>>> v2:
>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>     to make the use of the code paths more clear.
>>>>>>
>>>>>>    drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
>>>>>>    drivers/core/device.c | 20 ++++++++++++++++++++
>>>>>>    2 files changed, 50 insertions(+)
>>>>>
>>>>>
>>>>> Applied to u-boot-dm, thanks!
>>>>
>>>>
>>>> When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no
>>>> longer works. git bisect leads to this commit. Somehow I missed this
>>>> patch before although I see the commit message get me cc'ed but the
>>>> email did not bring to my attention.
>>>>
>>>> I see this patch introduced OF_TRANSLATE and by default set it to y.
>>>> This makes the code logic in dev_get_addr() go through
>>>> fdt_translate_address(), which breaks the things.
>>>
>>>
>>> I'm a bit surprised that using the common fdt_translate_address()
>>> function instead of the DM internal simple_bus_translate() causes
>>> problems on your platform. Are you sure that the ranges are
>>> described correctly in your dts? Is the dts a copy from the Linux
>>> original one? Ah, probably not, since we're talking about x86
>>> which has no DT support in Linux, right?
>>>
>>
>> Is fdt_translate_address() able to handle PCI bus ranges property? PCI
>> has special ranges.
>>
>> The arch/x86/dts/crownbay.dts has something like below:
>>
>>   90         pci {
>>   91                 #address-cells = <3>;
>>   92                 #size-cells = <2>;
>>   93                 compatible = "pci-x86";
>>   94                 u-boot,dm-pre-reloc;
>>   95                 ranges = <0x02000000 0x0 0x40000000 0x40000000 0 0x80000000
>>   96                           0x42000000 0x0 0xc0000000 0xc0000000 0 0x20000000
>>   97                           0x01000000 0x0 0x2000 0x2000 0 0xe000>;
>>   98
>>   99                 pcie at 17,0 {
>> 100                         #address-cells = <3>;
>> 101                         #size-cells = <2>;
>> 102                         compatible = "pci-bridge";
>> 103                         u-boot,dm-pre-reloc;
>> 104                         reg = <0x0000b800 0x0 0x0 0x0 0x0>;
>>
>>>> Should we set
>>>> OF_TRANSLATE to n by default? If set to y, this requires dts to have
>>>> complete ranges property everywhere.
>>>
>>>
>>> My understanding here is that x86 is a special case. As it doesn't
>>> use the full-blown dts sources from Linux. But most likely some
>>> "simple" ones, written exactly for U-Boot / DM.
>>>
>>> I would still prefer to have this OF_TRANSLATE set to y as default.
>>> As its needed for at least some platforms. But if we decide to
>>> set it to n, I can live with it as well.
>>>
>
> Looks like the issue is:
>
> dev_get_addr() return value is of type fdt_addr_t, and if no valid
> address found returns FDT_ADDR_T_NONE. But FDT_ADDR_T_NONE is defined
> as follows:
>
> #ifdef CONFIG_PHYS_64BIT
> #define FDT_ADDR_T_NONE (-1ULL)
> #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
> #define fdt_size_to_cpu(reg) be64_to_cpu(reg)
> #else
> #define FDT_ADDR_T_NONE (-1U)
> #define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
> #define fdt_size_to_cpu(reg) be32_to_cpu(reg)
> #endif
>
> On x86, CONFIG_PHYS_64BIT is not defined, so FDT_ADDR_T_NONE becomes -1U.
>
> In the ns16550 driver, the code logic is:
>
> /* try Processor Local Bus device first */
> addr = dev_get_addr(dev);
> #ifdef CONFIG_PCI
>      if (addr == FDT_ADDR_T_NONE) {
>      /* then try pci device */
>
> With OF_TRANSLATE set to y, dev_get_addr() returns OF_BAD_ADDR if no
> valid address found, but OF_BAD_ADDR is defined as:
>
> #define OF_BAD_ADDR ((u64)-1)
>
> This creates a size mismatch as FDT_ADDR_T_NONE can be -1U or -1ULL
> depending on CONFIG_PHYS_64BIT but OF_BAD_ADDR is always -1ULL.
>
> The patch below fixes this issue:
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index f86365e..8930f34 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -16,6 +16,7 @@
>   #include <libfdt.h>
>   #include <fdt_support.h>
>   #include <exports.h>
> +#include <fdtdec.h>
>
>   /**
>    * fdt_getprop_u32_default_node - Return a node's property or a default
> @@ -945,7 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
>
>   /* Max address size we deal with */
>   #define OF_MAX_ADDR_CELLS      4
> -#define OF_BAD_ADDR    ((u64)-1)
> +#define OF_BAD_ADDR    FDT_ADDR_T_NONE
>   #define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
> OF_MAX_ADDR_CELLS && \
>                          (ns) > 0)

I remember stumbling over such a related problem as well a few
weeks ago. With a mismatch of address-cells size and non-64bit
platform support. But I got distracted from this issue at that
time.

Thanks for looking into this. This change looks good to me. Please
send a patch to the list.

Thanks,
Stefan