From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Fri, 4 Dec 2015 08:52:09 +0100 Subject: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address() In-Reply-To: References: <1441174944-8878-1-git-send-email-sr@denx.de> <1443589243-9521-1-git-send-email-sr@denx.de> <56604DC3.1030006@denx.de> Message-ID: <56614629.6060203@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Bin, On 04.12.2015 07:17, Bin Meng wrote: > Hi, > > On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng wrote: >> Hi Stefan, >> >> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese 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 wrote: >>>>> >>>>> On 29 September 2015 at 23:00, Stefan Roese 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 >>>>>> Cc: Simon Glass >>>>>> Cc: Bin Meng >>>>>> Cc: Marek Vasut >>>>>> Cc: Masahiro Yamada >>>>>> Cc: Stephen Warren >>>>>> Cc: Lukasz Majewski >>>>>> --- >>>>>> 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 > #include > #include > +#include > > /** > * 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