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

[Bin Meng]

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)

Regards,
Bin