[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Bin Meng
bmeng.cn at gmail.com
Fri Dec 4 16:01:40 CET 2015
Hi Stefan,
On Fri, Dec 4, 2015 at 3:52 PM, Stefan Roese <sr at denx.de> wrote:
> 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.
Yep, actually I suspect there might still be other potential issue
with this dev_get_addr() API. As its return value is of type
fdt_addr_t which is phys_addr_t which can be either 32-bit or 64-bit,
but fdt_translate_address() always return u64. I see most APIs in
common/fdt_support.c accept or return a u64 address value.
>
> Thanks for looking into this. This change looks good to me. Please
> send a patch to the list.
>
Will do.
Regards,
Bin
More information about the U-Boot
mailing list