Hi Stefan, On Fri, Dec 4, 2015 at 3:52 PM, Stefan Roese <s...@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...@gmail.com> wrote: >>> >>> Hi Stefan, >>> >>> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <s...@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 <s...@chromium.org> wrote: >>>>>> >>>>>> >>>>>> On 29 September 2015 at 23:00, Stefan Roese <s...@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 <s...@denx.de> >>>>>>> Cc: Simon Glass <s...@chromium.org> >>>>>>> Cc: Bin Meng <bmeng...@gmail.com> >>>>>>> Cc: Marek Vasut <ma...@denx.de> >>>>>>> Cc: Masahiro Yamada <yamada.masah...@socionext.com> >>>>>>> Cc: Stephen Warren <swar...@nvidia.com> >>>>>>> Cc: Lukasz Majewski <l.majew...@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot