[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

This patch adds the additional platform_translate_address() call to
dev_get_addr(). A weak default with a 1-to-1 translation is also
provided. Platforms that need a special address translation can
overwrite this function.

Here the explanation, why this is needed for MVEBU:

When using DM with DT address translation, this does not work
with the standard fdt_translate_address() function on MVEBU
in SPL. Since the DT translates to the 0xf100.0000 base
address for the internal registers. But SPL still has the
registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
address that is used by the BootROM. This is because SPL
may return to the BootROM for boot continuation (e.g. UART
xmodem boot mode).

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Luka Perkov <luka.perkov@sartura.hr>
Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
---
 drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 758f390..27c4288 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -581,6 +581,12 @@ const char *dev_get_uclass_name(struct udevice *dev)
 	return dev->uclass->uc_drv->name;
 }
 
+__weak fdt_addr_t platform_translate_address(void *blob, int node_offset,
+					     fdt_addr_t addr)
+{
+	return addr;
+}
+
 fdt_addr_t dev_get_addr(struct udevice *dev)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
@@ -597,22 +603,30 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
 		 * Use the full-fledged translate function for complex
 		 * bus setups.
 		 */
-		return fdt_translate_address((void *)gd->fdt_blob,
+		addr = fdt_translate_address((void *)gd->fdt_blob,
 					     dev->of_offset, reg);
+	} else {
+		/*
+		 * Use the "simple" translate function for less complex
+		 * bus setups.
+		 */
+		addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
+							dev->parent->of_offset,
+							dev->of_offset, "reg",
+							0, NULL);
+		if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
+			if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
+				addr = simple_bus_translate(dev->parent, addr);
+		}
 	}
 
 	/*
-	 * Use the "simple" translate function for less complex
-	 * bus setups.
+	 * Some platforms need a special address translation. Those
+	 * platforms (e.g. mvebu in SPL) can provide a platform specific
+	 * translation function which is called now.
 	 */
-	addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
-						dev->parent->of_offset,
-						dev->of_offset, "reg",
-						0, NULL);
-	if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
-		if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
-			addr = simple_bus_translate(dev->parent, addr);
-	}
+	addr = platform_translate_address((void *)gd->fdt_blob,
+					  dev->of_offset, addr);
 
 	return addr;
 #else
-- 
2.6.3

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

Hi,

On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
> This patch adds the additional platform_translate_address() call to
> dev_get_addr(). A weak default with a 1-to-1 translation is also
> provided. Platforms that need a special address translation can
> overwrite this function.
>
> Here the explanation, why this is needed for MVEBU:
>
> When using DM with DT address translation, this does not work
> with the standard fdt_translate_address() function on MVEBU
> in SPL. Since the DT translates to the 0xf100.0000 base
> address for the internal registers. But SPL still has the
> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
> address that is used by the BootROM. This is because SPL
> may return to the BootROM for boot continuation (e.g. UART
> xmodem boot mode).
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
> ---
>  drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)

I wonder if there is a way to handle this with device tree? I would
very much like to avoid adding weak functions and other types of
hooks. Are you saying that there are two values for 'ranges', one in
SPL and one for U-Boot proper? What actually triggers the change?

One option would be to have a ranges-spl property, or similar.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

Hi Simon,

On 27.11.2015 19:36, Simon Glass wrote:
> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>> This patch adds the additional platform_translate_address() call to
>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>> provided. Platforms that need a special address translation can
>> overwrite this function.
>>
>> Here the explanation, why this is needed for MVEBU:
>>
>> When using DM with DT address translation, this does not work
>> with the standard fdt_translate_address() function on MVEBU
>> in SPL. Since the DT translates to the 0xf100.0000 base
>> address for the internal registers. But SPL still has the
>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>> address that is used by the BootROM. This is because SPL
>> may return to the BootROM for boot continuation (e.g. UART
>> xmodem boot mode).
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>> ---
>>   drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> I wonder if there is a way to handle this with device tree? I would
> very much like to avoid adding weak functions and other types of
> hooks.

I've thought about this also for quite a bit. But couldn't come
up with a "better", less intrusive implementation for this
problem yet. 

> Are you saying that there are two values for 'ranges', one in
> SPL and one for U-Boot proper?

You can think of it as 2 values for "ranges", yes. Basically
its a difference in the upper 8 bits of all addresses of the
internal registers (e.g. UART, SDRAM controller...).

The BootROM starts with 0xd000.0000 and SPL also needs to
use this value. As SPL returns back into the BootROM in
some cases. And Linux (and other OS'es) expect 0xf100.0000
as base address. So the main U-Boot reconfigured the base
address to this value quite early.

> What actually triggers the change?

This is no change. Its just, that now SPL has added DM and DTS
support. Before this SPL-DM support this was handled by
something like this:

#if defined(CONFIG_SPL_BUILD)
#define SOC_REGS_PHY_BASE       0xd0000000
#else
#define SOC_REGS_PHY_BASE       0xf1000000
#endif
#define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
#define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
#define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
...

And now (nearly) all addresses are taken from the DT. And the
SPL vs. U-Boot proper base address difference needs to get
handled otherwise - here in the DT.
 
> One option would be to have a ranges-spl property, or similar.

Hmmm. you mean to add these "ranges-spl" properties additionally
to the normal "ranges" properties? I would really like to not
change the "ranges" in the dts files. As especially in the
MVEBU cases (Armada XP / 38x etc), the occurrences are very
high. And this would result in quite a big difference to the
"mainline Linux dts" version.

I could also add this functionality via a new Kconfig option.
Like this:

+	if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
+		addr = platform_translate_address((void *)gd->fdt_blob,
+						  dev->of_offset, addr);
+	}

So no weak default would be needed. Just let me know if you
would prefer it this way. And I'll send a v2 using this
approach.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

Hi Stefan,

On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> On 27.11.2015 19:36, Simon Glass wrote:
>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>> This patch adds the additional platform_translate_address() call to
>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>> provided. Platforms that need a special address translation can
>>> overwrite this function.
>>>
>>> Here the explanation, why this is needed for MVEBU:
>>>
>>> When using DM with DT address translation, this does not work
>>> with the standard fdt_translate_address() function on MVEBU
>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>> address for the internal registers. But SPL still has the
>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>> address that is used by the BootROM. This is because SPL
>>> may return to the BootROM for boot continuation (e.g. UART
>>> xmodem boot mode).
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>> ---
>>>   drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> I wonder if there is a way to handle this with device tree? I would
>> very much like to avoid adding weak functions and other types of
>> hooks.
>
> I've thought about this also for quite a bit. But couldn't come
> up with a "better", less intrusive implementation for this
> problem yet.
>
>> Are you saying that there are two values for 'ranges', one in
>> SPL and one for U-Boot proper?
>
> You can think of it as 2 values for "ranges", yes. Basically
> its a difference in the upper 8 bits of all addresses of the
> internal registers (e.g. UART, SDRAM controller...).
>
> The BootROM starts with 0xd000.0000 and SPL also needs to
> use this value. As SPL returns back into the BootROM in
> some cases. And Linux (and other OS'es) expect 0xf100.0000
> as base address. So the main U-Boot reconfigured the base
> address to this value quite early.
>
>> What actually triggers the change?
>
> This is no change. Its just, that now SPL has added DM and DTS
> support. Before this SPL-DM support this was handled by
> something like this:
>
> #if defined(CONFIG_SPL_BUILD)
> #define SOC_REGS_PHY_BASE       0xd0000000
> #else
> #define SOC_REGS_PHY_BASE       0xf1000000
> #endif
> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
> ...
>
> And now (nearly) all addresses are taken from the DT. And the
> SPL vs. U-Boot proper base address difference needs to get
> handled otherwise - here in the DT.

No, I mean what causes the hardware address to move? Is there a
register somewhere that it adjusted to tell the addressing to change?

>
>> One option would be to have a ranges-spl property, or similar.
>
> Hmmm. you mean to add these "ranges-spl" properties additionally
> to the normal "ranges" properties? I would really like to not
> change the "ranges" in the dts files. As especially in the
> MVEBU cases (Armada XP / 38x etc), the occurrences are very
> high. And this would result in quite a big difference to the
> "mainline Linux dts" version.

Yes I mean a new property. After all, the existing one is incorrect
for your hardware at least in some configuration.

>
> I could also add this functionality via a new Kconfig option.
> Like this:
>
> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
> +               addr = platform_translate_address((void *)gd->fdt_blob,
> +                                                 dev->of_offset, addr);
> +       }
>
> So no weak default would be needed. Just let me know if you
> would prefer it this way. And I'll send a v2 using this
> approach.

I'd like to exhaust the DT option first, as this adds another level of
complexity...the DT is supposed to describe the hardware.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

Hi Simon,

On 01.12.2015 00:17, Simon Glass wrote:
> Hi Stefan,
>
> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
>> Hi Simon,
>>
>> On 27.11.2015 19:36, Simon Glass wrote:
>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>>> This patch adds the additional platform_translate_address() call to
>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>> provided. Platforms that need a special address translation can
>>>> overwrite this function.
>>>>
>>>> Here the explanation, why this is needed for MVEBU:
>>>>
>>>> When using DM with DT address translation, this does not work
>>>> with the standard fdt_translate_address() function on MVEBU
>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>> address for the internal registers. But SPL still has the
>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>> address that is used by the BootROM. This is because SPL
>>>> may return to the BootROM for boot continuation (e.g. UART
>>>> xmodem boot mode).
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>> ---
>>>>    drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>    1 file changed, 25 insertions(+), 11 deletions(-)
>>>
>>> I wonder if there is a way to handle this with device tree? I would
>>> very much like to avoid adding weak functions and other types of
>>> hooks.
>>
>> I've thought about this also for quite a bit. But couldn't come
>> up with a "better", less intrusive implementation for this
>> problem yet.
>>
>>> Are you saying that there are two values for 'ranges', one in
>>> SPL and one for U-Boot proper?
>>
>> You can think of it as 2 values for "ranges", yes. Basically
>> its a difference in the upper 8 bits of all addresses of the
>> internal registers (e.g. UART, SDRAM controller...).
>>
>> The BootROM starts with 0xd000.0000 and SPL also needs to
>> use this value. As SPL returns back into the BootROM in
>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>> as base address. So the main U-Boot reconfigured the base
>> address to this value quite early.
>>
>>> What actually triggers the change?
>>
>> This is no change. Its just, that now SPL has added DM and DTS
>> support. Before this SPL-DM support this was handled by
>> something like this:
>>
>> #if defined(CONFIG_SPL_BUILD)
>> #define SOC_REGS_PHY_BASE       0xd0000000
>> #else
>> #define SOC_REGS_PHY_BASE       0xf1000000
>> #endif
>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
>> ...
>>
>> And now (nearly) all addresses are taken from the DT. And the
>> SPL vs. U-Boot proper base address difference needs to get
>> handled otherwise - here in the DT.
>
> No, I mean what causes the hardware address to move? Is there a
> register somewhere that it adjusted to tell the addressing to change?

Yes. U-Boot proper reconfigures this base address. Quite early
in arch_cpu_init(). Note that this change / configuration can't
be detected. So you have to know, where the internal registers
are mapped.

>>
>>> One option would be to have a ranges-spl property, or similar.
>>
>> Hmmm. you mean to add these "ranges-spl" properties additionally
>> to the normal "ranges" properties? I would really like to not
>> change the "ranges" in the dts files. As especially in the
>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>> high. And this would result in quite a big difference to the
>> "mainline Linux dts" version.
>
> Yes I mean a new property. After all, the existing one is incorrect
> for your hardware at least in some configuration.
>
>>
>> I could also add this functionality via a new Kconfig option.
>> Like this:
>>
>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>> +               addr = platform_translate_address((void *)gd->fdt_blob,
>> +                                                 dev->of_offset, addr);
>> +       }
>>
>> So no weak default would be needed. Just let me know if you
>> would prefer it this way. And I'll send a v2 using this
>> approach.
>
> I'd like to exhaust the DT option first, as this adds another level of
> complexity...the DT is supposed to describe the hardware.

I understand. But your suggestion of a new "ranges-spl" property
would result in changes to the dts files (for all MVEBU boards)
and additional support for this "ranges-spl" property in the
U-Boot code. This looks more complex than the 2 lines to the
common code I suggested above. And definitely easier to maintain.
As new MVEBU boards would always have to patch their dts files
for U-Boot.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

Hi Stefan,

On 30 November 2015 at 23:05, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
>
> On 01.12.2015 00:17, Simon Glass wrote:
>>
>> Hi Stefan,
>>
>> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 27.11.2015 19:36, Simon Glass wrote:
>>>>
>>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> This patch adds the additional platform_translate_address() call to
>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>>> provided. Platforms that need a special address translation can
>>>>> overwrite this function.
>>>>>
>>>>> Here the explanation, why this is needed for MVEBU:
>>>>>
>>>>> When using DM with DT address translation, this does not work
>>>>> with the standard fdt_translate_address() function on MVEBU
>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>>> address for the internal registers. But SPL still has the
>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>>> address that is used by the BootROM. This is because SPL
>>>>> may return to the BootROM for boot continuation (e.g. UART
>>>>> xmodem boot mode).
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>>> ---
>>>>>    drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>>    1 file changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>>
>>>> I wonder if there is a way to handle this with device tree? I would
>>>> very much like to avoid adding weak functions and other types of
>>>> hooks.
>>>
>>>
>>> I've thought about this also for quite a bit. But couldn't come
>>> up with a "better", less intrusive implementation for this
>>> problem yet.
>>>
>>>> Are you saying that there are two values for 'ranges', one in
>>>> SPL and one for U-Boot proper?
>>>
>>>
>>> You can think of it as 2 values for "ranges", yes. Basically
>>> its a difference in the upper 8 bits of all addresses of the
>>> internal registers (e.g. UART, SDRAM controller...).
>>>
>>> The BootROM starts with 0xd000.0000 and SPL also needs to
>>> use this value. As SPL returns back into the BootROM in
>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>>> as base address. So the main U-Boot reconfigured the base
>>> address to this value quite early.
>>>
>>>> What actually triggers the change?
>>>
>>>
>>> This is no change. Its just, that now SPL has added DM and DTS
>>> support. Before this SPL-DM support this was handled by
>>> something like this:
>>>
>>> #if defined(CONFIG_SPL_BUILD)
>>> #define SOC_REGS_PHY_BASE       0xd0000000
>>> #else
>>> #define SOC_REGS_PHY_BASE       0xf1000000
>>> #endif
>>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
>>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
>>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
>>> ...
>>>
>>> And now (nearly) all addresses are taken from the DT. And the
>>> SPL vs. U-Boot proper base address difference needs to get
>>> handled otherwise - here in the DT.
>>
>>
>> No, I mean what causes the hardware address to move? Is there a
>> register somewhere that it adjusted to tell the addressing to change?
>
>
> Yes. U-Boot proper reconfigures this base address. Quite early
> in arch_cpu_init(). Note that this change / configuration can't
> be detected. So you have to know, where the internal registers
> are mapped.
>
>>>
>>>> One option would be to have a ranges-spl property, or similar.
>>>
>>>
>>> Hmmm. you mean to add these "ranges-spl" properties additionally
>>> to the normal "ranges" properties? I would really like to not
>>> change the "ranges" in the dts files. As especially in the
>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>>> high. And this would result in quite a big difference to the
>>> "mainline Linux dts" version.
>>
>>
>> Yes I mean a new property. After all, the existing one is incorrect
>> for your hardware at least in some configuration.
>>
>>>
>>> I could also add this functionality via a new Kconfig option.
>>> Like this:
>>>
>>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>>> +               addr = platform_translate_address((void *)gd->fdt_blob,
>>> +                                                 dev->of_offset, addr);
>>> +       }
>>>
>>> So no weak default would be needed. Just let me know if you
>>> would prefer it this way. And I'll send a v2 using this
>>> approach.
>>
>>
>> I'd like to exhaust the DT option first, as this adds another level of
>> complexity...the DT is supposed to describe the hardware.
>
>
> I understand. But your suggestion of a new "ranges-spl" property
> would result in changes to the dts files (for all MVEBU boards)
> and additional support for this "ranges-spl" property in the
> U-Boot code. This looks more complex than the 2 lines to the
> common code I suggested above. And definitely easier to maintain.
> As new MVEBU boards would always have to patch their dts files
> for U-Boot.

But from another perspective, the dts file is clearly wrong IMO - it
does not describe the memory of the system fully. It is only accurate
once you 'flip the bit' to change the address space.

This creates a callback to board-specific code. That's something I'm
really trying to avoid with driver model. We should not have drivers
calling into board code for things.

Other options, none of which I am recommending, just trying to
suggestion options we can discuss:

- Describe the mapping somewhere else - e.g. in a /config node
property like 'u-boot,range-offset'
- Add some sort of 'core' driver model address translation setting,
which your board code can set up with a function call, and
dev_get_addr() uses
- Add a uclass and driver for address translation, and call it from
here (ugh...)

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

Hi Simon,

On 01.12.2015 21:02, Simon Glass wrote:
> Hi Stefan,
> 
> On 30 November 2015 at 23:05, Stefan Roese <sr@denx.de> wrote:
>> Hi Simon,
>>
>>
>> On 01.12.2015 00:17, Simon Glass wrote:
>>>
>>> Hi Stefan,
>>>
>>> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 27.11.2015 19:36, Simon Glass wrote:
>>>>>
>>>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> This patch adds the additional platform_translate_address() call to
>>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>>>> provided. Platforms that need a special address translation can
>>>>>> overwrite this function.
>>>>>>
>>>>>> Here the explanation, why this is needed for MVEBU:
>>>>>>
>>>>>> When using DM with DT address translation, this does not work
>>>>>> with the standard fdt_translate_address() function on MVEBU
>>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>>>> address for the internal registers. But SPL still has the
>>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>>>> address that is used by the BootROM. This is because SPL
>>>>>> may return to the BootROM for boot continuation (e.g. UART
>>>>>> xmodem boot mode).
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>>>> ---
>>>>>>     drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>>>     1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>
>>>>>
>>>>> I wonder if there is a way to handle this with device tree? I would
>>>>> very much like to avoid adding weak functions and other types of
>>>>> hooks.
>>>>
>>>>
>>>> I've thought about this also for quite a bit. But couldn't come
>>>> up with a "better", less intrusive implementation for this
>>>> problem yet.
>>>>
>>>>> Are you saying that there are two values for 'ranges', one in
>>>>> SPL and one for U-Boot proper?
>>>>
>>>>
>>>> You can think of it as 2 values for "ranges", yes. Basically
>>>> its a difference in the upper 8 bits of all addresses of the
>>>> internal registers (e.g. UART, SDRAM controller...).
>>>>
>>>> The BootROM starts with 0xd000.0000 and SPL also needs to
>>>> use this value. As SPL returns back into the BootROM in
>>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>>>> as base address. So the main U-Boot reconfigured the base
>>>> address to this value quite early.
>>>>
>>>>> What actually triggers the change?
>>>>
>>>>
>>>> This is no change. Its just, that now SPL has added DM and DTS
>>>> support. Before this SPL-DM support this was handled by
>>>> something like this:
>>>>
>>>> #if defined(CONFIG_SPL_BUILD)
>>>> #define SOC_REGS_PHY_BASE       0xd0000000
>>>> #else
>>>> #define SOC_REGS_PHY_BASE       0xf1000000
>>>> #endif
>>>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
>>>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
>>>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
>>>> ...
>>>>
>>>> And now (nearly) all addresses are taken from the DT. And the
>>>> SPL vs. U-Boot proper base address difference needs to get
>>>> handled otherwise - here in the DT.
>>>
>>>
>>> No, I mean what causes the hardware address to move? Is there a
>>> register somewhere that it adjusted to tell the addressing to change?
>>
>>
>> Yes. U-Boot proper reconfigures this base address. Quite early
>> in arch_cpu_init(). Note that this change / configuration can't
>> be detected. So you have to know, where the internal registers
>> are mapped.
>>
>>>>
>>>>> One option would be to have a ranges-spl property, or similar.
>>>>
>>>>
>>>> Hmmm. you mean to add these "ranges-spl" properties additionally
>>>> to the normal "ranges" properties? I would really like to not
>>>> change the "ranges" in the dts files. As especially in the
>>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>>>> high. And this would result in quite a big difference to the
>>>> "mainline Linux dts" version.
>>>
>>>
>>> Yes I mean a new property. After all, the existing one is incorrect
>>> for your hardware at least in some configuration.
>>>
>>>>
>>>> I could also add this functionality via a new Kconfig option.
>>>> Like this:
>>>>
>>>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>>>> +               addr = platform_translate_address((void *)gd->fdt_blob,
>>>> +                                                 dev->of_offset, addr);
>>>> +       }
>>>>
>>>> So no weak default would be needed. Just let me know if you
>>>> would prefer it this way. And I'll send a v2 using this
>>>> approach.
>>>
>>>
>>> I'd like to exhaust the DT option first, as this adds another level of
>>> complexity...the DT is supposed to describe the hardware.
>>
>>
>> I understand. But your suggestion of a new "ranges-spl" property
>> would result in changes to the dts files (for all MVEBU boards)
>> and additional support for this "ranges-spl" property in the
>> U-Boot code. This looks more complex than the 2 lines to the
>> common code I suggested above. And definitely easier to maintain.
>> As new MVEBU boards would always have to patch their dts files
>> for U-Boot.
> 
> But from another perspective, the dts file is clearly wrong IMO - it
> does not describe the memory of the system fully. It is only accurate
> once you 'flip the bit' to change the address space.
> 
> This creates a callback to board-specific code. That's something I'm
> really trying to avoid with driver model. We should not have drivers
> calling into board code for things.
> 
> Other options, none of which I am recommending, just trying to
> suggestion options we can discuss:
> 
> - Describe the mapping somewhere else - e.g. in a /config node
> property like 'u-boot,range-offset'

I've started with a test version for this alternative. But this
also seems a bit clumsy, as I now need to conditionally use
this 'u-boot,range-offset' in get_dev_addr() - depending on
CONFIG_SPL_BUILD.

> - Add some sort of 'core' driver model address translation setting,
> which your board code can set up with a function call, and
> dev_get_addr() uses
> - Add a uclass and driver for address translation, and call it from
> here (ugh...)

All this doesn't sound very "promising". At least not to me. But
I had another idea, which might be a good alternative. Use a new,
different dts for SPL. This has most likely been discussed before,
not sure. The idea here is, to re-use the existing dts and include
it in the new dts. Something like:

U-Boot proper: armada-xp-gp.dts
SPL U-Boot:    armada-xp-gp-spl.dts

And this new SPL dts includes the original dts and only changes
what needs to get changed for SPL. This could include all the
"u-boot,dm-pre-reloc" properties as well and look like this:

---<-----------------------
/*
 * Device Tree file for Marvell Armada XP development board
 * (DB-MV784MP-GP)
 */

#include "armada-xp-gp.dts"

/ {
	soc {
		/*
		 * Use 0xd0000000 as base address for the internal registers
		 * in SPL U-Boot
		 */
		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
		u-boot,dm-pre-reloc;

		internal-regs {
			u-boot,dm-pre-reloc;

			serial at 12000 {
				u-boot,dm-pre-reloc;
			};
		};
	};
};
---<-----------------------

Unfortunately this approach does not work right now. The dtc
seems to not overlay the new values in the resulting dtb.

But has this approach been discussed before? Or if not, what do
you think of it (if and once it really works)?

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

Hi Stefan,

On 2 December 2015 at 07:11, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> On 01.12.2015 21:02, Simon Glass wrote:
>> Hi Stefan,
>>
>> On 30 November 2015 at 23:05, Stefan Roese <sr@denx.de> wrote:
>>> Hi Simon,
>>>
>>>
>>> On 01.12.2015 00:17, Simon Glass wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 27.11.2015 19:36, Simon Glass wrote:
>>>>>>
>>>>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>> This patch adds the additional platform_translate_address() call to
>>>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>>>>> provided. Platforms that need a special address translation can
>>>>>>> overwrite this function.
>>>>>>>
>>>>>>> Here the explanation, why this is needed for MVEBU:
>>>>>>>
>>>>>>> When using DM with DT address translation, this does not work
>>>>>>> with the standard fdt_translate_address() function on MVEBU
>>>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>>>>> address for the internal registers. But SPL still has the
>>>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>>>>> address that is used by the BootROM. This is because SPL
>>>>>>> may return to the BootROM for boot continuation (e.g. UART
>>>>>>> xmodem boot mode).
>>>>>>>
>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>>>>> ---
>>>>>>>     drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>>>>     1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>>
>>>>>>
>>>>>> I wonder if there is a way to handle this with device tree? I would
>>>>>> very much like to avoid adding weak functions and other types of
>>>>>> hooks.
>>>>>
>>>>>
>>>>> I've thought about this also for quite a bit. But couldn't come
>>>>> up with a "better", less intrusive implementation for this
>>>>> problem yet.
>>>>>
>>>>>> Are you saying that there are two values for 'ranges', one in
>>>>>> SPL and one for U-Boot proper?
>>>>>
>>>>>
>>>>> You can think of it as 2 values for "ranges", yes. Basically
>>>>> its a difference in the upper 8 bits of all addresses of the
>>>>> internal registers (e.g. UART, SDRAM controller...).
>>>>>
>>>>> The BootROM starts with 0xd000.0000 and SPL also needs to
>>>>> use this value. As SPL returns back into the BootROM in
>>>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>>>>> as base address. So the main U-Boot reconfigured the base
>>>>> address to this value quite early.
>>>>>
>>>>>> What actually triggers the change?
>>>>>
>>>>>
>>>>> This is no change. Its just, that now SPL has added DM and DTS
>>>>> support. Before this SPL-DM support this was handled by
>>>>> something like this:
>>>>>
>>>>> #if defined(CONFIG_SPL_BUILD)
>>>>> #define SOC_REGS_PHY_BASE       0xd0000000
>>>>> #else
>>>>> #define SOC_REGS_PHY_BASE       0xf1000000
>>>>> #endif
>>>>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
>>>>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
>>>>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
>>>>> ...
>>>>>
>>>>> And now (nearly) all addresses are taken from the DT. And the
>>>>> SPL vs. U-Boot proper base address difference needs to get
>>>>> handled otherwise - here in the DT.
>>>>
>>>>
>>>> No, I mean what causes the hardware address to move? Is there a
>>>> register somewhere that it adjusted to tell the addressing to change?
>>>
>>>
>>> Yes. U-Boot proper reconfigures this base address. Quite early
>>> in arch_cpu_init(). Note that this change / configuration can't
>>> be detected. So you have to know, where the internal registers
>>> are mapped.
>>>
>>>>>
>>>>>> One option would be to have a ranges-spl property, or similar.
>>>>>
>>>>>
>>>>> Hmmm. you mean to add these "ranges-spl" properties additionally
>>>>> to the normal "ranges" properties? I would really like to not
>>>>> change the "ranges" in the dts files. As especially in the
>>>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>>>>> high. And this would result in quite a big difference to the
>>>>> "mainline Linux dts" version.
>>>>
>>>>
>>>> Yes I mean a new property. After all, the existing one is incorrect
>>>> for your hardware at least in some configuration.
>>>>
>>>>>
>>>>> I could also add this functionality via a new Kconfig option.
>>>>> Like this:
>>>>>
>>>>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>>>>> +               addr = platform_translate_address((void *)gd->fdt_blob,
>>>>> +                                                 dev->of_offset, addr);
>>>>> +       }
>>>>>
>>>>> So no weak default would be needed. Just let me know if you
>>>>> would prefer it this way. And I'll send a v2 using this
>>>>> approach.
>>>>
>>>>
>>>> I'd like to exhaust the DT option first, as this adds another level of
>>>> complexity...the DT is supposed to describe the hardware.
>>>
>>>
>>> I understand. But your suggestion of a new "ranges-spl" property
>>> would result in changes to the dts files (for all MVEBU boards)
>>> and additional support for this "ranges-spl" property in the
>>> U-Boot code. This looks more complex than the 2 lines to the
>>> common code I suggested above. And definitely easier to maintain.
>>> As new MVEBU boards would always have to patch their dts files
>>> for U-Boot.
>>
>> But from another perspective, the dts file is clearly wrong IMO - it
>> does not describe the memory of the system fully. It is only accurate
>> once you 'flip the bit' to change the address space.
>>
>> This creates a callback to board-specific code. That's something I'm
>> really trying to avoid with driver model. We should not have drivers
>> calling into board code for things.
>>
>> Other options, none of which I am recommending, just trying to
>> suggestion options we can discuss:
>>
>> - Describe the mapping somewhere else - e.g. in a /config node
>> property like 'u-boot,range-offset'
>
> I've started with a test version for this alternative. But this
> also seems a bit clumsy, as I now need to conditionally use
> this 'u-boot,range-offset' in get_dev_addr() - depending on
> CONFIG_SPL_BUILD.

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. I prefer adding a
new ranges property anyway.

>
>> - Add some sort of 'core' driver model address translation setting,
>> which your board code can set up with a function call, and
>> dev_get_addr() uses
>> - Add a uclass and driver for address translation, and call it from
>> here (ugh...)
>
> All this doesn't sound very "promising". At least not to me. But
> I had another idea, which might be a good alternative. Use a new,
> different dts for SPL. This has most likely been discussed before,
> not sure. The idea here is, to re-use the existing dts and include
> it in the new dts. Something like:
>
> U-Boot proper: armada-xp-gp.dts
> SPL U-Boot:    armada-xp-gp-spl.dts
>
> And this new SPL dts includes the original dts and only changes
> what needs to get changed for SPL. This could include all the
> "u-boot,dm-pre-reloc" properties as well and look like this:
>
> ---<-----------------------
> /*
>  * Device Tree file for Marvell Armada XP development board
>  * (DB-MV784MP-GP)
>  */
>
> #include "armada-xp-gp.dts"
>
> / {
>         soc {
>                 /*
>                  * Use 0xd0000000 as base address for the internal registers
>                  * in SPL U-Boot
>                  */
>                 ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
>                           MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
>                           MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
>                 u-boot,dm-pre-reloc;
>
>                 internal-regs {
>                         u-boot,dm-pre-reloc;
>
>                         serial at 12000 {
>                                 u-boot,dm-pre-reloc;
>                         };
>                 };
>         };
> };
> ---<-----------------------
>
> Unfortunately this approach does not work right now. The dtc
> seems to not overlay the new values in the resulting dtb.
>
> But has this approach been discussed before? Or if not, what do
> you think of it (if and once it really works)?

The dtc should overlay the values - we depend on this bahaviour in
various places. But to me this approach seems a bit clumsy.

I can't help thinking we are missing something here. The DT is
supposed to represent the hardware, and here we have some hardware
which is not being represented correctly. Your complaint that you will
need to change all the files to add a ranges-initial property (or
whatever) doesn't really seem like a problem to me - you only do it
once. We can have a setting as to which property to use, and the
platform can change it when needed. It can fall back to 'ranges' if
there is no ranges-initial property. I used a similar approach for
memory on pit - definitions for both SRAM and SDRAM, and allowing
selection of which one to use. There is nothing in DT that prevents us
from handling this sort of thing.

Perhaps you could ask about this on the device-tree-discuss list? If
you like I can start a thread.

Also, just to get this off my chest, I would like to expand a bit more
on why I don't think we should call board code from drivers. The
drivers are supposed to be hermetic, and work on any platform. The
information needed by the driver to work should come from data, not
code. Then we can represent it in the device tree, or in platform data
(to which device tree is often converted). If we have the drivers
calling out to code to get their information, then it adds more
coupling between the drivers and the platforms. At present we can drop
a driver in by enabling it in Kconfig and adding a device tree node.
We don't need to modify the board code at all. I think that is worth
protecting. It creates a nice clear wall between the driver and the
board code, conceptually free of hidden interactions, etc. It is much
easier to scale to larger and more complex platforms with this in
place.

For similar reasons I think we should avoid calling drivers from other
drivers - this should always be done through the driver's API (e.g.
the uclass header file). That way we don't have (e.g.) a SPI flash
driver with a compile- or link-time dependency on a particular SPI
driver. It may depend on features that the SPI driver may or may not
provide, but in principle any SPI driver can provide those features,
and if they are publicly declared in the uclass API it is clear what
these features are.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

Hi Simon,

On 02.12.2015 15:49, Simon Glass wrote:
> Hi Stefan,
> 
> On 2 December 2015 at 07:11, Stefan Roese <sr@denx.de> wrote:
>> Hi Simon,
>>
>> On 01.12.2015 21:02, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On 30 November 2015 at 23:05, Stefan Roese <sr@denx.de> wrote:
>>>> Hi Simon,
>>>>
>>>>
>>>> On 01.12.2015 00:17, Simon Glass wrote:
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 27.11.2015 19:36, Simon Glass wrote:
>>>>>>>
>>>>>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>
>>>>>>>> This patch adds the additional platform_translate_address() call to
>>>>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>>>>>> provided. Platforms that need a special address translation can
>>>>>>>> overwrite this function.
>>>>>>>>
>>>>>>>> Here the explanation, why this is needed for MVEBU:
>>>>>>>>
>>>>>>>> When using DM with DT address translation, this does not work
>>>>>>>> with the standard fdt_translate_address() function on MVEBU
>>>>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>>>>>> address for the internal registers. But SPL still has the
>>>>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>>>>>> address that is used by the BootROM. This is because SPL
>>>>>>>> may return to the BootROM for boot continuation (e.g. UART
>>>>>>>> xmodem boot mode).
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>>>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>>>>>> ---
>>>>>>>>      drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>>>>>      1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>>
>>>>>>> I wonder if there is a way to handle this with device tree? I would
>>>>>>> very much like to avoid adding weak functions and other types of
>>>>>>> hooks.
>>>>>>
>>>>>>
>>>>>> I've thought about this also for quite a bit. But couldn't come
>>>>>> up with a "better", less intrusive implementation for this
>>>>>> problem yet.
>>>>>>
>>>>>>> Are you saying that there are two values for 'ranges', one in
>>>>>>> SPL and one for U-Boot proper?
>>>>>>
>>>>>>
>>>>>> You can think of it as 2 values for "ranges", yes. Basically
>>>>>> its a difference in the upper 8 bits of all addresses of the
>>>>>> internal registers (e.g. UART, SDRAM controller...).
>>>>>>
>>>>>> The BootROM starts with 0xd000.0000 and SPL also needs to
>>>>>> use this value. As SPL returns back into the BootROM in
>>>>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>>>>>> as base address. So the main U-Boot reconfigured the base
>>>>>> address to this value quite early.
>>>>>>
>>>>>>> What actually triggers the change?
>>>>>>
>>>>>>
>>>>>> This is no change. Its just, that now SPL has added DM and DTS
>>>>>> support. Before this SPL-DM support this was handled by
>>>>>> something like this:
>>>>>>
>>>>>> #if defined(CONFIG_SPL_BUILD)
>>>>>> #define SOC_REGS_PHY_BASE       0xd0000000
>>>>>> #else
>>>>>> #define SOC_REGS_PHY_BASE       0xf1000000
>>>>>> #endif
>>>>>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
>>>>>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
>>>>>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
>>>>>> ...
>>>>>>
>>>>>> And now (nearly) all addresses are taken from the DT. And the
>>>>>> SPL vs. U-Boot proper base address difference needs to get
>>>>>> handled otherwise - here in the DT.
>>>>>
>>>>>
>>>>> No, I mean what causes the hardware address to move? Is there a
>>>>> register somewhere that it adjusted to tell the addressing to change?
>>>>
>>>>
>>>> Yes. U-Boot proper reconfigures this base address. Quite early
>>>> in arch_cpu_init(). Note that this change / configuration can't
>>>> be detected. So you have to know, where the internal registers
>>>> are mapped.
>>>>
>>>>>>
>>>>>>> One option would be to have a ranges-spl property, or similar.
>>>>>>
>>>>>>
>>>>>> Hmmm. you mean to add these "ranges-spl" properties additionally
>>>>>> to the normal "ranges" properties? I would really like to not
>>>>>> change the "ranges" in the dts files. As especially in the
>>>>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>>>>>> high. And this would result in quite a big difference to the
>>>>>> "mainline Linux dts" version.
>>>>>
>>>>>
>>>>> Yes I mean a new property. After all, the existing one is incorrect
>>>>> for your hardware at least in some configuration.
>>>>>
>>>>>>
>>>>>> I could also add this functionality via a new Kconfig option.
>>>>>> Like this:
>>>>>>
>>>>>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>>>>>> +               addr = platform_translate_address((void *)gd->fdt_blob,
>>>>>> +                                                 dev->of_offset, addr);
>>>>>> +       }
>>>>>>
>>>>>> So no weak default would be needed. Just let me know if you
>>>>>> would prefer it this way. And I'll send a v2 using this
>>>>>> approach.
>>>>>
>>>>>
>>>>> I'd like to exhaust the DT option first, as this adds another level of
>>>>> complexity...the DT is supposed to describe the hardware.
>>>>
>>>>
>>>> I understand. But your suggestion of a new "ranges-spl" property
>>>> would result in changes to the dts files (for all MVEBU boards)
>>>> and additional support for this "ranges-spl" property in the
>>>> U-Boot code. This looks more complex than the 2 lines to the
>>>> common code I suggested above. And definitely easier to maintain.
>>>> As new MVEBU boards would always have to patch their dts files
>>>> for U-Boot.
>>>
>>> But from another perspective, the dts file is clearly wrong IMO - it
>>> does not describe the memory of the system fully. It is only accurate
>>> once you 'flip the bit' to change the address space.
>>>
>>> This creates a callback to board-specific code. That's something I'm
>>> really trying to avoid with driver model. We should not have drivers
>>> calling into board code for things.
>>>
>>> Other options, none of which I am recommending, just trying to
>>> suggestion options we can discuss:
>>>
>>> - Describe the mapping somewhere else - e.g. in a /config node
>>> property like 'u-boot,range-offset'
>>
>> I've started with a test version for this alternative. But this
>> also seems a bit clumsy, as I now need to conditionally use
>> this 'u-boot,range-offset' in get_dev_addr() - depending on
>> CONFIG_SPL_BUILD.
> 
> 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:

 ...
 * Note: this Device Tree assumes that the bootloader has remapped the
 * internal registers to 0xf1000000 (instead of the default
 * 0xd0000000). The 0xf1000000 is the default used by the recent,
 * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
 * boards were delivered with an older version of the bootloader that
 * left internal registers mapped at 0xd0000000. If you are in this
 * situation, you should either update your bootloader (preferred
 * solution) or the below Device Tree should be adjusted.
 ...

It really boils down to a compile-time option for these platforms,
to "detect" where the internal registers are located  (SPL or not).

> I prefer adding a
> new ranges property anyway.

So please bear with me, and explain (again?) how exactly this new
ranges property could solve this problem. Keeping the undetectable
nature of this address change in mind. And without adding some
more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your
remark to your similar "pit" solution).

>>
>>> - Add some sort of 'core' driver model address translation setting,
>>> which your board code can set up with a function call, and
>>> dev_get_addr() uses
>>> - Add a uclass and driver for address translation, and call it from
>>> here (ugh...)
>>
>> All this doesn't sound very "promising". At least not to me. But
>> I had another idea, which might be a good alternative. Use a new,
>> different dts for SPL. This has most likely been discussed before,
>> not sure. The idea here is, to re-use the existing dts and include
>> it in the new dts. Something like:
>>
>> U-Boot proper: armada-xp-gp.dts
>> SPL U-Boot:    armada-xp-gp-spl.dts
>>
>> And this new SPL dts includes the original dts and only changes
>> what needs to get changed for SPL. This could include all the
>> "u-boot,dm-pre-reloc" properties as well and look like this:
>>
>> ---<-----------------------
>> /*
>>   * Device Tree file for Marvell Armada XP development board
>>   * (DB-MV784MP-GP)
>>   */
>>
>> #include "armada-xp-gp.dts"
>>
>> / {
>>          soc {
>>                  /*
>>                   * Use 0xd0000000 as base address for the internal registers
>>                   * in SPL U-Boot
>>                   */
>>                  ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
>>                            MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
>>                            MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
>>                  u-boot,dm-pre-reloc;
>>
>>                  internal-regs {
>>                          u-boot,dm-pre-reloc;
>>
>>                          serial at 12000 {
>>                                  u-boot,dm-pre-reloc;
>>                          };
>>                  };
>>          };
>> };
>> ---<-----------------------
>>
>> Unfortunately this approach does not work right now. The dtc
>> seems to not overlay the new values in the resulting dtb.
>>
>> But has this approach been discussed before? Or if not, what do
>> you think of it (if and once it really works)?
> 
> The dtc should overlay the values - we depend on this bahaviour in
> various places.

Right, I know. But it just doesn't work here right now. I've
unsuccessfully tested it.

> But to me this approach seems a bit clumsy.

I don't find this approach clumsy. Just my personal feeling. But
using this approach we could abstract all the U-Boot properties
and nodes into a separate file. And don't have to make any
changes to original dtsi / dts files.

Of course it would be even better, if all of these U-Boot additions
would be accepted to the main DeviceTree source. But this does
not seem to happen, unfortunately.
 
> I can't help thinking we are missing something here. The DT is
> supposed to represent the hardware, and here we have some hardware
> which is not being represented correctly. Your complaint that you will
> need to change all the files to add a ranges-initial property (or
> whatever) doesn't really seem like a problem to me - you only do it
> once.

Not really. We need to change this at least for every now SoC
having this problem. But yes, this should not too much trouble.

> We can have a setting as to which property to use, and the
> platform can change it when needed. It can fall back to 'ranges' if
> there is no ranges-initial property. I used a similar approach for
> memory on pit - definitions for both SRAM and SDRAM, and allowing
> selection of which one to use. There is nothing in DT that prevents us
> from handling this sort of thing.

Again, how does this work exactly? Is there some code for "pit"
that I could take as an example?
 
> Perhaps you could ask about this on the device-tree-discuss list? If
> you like I can start a thread.
> 
> Also, just to get this off my chest, I would like to expand a bit more
> on why I don't think we should call board code from drivers. The
> drivers are supposed to be hermetic, and work on any platform. The
> information needed by the driver to work should come from data, not
> code. Then we can represent it in the device tree, or in platform data
> (to which device tree is often converted). If we have the drivers
> calling out to code to get their information, then it adds more
> coupling between the drivers and the platforms. At present we can drop
> a driver in by enabling it in Kconfig and adding a device tree node.
> We don't need to modify the board code at all. I think that is worth
> protecting. It creates a nice clear wall between the driver and the
> board code, conceptually free of hidden interactions, etc. It is much
> easier to scale to larger and more complex platforms with this in
> place.
> 
> For similar reasons I think we should avoid calling drivers from other
> drivers - this should always be done through the driver's API (e.g.
> the uclass header file). That way we don't have (e.g.) a SPI flash
> driver with a compile- or link-time dependency on a particular SPI
> driver. It may depend on features that the SPI driver may or may not
> provide, but in principle any SPI driver can provide those features,
> and if they are publicly declared in the uclass API it is clear what
> these features are.

Thanks for the detailed explanation for your reasoning. I understand
your point of view.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

Hi Stefan,

On 2 December 2015 at 08:24, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 02.12.2015 15:49, Simon Glass wrote:
> > Hi Stefan,
> >
> > On 2 December 2015 at 07:11, Stefan Roese <sr@denx.de> wrote:
> >> Hi Simon,
> >>
> >> On 01.12.2015 21:02, Simon Glass wrote:
> >>> Hi Stefan,
> >>>
> >>> On 30 November 2015 at 23:05, Stefan Roese <sr@denx.de> wrote:
> >>>> Hi Simon,
> >>>>
> >>>>
> >>>> On 01.12.2015 00:17, Simon Glass wrote:
> >>>>>
> >>>>> Hi Stefan,
> >>>>>
> >>>>> On 29 November 2015 at 23:52, Stefan Roese <sr@denx.de> wrote:
> >>>>>>
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 27.11.2015 19:36, Simon Glass wrote:
> >>>>>>>
> >>>>>>> On 27 November 2015 at 02:22, Stefan Roese <sr@denx.de> wrote:
> >>>>>>>>
> >>>>>>>> This patch adds the additional platform_translate_address() call to
> >>>>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
> >>>>>>>> provided. Platforms that need a special address translation can
> >>>>>>>> overwrite this function.
> >>>>>>>>
> >>>>>>>> Here the explanation, why this is needed for MVEBU:
> >>>>>>>>
> >>>>>>>> When using DM with DT address translation, this does not work
> >>>>>>>> with the standard fdt_translate_address() function on MVEBU
> >>>>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
> >>>>>>>> address for the internal registers. But SPL still has the
> >>>>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
> >>>>>>>> address that is used by the BootROM. This is because SPL
> >>>>>>>> may return to the BootROM for boot continuation (e.g. UART
> >>>>>>>> xmodem boot mode).
> >>>>>>>>
> >>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
> >>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
> >>>>>>>> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
> >>>>>>>> ---
> >>>>>>>>      drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
> >>>>>>>>      1 file changed, 25 insertions(+), 11 deletions(-)
> >>>>>>>
> >>>>>>>
> >>>>>>> I wonder if there is a way to handle this with device tree? I would
> >>>>>>> very much like to avoid adding weak functions and other types of
> >>>>>>> hooks.
> >>>>>>
> >>>>>>
> >>>>>> I've thought about this also for quite a bit. But couldn't come
> >>>>>> up with a "better", less intrusive implementation for this
> >>>>>> problem yet.
> >>>>>>
> >>>>>>> Are you saying that there are two values for 'ranges', one in
> >>>>>>> SPL and one for U-Boot proper?
> >>>>>>
> >>>>>>
> >>>>>> You can think of it as 2 values for "ranges", yes. Basically
> >>>>>> its a difference in the upper 8 bits of all addresses of the
> >>>>>> internal registers (e.g. UART, SDRAM controller...).
> >>>>>>
> >>>>>> The BootROM starts with 0xd000.0000 and SPL also needs to
> >>>>>> use this value. As SPL returns back into the BootROM in
> >>>>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
> >>>>>> as base address. So the main U-Boot reconfigured the base
> >>>>>> address to this value quite early.
> >>>>>>
> >>>>>>> What actually triggers the change?
> >>>>>>
> >>>>>>
> >>>>>> This is no change. Its just, that now SPL has added DM and DTS
> >>>>>> support. Before this SPL-DM support this was handled by
> >>>>>> something like this:
> >>>>>>
> >>>>>> #if defined(CONFIG_SPL_BUILD)
> >>>>>> #define SOC_REGS_PHY_BASE       0xd0000000
> >>>>>> #else
> >>>>>> #define SOC_REGS_PHY_BASE       0xf1000000
> >>>>>> #endif
> >>>>>> #define MVEBU_REGISTER(x)       (SOC_REGS_PHY_BASE + x)
> >>>>>> #define MVEBU_SDRAM_SCRATCH     (MVEBU_REGISTER(0x01504))
> >>>>>> #define MVEBU_L2_CACHE_BASE     (MVEBU_REGISTER(0x08000))
> >>>>>> ...
> >>>>>>
> >>>>>> And now (nearly) all addresses are taken from the DT. And the
> >>>>>> SPL vs. U-Boot proper base address difference needs to get
> >>>>>> handled otherwise - here in the DT.
> >>>>>
> >>>>>
> >>>>> No, I mean what causes the hardware address to move? Is there a
> >>>>> register somewhere that it adjusted to tell the addressing to change?
> >>>>
> >>>>
> >>>> Yes. U-Boot proper reconfigures this base address. Quite early
> >>>> in arch_cpu_init(). Note that this change / configuration can't
> >>>> be detected. So you have to know, where the internal registers
> >>>> are mapped.
> >>>>
> >>>>>>
> >>>>>>> One option would be to have a ranges-spl property, or similar.
> >>>>>>
> >>>>>>
> >>>>>> Hmmm. you mean to add these "ranges-spl" properties additionally
> >>>>>> to the normal "ranges" properties? I would really like to not
> >>>>>> change the "ranges" in the dts files. As especially in the
> >>>>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
> >>>>>> high. And this would result in quite a big difference to the
> >>>>>> "mainline Linux dts" version.
> >>>>>
> >>>>>
> >>>>> Yes I mean a new property. After all, the existing one is incorrect
> >>>>> for your hardware at least in some configuration.
> >>>>>
> >>>>>>
> >>>>>> I could also add this functionality via a new Kconfig option.
> >>>>>> Like this:
> >>>>>>
> >>>>>> +       if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
> >>>>>> +               addr = platform_translate_address((void *)gd->fdt_blob,
> >>>>>> +                                                 dev->of_offset, addr);
> >>>>>> +       }
> >>>>>>
> >>>>>> So no weak default would be needed. Just let me know if you
> >>>>>> would prefer it this way. And I'll send a v2 using this
> >>>>>> approach.
> >>>>>
> >>>>>
> >>>>> I'd like to exhaust the DT option first, as this adds another level of
> >>>>> complexity...the DT is supposed to describe the hardware.
> >>>>
> >>>>
> >>>> I understand. But your suggestion of a new "ranges-spl" property
> >>>> would result in changes to the dts files (for all MVEBU boards)
> >>>> and additional support for this "ranges-spl" property in the
> >>>> U-Boot code. This looks more complex than the 2 lines to the
> >>>> common code I suggested above. And definitely easier to maintain.
> >>>> As new MVEBU boards would always have to patch their dts files
> >>>> for U-Boot.
> >>>
> >>> But from another perspective, the dts file is clearly wrong IMO - it
> >>> does not describe the memory of the system fully. It is only accurate
> >>> once you 'flip the bit' to change the address space.
> >>>
> >>> This creates a callback to board-specific code. That's something I'm
> >>> really trying to avoid with driver model. We should not have drivers
> >>> calling into board code for things.
> >>>
> >>> Other options, none of which I am recommending, just trying to
> >>> suggestion options we can discuss:
> >>>
> >>> - Describe the mapping somewhere else - e.g. in a /config node
> >>> property like 'u-boot,range-offset'
> >>
> >> I've started with a test version for this alternative. But this
> >> also seems a bit clumsy, as I now need to conditionally use
> >> this 'u-boot,range-offset' in get_dev_addr() - depending on
> >> CONFIG_SPL_BUILD.
> >
> > 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.

>
>  ...
>  * Note: this Device Tree assumes that the bootloader has remapped the
>  * internal registers to 0xf1000000 (instead of the default
>  * 0xd0000000). The 0xf1000000 is the default used by the recent,
>  * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
>  * boards were delivered with an older version of the bootloader that
>  * left internal registers mapped at 0xd0000000. If you are in this
>  * situation, you should either update your bootloader (preferred
>  * solution) or the below Device Tree should be adjusted.
>  ...
>
> It really boils down to a compile-time option for these platforms,
> to "detect" where the internal registers are located  (SPL or not).

Yes.

>
> > I prefer adding a
> > new ranges property anyway.
>
> So please bear with me, and explain (again?) how exactly this new
> ranges property could solve this problem. Keeping the undetectable
> nature of this address change in mind. And without adding some
> more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your
> remark to your similar "pit" solution).

We need to get to the bottom of this 'detection' thing first (as
above). But if the only way of detecting is 'am I in SPL or not' then
you can always have some code that selects the range based on that.

>
> >>
> >>> - Add some sort of 'core' driver model address translation setting,
> >>> which your board code can set up with a function call, and
> >>> dev_get_addr() uses
> >>> - Add a uclass and driver for address translation, and call it from
> >>> here (ugh...)
> >>
> >> All this doesn't sound very "promising". At least not to me. But
> >> I had another idea, which might be a good alternative. Use a new,
> >> different dts for SPL. This has most likely been discussed before,
> >> not sure. The idea here is, to re-use the existing dts and include
> >> it in the new dts. Something like:
> >>
> >> U-Boot proper: armada-xp-gp.dts
> >> SPL U-Boot:    armada-xp-gp-spl.dts
> >>
> >> And this new SPL dts includes the original dts and only changes
> >> what needs to get changed for SPL. This could include all the
> >> "u-boot,dm-pre-reloc" properties as well and look like this:
> >>
> >> ---<-----------------------
> >> /*
> >>   * Device Tree file for Marvell Armada XP development board
> >>   * (DB-MV784MP-GP)
> >>   */
> >>
> >> #include "armada-xp-gp.dts"
> >>
> >> / {
> >>          soc {
> >>                  /*
> >>                   * Use 0xd0000000 as base address for the internal registers
> >>                   * in SPL U-Boot
> >>                   */
> >>                  ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
> >>                            MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
> >>                            MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
> >>                  u-boot,dm-pre-reloc;
> >>
> >>                  internal-regs {
> >>                          u-boot,dm-pre-reloc;
> >>
> >>                          serial at 12000 {
> >>                                  u-boot,dm-pre-reloc;
> >>                          };
> >>                  };
> >>          };
> >> };
> >> ---<-----------------------
> >>
> >> Unfortunately this approach does not work right now. The dtc
> >> seems to not overlay the new values in the resulting dtb.
> >>
> >> But has this approach been discussed before? Or if not, what do
> >> you think of it (if and once it really works)?
> >
> > The dtc should overlay the values - we depend on this bahaviour in
> > various places.
>
> Right, I know. But it just doesn't work here right now. I've
> unsuccessfully tested it.

