From patchwork Fri Dec 11 05:54:39 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Roese X-Patchwork-Id: 555564 X-Patchwork-Delegate: sjg@chromium.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 96E751401DA for ; Fri, 11 Dec 2015 16:54:55 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 5B5884B6B2; Fri, 11 Dec 2015 06:54:52 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FDImxxDvaemg; Fri, 11 Dec 2015 06:54:52 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id D5DD44B699; Fri, 11 Dec 2015 06:54:51 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 73FCD4B699 for ; Fri, 11 Dec 2015 06:54:46 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rO36BJKf72T5 for ; Fri, 11 Dec 2015 06:54:46 +0100 (CET) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by theia.denx.de (Postfix) with ESMTPS id 2CC874B64D for ; Fri, 11 Dec 2015 06:54:41 +0100 (CET) Received: by mail-wm0-f53.google.com with SMTP id c201so62400402wme.0 for ; Thu, 10 Dec 2015 21:54:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type; bh=yYFz+7qb1YJx/rMyNv/Fe6axXjMf5Tb8p+fGEh/l7rg=; b=eV6h6uQOolO7KqZN+iDFDX//Tem4KispcC3BTbZTwaQJfNAfrromjk1IK6EDScUYMZ pMwudSMSHK3Y4yW6Qtz9ABztMn8uijjcLIySnyH5juQ9aojO/FpmLa7p8lj/orQswwT+ PrYmIYan2nMmb8OAOdM3VZAkEPlNZICa0tx8ClDoQbbrbeVK8+U8nppFFXSpU7CP8HT9 KAU7nY59AsOyBHfmaN7lwciTmZYNvOxAJAK9r3wMsOQjzD0EINH+6RTpTFJ7Ek+DnXWr Bm/cd6hVoYCHL5HFdCISPulDo1I2uMpu2dXe5yqrIaOryCoaZKJ+9eVGYb/9ort9Bo2I pFnw== X-Received: by 10.194.250.39 with SMTP id yz7mr18979090wjc.92.1449813281350; Thu, 10 Dec 2015 21:54:41 -0800 (PST) Received: from [192.168.1.54] (b9168f50.cgn.dg-w.de. [185.22.143.80]) by smtp.gmail.com with ESMTPSA id bh6sm15479000wjb.0.2015.12.10.21.54.40 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 10 Dec 2015 21:54:40 -0800 (PST) To: Simon Glass References: <1448619748-26759-1-git-send-email-sr@denx.de> <565BF232.4030602@denx.de> <565D38B3.6010602@denx.de> <565EFC09.3070209@denx.de> <565F0D37.30700@denx.de> <565F15AE.7010004@denx.de> <565F2DC1.7040608@denx.de> <56602819.5040006@denx.de> <56614486.20300@denx.de> <56692290.6010108@denx.de> <566A54EC.1080409@denx.de> From: Stefan Roese Message-ID: <566A651F.5080408@denx.de> Date: Fri, 11 Dec 2015 06:54:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <566A54EC.1080409@denx.de> Cc: U-Boot Mailing List , Luka Perkov Subject: Re: [U-Boot] [PATCH] dm: core: Add platform specific bus translation function X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" Hi Simon, On 11.12.2015 05:45, Stefan Roese wrote: > Hi Simon, > > On 10.12.2015 16:36, Simon Glass wrote: >> Hi Stefan, >> >> On 9 December 2015 at 23:58, Stefan Roese wrote: >>> Hi Simon, >>> >>> >>> On 08.12.2015 03:46, Simon Glass wrote: >>>> >>>> Hi Stefan, >>>> >>>> On 4 December 2015 at 00:45, Stefan Roese wrote: >>>>> >>>>> Hi Simon, >>>>> >>>>> On 03.12.2015 18:21, Simon Glass wrote: >>>>>> >>>>>> Hi Stefan, >>>>>> >>>>>> On 3 December 2015 at 04:31, Stefan Roese wrote: >>>>>>> >>>>>>> >>>>>>> Hi Simon, >>>>>>> >>>>>>> On 02.12.2015 18:45, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Stefan, >>>>>>>> >>>>>>>> On 2 December 2015 at 10:43, Stefan Roese wrote: >>>>>>>>> >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> ( Last mail for tonight - a glass of quite nice red wine is >>>>>>>>> waiting for me ... ;) ) >>>>>>>>> >>>>>>>> >>>>>>>> That's the only sad thing about me being so many hours behind. >>>>>>>> Still I >>>>>>>> can do the same thing with people in Asia I suppose :-) >>>>>>> >>>>>>> >>>>>>> Right. I'm not sure about the wine quality in Asia though... ;) >>>>>>> >>>>>>>>> >>>>>>>>> On 02.12.2015 17:53, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi Stefan, >>>>>>>>>> >>>>>>>>>> On 2 December 2015 at 09:00, Stefan Roese wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On 02.12.2015 16:50, Simon Glass wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>>>> I think it would be better to make it depend on whether >>>>>>>>>>>>>> the bit >>>>>>>>>>>>>> is >>>>>>>>>>>>>> flipped, rather than whether you are in SPL or not. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> You simply can't detect if this "bit is flipped". You just >>>>>>>>>>>>> have >>>>>>>>>>>>> to know. This is a long lasting ugly thing on some Marvell >>>>>>>>>>>>> patforms. Here the comment from armada-xp-gp.dts: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Can you point me to the place in U-Boot where this bit is >>>>>>>>>>>> flipped? >>>>>>>>>>>> Something, somewhere has to make the change. So something >>>>>>>>>>>> has to >>>>>>>>>>>> know. >>>>>>>>>>>> Before it makes the change, the range works one way. >>>>>>>>>>>> Afterwards it >>>>>>>>>>>> works another way. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Sure. I've mentioned this before. Its here: >>>>>>>>>>> >>>>>>>>>>> arch/arm/mach-mvebu/cpu.c: >>>>>>>>>>> >>>>>>>>>>> int arch_cpu_init(void) >>>>>>>>>>> { >>>>>>>>>>> ... >>>>>>>>>>> >>>>>>>>>>> /* Linux expects the internal registers to be at >>>>>>>>>>> 0xf1000000 */ >>>>>>>>>>> writel(SOC_REGS_PHY_BASE, INTREG_BASE_ADDR_REG); >>>>>>>>>>> >>>>>>>>>>> This is the line that changes the register base address. And >>>>>>>>>>> to change it back you need to write to the new address, as the >>>>>>>>>>> address holding this base address is also moved. Quite ugly! >>>>>>>>>>> >>>>>>>>>>> So its really right at the start of U-Boot proper. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> OK I see. So really we can determine which way the address >>>>>>>>>> 'switch' >>>>>>>>>> it. It's just a case of making the change when we are ready, and >>>>>>>>>> keeping a record of that. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Yes. But how is the "common code" in dev_get_addr() supposed to >>>>>>>>> know >>>>>>>>> which version of U-Boot we are running on? This boils down to some >>>>>>>>> callback again, or not? Or even worse the ugly #ifdef. >>>>>>>> >>>>>>>> >>>>>>>> You would call a driver-model core function to select the ranges >>>>>>>> property to prefer. Then driver model will remember this setting >>>>>>>> and >>>>>>>> use it. >>>>>>> >>>>>>> >>>>>>> Yes. This can be done. I've taken the time to implement such a >>>>>>> version. And attached a small patch in a hackish version, just as >>>>>>> an RFC. As you will see, I've added the "ranges-spl" property to >>>>>>> some of the DT nodes. And added the DM core functions to enable to >>>>>>> usage of a different, non-standard "ranges" property name. >>>>>>> >>>>>>> All this is not really "clean" and will definitely break non-DM >>>>>>> usage of fdt_support.c. Not sure where to go from here. I would >>>>>>> still prefer my first patch version, even though I know that >>>>>>> you don't like to add this hook / callback into the DM core code. >>>>>>> >>>>>>> Let me know what you think. >>>>>> >>>>>> >>>>>> Actually that looks pretty good to me. I think the root uclass needs >>>>>> to grow a private struct, where you store the ranges name. It is >>>>>> slightly odd to have fdtdec calling back into DM, but I don't see a >>>>>> big problem with it. The two are strongly coupled anyway. You can put >>>>>> an #ifdef CONFIG_DM in fdtdec to solve your problem I suppose. >>>>> >>>>> >>>>> Its not only fdtdec.c but also fdt_support.c that needs this callback >>>>> into DM. And fdt_support.c is currently not coupled with DM at all. >>>>> Making this change generic, we really need to exchange all "ranges" >>>>> occurrences in the whole U-Boot source tree: >>>>> >>>>> $ git grep "\"ranges\"" >>>>> arch/powerpc/cpu/mpc85xx/portals.c: range = >>>>> fdt_getprop_w(blob, off, "ranges", &len); >>>>> arch/powerpc/cpu/mpc85xx/portals.c: >>>>> fdt_setprop_inplace(blob, >>>>> off, "ranges", range, len); >>>>> arch/powerpc/cpu/ppc4xx/fdt.c: rc = fdt_find_and_setprop(blob, >>>>> ebc_path, >>>>> "ranges", ranges, >>>>> arch/sparc/include/asm/prom.h:/* Element of the "ranges" vector */ >>>>> board/ifm/o2dnt2/o2dnt2.c: prop = fdt_get_property_w(blob, off, >>>>> "ranges", &len); >>>>> board/ifm/o2dnt2/o2dnt2.c: fdt_setprop(blob, off, >>>>> "ranges", >>>>> reg2, len); >>>>> board/intercontrol/digsy_mtc/digsy_mtc.c: prop = >>>>> fdt_get_property_w(blob, off, "ranges", &len); >>>>> board/intercontrol/digsy_mtc/digsy_mtc.c: >>>>> fdt_setprop(blob, >>>>> off, "ranges", reg2, len); >>>>> board/pdm360ng/pdm360ng.c: rc = fdt_find_and_setprop(blob, >>>>> "/localbus", "ranges", >>>>> board/socrates/socrates.c: rc = fdt_find_and_setprop(blob, >>>>> "/localbus", "ranges", >>>>> common/fdt_support.c: /* Normally, an absence of a "ranges" property >>>>> means we are >>>>> common/fdt_support.c: * /ht nodes with no "ranges" property and >>>>> a lot >>>>> of perfectly >>>>> common/fdt_support.c: * "ranges" as equivalent to an empty "ranges" >>>>> property which means >>>>> common/fdt_suppord0-t.c: return __of_translate_address(blob, >>>>> node_offset, in_addr, "ranges"); >>>>> common/fdt_support.c: prop = fdt_getprop(fdt, node, "ranges", >>>>> &size); >>>>> common/fdt_support.c: * a number of the "ranges" property array. >>>>> common/fdt_support.c: * The "ranges" property is an array of >>>>> common/fdt_support.c: ranges = fdt_getprop(fdt, node, "ranges", >>>>> &ranges_len); >>>>> drivers/core/Kconfig: on some platforms (e.g. MVEBU) using complex >>>>> "ranges" >>>>> drivers/core/Kconfig: on some platforms (e.g. MVEBU) using complex >>>>> "ranges" >>>>> drivers/core/simple-bus.c: ret = >>>>> fdtdec_get_int_array(gd->fdt_blob, >>>>> dev->of_offset, "ranges", >>>>> drivers/pci/pci-uclass.c: prop = fdt_getprop(blob, node, >>>>> "ranges", >>>>> &len); >>>>> >>>>> So at least pci-class.c should get changes as well. This looks not >>>>> really promising to me. So yes, this works, but I think its quite >>>>> clumsy and generates much more code and necessary changes, >>>>> especially to the dts files, where all the ranges properties now >>>>> need to get duplicated. >>>>> >>>>>> What about the device tree mailing list. Should I send an email >>>>>> there? >>>>> >>>>> >>>>> Sure. We could try to ask about their opinion as well. >>>> >>>> >>>> What about the idea of setting up an offset in device core. Is it a >>>> simple offset? >>> >>> >>> The internal registers are mapped at 0xd00x.xxxx in the SPL case. And >>> as 0xf10x.xxxx in the main U-Boot case. So this difference can >>> definitely be described as an offset, yes. Adding 0xdf00.0000 to >>> all device addresses should work in the SPL case. This is what my >>> first patch version with the platform specific device address fixup >>> (with the weak function) does. >>> >>> So what do you have in mind? Is some other device offset functionality >>> acceptable for you? >> >> I just mean that you could make a call to the driver model core code >> to set up this offset, and then dev_get_addr() can handle it >> automatically from there. A bit like the patch you sent but without >> the device tree component. It is just the call out to board code from >> drivers that I am not keen on. > > Okay. I'll try to come up with an RFC patch for this. Please find this RFC patch attached. Its still a bit hackish, as its on top of the current implementation. But you will get the idea. Is this what you had in mind? If yes, I'll gladly send a clean patch version to the list. Thanks, Stefan From 4b2ba8c142519eff1cce6bd51720b7fd54a0f635 Mon Sep 17 00:00:00 2001 From: Stefan Roese Date: Fri, 11 Dec 2015 06:04:36 +0100 Subject: [PATCH RFC] dm: Add option to configure an offset for the address translation Some platforms need to ability to configure an offset to the standard addresses extracted from the device-tree. This patch allows this by adding a function to DM to configure this offset (if needed). Signed-off-by: Stefan Roese --- arch/arm/mach-mvebu/spl.c | 6 ++++++ drivers/core/device.c | 4 ++++ drivers/core/root.c | 23 +++++++++++++++++++++++ include/dm/device.h | 4 ++++ 4 files changed, 37 insertions(+) diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c index 8edb2f3..fc3f57a 100644 --- a/arch/arm/mach-mvebu/spl.c +++ b/arch/arm/mach-mvebu/spl.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include @@ -60,6 +61,9 @@ void board_init_f(ulong dummy) hang(); } + /* Use special translation offset for SPL */ + dm_set_translation_offset(0xd0000000 - 0xf1000000); + preloader_console_init(); timer_init(); @@ -86,6 +90,7 @@ void board_init_f(ulong dummy) #endif } +#if 0 /* * When using DM with DT address translation, this does not work * with the standard fdt_translate_address() function on MVEBU @@ -101,3 +106,4 @@ fdt_addr_t platform_translate_address(void *blob, int node_offset, { return (addr & 0x00ffffff) | SOC_REGS_PHY_BASE; } +#endif diff --git a/drivers/core/device.c b/drivers/core/device.c index 27c4288..2c95516 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -625,8 +625,12 @@ fdt_addr_t dev_get_addr(struct udevice *dev) * platforms (e.g. mvebu in SPL) can provide a platform specific * translation function which is called now. */ +#if 0 addr = platform_translate_address((void *)gd->fdt_blob, dev->of_offset, addr); +#else + addr += dm_get_translation_offset(); +#endif return addr; #else diff --git a/drivers/core/root.c b/drivers/core/root.c index e7b1f24..218af34 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -37,6 +37,29 @@ struct udevice *dm_root(void) return gd->dm_root; } +uint32_t dm_get_translation_offset(void) +{ + struct udevice *root; + + root = dm_root(); + if (root) { + if (root->translation_offset) + return root->translation_offset; + } + + /* No offset as default */ + return 0; +} + +void dm_set_translation_offset(uint32_t offs) +{ + struct udevice *root; + + root = dm_root(); + if (root) + root->translation_offset = offs; +} + #if defined(CONFIG_NEEDS_MANUAL_RELOC) void fix_drivers(void) { diff --git a/include/dm/device.h b/include/dm/device.h index 7fb9935..26e3258 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -105,6 +105,7 @@ struct udevice { #ifdef CONFIG_DEVRES struct list_head devres_head; #endif + uint32_t translation_offset; }; /* Maximum sequence number supported */ @@ -776,4 +777,7 @@ static inline void devm_kfree(struct udevice *dev, void *ptr) #endif /* ! CONFIG_DEVRES */ +uint32_t dm_get_translation_offset(void); +void dm_set_translation_offset(uint32_t offs); + #endif -- 2.6.3