| Message ID | 566A651F.5080408@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
HI Stefan, On 10 December 2015 at 22:54, Stefan Roese <sr@denx.de> wrote: > > 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 <sr@denx.de> wrote: >>>> >>>> Hi Simon, >>>> >>>> >>>> On 08.12.2015 03:46, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stefan, >>>>> >>>>> On 4 December 2015 at 00:45, Stefan Roese <sr@denx.de> wrote: >>>>>> >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> On 03.12.2015 18:21, Simon Glass wrote: >>>>>>> >>>>>>> >>>>>>> Hi Stefan, >>>>>>> >>>>>>> On 3 December 2015 at 04:31, Stefan Roese <sr@denx.de> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On 02.12.2015 18:45, Simon Glass wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Stefan, >>>>>>>>> >>>>>>>>> On 2 December 2015 at 10:43, Stefan Roese <sr@denx.de> 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 <sr@denx.de> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Simon, >>>>>>>>>>>> >>>>>>>>>>>> On 02.12.2015 16:50, Simon Glass wrote: >>>>>>>>>>>> >>>>>>>>>>>> <snip> >>>>>>>>>>>> >>>>>>>>>>>>>>> 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. Yes this looks good - the setting is maintained by driver model rather than looked up each time with a function call. When you send the patch can you put the offset in the uclass-private data for root, rather than in struct udevice? Regards, Simon
From 4b2ba8c142519eff1cce6bd51720b7fd54a0f635 Mon Sep 17 00:00:00 2001 From: Stefan Roese <sr@denx.de> 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 <sr@denx.de> --- 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 <common.h> +#include <dm.h> #include <debug_uart.h> #include <fdtdec.h> #include <spl.h> @@ -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