Hmmm.

>
> > But to me this approach seems a bit clumsy.
>
> I don't find this approach clumsy. Just my personal feeling. But
> using this approach we could abstract all the U-Boot properties
> and nodes into a separate file. And don't have to make any
> changes to original dtsi / dts files.
>
> Of course it would be even better, if all of these U-Boot additions
> would be accepted to the main DeviceTree source. But this does
> not seem to happen, unfortunately.

Right, and I've complained about that, ineffectively. It is really
unfortunate. Still it is worth sending a patch to the list and see
what happens. I'll make time to send a few more also.

>
> > I can't help thinking we are missing something here. The DT is
> > supposed to represent the hardware, and here we have some hardware
> > which is not being represented correctly. Your complaint that you will
> > need to change all the files to add a ranges-initial property (or
> > whatever) doesn't really seem like a problem to me - you only do it
> > once.
>
> Not really. We need to change this at least for every now SoC
> having this problem. But yes, this should not too much trouble.
>
> > We can have a setting as to which property to use, and the
> > platform can change it when needed. It can fall back to 'ranges' if
> > there is no ranges-initial property. I used a similar approach for
> > memory on pit - definitions for both SRAM and SDRAM, and allowing
> > selection of which one to use. There is nothing in DT that prevents us
> > from handling this sort of thing.
>
> Again, how does this work exactly? Is there some code for "pit"
> that I could take as an example?

You can see an upstream version of something similar here:

http://patchwork.ozlabs.org/patch/402714/

I can point you to the Chrome OS tree for the downstream part if you like.

>
> > Perhaps you could ask about this on the device-tree-discuss list? If
> > you like I can start a thread.
> >
> > Also, just to get this off my chest, I would like to expand a bit more
> > on why I don't think we should call board code from drivers. The
> > drivers are supposed to be hermetic, and work on any platform. The
> > information needed by the driver to work should come from data, not
> > code. Then we can represent it in the device tree, or in platform data
> > (to which device tree is often converted). If we have the drivers
> > calling out to code to get their information, then it adds more
> > coupling between the drivers and the platforms. At present we can drop
> > a driver in by enabling it in Kconfig and adding a device tree node.
> > We don't need to modify the board code at all. I think that is worth
> > protecting. It creates a nice clear wall between the driver and the
> > board code, conceptually free of hidden interactions, etc. It is much
> > easier to scale to larger and more complex platforms with this in
> > place.
> >
> > For similar reasons I think we should avoid calling drivers from other
> > drivers - this should always be done through the driver's API (e.g.
> > the uclass header file). That way we don't have (e.g.) a SPI flash
> > driver with a compile- or link-time dependency on a particular SPI
> > driver. It may depend on features that the SPI driver may or may not
> > provide, but in principle any SPI driver can provide those features,
> > and if they are publicly declared in the uclass API it is clear what
> > these features are.
>
> Thanks for the detailed explanation for your reasoning. I understand
> your point of view.

OK good. I do understand that this is restrictive and has costs also.
I'm interested to hear other points of view, etc.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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.

>>
>>   ...
>>   * Note: this Device Tree assumes that the bootloader has remapped the
>>   * internal registers to 0xf1000000 (instead of the default
>>   * 0xd0000000). The 0xf1000000 is the default used by the recent,
>>   * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
>>   * boards were delivered with an older version of the bootloader that
>>   * left internal registers mapped at 0xd0000000. If you are in this
>>   * situation, you should either update your bootloader (preferred
>>   * solution) or the below Device Tree should be adjusted.
>>   ...
>>
>> It really boils down to a compile-time option for these platforms,
>> to "detect" where the internal registers are located  (SPL or not).
>
> Yes.
>
>>
>>> I prefer adding a
>>> new ranges property anyway.
>>
>> So please bear with me, and explain (again?) how exactly this new
>> ranges property could solve this problem. Keeping the undetectable
>> nature of this address change in mind. And without adding some
>> more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your
>> remark to your similar "pit" solution).
>
> We need to get to the bottom of this 'detection' thing first (as
> above). But if the only way of detecting is 'am I in SPL or not' then
> you can always have some code that selects the range based on that.

Yes, this is what we need to do.

>>
>>>>
>>>>> - Add some sort of 'core' driver model address translation setting,
>>>>> which your board code can set up with a function call, and
>>>>> dev_get_addr() uses
>>>>> - Add a uclass and driver for address translation, and call it from
>>>>> here (ugh...)
>>>>
>>>> All this doesn't sound very "promising". At least not to me. But
>>>> I had another idea, which might be a good alternative. Use a new,
>>>> different dts for SPL. This has most likely been discussed before,
>>>> not sure. The idea here is, to re-use the existing dts and include
>>>> it in the new dts. Something like:
>>>>
>>>> U-Boot proper: armada-xp-gp.dts
>>>> SPL U-Boot:    armada-xp-gp-spl.dts
>>>>
>>>> And this new SPL dts includes the original dts and only changes
>>>> what needs to get changed for SPL. This could include all the
>>>> "u-boot,dm-pre-reloc" properties as well and look like this:
>>>>
>>>> ---<-----------------------
>>>> /*
>>>>    * Device Tree file for Marvell Armada XP development board
>>>>    * (DB-MV784MP-GP)
>>>>    */
>>>>
>>>> #include "armada-xp-gp.dts"
>>>>
>>>> / {
>>>>           soc {
>>>>                   /*
>>>>                    * Use 0xd0000000 as base address for the internal registers
>>>>                    * in SPL U-Boot
>>>>                    */
>>>>                   ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
>>>>                             MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
>>>>                             MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
>>>>                   u-boot,dm-pre-reloc;
>>>>
>>>>                   internal-regs {
>>>>                           u-boot,dm-pre-reloc;
>>>>
>>>>                           serial at 12000 {
>>>>                                   u-boot,dm-pre-reloc;
>>>>                           };
>>>>                   };
>>>>           };
>>>> };
>>>> ---<-----------------------
>>>>
>>>> Unfortunately this approach does not work right now. The dtc
>>>> seems to not overlay the new values in the resulting dtb.
>>>>
>>>> But has this approach been discussed before? Or if not, what do
>>>> you think of it (if and once it really works)?
>>>
>>> The dtc should overlay the values - we depend on this bahaviour in
>>> various places.
>>
>> Right, I know. But it just doesn't work here right now. I've
>> unsuccessfully tested it.
>
> Hmmm.
>
>>
>>> But to me this approach seems a bit clumsy.
>>
>> I don't find this approach clumsy. Just my personal feeling. But
>> using this approach we could abstract all the U-Boot properties
>> and nodes into a separate file. And don't have to make any
>> changes to original dtsi / dts files.
>>
>> Of course it would be even better, if all of these U-Boot additions
>> would be accepted to the main DeviceTree source. But this does
>> not seem to happen, unfortunately.
>
> Right, and I've complained about that, ineffectively. It is really
> unfortunate. Still it is worth sending a patch to the list and see
> what happens. I'll make time to send a few more also.
>
>>
>>> I can't help thinking we are missing something here. The DT is
>>> supposed to represent the hardware, and here we have some hardware
>>> which is not being represented correctly. Your complaint that you will
>>> need to change all the files to add a ranges-initial property (or
>>> whatever) doesn't really seem like a problem to me - you only do it
>>> once.
>>
>> Not really. We need to change this at least for every now SoC
>> having this problem. But yes, this should not too much trouble.
>>
>>> We can have a setting as to which property to use, and the
>>> platform can change it when needed. It can fall back to 'ranges' if
>>> there is no ranges-initial property. I used a similar approach for
>>> memory on pit - definitions for both SRAM and SDRAM, and allowing
>>> selection of which one to use. There is nothing in DT that prevents us
>>> from handling this sort of thing.
>>
>> Again, how does this work exactly? Is there some code for "pit"
>> that I could take as an example?
>
> You can see an upstream version of something similar here:
>
> http://patchwork.ozlabs.org/patch/402714/

I'll try to find some time to take a look at it tomorrow. Thanks.

> I can point you to the Chrome OS tree for the downstream part if you like.

Sure. The more the merrier... ;)

>>
>>> Perhaps you could ask about this on the device-tree-discuss list? If
>>> you like I can start a thread.
>>>
>>> Also, just to get this off my chest, I would like to expand a bit more
>>> on why I don't think we should call board code from drivers. The
>>> drivers are supposed to be hermetic, and work on any platform. The
>>> information needed by the driver to work should come from data, not
>>> code. Then we can represent it in the device tree, or in platform data
>>> (to which device tree is often converted). If we have the drivers
>>> calling out to code to get their information, then it adds more
>>> coupling between the drivers and the platforms. At present we can drop
>>> a driver in by enabling it in Kconfig and adding a device tree node.
>>> We don't need to modify the board code at all. I think that is worth
>>> protecting. It creates a nice clear wall between the driver and the
>>> board code, conceptually free of hidden interactions, etc. It is much
>>> easier to scale to larger and more complex platforms with this in
>>> place.
>>>
>>> For similar reasons I think we should avoid calling drivers from other
>>> drivers - this should always be done through the driver's API (e.g.
>>> the uclass header file). That way we don't have (e.g.) a SPI flash
>>> driver with a compile- or link-time dependency on a particular SPI
>>> driver. It may depend on features that the SPI driver may or may not
>>> provide, but in principle any SPI driver can provide those features,
>>> and if they are publicly declared in the uclass API it is clear what
>>> these features are.
>>
>> Thanks for the detailed explanation for your reasoning. I understand
>> your point of view.
>
> OK good. I do understand that this is restrictive and has costs also.
> I'm interested to hear other points of view, etc.

Yes, I would also like to hear other opinions on this. If this
2 lines of call-back code are acceptable for others or not?

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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.

>
>>>
>>>   ...
>>>   * Note: this Device Tree assumes that the bootloader has remapped the
>>>   * internal registers to 0xf1000000 (instead of the default
>>>   * 0xd0000000). The 0xf1000000 is the default used by the recent,
>>>   * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
>>>   * boards were delivered with an older version of the bootloader that
>>>   * left internal registers mapped at 0xd0000000. If you are in this
>>>   * situation, you should either update your bootloader (preferred
>>>   * solution) or the below Device Tree should be adjusted.
>>>   ...
>>>
>>> It really boils down to a compile-time option for these platforms,
>>> to "detect" where the internal registers are located  (SPL or not).
>>
>>
>> Yes.
>>
>>>
>>>> I prefer adding a
>>>> new ranges property anyway.
>>>
>>>
>>> So please bear with me, and explain (again?) how exactly this new
>>> ranges property could solve this problem. Keeping the undetectable
>>> nature of this address change in mind. And without adding some
>>> more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your
>>> remark to your similar "pit" solution).
>>
>>
>> We need to get to the bottom of this 'detection' thing first (as
>> above). But if the only way of detecting is 'am I in SPL or not' then
>> you can always have some code that selects the range based on that.
>
>
> Yes, this is what we need to do.
>
>
>>>
>>>>>
>>>>>> - Add some sort of 'core' driver model address translation setting,
>>>>>> which your board code can set up with a function call, and
>>>>>> dev_get_addr() uses
>>>>>> - Add a uclass and driver for address translation, and call it from
>>>>>> here (ugh...)
>>>>>
>>>>>
>>>>> All this doesn't sound very "promising". At least not to me. But
>>>>> I had another idea, which might be a good alternative. Use a new,
>>>>> different dts for SPL. This has most likely been discussed before,
>>>>> not sure. The idea here is, to re-use the existing dts and include
>>>>> it in the new dts. Something like:
>>>>>
>>>>> U-Boot proper: armada-xp-gp.dts
>>>>> SPL U-Boot:    armada-xp-gp-spl.dts
>>>>>
>>>>> And this new SPL dts includes the original dts and only changes
>>>>> what needs to get changed for SPL. This could include all the
>>>>> "u-boot,dm-pre-reloc" properties as well and look like this:
>>>>>
>>>>> ---<-----------------------
>>>>> /*
>>>>>    * Device Tree file for Marvell Armada XP development board
>>>>>    * (DB-MV784MP-GP)
>>>>>    */
>>>>>
>>>>> #include "armada-xp-gp.dts"
>>>>>
>>>>> / {
>>>>>           soc {
>>>>>                   /*
>>>>>                    * Use 0xd0000000 as base address for the internal registers
>>>>>                    * in SPL U-Boot
>>>>>                    */
>>>>>                   ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
>>>>>                             MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
>>>>>                             MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
>>>>>                   u-boot,dm-pre-reloc;
>>>>>
>>>>>                   internal-regs {
>>>>>                           u-boot,dm-pre-reloc;
>>>>>
>>>>>                           serial at 12000 {
>>>>>                                   u-boot,dm-pre-reloc;
>>>>>                           };
>>>>>                   };
>>>>>           };
>>>>> };
>>>>> ---<-----------------------
>>>>>
>>>>> Unfortunately this approach does not work right now. The dtc
>>>>> seems to not overlay the new values in the resulting dtb.
>>>>>
>>>>> But has this approach been discussed before? Or if not, what do
>>>>> you think of it (if and once it really works)?
>>>>
>>>>
>>>> The dtc should overlay the values - we depend on this bahaviour in
>>>> various places.
>>>
>>>
>>> Right, I know. But it just doesn't work here right now. I've
>>> unsuccessfully tested it.
>>
>>
>> Hmmm.
>>
>>>
>>>> But to me this approach seems a bit clumsy.
>>>
>>>
>>> I don't find this approach clumsy. Just my personal feeling. But
>>> using this approach we could abstract all the U-Boot properties
>>> and nodes into a separate file. And don't have to make any
>>> changes to original dtsi / dts files.
>>>
>>> Of course it would be even better, if all of these U-Boot additions
>>> would be accepted to the main DeviceTree source. But this does
>>> not seem to happen, unfortunately.
>>
>>
>> Right, and I've complained about that, ineffectively. It is really
>> unfortunate. Still it is worth sending a patch to the list and see
>> what happens. I'll make time to send a few more also.
>>
>>>
>>>> I can't help thinking we are missing something here. The DT is
>>>> supposed to represent the hardware, and here we have some hardware
>>>> which is not being represented correctly. Your complaint that you will
>>>> need to change all the files to add a ranges-initial property (or
>>>> whatever) doesn't really seem like a problem to me - you only do it
>>>> once.
>>>
>>>
>>> Not really. We need to change this at least for every now SoC
>>> having this problem. But yes, this should not too much trouble.
>>>
>>>> We can have a setting as to which property to use, and the
>>>> platform can change it when needed. It can fall back to 'ranges' if
>>>> there is no ranges-initial property. I used a similar approach for
>>>> memory on pit - definitions for both SRAM and SDRAM, and allowing
>>>> selection of which one to use. There is nothing in DT that prevents us
>>>> from handling this sort of thing.
>>>
>>>
>>> Again, how does this work exactly? Is there some code for "pit"
>>> that I could take as an example?
>>
>>
>> You can see an upstream version of something similar here:
>>
>> http://patchwork.ozlabs.org/patch/402714/
>
>
> I'll try to find some time to take a look at it tomorrow. Thanks.
>
>> I can point you to the Chrome OS tree for the downstream part if you like.
>
>
> Sure. The more the merrier... ;)

It's in the chromeos-v2013.06 branch, e.g.

https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/chromeos-v2013.06/board/samsung/dts/exynos542x-peach.dtsi

You can see the /memory and /iram nodes there, and chromeos-config
which has properties to select the correct memory region. It allows
U-Boot to run from both IRAM or SDRAM, with run-time control.

>
>
>>>
>>>> Perhaps you could ask about this on the device-tree-discuss list? If
>>>> you like I can start a thread.
>>>>
>>>> Also, just to get this off my chest, I would like to expand a bit more
>>>> on why I don't think we should call board code from drivers. The
>>>> drivers are supposed to be hermetic, and work on any platform. The
>>>> information needed by the driver to work should come from data, not
>>>> code. Then we can represent it in the device tree, or in platform data
>>>> (to which device tree is often converted). If we have the drivers
>>>> calling out to code to get their information, then it adds more
>>>> coupling between the drivers and the platforms. At present we can drop
>>>> a driver in by enabling it in Kconfig and adding a device tree node.
>>>> We don't need to modify the board code at all. I think that is worth
>>>> protecting. It creates a nice clear wall between the driver and the
>>>> board code, conceptually free of hidden interactions, etc. It is much
>>>> easier to scale to larger and more complex platforms with this in
>>>> place.
>>>>
>>>> For similar reasons I think we should avoid calling drivers from other
>>>> drivers - this should always be done through the driver's API (e.g.
>>>> the uclass header file). That way we don't have (e.g.) a SPI flash
>>>> driver with a compile- or link-time dependency on a particular SPI
>>>> driver. It may depend on features that the SPI driver may or may not
>>>> provide, but in principle any SPI driver can provide those features,
>>>> and if they are publicly declared in the uclass API it is clear what
>>>> these features are.
>>>
>>>
>>> Thanks for the detailed explanation for your reasoning. I understand
>>> your point of view.
>>
>>
>> OK good. I do understand that this is restrictive and has costs also.
>> I'm interested to hear other points of view, etc.
>
>
> Yes, I would also like to hear other opinions on this. If this
> 2 lines of call-back code are acceptable for others or not?
>
> Thanks,
> Stefan
>

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

Hi Simon,

( Last mail for tonight - a glass of quite nice red wine is
waiting for me ... ;) )

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.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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 :-)

>
> 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.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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.

Thanks,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dm-Add-option-to-configure-a-different-non-standard-.patch
Type: text/x-diff
Size: 5494 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151203/39791bc1/attachment.patch>

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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.

What about the device tree mailing list. Should I send an email there?

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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_support.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.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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_support.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?

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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?

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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.

Regards,
Simon

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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.

Thanks,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dm-Add-option-to-configure-an-offset-for-the-address.patch
Type: text/x-diff
Size: 3240 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151211/3a7c623a/attachment-0001.patch>

[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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