[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Hello,

The aim of this patch series is to move the internal registers space
of the Armada 370 and Armada XP support to 0xf1000000 instead of
0xd0000000, and prepare for the support of the new versions of U-Boot
installed by Marvell on Armada 370 and XP platforms that boot the
kernel with the internal registers space already mapped at 0xf1000000.

Before reading the patches, and especially the last one, please read
on for a description of the problem and the proposed solution. The
solution has been carefully thought of, and all the details explained
below are important to understand how the proposed solution was
designed.

On Marvell EBU SoCs, including earlier families such as Kirkwood or
Dove, all the peripheral registers belong to a memory window of 1 MB,
called the "internal registers window". This window of physical memory
can be freely moved to an arbitrary physical address. At reset, this
memory window is at 0xD0000000, but it can be changed by software to
another address if needed.

The *very* important thing to remember is that the mechanism to get or
change the base address of this "internal registers" window is a
register, which is itself part of this "internal registers"
window. The bottom line is that when you run code on a Marvell SoC,
you have to *know* where the internal registers are located, because
you can't guess dynamically by reading some hardware register. Again,
this is a *very* important characteristic, because it explains many of
the choices of the proposed solution below.

Historically, the bootloaders for Dove and Kirkwood have always
remapped the internal registers window at 0xF1000000, so for those
platforms, the Linux kernel has always made the assumption that
internal registers are accessible at 0xF1000000.

Unfortunately, for Armada 370 and Armada XP, the current generation of
bootloaders that are being shipped on Marvell evaluation boards, but
also on products like the Plathome OpenBlocks AX3 or the Globalscale
Mirabox do not do the remapping, and are leaving the internal
registers at 0xD0000000.

However, this is causing some problems:

 1 The internal registers window sits at 3 GB, in the middle of the
   0-4 GB area of physical memory, which is quite annoying when you
   have more than 3 GB of memory. Having them at 0xF1000000 means that
   you lose less RAM when you have more than 3 GB of physical memory
   installed. And since Armada XP is LPAE capable, you can even have
   more than 4 GB memory: we already have boards with 8 GB of memory
   installed.

 2 This is different from Kirkwood and Dove, which makes sharing some
   early code like earlyprintk complicated, while our goal is
   ultimately to merge all Marvell EBU platforms into mach-mvebu.

(1) is really the primary motivation here, (2) is only a nice
side-effect.

So, the new generation of bootloaders that are shipped on new Marvell
Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
starting Linux. The current kernel cannot boot on such platforms.

One solution is to move the kernel to use 0xF1000000, and assume the
bootloader has already done the remapping. This can be one option, but
it means that newer kernels would no longer boot on platforms that use
an old bootloader. All existing users of Globalscale Mirabox or
Plathome OpenBlocks AX3, next time they upgrade their kernel, would
have a kernel that simply doesn't boot (and without showing *any*
message, because even the address of the UART has changed).

If the only Armada 370/XP platforms in use today had been evaluation
boards that are available only to a selected number of people,
dropping the compatibility would probably have been
acceptable. However, with a large number of Mirabox and OpenBlocks AX3
devices being already shipped, we believe that breaking the
compatibility is not an option.

Therefore, the last patch of this series adds some early code in the
kernel, at the ->map_io() stage, to switch the internal registers from
0xD0000000 to 0xF1000000 if this has not been done already by the
bootloader. As it was explained above, we unfortunately can't read the
current base address of the internal register window, so we need a
different mechanism to know if the bootloader has done the remapping
at 0xF1000000 (new generation bootloader) or has left the internal
registers at 0xD0000000 (old generation bootloader). In order to
distinguish between those two cases, a CP15 bit is being used. Old
bootloaders do not touch this CP15, so it is set to 0. New bootloaders
set this CP15 bit to 1, so that the kernel knows that the remapping
has already been done. The ->map_io() code looks at this bit to know
if the remapping should be done or not.

Unfortunately, tweaking ->map_io() is not sufficient: we also want
earlyprintk to work. And earlyprintk is used *before* ->map_io() is
called, and *after* ->map_io() is called. So in the earlyprintk code,
we also need to do a little bit of magic to know if we're booted on an
old or new bootloader, and if the remapping has occured or not in the
case of an old bootloader. Since the CP15 bit that we use gets cleared
the first time the 'wfi' instruction is cleared, we use this bit only
in the early kernel boot. As soon as ->map_io() as done its job, it
sets a global variable that tells the earlyprintk code that the
registers are now at 0xF1000000.

I hope this long explanation gives enough context to understand the
problem and the proposed solution. Do not hesitate to ask for more
details if needed.

Notice that the solution would be much simpler if we could do this
remapping earlier in the kernel initialization. Unfortunately,
->map_io() is really the first piece of code that is being called into
SoC-specific code, so it's the only way of doing this without making
changes in generic places in the kernel.

In detail, the patch series has:

 * Patches 1 and 2 fix the Device Tree informations that were
   incorrect for some peripherals: the register area length was
   incorrect. This was not visible until the removal of the bit static
   I/O mapping in patch 6, but is clearly a bug.

   Presumably those patches should be applied on 3.10-rc if possible,
   and therefore they should most likely be applied to mvebu/fixes.

 * Patch 3 is a cleanup, adds 'static' to some functions that lacked
   this qualifier.

   To be applied to mvebu/cleanups

 * Patch 4 and 5 slightly rework the way the SMP initialization and
   the coherency unit initialization is done, in order to avoid the
   hardcoded virtual address in the coherency.c file.

   To be applied to mvebu/soc

 * Patch 6 moves the L2 cache and mvebu-mbus initialization a bit
   later (i.e from ->init_early() to ->init_time()) so that they can
   ioremap() their registers without crashing once we remove the
   static I/O mapping (next patch).

 * Patch 7 removes the static I/O mapping that was set up at
   ->map_io() time to cover the entire "internal registers" area. This
   is possible thanks to patches 1, 2, 4, 5 and 6.

   To be applied to mvebu/soc

 * Patch 8 removes the hardcoded physical address in the SMP
   initialization code. This is made possible thanks to patches 4 and
   5.

   To be applied to mvebu/soc

 * Patch 9 implements the internal register address change itself. The
   DTS changes are included in the same patch as the code changes. It
   is possible to split them in two patches, but applying just one
   patch and not the other would lead to a kernel that doesn't boot at
   all.

   To be applied to mvebu/soc (even though there are admittedly some
   DT changes in here).

Best regards,

Thomas

Thomas Petazzoni (9):
  arm: mvebu: fix length of SATA registers area in .dtsi
  arm: mvebu: fix length of Ethernet registers area in .dtsi
  arm: mvebu: mark functions of armada-370-xp.c as static
  arm: mvebu: remove dependency of SMP init on static I/O mapping
  arm: mvebu: avoid hardcoded virtual address in coherency code
  arm: mvebu: move cache and mvebu-mbus initialization later
  arm: mvebu: remove hardcoded static I/O mapping
  arm: mvebu: don't hardcode a physical address in headsmp.S
  arm: mvebu: switch internal register address at runtime if needed

 arch/arm/boot/dts/armada-370-xp.dtsi     |    8 +-
 arch/arm/boot/dts/armada-370.dtsi        |    2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |    2 +-
 arch/arm/boot/dts/armada-xp.dtsi         |    2 +-
 arch/arm/include/debug/mvebu.S           |   77 +++++++++++++++--
 arch/arm/mach-mvebu/armada-370-xp.c      |  137 +++++++++++++++++++++++++-----
 arch/arm/mach-mvebu/armada-370-xp.h      |    4 +-
 arch/arm/mach-mvebu/coherency.c          |   36 +++-----
 arch/arm/mach-mvebu/coherency.h          |    4 -
 arch/arm/mach-mvebu/common.h             |    2 +
 arch/arm/mach-mvebu/headsmp.S            |   20 +++--
 arch/arm/mach-mvebu/platsmp.c            |   10 ++-
 12 files changed, 231 insertions(+), 73 deletions(-)

-- 
1.7.9.5

[PATCH 1/9] arm: mvebu: fix length of SATA registers area in .dtsi

[Thomas Petazzoni]

The length of the registers area for the Marvell 370/XP SATA
controller was incorrect in the .dtsi: 0x2400 while it should have
been 0x5000. Until now, this problem wasn't noticed because there was
a large static mapping for all I/Os set up by ->map_io(). But since
we're going to get rid of this static mapping, we need to ensure that
the register areas are properly sized.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 272bbc6..75de3fe 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -79,7 +79,7 @@
 
 			sata at a0000 {
 				compatible = "marvell,orion-sata";
-				reg = <0xa0000 0x2400>;
+				reg = <0xa0000 0x5000>;
 				interrupts = <55>;
 				clocks = <&gateclk 15>, <&gateclk 30>;
 				clock-names = "0", "1";
-- 
1.7.9.5

[PATCH 2/9] arm: mvebu: fix length of Ethernet registers area in .dtsi

[Thomas Petazzoni]

The length of the registers area for the Marvell 370/XP Ethernet
controller was incorrect in the .dtsi: 0x2400 while it should have
been 0x4000. Until now, this problem wasn't noticed because there was
a large static mapping for all I/Os set up by ->map_io(). But since
we're going to get rid of this static mapping, we need to ensure that
the register areas are properly sized.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi     |    4 ++--
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |    2 +-
 arch/arm/boot/dts/armada-xp.dtsi         |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 75de3fe..96a6bcd 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -95,7 +95,7 @@
 
 			ethernet at 70000 {
 				compatible = "marvell,armada-370-neta";
-				reg = <0x70000 0x2500>;
+				reg = <0x70000 0x4000>;
 				interrupts = <8>;
 				clocks = <&gateclk 4>;
 				status = "disabled";
@@ -103,7 +103,7 @@
 
 			ethernet at 74000 {
 				compatible = "marvell,armada-370-neta";
-				reg = <0x74000 0x2500>;
+				reg = <0x74000 0x4000>;
 				interrupts = <10>;
 				clocks = <&gateclk 3>;
 				status = "disabled";
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 6ab56bd..488ca5e 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -107,7 +107,7 @@
 
 			ethernet at 34000 {
 				compatible = "marvell,armada-370-neta";
-				reg = <0x34000 0x2500>;
+				reg = <0x34000 0x4000>;
 				interrupts = <14>;
 				clocks = <&gateclk 1>;
 				status = "disabled";
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index bacab11..67936f89 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -88,7 +88,7 @@
 
 			ethernet at 30000 {
 				compatible = "marvell,armada-370-neta";
-				reg = <0x30000 0x2500>;
+				reg = <0x30000 0x4000>;
 				interrupts = <12>;
 				clocks = <&gateclk 2>;
 				status = "disabled";
-- 
1.7.9.5

[PATCH 3/9] arm: mvebu: mark functions of armada-370-xp.c as static

[Thomas Petazzoni]

All the functions in armada-370-xp.c are called from the
DT_MACHINE_START function pointers, so there is no need for them to be
visible outside of this file, and we therefore mark them as static.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 42a4cb3..bfe337d 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -38,18 +38,18 @@ static struct map_desc armada_370_xp_io_desc[] __initdata = {
 	},
 };
 
-void __init armada_370_xp_map_io(void)
+static void __init armada_370_xp_map_io(void)
 {
 	iotable_init(armada_370_xp_io_desc, ARRAY_SIZE(armada_370_xp_io_desc));
 }
 
-void __init armada_370_xp_timer_and_clk_init(void)
+static void __init armada_370_xp_timer_and_clk_init(void)
 {
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
 }
 
-void __init armada_370_xp_init_early(void)
+static void __init armada_370_xp_init_early(void)
 {
 	char *mbus_soc_name;
 
-- 
1.7.9.5

[PATCH 4/9] arm: mvebu: remove dependency of SMP init on static I/O mapping

[Thomas Petazzoni]

The ->smp_init_cpus() function is called very early during boot, at a
point where dynamic I/O mappings are not yet possible. However, in the
Armada 370/XP implementation of this function, we have to get the
number of CPUs. We used to do that by accessing a hardware register,
which requires relying on a static I/O mapping set up by
->map_io(). Not only this requires hardcoding a virtual address, but
it also prevents us from removing the static I/O mapping.

So this commit changes the way used to get the number of CPUs: we now
use the Device Tree, which is a representation of the hardware, and
provides us the number of available CPUs. This is also more accurate,
because it potentially allows to boot the Linux kernel on only a
number of CPUs given by the Device Tree, instead of unconditionally on
all CPUs.

As a consequence, the coherency_get_cpu_count() function becomes no
longer used, so we remove it.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c |   12 ------------
 arch/arm/mach-mvebu/coherency.h |    4 ----
 arch/arm/mach-mvebu/common.h    |    2 ++
 arch/arm/mach-mvebu/platsmp.c   |   10 +++++++++-
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 8278960..46d66c0 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -47,18 +47,6 @@ static struct of_device_id of_coherency_table[] = {
 	{ /* end of list */ },
 };
 
-#ifdef CONFIG_SMP
-int coherency_get_cpu_count(void)
-{
-	int reg, cnt;
-
-	reg = readl(coherency_base + COHERENCY_FABRIC_CFG_OFFSET);
-	cnt = (reg & 0xF) + 1;
-
-	return cnt;
-}
-#endif
-
 /* Function defined in coherency_ll.S */
 int ll_set_cpu_coherent(void __iomem *base_addr, unsigned int hw_cpu_id);
 
diff --git a/arch/arm/mach-mvebu/coherency.h b/arch/arm/mach-mvebu/coherency.h
index 2f42813..df33ad8 100644
--- a/arch/arm/mach-mvebu/coherency.h
+++ b/arch/arm/mach-mvebu/coherency.h
@@ -14,10 +14,6 @@
 #ifndef __MACH_370_XP_COHERENCY_H
 #define __MACH_370_XP_COHERENCY_H
 
-#ifdef CONFIG_SMP
-int coherency_get_cpu_count(void);
-#endif
-
 int set_cpu_coherent(int cpu_id, int smp_group_id);
 int coherency_init(void);
 
diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-mvebu/common.h
index aa27bc2..98defd5 100644
--- a/arch/arm/mach-mvebu/common.h
+++ b/arch/arm/mach-mvebu/common.h
@@ -15,6 +15,8 @@
 #ifndef __ARCH_MVEBU_COMMON_H
 #define __ARCH_MVEBU_COMMON_H
 
+#define ARMADA_XP_MAX_CPUS 4
+
 void mvebu_restart(char mode, const char *cmd);
 
 void armada_370_xp_init_irq(void);
diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index 875ea74..93f2f3a 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -88,8 +88,16 @@ static int __cpuinit armada_xp_boot_secondary(unsigned int cpu,
 
 static void __init armada_xp_smp_init_cpus(void)
 {
+	struct device_node *np;
 	unsigned int i, ncores;
-	ncores = coherency_get_cpu_count();
+
+	np = of_find_node_by_name(NULL, "cpus");
+	if (!np)
+		panic("No 'cpus' node found\n");
+
+	ncores = of_get_child_count(np);
+	if (ncores == 0 || ncores > ARMADA_XP_MAX_CPUS)
+		panic("Invalid number of CPUs in DT\n");
 
 	/* Limit possible CPUs to defconfig */
 	if (ncores > nr_cpu_ids) {
-- 
1.7.9.5

[PATCH 5/9] arm: mvebu: avoid hardcoded virtual address in coherency code

[Thomas Petazzoni]

Now that the coherency_get_cpu_count() function no longer requires a
very early mapping of the coherency unit registers, we can avoid the
hardcoded virtual address in coherency.c. However, the coherency
features are still used quite early, so we need to do the of_iomap()
early enough, at the ->init_timer() level, so we have the call of
coherency_init() at this point.

Unfortunately, at ->init_timer() time, it is not possible to register
a bus notifier, so we add a separate coherency_late_init() function
that gets called as as postcore_initcall(), when bus notifiers are
available.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c |    2 +-
 arch/arm/mach-mvebu/coherency.c     |   20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index bfe337d..3af5628 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -47,6 +47,7 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 {
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
+	coherency_init();
 }
 
 static void __init armada_370_xp_init_early(void)
@@ -83,7 +84,6 @@ static void __init armada_370_xp_init_early(void)
 static void __init armada_370_xp_dt_init(void)
 {
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-	coherency_init();
 }
 
 static const char * const armada_370_xp_dt_compat[] = {
diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 46d66c0..d74794a 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -27,14 +27,7 @@
 #include <asm/smp_plat.h>
 #include "armada-370-xp.h"
 
-/*
- * Some functions in this file are called very early during SMP
- * initialization. At that time the device tree framework is not yet
- * ready, and it is not possible to get the register address to
- * ioremap it. That's why the pointer below is given with an initial
- * value matching its virtual mapping
- */
-static void __iomem *coherency_base = ARMADA_370_XP_REGS_VIRT_BASE + 0x20200;
+static void __iomem *coherency_base;
 static void __iomem *coherency_cpu_base;
 
 /* Coherency fabric registers */
@@ -135,9 +128,16 @@ int __init coherency_init(void)
 		coherency_base = of_iomap(np, 0);
 		coherency_cpu_base = of_iomap(np, 1);
 		set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
-		bus_register_notifier(&platform_bus_type,
-					&mvebu_hwcc_platform_nb);
 	}
 
 	return 0;
 }
+
+static int __init coherency_late_init(void)
+{
+	bus_register_notifier(&platform_bus_type,
+			      &mvebu_hwcc_platform_nb);
+	return 0;
+}
+
+postcore_initcall(coherency_late_init);
-- 
1.7.9.5

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Thomas Petazzoni]

Current, the L2 cache and the mvebu-mbus drivers are initialized at
->init_early() time. However, at ->init_early() time, ioremap() only
works if a static I/O mapping has already been put in place. If it's
not the case, it tries to do a memory allocation with kmalloc() which
is not possible so early at this stage of the initialization.

Since we want to get rid of the static I/O mapping, we cannot
initialize the L2 cache driver and the mvebu-mbus driver so early. So,
we move their initialization to the ->init_time() level, which is
slightly later (so ioremap() works properly), but sufficiently early
to be before the call of the ->smp_prepare_cpus() hook, which creates
an address decoding window for the BootROM, which requires the
mvebu-mbus driver to be properly initialized.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 3af5628..23cdabd 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -45,21 +45,11 @@ static void __init armada_370_xp_map_io(void)
 
 static void __init armada_370_xp_timer_and_clk_init(void)
 {
+	char *mbus_soc_name;
+
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
 	coherency_init();
-}
-
-static void __init armada_370_xp_init_early(void)
-{
-	char *mbus_soc_name;
-
-	/*
-	 * Some Armada 370/XP devices allocate their coherent buffers
-	 * from atomic context. Increase size of atomic coherent pool
-	 * to make sure such the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 
 	/*
 	 * This initialization will be replaced by a DT-based
@@ -81,6 +71,16 @@ static void __init armada_370_xp_init_early(void)
 #endif
 }
 
+static void __init armada_370_xp_init_early(void)
+{
+	/*
+	 * Some Armada 370/XP devices allocate their coherent buffers
+	 * from atomic context. Increase size of atomic coherent pool
+	 * to make sure such the allocations won't fail.
+	 */
+	init_dma_coherent_pool_size(SZ_1M);
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-- 
1.7.9.5

[PATCH 7/9] arm: mvebu: remove hardcoded static I/O mapping

[Thomas Petazzoni]

Now that we have removed the need of the static I/O mapping for early
initialization reasons, and fixed the registers area length that were
broken, we can get rid of the static I/O mapping. Only the earlyprintk
mapping needs to be set up, using the debug_ll_io_init() helper
function.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c |   11 +----------
 arch/arm/mach-mvebu/armada-370-xp.h |    2 --
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 23cdabd..90cc0e8 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -29,18 +29,9 @@
 #include "common.h"
 #include "coherency.h"
 
-static struct map_desc armada_370_xp_io_desc[] __initdata = {
-	{
-		.virtual	= (unsigned long) ARMADA_370_XP_REGS_VIRT_BASE,
-		.pfn		= __phys_to_pfn(ARMADA_370_XP_REGS_PHYS_BASE),
-		.length		= ARMADA_370_XP_REGS_SIZE,
-		.type		= MT_DEVICE,
-	},
-};
-
 static void __init armada_370_xp_map_io(void)
 {
-	iotable_init(armada_370_xp_io_desc, ARRAY_SIZE(armada_370_xp_io_desc));
+	debug_ll_io_init();
 }
 
 static void __init armada_370_xp_timer_and_clk_init(void)
diff --git a/arch/arm/mach-mvebu/armada-370-xp.h b/arch/arm/mach-mvebu/armada-370-xp.h
index 2070e1b..585e147 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.h
+++ b/arch/arm/mach-mvebu/armada-370-xp.h
@@ -16,8 +16,6 @@
 #define __MACH_ARMADA_370_XP_H
 
 #define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
-#define ARMADA_370_XP_REGS_VIRT_BASE	IOMEM(0xfec00000)
-#define ARMADA_370_XP_REGS_SIZE		SZ_1M
 
 /* These defines can go away once mvebu-mbus has a DT binding */
 #define ARMADA_370_XP_MBUS_WINS_BASE    (ARMADA_370_XP_REGS_PHYS_BASE + 0x20000)
-- 
1.7.9.5

[PATCH 8/9] arm: mvebu: don't hardcode a physical address in headsmp.S

[Thomas Petazzoni]

Now that the coherency_init() function is called a bit earlier, we can
actually read the physical address of the coherency unit registers
from the Device Tree, and communicate that to the headsmp.S code,
which avoids hardcoding a physical address.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c |    4 ++++
 arch/arm/mach-mvebu/headsmp.S   |   20 +++++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index d74794a..3d8f40f 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -27,6 +27,7 @@
 #include <asm/smp_plat.h>
 #include "armada-370-xp.h"
 
+unsigned long __cpuinitdata coherency_phys_base;
 static void __iomem *coherency_base;
 static void __iomem *coherency_cpu_base;
 
@@ -124,7 +125,10 @@ int __init coherency_init(void)
 
 	np = of_find_matching_node(NULL, of_coherency_table);
 	if (np) {
+		struct resource res;
 		pr_info("Initializing Coherency fabric\n");
+		of_address_to_resource(np, 0, &res);
+		coherency_phys_base = res.start;
 		coherency_base = of_iomap(np, 0);
 		coherency_cpu_base = of_iomap(np, 1);
 		set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
index a06e0ed..095b590 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -21,12 +21,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 
-/*
- * At this stage the secondary CPUs don't have acces yet to the MMU, so
- * we have to provide physical addresses
- */
-#define ARMADA_XP_CFB_BASE	     0xD0020200
-
 	__CPUINIT
 
 /*
@@ -35,15 +29,23 @@
  * startup
  */
 ENTRY(armada_xp_secondary_startup)
+	/* Add CPU to coherency fabric */
+	adr	r0, 1f
+	ldmia	r0, {r1, r2}
+	sub	r0, r0, r1
+	add	r2, r2, r0
+	ldr	r0, [r2]
 
 	/* Read CPU id */
 	mrc     p15, 0, r1, c0, c0, 5
 	and     r1, r1, #0xF
 
-	/* Add CPU to coherency fabric */
-	ldr     r0, =ARMADA_XP_CFB_BASE
-
 	bl	ll_set_cpu_coherent
 	b	secondary_startup
 
 ENDPROC(armada_xp_secondary_startup)
+
+	.align 2
+1:
+	.long	.
+	.long	coherency_phys_base
-- 
1.7.9.5

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Thomas Petazzoni]

On Marvell platforms, the "internal registers" is a window of 1 MB of
the physical address space that contains all the registers for the
various peripherals of the SoC (timers, interrupt controllers, GPIO
controllers, Ethernet, etc.). The base address of this 1 MB window is
configurable, and this base address is configured using a register
that is itself part of this window.

All earlier families of Marvell EBU SoCs supported by Linux used
0xf1000000 as the base address of the internal registers window. For
some reason, early versions of the Marvell 370/XP platforms used
0xd0000000 as the base address. This base address is set up by the
bootloader. However, in order to keep as much physical address space
to address RAM below the 4 GB limit, we had to switch the base address
of the internal registers window back to 0xf1000000, and we wanted to
achieve that without breaking existing platforms, where the bootloader
was initially setting the base address to 0xd0000000. So the kernel is
responsible for switching to 0xf1000000 when needed.

One particular bit of a CP15 register has been chosen to indicate
whether we use an old bootloader (setting the address at 0xd0000000)
or a new bootloader (setting the address at 0xf1000000). In the
->map_io() function, when the bit is already set, there is nothing to
do, we are already using a new bootloader and internal registers are
at 0xf1000000.

However, when the bit is not set, we do a static mapping of the
register area that allows to change the internal register address, we
change this address, and set this bit.

Note that this mechanism also needs cooperation from the earlyprintk
addruart handler (in arch/arm/include/debug/mvebu.S). This is where
things get complex: the CP15 bit that has been chosen gets reset to
zero at the first call to the 'wfi' instruction. So it cannot be used
during the entire life of the system. Instead, whenever the ->map_io()
function has switched to the internal register space to the new
address, it sets a global variable 'mvebu_switched_regs', which the
earlyprintk reads. If it's true, then the move has already been done
and there is no need to read the CP15 register. If
'mvebu_switched_regs' is false, we are before the register move, so we
need to read the CP15 bit to find out whether we're being booted from
an old bootloader (which mapped the register space at 0xd0000000) or a
new bootloader (which mapped the register space at 0xf1000000).

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi |    2 +-
 arch/arm/boot/dts/armada-370.dtsi    |    2 +-
 arch/arm/include/debug/mvebu.S       |   77 +++++++++++++++++++++++--
 arch/arm/mach-mvebu/armada-370-xp.c  |  104 ++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/armada-370-xp.h  |    2 +-
 5 files changed, 178 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 96a6bcd..1e7561e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -33,7 +33,7 @@
 		#size-cells = <1>;
 		compatible = "simple-bus";
 		interrupt-parent = <&mpic>;
-		ranges = <0 0 0xd0000000 0x100000>;
+		ranges = <0 0 0xf1000000 0x100000>;
 
 		internal-regs {
 			compatible = "simple-bus";
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index b2c1b5a..7ff9c1b 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -29,7 +29,7 @@
 	};
 
 	soc {
-		ranges = <0 0xd0000000 0x100000>;
+		ranges = <0 0xf1000000 0x100000>;
 		internal-regs {
 			system-controller at 18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
diff --git a/arch/arm/include/debug/mvebu.S b/arch/arm/include/debug/mvebu.S
index df191af..dba702a 100644
--- a/arch/arm/include/debug/mvebu.S
+++ b/arch/arm/include/debug/mvebu.S
@@ -5,20 +5,85 @@
  *
  * Lior Amsalem <alior@marvell.com>
  * Gregory Clement <gregory.clement@free-electrons.com>
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
 */
 
-#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
-#define ARMADA_370_XP_REGS_VIRT_BASE	0xfec00000
+#define ARMADA_370_XP_UART_PHYS_BASE_OLD	0xd0012000
+#define ARMADA_370_XP_UART_PHYS_BASE_NEW	0xf1012000
+#define ARMADA_370_XP_UART_VIRT_BASE_OLD	0xfec12000
+#define ARMADA_370_XP_UART_VIRT_BASE_NEW	0xfebff000
 
 	.macro	addruart, rp, rv, tmp
-	ldr	\rp, =ARMADA_370_XP_REGS_PHYS_BASE
-	ldr	\rv, =ARMADA_370_XP_REGS_VIRT_BASE
-	orr	\rp, \rp, #0x00012000
-	orr	\rv, \rv, #0x00012000
+
+#ifndef ZIMAGE
+	/*
+	 * This loads the value of mvebu_switched_regs. We need a
+	 * little bit of gymnastic here to handle the fact that the
+	 * addruart code is duplicated in different places, and called
+	 * sometimes with the MMU disabled, sometimes the MMU enabled.
+	 *
+	 * Notice that when this earlyprintk code is linked into the
+	 * kernel decompressor (i.e ZIMAGE is defined), this code isn't
+	 * compiled. The reason is because the global mvebu_switched_regs
+	 * variable is not available in the decompressor code. At this
+	 * early stage, we simply test the CP15 bit (below) to know where
+	 * the internal registers are.
+	 */
+	adr	\rp, addruart_switched_regs_\@
+	ldr	\rv, [\rp]
+	sub	\rv, \rv, \rp
+	ldr	\rp, [\rp, #4]
+	sub	\tmp, \rp, \rv
+	ldr	\rp, [\tmp, #0]
+	cmp	\rp, #0
+
+	/* If we have already switched, then use the new register
+	   address */
+	bne	addruart_new_\@
+#endif
+
+	/*
+	 * We haven't switched the register location, so test the CP15
+	 * register bit to find whether we are:
+	 * - with an old bootloader setting the base address to
+	 *   0xd0000000
+	 * - with a new bootloader that has already set the
+	 *   base address to 0xf1000000
+	 * We adapt the physical address returned depending on the
+	 * value of this bit, but also the virtual address, because we
+	 * can't remap the old physical address and the new physical
+	 * address at the same location.
+	 */
+	mrc	p15, 0, \tmp, c5, c0, 0
+	and	\tmp, \tmp, #(1 << 11)
+	cmp	\tmp, #0
+	bne	addruart_new_\@
+
+addruart_old_\@:
+	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_OLD
+	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_OLD
+	b	addruart_out_\@
+addruart_new_\@:
+	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_NEW
+	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_NEW
+	b	addruart_out_\@
+
+#ifndef ZIMAGE
+	/*
+	 * Global variable mvebu_switched_regs not visible in the
+	 * decompressor code
+	 */
+	.align
+addruart_switched_regs_\@:
+	.long	.
+	.long	mvebu_switched_regs
+#endif
+
+addruart_out_\@:
 	.endm
 
 #define UART_SHIFT	2
diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 90cc0e8..c56875e 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -29,8 +29,112 @@
 #include "common.h"
 #include "coherency.h"
 
+/*
+ * Note on the internal register address.
+ *
+ * On Marvell platforms, the "internal registers" is a window of 1 MB
+ * of the physical address space that contains all the registers for
+ * the various peripherals of the SoC (timers, interrupt controllers,
+ * GPIO controllers, Ethernet, etc.). The base address of this 1 MB
+ * window is configurable, and this base address is configured using a
+ * register that is itself part of this window.
+ *
+ * All earlier families of Marvell EBU SoCs supported by Linux used
+ * 0xf1000000 as the base address of the internal registers
+ * window. For some reason, early versions of the Marvell 370/XP
+ * platforms used 0xd0000000 as the base address. This base address is
+ * set up by the bootloader. However, in order to keep as much
+ * physical address space to address RAM below the 4 GB limit, we had
+ * to switch the base address of the internal registers window back to
+ * 0xf1000000, and we wanted to achieve that without breaking existing
+ * platforms, where the bootloader was initially setting the base
+ * address to 0xd0000000. So the kernel is responsible for switching
+ * to 0xf1000000 when needed.
+ *
+ * One particular bit of a CP15 register has been chosen to indicate
+ * whether we use an old bootloader (setting the address at
+ * 0xd0000000) or a new bootloader (setting the address at
+ * 0xf1000000). In the ->map_io() function, when the bit is already
+ * set, there is nothing to do, we are already using a new bootloader
+ * and internal registers are at 0xf1000000.
+ *
+ * However, when the bit is not set, we do a static mapping of the
+ * register area that allows to change the internal register address,
+ * we change this address, and set this bit.
+ *
+ * Note that this mechanism also needs cooperation from the
+ * earlyprintk addruart handler (in arch/arm/include/debug/mvebu.S)
+ * and the early secondary CPU initialization code (in
+ * arch/arm/mach-mvebu/headsmp.S).
+ */
+
+#define NEW_INTERNAL_REGS_ADDR  0xf1000000
+#define OLD_INTERNAL_REGS_ADDR  0xd0000000
+#define MBUS_OLD_PHYS_ADDR      (OLD_INTERNAL_REGS_ADDR + 0x20000)
+#define MBUS_OLD_VIRT_ADDR      0xfebfe000
+#define  MBUS_INTERNAL_REGS_ADDR      0x80
+
+/*
+ * We really want this variable to be part of the .data section,
+ * because it gets accessed by the earlyprintk code before BSS gets
+ * initialized to zero. This variable indicates whether the switch to
+ * the new register has taken place or not. It is needed because the
+ * CP15 bit used by the bootloader to tell the kernel where the
+ * registers are mapped is cleared to 0 at the first 'wfi'
+ * instruction. So we can't use it for the entire life of the system,
+ * and we instead use this boolean to indicate to the earlyprintk code
+ * that the switch has occured and it doesn't need to look at the CP15
+ * bit anymore.
+ */
+unsigned int __section(.data) mvebu_switched_regs = 0;
+
+/*
+ * Detect if we're running an old bootloader that has set the physical
+ * base address of internal registers to 0xd0000000
+ */
+static int armada_370_xp_has_old_bootloader(void)
+{
+	u32 val;
+	asm volatile("mrc p15, 0, %0, c5, c0, 0" : "=r"(val));
+	return !(val & BIT(11));
+}
+
 static void __init armada_370_xp_map_io(void)
 {
+	if (armada_370_xp_has_old_bootloader()) {
+		struct map_desc desc;
+		void __iomem *mbus = IOMEM(MBUS_OLD_VIRT_ADDR);
+
+		desc.virtual = MBUS_OLD_VIRT_ADDR;
+		desc.pfn     = __phys_to_pfn(MBUS_OLD_PHYS_ADDR);
+		desc.length  = PAGE_SIZE;
+		desc.type    = MT_DEVICE;
+
+		/*
+		 * Map the area that contains the registers that
+		 * allows to change the base address of internal
+		 * registers.
+		 */
+		iotable_init(&desc, 1);
+
+		/*
+		 * Change the physical base address of the
+		 * registers. From now on, and until
+		 * debug_ll_io_init() is called below, it is not
+		 * possible to do any print, because earlyprintk will
+		 * not work.
+		 */
+		writel(NEW_INTERNAL_REGS_ADDR, mbus + MBUS_INTERNAL_REGS_ADDR);
+	}
+
+	/*
+	 * Either the registers have been switched by the above code,
+	 * or the bootloader had already set the register space at the
+	 * right location. In both cases, tell the earlyprintk code it
+	 * can now use the new location.
+	 */
+	mvebu_switched_regs = 1;
+
 	debug_ll_io_init();
 }
 
diff --git a/arch/arm/mach-mvebu/armada-370-xp.h b/arch/arm/mach-mvebu/armada-370-xp.h
index 585e147..ff9adc4 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.h
+++ b/arch/arm/mach-mvebu/armada-370-xp.h
@@ -15,7 +15,7 @@
 #ifndef __MACH_ARMADA_370_XP_H
 #define __MACH_ARMADA_370_XP_H
 
-#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
+#define ARMADA_370_XP_REGS_PHYS_BASE	0xf1000000
 
 /* These defines can go away once mvebu-mbus has a DT binding */
 #define ARMADA_370_XP_MBUS_WINS_BASE    (ARMADA_370_XP_REGS_PHYS_BASE + 0x20000)
-- 
1.7.9.5

[PATCH 1/9] arm: mvebu: fix length of SATA registers area in .dtsi

[Jason Cooper]

On Tue, May 21, 2013 at 12:33:26PM +0200, Thomas Petazzoni wrote:
> The length of the registers area for the Marvell 370/XP SATA
> controller was incorrect in the .dtsi: 0x2400 while it should have
> been 0x5000. Until now, this problem wasn't noticed because there was
> a large static mapping for all I/Os set up by ->map_io(). But since
> we're going to get rid of this static mapping, we need to ensure that
> the register areas are properly sized.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to mvebu/fixes

thx,

Jason.

[PATCH 2/9] arm: mvebu: fix length of Ethernet registers area in .dtsi

[Jason Cooper]

On Tue, May 21, 2013 at 12:33:27PM +0200, Thomas Petazzoni wrote:
> The length of the registers area for the Marvell 370/XP Ethernet
> controller was incorrect in the .dtsi: 0x2400 while it should have
> been 0x4000. Until now, this problem wasn't noticed because there was
> a large static mapping for all I/Os set up by ->map_io(). But since
> we're going to get rid of this static mapping, we need to ensure that
> the register areas are properly sized.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi     |    4 ++--
>  arch/arm/boot/dts/armada-xp-mv78460.dtsi |    2 +-
>  arch/arm/boot/dts/armada-xp.dtsi         |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Applied to mvebu/fixes

thx,

Jason.

[PATCH 3/9] arm: mvebu: mark functions of armada-370-xp.c as static

[Jason Cooper]

On Tue, May 21, 2013 at 12:33:28PM +0200, Thomas Petazzoni wrote:
> All the functions in armada-370-xp.c are called from the
> DT_MACHINE_START function pointers, so there is no need for them to be
> visible outside of this file, and we therefore mark them as static.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied to mvebu/cleanup

thx,

Jason.

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Jason Cooper]

On Tue, May 21, 2013 at 12:33:31PM +0200, Thomas Petazzoni wrote:
> Current, the L2 cache and the mvebu-mbus drivers are initialized at
> ->init_early() time. However, at ->init_early() time, ioremap() only
> works if a static I/O mapping has already been put in place. If it's
> not the case, it tries to do a memory allocation with kmalloc() which
> is not possible so early at this stage of the initialization.
> 
> Since we want to get rid of the static I/O mapping, we cannot
> initialize the L2 cache driver and the mvebu-mbus driver so early. So,
> we move their initialization to the ->init_time() level, which is
> slightly later (so ioremap() works properly), but sufficiently early
> to be before the call of the ->smp_prepare_cpus() hook, which creates
> an address decoding window for the BootROM, which requires the
> mvebu-mbus driver to be properly initialized.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c |   24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

This doesn't apply when based on mvebu/cleanup because of:

  49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()

I tried hacking it up to put it in mvebu/soc-internal_regs for a few
rounds of testing in linux-next during review, however, I'd prefer you
rebase this on top of mvebu/cleanup.

My version ended up looking like:

---8<----
diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index b9319c4..75ebf56 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -44,14 +44,11 @@ static void __init armada_370_xp_map_io(void)
 
 static void __init armada_370_xp_timer_and_clk_init(void)
 {
+	char *mbus_soc_name;
+
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
 	coherency_init();
-}
-
-static void __init armada_370_xp_init_early(void)
-{
-	char *mbus_soc_name;
 
 	/*
 	 * This initialization will be replaced by a DT-based
@@ -87,7 +84,6 @@ DT_MACHINE_START(ARMADA_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= armada_370_xp_dt_init,
 	.map_io		= armada_370_xp_map_io,
-	.init_early	= armada_370_xp_init_early,
 	.init_time	= armada_370_xp_timer_and_clk_init,
 	.restart	= mvebu_restart,
 	.dt_compat	= armada_370_xp_dt_compat,

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 21 May 2013 10:16:21 -0400, Jason Cooper wrote:

> This doesn't apply when based on mvebu/cleanup because of:
> 
>   49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()

Right.

> I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> rounds of testing in linux-next during review, however, I'd prefer you
> rebase this on top of mvebu/cleanup.

So you mean my next version of the "Internal registers switch" should
be based on mvebu/cleanup, right? There will be some next version in
any case, because I just found a bug in the latest patch when booting
from a bootloader that has already done the switch to 0xF1.

> My version ended up looking like:
> 
> ---8<----
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index b9319c4..75ebf56 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -44,14 +44,11 @@ static void __init armada_370_xp_map_io(void)
>  
>  static void __init armada_370_xp_timer_and_clk_init(void)
>  {
> +	char *mbus_soc_name;
> +
>  	mvebu_clocks_init();
>  	armada_370_xp_timer_init();
>  	coherency_init();
> -}
> -
> -static void __init armada_370_xp_init_early(void)
> -{
> -	char *mbus_soc_name;
>  
>  	/*
>  	 * This initialization will be replaced by a DT-based
> @@ -87,7 +84,6 @@ DT_MACHINE_START(ARMADA_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>  	.smp		= smp_ops(armada_xp_smp_ops),
>  	.init_machine	= armada_370_xp_dt_init,
>  	.map_io		= armada_370_xp_map_io,
> -	.init_early	= armada_370_xp_init_early,
>  	.init_time	= armada_370_xp_timer_and_clk_init,
>  	.restart	= mvebu_restart,
>  	.dt_compat	= armada_370_xp_dt_compat,

>From a quick look your version looks correct. Everything ends up into
the ->init_time() hook, which is correct. The only thing I had left in
->init_early() in my version was the DMA coherent thing, which is no
longer needed.

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Jason Cooper]

Thomas,

On Tue, May 21, 2013 at 05:37:07PM +0200, Thomas Petazzoni wrote:
> On Tue, 21 May 2013 10:16:21 -0400, Jason Cooper wrote:
> > This doesn't apply when based on mvebu/cleanup because of:
> > 
> >   49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()
> 
> Right.
> 
> > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > rounds of testing in linux-next during review, however, I'd prefer you
> > rebase this on top of mvebu/cleanup.
> 
> So you mean my next version of the "Internal registers switch" should
> be based on mvebu/cleanup, right?

Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
well for the next version, since I've pulled those.

> There will be some next version in any case, because I just found a
> bug in the latest patch when booting from a bootloader that has
> already done the switch to 0xF1.

Ok, I'll hold off putting it up for testing until v2.

thx,

Jason.

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:

> > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > rounds of testing in linux-next during review, however, I'd prefer you
> > > rebase this on top of mvebu/cleanup.
> > 
> > So you mean my next version of the "Internal registers switch" should
> > be based on mvebu/cleanup, right?
> 
> Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> well for the next version, since I've pulled those.

Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
patches will be there (which doesn't make the thing easy for testing
here, because to test, I actually need all the patches in the same
branch). Just trying to figure the workflow :)

> > There will be some next version in any case, because I just found a
> > bug in the latest patch when booting from a bootloader that has
> > already done the switch to 0xF1.
> 
> Ok, I'll hold off putting it up for testing until v2.

Well, the current version should be ok build-wise, and works with all
currently available Marvell bootloaders. The bug can only be seen with
a bootloader version that has not yet been really released by Marvell.

But anyway, v2 will follow shortly. With this v1, I was mainly hoping
for some feedback, to see if the solution was acceptable or not.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Jason Cooper]

Thomas,

On Tue, May 21, 2013 at 06:10:18PM +0200, Thomas Petazzoni wrote:
> On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:
> > > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > > rounds of testing in linux-next during review, however, I'd prefer you
> > > > rebase this on top of mvebu/cleanup.
> > > 
> > > So you mean my next version of the "Internal registers switch" should
> > > be based on mvebu/cleanup, right?
> > 
> > Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> > well for the next version, since I've pulled those.
> 
> Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
> suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
> mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
> patches will be there (which doesn't make the thing easy for testing
> here, because to test, I actually need all the patches in the same
> branch). Just trying to figure the workflow :)

For your development, I would checkout v3.10-rc2, merge /fixes, then
merge /cleanup, the put 4-9 on top.  Then, when sending, just do 4-9
with a note about the merge dependency on /cleanup, and the boot
dependency on /fixes.

If you'd like, I can hold off on merging it until I can base the series
on an -rc including patches 1 and 2.  As is, I might move #3 into the
same branch as 4-9, but we'll still have the conflict I highlighted
earlier.  And anyone resolving it will probably miss the .init_early
removal.

> > > There will be some next version in any case, because I just found a
> > > bug in the latest patch when booting from a bootloader that has
> > > already done the switch to 0xF1.
> > 
> > Ok, I'll hold off putting it up for testing until v2.
> 
> Well, the current version should be ok build-wise, and works with all
> currently available Marvell bootloaders. The bug can only be seen with
> a bootloader version that has not yet been really released by Marvell.
> 
> But anyway, v2 will follow shortly. With this v1, I was mainly hoping
> for some feedback, to see if the solution was acceptable or not.

That was a great explanation of a Schr?dinger's Register ;-)

thx,

Jason.

[PATCH 6/9] arm: mvebu: move cache and mvebu-mbus initialization later

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 21 May 2013 12:37:25 -0400, Jason Cooper wrote:

> On Tue, May 21, 2013 at 06:10:18PM +0200, Thomas Petazzoni wrote:
> > On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:
> > > > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > > > rounds of testing in linux-next during review, however, I'd prefer you
> > > > > rebase this on top of mvebu/cleanup.
> > > > 
> > > > So you mean my next version of the "Internal registers switch" should
> > > > be based on mvebu/cleanup, right?
> > > 
> > > Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> > > well for the next version, since I've pulled those.
> > 
> > Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
> > suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
> > mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
> > patches will be there (which doesn't make the thing easy for testing
> > here, because to test, I actually need all the patches in the same
> > branch). Just trying to figure the workflow :)
> 
> For your development, I would checkout v3.10-rc2, merge /fixes, then
> merge /cleanup, the put 4-9 on top.  Then, when sending, just do 4-9
> with a note about the merge dependency on /cleanup, and the boot
> dependency on /fixes.

Ok, sounds good.

> If you'd like, I can hold off on merging it until I can base the series
> on an -rc including patches 1 and 2.  As is, I might move #3 into the
> same branch as 4-9, but we'll still have the conflict I highlighted
> earlier.  And anyone resolving it will probably miss the .init_early
> removal.

Don't worry, the solution you've proposed earlier sounds fine to me.

> > Well, the current version should be ok build-wise, and works with all
> > currently available Marvell bootloaders. The bug can only be seen with
> > a bootloader version that has not yet been really released by Marvell.
> > 
> > But anyway, v2 will follow shortly. With this v1, I was mainly hoping
> > for some feedback, to see if the solution was acceptable or not.
> 
> That was a great explanation of a Schr?dinger's Register ;-)

That's a nice name for such a complicated register, indeed :)

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Willy Tarreau]

Hi Thomas,

On Tue, May 21, 2013 at 12:33:25PM +0200, Thomas Petazzoni wrote:
(...)
> As it was explained above, we unfortunately can't read the
> current base address of the internal register window, so we need a
> different mechanism to know if the bootloader has done the remapping
> at 0xF1000000 (new generation bootloader) or has left the internal
> registers at 0xD0000000 (old generation bootloader).

Just out of curiosity, what happens if you touch the register at the
wrong address ? I mean, if you blindly write to the 0xD0xxxxxx address
that you want the registers to be mapped at 0xF1xxxxxx. Will the chip
hang, will it silently ignore the sequence ? Because maybe you don't
need to detect using CP15 whether the remapping was done, you could
simply perform it unconditionally. Another point would be that if the
boot loader is the only one to know whether the registeres were remapped,
then maybe it could put something into the atags or DT about this. And
maybe we'll simply find that DT-enabled boot loaders are remapped and
that non-DT ones are not.

Just a few ideas since you feel a bit worried about the fragility of
the CP15 bit :-)

Willy

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Willy Tarreau,

On Tue, 21 May 2013 21:38:03 +0200, Willy Tarreau wrote:

> Just out of curiosity, what happens if you touch the register at the
> wrong address ?

On Armada 370/XP, it hangs the CPU.

> I mean, if you blindly write to the 0xD0xxxxxx address
> that you want the registers to be mapped at 0xF1xxxxxx. Will the chip
> hang, will it silently ignore the sequence ?

Answered above: it hangs.

> Because maybe you don't need to detect using CP15 whether the remapping was done,
> you could simply perform it unconditionally. Another point would be that if the
> boot loader is the only one to know whether the registeres were remapped,
> then maybe it could put something into the atags or DT about this. And
> maybe we'll simply find that DT-enabled boot loaders are remapped and
> that non-DT ones are not.
> 
> Just a few ideas since you feel a bit worried about the fragility of
> the CP15 bit :-)

No, as explained above, we have considered a way of "detecting" whether
the remapping occurred, or to unconditionally do the remapping. But
none of those solutions work: we need to know, through some external
mechanism, whether the remapping has already taken place or not.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Willy Tarreau]

On Tue, May 21, 2013 at 10:12:35PM +0200, Thomas Petazzoni wrote:
> Dear Willy Tarreau,
> 
> On Tue, 21 May 2013 21:38:03 +0200, Willy Tarreau wrote:
> 
> > Just out of curiosity, what happens if you touch the register at the
> > wrong address ?
> 
> On Armada 370/XP, it hangs the CPU.
> 
> > I mean, if you blindly write to the 0xD0xxxxxx address
> > that you want the registers to be mapped at 0xF1xxxxxx. Will the chip
> > hang, will it silently ignore the sequence ?
> 
> Answered above: it hangs.
> 
> > Because maybe you don't need to detect using CP15 whether the remapping was done,
> > you could simply perform it unconditionally. Another point would be that if the
> > boot loader is the only one to know whether the registeres were remapped,
> > then maybe it could put something into the atags or DT about this. And
> > maybe we'll simply find that DT-enabled boot loaders are remapped and
> > that non-DT ones are not.
> > 
> > Just a few ideas since you feel a bit worried about the fragility of
> > the CP15 bit :-)
> 
> No, as explained above, we have considered a way of "detecting" whether
> the remapping occurred, or to unconditionally do the remapping. But
> none of those solutions work: we need to know, through some external
> mechanism, whether the remapping has already taken place or not.

OK, thank you for these precisions.

Best regards,
Willy

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Sebastian Hesselbarth]

On 05/21/2013 09:38 PM, Willy Tarreau wrote:
> On Tue, May 21, 2013 at 12:33:25PM +0200, Thomas Petazzoni wrote:
> (...)
>> As it was explained above, we unfortunately can't read the
>> current base address of the internal register window, so we need a
>> different mechanism to know if the bootloader has done the remapping
>> at 0xF1000000 (new generation bootloader) or has left the internal
>> registers at 0xD0000000 (old generation bootloader).
>
> Just out of curiosity, what happens if you touch the register at the
> wrong address ? I mean, if you blindly write to the 0xD0xxxxxx address
> that you want the registers to be mapped at 0xF1xxxxxx. Will the chip
> hang, will it silently ignore the sequence ? Because maybe you don't
> need to detect using CP15 whether the remapping was done, you could
> simply perform it unconditionally.

Willy,

I tried the above on Dove which is possibly affected by the workaround
as it is also ARMv7. In Dove you can access unmapped addresses without
any problems, but as Thomas already stated Armada 370/XP just hang.
Must be some default AHB slave in Dove, which Armada 370/XP is missing.

Sebastian

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Greg]

Le 22/05/2013 12:01, Sebastian Hesselbarth a ?crit :
> On 05/21/2013 09:38 PM, Willy Tarreau wrote:
>> On Tue, May 21, 2013 at 12:33:25PM +0200, Thomas Petazzoni wrote:
>> (...)
>>> As it was explained above, we unfortunately can't read the
>>> current base address of the internal register window, so we need a
>>> different mechanism to know if the bootloader has done the remapping
>>> at 0xF1000000 (new generation bootloader) or has left the internal
>>> registers at 0xD0000000 (old generation bootloader).
>>
>> Just out of curiosity, what happens if you touch the register at the
>> wrong address ? I mean, if you blindly write to the 0xD0xxxxxx address
>> that you want the registers to be mapped at 0xF1xxxxxx. Will the chip
>> hang, will it silently ignore the sequence ? Because maybe you don't
>> need to detect using CP15 whether the remapping was done, you could
>> simply perform it unconditionally.
>
> Willy,
>
> I tried the above on Dove which is possibly affected by the workaround
> as it is also ARMv7. In Dove you can access unmapped addresses without
> any problems, but as Thomas already stated Armada 370/XP just hang.
> Must be some default AHB slave in Dove, which Armada 370/XP is missing.

According to functionnal specs, when accessing a unmapped address, you 
will trigger a "coherency fabric" error named AccessErr (register 
0x00020258).
I guess this will trigger an interrupt which will hang the CPU if the 
vector is not handled.

Greg

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Thomas Petazzoni]

Arnd, Olof, Jason, Andrew, Gregory,

(Taking the terrible freedom of doing some top-posting)

As the arm-soc and Marvell maintainers, would you mind having a look
specifically at the below patch, and give your Acked-by or comments?

I believe the other 8 patches that come earlier in this series are
relatively straightforward. However, this specific patch is a bit
complex, and may raise some eye balls.

So rather than having the eye balls raised late in the development
cycle, I'd like to have them raised now, so that if some comments are
made, I have enough time to either discuss them and/or rework the code
accordingly.

Before making comments, please also have a read to the cover letter of
the patch series
(http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/169701.html),
which explains other details of why this solution is needed. The entire
problem cannot be understood if you don't have a serious read at the
commit log and the cover letter.

Thanks,

Thomas

On Tue, 21 May 2013 12:33:34 +0200, Thomas Petazzoni wrote:
> On Marvell platforms, the "internal registers" is a window of 1 MB of
> the physical address space that contains all the registers for the
> various peripherals of the SoC (timers, interrupt controllers, GPIO
> controllers, Ethernet, etc.). The base address of this 1 MB window is
> configurable, and this base address is configured using a register
> that is itself part of this window.
> 
> All earlier families of Marvell EBU SoCs supported by Linux used
> 0xf1000000 as the base address of the internal registers window. For
> some reason, early versions of the Marvell 370/XP platforms used
> 0xd0000000 as the base address. This base address is set up by the
> bootloader. However, in order to keep as much physical address space
> to address RAM below the 4 GB limit, we had to switch the base address
> of the internal registers window back to 0xf1000000, and we wanted to
> achieve that without breaking existing platforms, where the bootloader
> was initially setting the base address to 0xd0000000. So the kernel is
> responsible for switching to 0xf1000000 when needed.
> 
> One particular bit of a CP15 register has been chosen to indicate
> whether we use an old bootloader (setting the address at 0xd0000000)
> or a new bootloader (setting the address at 0xf1000000). In the
> ->map_io() function, when the bit is already set, there is nothing to
> do, we are already using a new bootloader and internal registers are
> at 0xf1000000.
> 
> However, when the bit is not set, we do a static mapping of the
> register area that allows to change the internal register address, we
> change this address, and set this bit.
> 
> Note that this mechanism also needs cooperation from the earlyprintk
> addruart handler (in arch/arm/include/debug/mvebu.S). This is where
> things get complex: the CP15 bit that has been chosen gets reset to
> zero at the first call to the 'wfi' instruction. So it cannot be used
> during the entire life of the system. Instead, whenever the ->map_io()
> function has switched to the internal register space to the new
> address, it sets a global variable 'mvebu_switched_regs', which the
> earlyprintk reads. If it's true, then the move has already been done
> and there is no need to read the CP15 register. If
> 'mvebu_switched_regs' is false, we are before the register move, so we
> need to read the CP15 bit to find out whether we're being booted from
> an old bootloader (which mapped the register space at 0xd0000000) or a
> new bootloader (which mapped the register space at 0xf1000000).
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi |    2 +-
>  arch/arm/boot/dts/armada-370.dtsi    |    2 +-
>  arch/arm/include/debug/mvebu.S       |   77 +++++++++++++++++++++++--
>  arch/arm/mach-mvebu/armada-370-xp.c  |  104 ++++++++++++++++++++++++++++++++++
>  arch/arm/mach-mvebu/armada-370-xp.h  |    2 +-
>  5 files changed, 178 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 96a6bcd..1e7561e 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -33,7 +33,7 @@
>  		#size-cells = <1>;
>  		compatible = "simple-bus";
>  		interrupt-parent = <&mpic>;
> -		ranges = <0 0 0xd0000000 0x100000>;
> +		ranges = <0 0 0xf1000000 0x100000>;
>  
>  		internal-regs {
>  			compatible = "simple-bus";
> diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
> index b2c1b5a..7ff9c1b 100644
> --- a/arch/arm/boot/dts/armada-370.dtsi
> +++ b/arch/arm/boot/dts/armada-370.dtsi
> @@ -29,7 +29,7 @@
>  	};
>  
>  	soc {
> -		ranges = <0 0xd0000000 0x100000>;
> +		ranges = <0 0xf1000000 0x100000>;
>  		internal-regs {
>  			system-controller at 18200 {
>  				compatible = "marvell,armada-370-xp-system-controller";
> diff --git a/arch/arm/include/debug/mvebu.S b/arch/arm/include/debug/mvebu.S
> index df191af..dba702a 100644
> --- a/arch/arm/include/debug/mvebu.S
> +++ b/arch/arm/include/debug/mvebu.S
> @@ -5,20 +5,85 @@
>   *
>   * Lior Amsalem <alior@marvell.com>
>   * Gregory Clement <gregory.clement@free-electrons.com>
> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>  */
>  
> -#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
> -#define ARMADA_370_XP_REGS_VIRT_BASE	0xfec00000
> +#define ARMADA_370_XP_UART_PHYS_BASE_OLD	0xd0012000
> +#define ARMADA_370_XP_UART_PHYS_BASE_NEW	0xf1012000
> +#define ARMADA_370_XP_UART_VIRT_BASE_OLD	0xfec12000
> +#define ARMADA_370_XP_UART_VIRT_BASE_NEW	0xfebff000
>  
>  	.macro	addruart, rp, rv, tmp
> -	ldr	\rp, =ARMADA_370_XP_REGS_PHYS_BASE
> -	ldr	\rv, =ARMADA_370_XP_REGS_VIRT_BASE
> -	orr	\rp, \rp, #0x00012000
> -	orr	\rv, \rv, #0x00012000
> +
> +#ifndef ZIMAGE
> +	/*
> +	 * This loads the value of mvebu_switched_regs. We need a
> +	 * little bit of gymnastic here to handle the fact that the
> +	 * addruart code is duplicated in different places, and called
> +	 * sometimes with the MMU disabled, sometimes the MMU enabled.
> +	 *
> +	 * Notice that when this earlyprintk code is linked into the
> +	 * kernel decompressor (i.e ZIMAGE is defined), this code isn't
> +	 * compiled. The reason is because the global mvebu_switched_regs
> +	 * variable is not available in the decompressor code. At this
> +	 * early stage, we simply test the CP15 bit (below) to know where
> +	 * the internal registers are.
> +	 */
> +	adr	\rp, addruart_switched_regs_\@
> +	ldr	\rv, [\rp]
> +	sub	\rv, \rv, \rp
> +	ldr	\rp, [\rp, #4]
> +	sub	\tmp, \rp, \rv
> +	ldr	\rp, [\tmp, #0]
> +	cmp	\rp, #0
> +
> +	/* If we have already switched, then use the new register
> +	   address */
> +	bne	addruart_new_\@
> +#endif
> +
> +	/*
> +	 * We haven't switched the register location, so test the CP15
> +	 * register bit to find whether we are:
> +	 * - with an old bootloader setting the base address to
> +	 *   0xd0000000
> +	 * - with a new bootloader that has already set the
> +	 *   base address to 0xf1000000
> +	 * We adapt the physical address returned depending on the
> +	 * value of this bit, but also the virtual address, because we
> +	 * can't remap the old physical address and the new physical
> +	 * address at the same location.
> +	 */
> +	mrc	p15, 0, \tmp, c5, c0, 0
> +	and	\tmp, \tmp, #(1 << 11)
> +	cmp	\tmp, #0
> +	bne	addruart_new_\@
> +
> +addruart_old_\@:
> +	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_OLD
> +	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_OLD
> +	b	addruart_out_\@
> +addruart_new_\@:
> +	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_NEW
> +	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_NEW
> +	b	addruart_out_\@
> +
> +#ifndef ZIMAGE
> +	/*
> +	 * Global variable mvebu_switched_regs not visible in the
> +	 * decompressor code
> +	 */
> +	.align
> +addruart_switched_regs_\@:
> +	.long	.
> +	.long	mvebu_switched_regs
> +#endif
> +
> +addruart_out_\@:
>  	.endm
>  
>  #define UART_SHIFT	2
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index 90cc0e8..c56875e 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -29,8 +29,112 @@
>  #include "common.h"
>  #include "coherency.h"
>  
> +/*
> + * Note on the internal register address.
> + *
> + * On Marvell platforms, the "internal registers" is a window of 1 MB
> + * of the physical address space that contains all the registers for
> + * the various peripherals of the SoC (timers, interrupt controllers,
> + * GPIO controllers, Ethernet, etc.). The base address of this 1 MB
> + * window is configurable, and this base address is configured using a
> + * register that is itself part of this window.
> + *
> + * All earlier families of Marvell EBU SoCs supported by Linux used
> + * 0xf1000000 as the base address of the internal registers
> + * window. For some reason, early versions of the Marvell 370/XP
> + * platforms used 0xd0000000 as the base address. This base address is
> + * set up by the bootloader. However, in order to keep as much
> + * physical address space to address RAM below the 4 GB limit, we had
> + * to switch the base address of the internal registers window back to
> + * 0xf1000000, and we wanted to achieve that without breaking existing
> + * platforms, where the bootloader was initially setting the base
> + * address to 0xd0000000. So the kernel is responsible for switching
> + * to 0xf1000000 when needed.
> + *
> + * One particular bit of a CP15 register has been chosen to indicate
> + * whether we use an old bootloader (setting the address at
> + * 0xd0000000) or a new bootloader (setting the address at
> + * 0xf1000000). In the ->map_io() function, when the bit is already
> + * set, there is nothing to do, we are already using a new bootloader
> + * and internal registers are at 0xf1000000.
> + *
> + * However, when the bit is not set, we do a static mapping of the
> + * register area that allows to change the internal register address,
> + * we change this address, and set this bit.
> + *
> + * Note that this mechanism also needs cooperation from the
> + * earlyprintk addruart handler (in arch/arm/include/debug/mvebu.S)
> + * and the early secondary CPU initialization code (in
> + * arch/arm/mach-mvebu/headsmp.S).
> + */
> +
> +#define NEW_INTERNAL_REGS_ADDR  0xf1000000
> +#define OLD_INTERNAL_REGS_ADDR  0xd0000000
> +#define MBUS_OLD_PHYS_ADDR      (OLD_INTERNAL_REGS_ADDR + 0x20000)
> +#define MBUS_OLD_VIRT_ADDR      0xfebfe000
> +#define  MBUS_INTERNAL_REGS_ADDR      0x80
> +
> +/*
> + * We really want this variable to be part of the .data section,
> + * because it gets accessed by the earlyprintk code before BSS gets
> + * initialized to zero. This variable indicates whether the switch to
> + * the new register has taken place or not. It is needed because the
> + * CP15 bit used by the bootloader to tell the kernel where the
> + * registers are mapped is cleared to 0 at the first 'wfi'
> + * instruction. So we can't use it for the entire life of the system,
> + * and we instead use this boolean to indicate to the earlyprintk code
> + * that the switch has occured and it doesn't need to look at the CP15
> + * bit anymore.
> + */
> +unsigned int __section(.data) mvebu_switched_regs = 0;
> +
> +/*
> + * Detect if we're running an old bootloader that has set the physical
> + * base address of internal registers to 0xd0000000
> + */
> +static int armada_370_xp_has_old_bootloader(void)
> +{
> +	u32 val;
> +	asm volatile("mrc p15, 0, %0, c5, c0, 0" : "=r"(val));
> +	return !(val & BIT(11));
> +}
> +
>  static void __init armada_370_xp_map_io(void)
>  {
> +	if (armada_370_xp_has_old_bootloader()) {
> +		struct map_desc desc;
> +		void __iomem *mbus = IOMEM(MBUS_OLD_VIRT_ADDR);
> +
> +		desc.virtual = MBUS_OLD_VIRT_ADDR;
> +		desc.pfn     = __phys_to_pfn(MBUS_OLD_PHYS_ADDR);
> +		desc.length  = PAGE_SIZE;
> +		desc.type    = MT_DEVICE;
> +
> +		/*
> +		 * Map the area that contains the registers that
> +		 * allows to change the base address of internal
> +		 * registers.
> +		 */
> +		iotable_init(&desc, 1);
> +
> +		/*
> +		 * Change the physical base address of the
> +		 * registers. From now on, and until
> +		 * debug_ll_io_init() is called below, it is not
> +		 * possible to do any print, because earlyprintk will
> +		 * not work.
> +		 */
> +		writel(NEW_INTERNAL_REGS_ADDR, mbus + MBUS_INTERNAL_REGS_ADDR);
> +	}
> +
> +	/*
> +	 * Either the registers have been switched by the above code,
> +	 * or the bootloader had already set the register space at the
> +	 * right location. In both cases, tell the earlyprintk code it
> +	 * can now use the new location.
> +	 */
> +	mvebu_switched_regs = 1;
> +
>  	debug_ll_io_init();
>  }
>  
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.h b/arch/arm/mach-mvebu/armada-370-xp.h
> index 585e147..ff9adc4 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.h
> +++ b/arch/arm/mach-mvebu/armada-370-xp.h
> @@ -15,7 +15,7 @@
>  #ifndef __MACH_ARMADA_370_XP_H
>  #define __MACH_ARMADA_370_XP_H
>  
> -#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
> +#define ARMADA_370_XP_REGS_PHYS_BASE	0xf1000000
>  
>  /* These defines can go away once mvebu-mbus has a DT binding */
>  #define ARMADA_370_XP_MBUS_WINS_BASE    (ARMADA_370_XP_REGS_PHYS_BASE + 0x20000)



-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Wed, May 22, 2013 at 12:01:29PM +0200, Sebastian Hesselbarth wrote:
> I tried the above on Dove which is possibly affected by the workaround
> as it is also ARMv7. In Dove you can access unmapped addresses without
> any problems, but as Thomas already stated Armada 370/XP just hang.
> Must be some default AHB slave in Dove, which Armada 370/XP is missing.

It may be some clock setting somewhere on the 370/XP.  The Dove will hang
as well if you prod at peripherals which you've turned their clocks off.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Wed, 22 May 2013 14:43:36 +0100, Russell King - ARM Linux wrote:
> On Wed, May 22, 2013 at 12:01:29PM +0200, Sebastian Hesselbarth wrote:
> > I tried the above on Dove which is possibly affected by the workaround
> > as it is also ARMv7. In Dove you can access unmapped addresses without
> > any problems, but as Thomas already stated Armada 370/XP just hang.
> > Must be some default AHB slave in Dove, which Armada 370/XP is missing.
> 
> It may be some clock setting somewhere on the 370/XP.  The Dove will hang
> as well if you prod at peripherals which you've turned their clocks off.

The register in question (offset 0x20080 from the base address of the
internal register) is part of the "core" registers, so I clearly don't
see how the system could even be booting if this clock was gated. No?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Andrew Lunn]

On Wed, May 22, 2013 at 03:27:00PM +0200, Thomas Petazzoni wrote:
> Arnd, Olof, Jason, Andrew, Gregory,
> 
> (Taking the terrible freedom of doing some top-posting)
> 
> As the arm-soc and Marvell maintainers, would you mind having a look
> specifically at the below patch, and give your Acked-by or comments?
> 
> I believe the other 8 patches that come earlier in this series are
> relatively straightforward. However, this specific patch is a bit
> complex, and may raise some eye balls.
> 
> So rather than having the eye balls raised late in the development
> cycle, I'd like to have them raised now, so that if some comments are
> made, I have enough time to either discuss them and/or rework the code
> accordingly.
> 
> Before making comments, please also have a read to the cover letter of
> the patch series
> (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/169701.html),
> which explains other details of why this solution is needed. The entire
> problem cannot be understood if you don't have a serious read at the
> commit log and the cover letter.

Hi Thomas

You gave a good explanation why the CP15 bit its needed, etc. I liked
the comment.

It might be worth cross posting both u-boot and barebox lists, since
they are somewhat involved as well. Are there any other boot loaders
used on 370 and XP.

What happens with future chips. 375 springs to mind. I assume its
Marvell bootloader will do the mapping before handing over to
Linux. Is it required to also set the bit in CP15? Are we setting off
down a road where all future Marvell chips which are compatible with
370/XP now need to set this bit?

How does this effect dove? It will get compiled into the same multi
arch kernel as 370/XP. I'm thinking about
arch/arm/include/debug/mvebu.S which might be shared. Dove is not
going to have this bit set. Kirkwood could also share this code, now
that the serial ports are all in the same location.

What does the ARM reference documentation say about this bit? Is its
meaning defined? Is there any danger when this bit is used for its
intended purpose?

	Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Thomas Petazzoni wrote:
> Dear Russell King - ARM Linux,
> 
> On Wed, 22 May 2013 14:43:36 +0100, Russell King - ARM Linux wrote:
> > On Wed, May 22, 2013 at 12:01:29PM +0200, Sebastian Hesselbarth wrote:
> > > I tried the above on Dove which is possibly affected by the workaround
> > > as it is also ARMv7. In Dove you can access unmapped addresses without
> > > any problems, but as Thomas already stated Armada 370/XP just hang.
> > > Must be some default AHB slave in Dove, which Armada 370/XP is missing.
> > 
> > It may be some clock setting somewhere on the 370/XP.  The Dove will hang
> > as well if you prod at peripherals which you've turned their clocks off.
> 
> The register in question (offset 0x20080 from the base address of the
> internal register) is part of the "core" registers, so I clearly don't
> see how the system could even be booting if this clock was gated. No?

It would hang if you add that offset to the wrong base address though.

	Arnd

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Thomas Petazzoni]

Dear Andrew Lunn,

On Wed, 22 May 2013 16:04:44 +0200, Andrew Lunn wrote:

> You gave a good explanation why the CP15 bit its needed, etc. I liked
> the comment.

Thanks.

> It might be worth cross posting both u-boot and barebox lists, since
> they are somewhat involved as well. Are there any other boot loaders
> used on 370 and XP.

As far as Barebox is concerned, Sebastian Hesselbarth and myself have
recently started the Marvell SoC support, so we kind of know what to do.
The current status is that (thanks to the latest patches from
Sebastian), Barebox remaps registers to 0xf1 very early, on all
platforms (currently, we support Kirkwood, Armada 370/XP and Dove). The
CP15 bit is *not* set yet, simply because we don't have enough drivers
yet in Barebox to even load a kernel to then boot it. But once it
happens, I'll adjust the Armada 370/XP code to set this CP15 bit.

As far as mainline U-Boot is concerned, it has zero support for Armada
370/XP at the moment.

> What happens with future chips. 375 springs to mind. I assume its
> Marvell bootloader will do the mapping before handing over to
> Linux. Is it required to also set the bit in CP15? Are we setting off
> down a road where all future Marvell chips which are compatible with
> 370/XP now need to set this bit?

No, the bootloaders provided by Marvell for the future chips will not
have this CP15 bit set. It will only be set for Armada 370/XP platforms.

> How does this effect dove? It will get compiled into the same multi
> arch kernel as 370/XP. I'm thinking about
> arch/arm/include/debug/mvebu.S which might be shared. Dove is not
> going to have this bit set. Kirkwood could also share this code, now
> that the serial ports are all in the same location.

This code will have to be conditionally compiled. When we'll integrate
Dove support in mach-mvebu, we'll make the code conditional. So you'll
have a choice:

 * Either you want to run a multiplatform capable kernel with support
   for Dove, Armada 370 and Armada XP in the same kernel, and you'll
   have to run a recent enough bootloader.

 * Or you have an old bootloader and you don't want to upgrade it, in
   this case you'll have to build a kernel which will only work on
   Armada 370/XP.

My plan is also to make this code show a warning when it detects that
the user is running an old bootloader, in order to encourage users to
move to newer versions of the bootloader. I don't want to show this
message right now, because such new versions are not yet widely
available, but let's say in 1 or 2 release cycles (3.12 or 3.13), we
can add such a message.

Currently, the only two platforms that are widely available with Armada
370 or Armada XP are the Globalscale Mirabox and the Plathome
Openblocks AX3. Once Globalscale and Plathome have released new
versions of their bootloader, we can document how users can upgrade. So
that hopefully, in the future, we may even decide to get rid of this
'hack' entirely.

> What does the ARM reference documentation say about this bit? Is its
> meaning defined? Is there any danger when this bit is used for its
> intended purpose?

I haven't checked myself the ARM reference documentation for this CP15
bit, but I know it has been chosen with care by Marvell engineers. It
is used when the CPU goes idle with the 'wfi' instruction, so the
entire hack makes the assumption that the CPU doesn't go idle between
the moment the bootloader sets the CP15 bit and the moment the kernel
checks it to understand where the internal registers are. If you want,
I can do some more research to give a (hopefully) more compelling
explanation for the choice of this particular bit.

Thanks again for your review and all your questions!

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Sebastian Hesselbarth]

On 05/22/2013 04:04 PM, Andrew Lunn wrote:
> It might be worth cross posting both u-boot and barebox lists, since
> they are somewhat involved as well. Are there any other boot loaders
> used on 370 and XP.

barebox is not neccessary, Thomas and I just merged SoC init to always
map on KW, Dove, and A370/XP. It is in a very early stage (console-only)
and will take some time to actually boot linux. And we have SoC
specific init that can set CP15 there.

> How does this effect dove? It will get compiled into the same multi
> arch kernel as 370/XP. I'm thinking about
> arch/arm/include/debug/mvebu.S which might be shared. Dove is not
> going to have this bit set. Kirkwood could also share this code, now
> that the serial ports are all in the same location.

Dove is difficult because u-boot always remaps and never sets the bit
in CP15 _but_ doesn't hang on writing wrong addresses. So remap by
writing to 0xd0.. will do no harm.

Sebastian

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Wed, May 22, 2013 at 03:52:18PM +0200, Thomas Petazzoni wrote:
> Dear Russell King - ARM Linux,
> 
> On Wed, 22 May 2013 14:43:36 +0100, Russell King - ARM Linux wrote:
> > On Wed, May 22, 2013 at 12:01:29PM +0200, Sebastian Hesselbarth wrote:
> > > I tried the above on Dove which is possibly affected by the workaround
> > > as it is also ARMv7. In Dove you can access unmapped addresses without
> > > any problems, but as Thomas already stated Armada 370/XP just hang.
> > > Must be some default AHB slave in Dove, which Armada 370/XP is missing.
> > 
> > It may be some clock setting somewhere on the 370/XP.  The Dove will hang
> > as well if you prod at peripherals which you've turned their clocks off.
> 
> The register in question (offset 0x20080 from the base address of the
> internal register) is part of the "core" registers, so I clearly don't
> see how the system could even be booting if this clock was gated. No?

The question which was asked was:

	What happens if you access a register at 0xd0xxxxxx when the
	registers are remapped to 0xf1xxxxxx?

The answer which was given was:

	It hangs on XP / 370, but doesn't on Dove.

If the registers have been remapped, then 0xd0xxxxxx is _not_ going to
hit any of the "core" registers.  We're hitting "something else" and
what I'm saying is that Dove will also hang if you hit a peripheral with
its clocks turned off.  Now, it may not be a peripheral, but a bus
segment which is unclocked on those which hang.

As Dove only has clock gating in a few of its peripherals, it could be
that whatever you hit at 0xd0xxxxxx on Dove is allowed to succeed because
of this.  Is that also true on XP or 370?

What we need to know is: after the boot loader passes control to the
kernel, and before _we_ start fiddling around with any clocks, is the
region at 0xd0xxxxxx accessible without hanging the CPU?

That's the point at which we'd want to probe for the remapped registers,
so that's the point where the test access must be made - and with the
system clocks in whatever state was left by the reset+boot loader.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Sebastian Hesselbarth]

On 05/22/2013 04:17 PM, Russell King - ARM Linux wrote:
> The question which was asked was:
>
> 	What happens if you access a register at 0xd0xxxxxx when the
> 	registers are remapped to 0xf1xxxxxx?
>
> The answer which was given was:
>
> 	It hangs on XP / 370, but doesn't on Dove.
>
...
> What we need to know is: after the boot loader passes control to the
> kernel, and before _we_ start fiddling around with any clocks, is the
> region at 0xd0xxxxxx accessible without hanging the CPU?
>
> That's the point at which we'd want to probe for the remapped registers,
> so that's the point where the test access must be made - and with the
> system clocks in whatever state was left by the reset+boot loader.

Russell,

I tried both Armada 370 and Dove on a very minimal console-only barebox.
Poking the wrong addresses always hangs 370 while Dove happily returns
zero.

Sebastian

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Tuesday 21 May 2013, Thomas Petazzoni wrote:
> Unfortunately, for Armada 370 and Armada XP, the current generation of
> bootloaders that are being shipped on Marvell evaluation boards, but
> also on products like the Plathome OpenBlocks AX3 or the Globalscale
> Mirabox do not do the remapping, and are leaving the internal
> registers at 0xD0000000.
> 
> However, this is causing some problems:
> 
>  1 The internal registers window sits at 3 GB, in the middle of the
>    0-4 GB area of physical memory, which is quite annoying when you
>    have more than 3 GB of memory. Having them at 0xF1000000 means that
>    you lose less RAM when you have more than 3 GB of physical memory
>    installed. And since Armada XP is LPAE capable, you can even have
>    more than 4 GB memory: we already have boards with 8 GB of memory
>    installed.

Just for completeness:

Do any of the machines that use the old boot loader have more
than 3 GB of memory?

>  2 This is different from Kirkwood and Dove, which makes sharing some
>    early code like earlyprintk complicated, while our goal is
>    ultimately to merge all Marvell EBU platforms into mach-mvebu.
> 
> (1) is really the primary motivation here, (2) is only a nice
> side-effect.
> 
> So, the new generation of bootloaders that are shipped on new Marvell
> Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
> starting Linux. The current kernel cannot boot on such platforms.

Does it boot if you disable the DEBUG_LL code and apply your
patches 1-8? The reason I'm asking is that you already cannot have
DEBUG_LL on a multiplatform kernel targeted at systems which don't
use the same UART. Adding a further restriction that they must map
the internal registers to the same physical address does not change
this a lot.

> Therefore, the last patch of this series adds some early code in the
> kernel, at the ->map_io() stage, to switch the internal registers from
> 0xD0000000 to 0xF1000000 if this has not been done already by the
> bootloader. As it was explained above, we unfortunately can't read the
> current base address of the internal register window, so we need a
> different mechanism to know if the bootloader has done the remapping
> at 0xF1000000 (new generation bootloader) or has left the internal
> registers at 0xD0000000 (old generation bootloader). In order to
> distinguish between those two cases, a CP15 bit is being used. Old
> bootloaders do not touch this CP15, so it is set to 0. New bootloaders
> set this CP15 bit to 1, so that the kernel knows that the remapping
> has already been done. The ->map_io() code looks at this bit to know
> if the remapping should be done or not.

As you already admit, using the CP15 register is a hack. It sounds
to me that a cleaner approach would be to put the correct address
into the device tree and use that value everywhere, rather than
hardcoding one or more addresses.

> Unfortunately, tweaking ->map_io() is not sufficient: we also want
> earlyprintk to work. And earlyprintk is used *before* ->map_io() is
> called, and *after* ->map_io() is called.

Note that by the time map_io is called, we no longer care about
the physical address, since the uart is only accessed through the
virtual mapping after __enable_mmu.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Gregory CLEMENT]

On 05/22/2013 04:33 PM, Arnd Bergmann wrote:
> On Tuesday 21 May 2013, Thomas Petazzoni wrote:
>> Unfortunately, for Armada 370 and Armada XP, the current generation of
>> bootloaders that are being shipped on Marvell evaluation boards, but
>> also on products like the Plathome OpenBlocks AX3 or the Globalscale
>> Mirabox do not do the remapping, and are leaving the internal
>> registers at 0xD0000000.
>>
>> However, this is causing some problems:
>>
>>  1 The internal registers window sits at 3 GB, in the middle of the
>>    0-4 GB area of physical memory, which is quite annoying when you
>>    have more than 3 GB of memory. Having them at 0xF1000000 means that
>>    you lose less RAM when you have more than 3 GB of physical memory
>>    installed. And since Armada XP is LPAE capable, you can even have
>>    more than 4 GB memory: we already have boards with 8 GB of memory
>>    installed.
> 
> Just for completeness:
> 
> Do any of the machines that use the old boot loader have more
> than 3 GB of memory?

As far as I know at least the Armada XP GP boards.

Moreover the OpenBlocks AX3 have non soldered memory, so
it could be possible to plug any amount of memory you want.
Howevere it is true that in this case you will have to update the
bootloader to setup the memory chip.

> 
>>  2 This is different from Kirkwood and Dove, which makes sharing some
>>    early code like earlyprintk complicated, while our goal is
>>    ultimately to merge all Marvell EBU platforms into mach-mvebu.
>>
>> (1) is really the primary motivation here, (2) is only a nice
>> side-effect.
>>
>> So, the new generation of bootloaders that are shipped on new Marvell
>> Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
>> starting Linux. The current kernel cannot boot on such platforms.
> 
> Does it boot if you disable the DEBUG_LL code and apply your
> patches 1-8? The reason I'm asking is that you already cannot have
> DEBUG_LL on a multiplatform kernel targeted at systems which don't
> use the same UART. Adding a further restriction that they must map
> the internal registers to the same physical address does not change
> this a lot.

Thomas will confirm, but I am pretty sure he also made test without
DEBUG_LL

> 
>> Therefore, the last patch of this series adds some early code in the
>> kernel, at the ->map_io() stage, to switch the internal registers from
>> 0xD0000000 to 0xF1000000 if this has not been done already by the
>> bootloader. As it was explained above, we unfortunately can't read the
>> current base address of the internal register window, so we need a
>> different mechanism to know if the bootloader has done the remapping
>> at 0xF1000000 (new generation bootloader) or has left the internal
>> registers at 0xD0000000 (old generation bootloader). In order to
>> distinguish between those two cases, a CP15 bit is being used. Old
>> bootloaders do not touch this CP15, so it is set to 0. New bootloaders
>> set this CP15 bit to 1, so that the kernel knows that the remapping
>> has already been done. The ->map_io() code looks at this bit to know
>> if the remapping should be done or not.
> 
> As you already admit, using the CP15 register is a hack. It sounds
> to me that a cleaner approach would be to put the correct address
> into the device tree and use that value everywhere, rather than
> hardcoding one or more addresses.

That means reading and decoding the device tree very early in the
auto-uncompress part of the kernel.

> 
>> Unfortunately, tweaking ->map_io() is not sufficient: we also want
>> earlyprintk to work. And earlyprintk is used *before* ->map_io() is
>> called, and *after* ->map_io() is called.
> 
> Note that by the time map_io is called, we no longer care about
> the physical address, since the uart is only accessed through the
> virtual mapping after __enable_mmu.
> 
> 	Arnd
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Wed, 22 May 2013 16:33:46 +0200, Arnd Bergmann wrote:

> >  1 The internal registers window sits at 3 GB, in the middle of the
> >    0-4 GB area of physical memory, which is quite annoying when you
> >    have more than 3 GB of memory. Having them at 0xF1000000 means that
> >    you lose less RAM when you have more than 3 GB of physical memory
> >    installed. And since Armada XP is LPAE capable, you can even have
> >    more than 4 GB memory: we already have boards with 8 GB of memory
> >    installed.
> 
> Just for completeness:
> 
> Do any of the machines that use the old boot loader have more
> than 3 GB of memory?

As Gregory said: yes. The Armada XP GP board has 8 GB of RAM. And the
OpenBlocks has 1 GB soldered, and an expansion slot in which you can
add more RAM, possibly over 3 GB.

> >  2 This is different from Kirkwood and Dove, which makes sharing some
> >    early code like earlyprintk complicated, while our goal is
> >    ultimately to merge all Marvell EBU platforms into mach-mvebu.
> > 
> > (1) is really the primary motivation here, (2) is only a nice
> > side-effect.
> > 
> > So, the new generation of bootloaders that are shipped on new Marvell
> > Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
> > starting Linux. The current kernel cannot boot on such platforms.
> 
> Does it boot if you disable the DEBUG_LL code and apply your
> patches 1-8?

Patches 1-8 do not change anything in terms of the internal register
window base address. So just like the kernel was working with and
without DEBUG_LL before patches 1-8, it continues to work after those
cleanups/improvements.

> The reason I'm asking is that you already cannot have
> DEBUG_LL on a multiplatform kernel targeted at systems which don't
> use the same UART. Adding a further restriction that they must map
> the internal registers to the same physical address does not change
> this a lot.

I'm sorry, but I don't follow you here. When you have patches 1-8 (or
no patch applied at all), the UART for mach-mvebu is *always* at
0xd0012000.

> > Therefore, the last patch of this series adds some early code in the
> > kernel, at the ->map_io() stage, to switch the internal registers from
> > 0xD0000000 to 0xF1000000 if this has not been done already by the
> > bootloader. As it was explained above, we unfortunately can't read the
> > current base address of the internal register window, so we need a
> > different mechanism to know if the bootloader has done the remapping
> > at 0xF1000000 (new generation bootloader) or has left the internal
> > registers at 0xD0000000 (old generation bootloader). In order to
> > distinguish between those two cases, a CP15 bit is being used. Old
> > bootloaders do not touch this CP15, so it is set to 0. New bootloaders
> > set this CP15 bit to 1, so that the kernel knows that the remapping
> > has already been done. The ->map_io() code looks at this bit to know
> > if the remapping should be done or not.
> 
> As you already admit, using the CP15 register is a hack. It sounds
> to me that a cleaner approach would be to put the correct address
> into the device tree and use that value everywhere, rather than
> hardcoding one or more addresses.

I don't see how this can fix the problem. Which internal register base
address do we put in our Device Tree source in arch/arm/boot/dts? The
old one? The new one?

Again, I'd like people to carefully read and read once again the cover
letter and commit log before jumping into the conclusion that 'CP15 is
a hack', and come up with a solution that seriously takes into account
all the problems that are highlighted in the cover letter and commit
log. "Putting the correct address into the device tree and use that
value everywhere" simply doesn't work for earlyprintk, for example.

I'm afraid that the only alternative to the proposed solution is to
unconditionally assume that the switch to the 0xf1000000 base address
has been made by the bootloader. This will work for all new Marvell
platforms, but will break all existing users of Mirabox and OpenBlocks
platform.

> > Unfortunately, tweaking ->map_io() is not sufficient: we also want
> > earlyprintk to work. And earlyprintk is used *before* ->map_io() is
> > called, and *after* ->map_io() is called.
> 
> Note that by the time map_io is called, we no longer care about
> the physical address, since the uart is only accessed through the
> virtual mapping after __enable_mmu.

That's not correct. ->map_io() calls debug_ll_io_init(), which calls
the addruart code of earlyprintk, from which it gets the virtual
address *AND* physical address of the UART, and sets up a mapping with
create_mapping(). See the implementation of debug_ll_io_init().

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Gregory CLEMENT wrote:
> >> Therefore, the last patch of this series adds some early code in the
> >> kernel, at the ->map_io() stage, to switch the internal registers from
> >> 0xD0000000 to 0xF1000000 if this has not been done already by the
> >> bootloader. As it was explained above, we unfortunately can't read the
> >> current base address of the internal register window, so we need a
> >> different mechanism to know if the bootloader has done the remapping
> >> at 0xF1000000 (new generation bootloader) or has left the internal
> >> registers at 0xD0000000 (old generation bootloader). In order to
> >> distinguish between those two cases, a CP15 bit is being used. Old
> >> bootloaders do not touch this CP15, so it is set to 0. New bootloaders
> >> set this CP15 bit to 1, so that the kernel knows that the remapping
> >> has already been done. The ->map_io() code looks at this bit to know
> >> if the remapping should be done or not.
> > 
> > As you already admit, using the CP15 register is a hack. It sounds
> > to me that a cleaner approach would be to put the correct address
> > into the device tree and use that value everywhere, rather than
> > hardcoding one or more addresses.
> 
> That means reading and decoding the device tree very early in the
> auto-uncompress part of the kernel.

Why that? I was only implying that we would have no console output in
the uncompressor, which is already the case on multiplatform these
days when DEBUG_LL is disabled.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Thomas Petazzoni wrote:
> On Wed, 22 May 2013 16:33:46 +0200, Arnd Bergmann wrote:
> > >  1 The internal registers window sits at 3 GB, in the middle of the
> > >    0-4 GB area of physical memory, which is quite annoying when you
> > >    have more than 3 GB of memory. Having them at 0xF1000000 means that
> > >    you lose less RAM when you have more than 3 GB of physical memory
> > >    installed. And since Armada XP is LPAE capable, you can even have
> > >    more than 4 GB memory: we already have boards with 8 GB of memory
> > >    installed.
> > 
> > Just for completeness:
> > 
> > Do any of the machines that use the old boot loader have more
> > than 3 GB of memory?
> 
> As Gregory said: yes. The Armada XP GP board has 8 GB of RAM. And the
> OpenBlocks has 1 GB soldered, and an expansion slot in which you can
> add more RAM, possibly over 3 GB.

Ok, but going back to the discussion about remapping physical memory
addresses, would you actually be able to map more of the available
RAM when the registers are moved out of the way?

IIRC the translation is rather coarse-grained, and the new location
is still in the same 1GB window from 0xc0000000-0xffffffff.

> > > So, the new generation of bootloaders that are shipped on new Marvell
> > > Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
> > > starting Linux. The current kernel cannot boot on such platforms.
> > 
> > Does it boot if you disable the DEBUG_LL code and apply your
> > patches 1-8?
> 
> Patches 1-8 do not change anything in terms of the internal register
> window base address. So just like the kernel was working with and
> without DEBUG_LL before patches 1-8, it continues to work after those
> cleanups/improvements.

I mean with the new boot loader which breaks unmodified kernels.

> > The reason I'm asking is that you already cannot have
> > DEBUG_LL on a multiplatform kernel targeted at systems which don't
> > use the same UART. Adding a further restriction that they must map
> > the internal registers to the same physical address does not change
> > this a lot.
> 
> I'm sorry, but I don't follow you here. When you have patches 1-8 (or
> no patch applied at all), the UART for mach-mvebu is *always* at
> 0xd0012000.

No. UART0 is always at 0xd0012000 but there are also UARTs at 0xd0012100,
0xd0012200 and 0xd0012300. You don't currently support using them for
early output, but you might need to add that, similar to how other
SoC platforms handle this.

More importantly, you can build a kernel with any number of other SoCs
already, and if you have a combined ArmadaXP+OMAP+Highbank kernel,
there is no way to get a working early debug output.

> > > Therefore, the last patch of this series adds some early code in the
> > > kernel, at the ->map_io() stage, to switch the internal registers from
> > > 0xD0000000 to 0xF1000000 if this has not been done already by the
> > > bootloader. As it was explained above, we unfortunately can't read the
> > > current base address of the internal register window, so we need a
> > > different mechanism to know if the bootloader has done the remapping
> > > at 0xF1000000 (new generation bootloader) or has left the internal
> > > registers at 0xD0000000 (old generation bootloader). In order to
> > > distinguish between those two cases, a CP15 bit is being used. Old
> > > bootloaders do not touch this CP15, so it is set to 0. New bootloaders
> > > set this CP15 bit to 1, so that the kernel knows that the remapping
> > > has already been done. The ->map_io() code looks at this bit to know
> > > if the remapping should be done or not.
> > 
> > As you already admit, using the CP15 register is a hack. It sounds
> > to me that a cleaner approach would be to put the correct address
> > into the device tree and use that value everywhere, rather than
> > hardcoding one or more addresses.
> 
> I don't see how this can fix the problem. Which internal register base
> address do we put in our Device Tree source in arch/arm/boot/dts? The
> old one? The new one?

The one matching the boot loader for that board, i.e. the new address
for all boards but Mirabox and OpenBlocks.

> > > Unfortunately, tweaking ->map_io() is not sufficient: we also want
> > > earlyprintk to work. And earlyprintk is used *before* ->map_io() is
> > > called, and *after* ->map_io() is called.
> > 
> > Note that by the time map_io is called, we no longer care about
> > the physical address, since the uart is only accessed through the
> > virtual mapping after __enable_mmu.
> 
> That's not correct. ->map_io() calls debug_ll_io_init(), which calls
> the addruart code of earlyprintk, from which it gets the virtual
> address *AND* physical address of the UART, and sets up a mapping with
> create_mapping(). See the implementation of debug_ll_io_init().

Ah right, but that is also something that could easily be changed.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Gregory CLEMENT]

On 05/22/2013 05:18 PM, Arnd Bergmann wrote:
> On Wednesday 22 May 2013, Gregory CLEMENT wrote:
>>>> Therefore, the last patch of this series adds some early code in the
>>>> kernel, at the ->map_io() stage, to switch the internal registers from
>>>> 0xD0000000 to 0xF1000000 if this has not been done already by the
>>>> bootloader. As it was explained above, we unfortunately can't read the
>>>> current base address of the internal register window, so we need a
>>>> different mechanism to know if the bootloader has done the remapping
>>>> at 0xF1000000 (new generation bootloader) or has left the internal
>>>> registers at 0xD0000000 (old generation bootloader). In order to
>>>> distinguish between those two cases, a CP15 bit is being used. Old
>>>> bootloaders do not touch this CP15, so it is set to 0. New bootloaders
>>>> set this CP15 bit to 1, so that the kernel knows that the remapping
>>>> has already been done. The ->map_io() code looks at this bit to know
>>>> if the remapping should be done or not.
>>>
>>> As you already admit, using the CP15 register is a hack. It sounds
>>> to me that a cleaner approach would be to put the correct address
>>> into the device tree and use that value everywhere, rather than
>>> hardcoding one or more addresses.
>>
>> That means reading and decoding the device tree very early in the
>> auto-uncompress part of the kernel.
> 
> Why that? I was only implying that we would have no console output in
> the uncompressor, which is already the case on multiplatform these
> days when DEBUG_LL is disabled.

Loosing the earlyprintk would be a big regression. Having DEBUG_LL disabled
in multiplatform case is fine. If the kernel seems to not run at all we
can still build a kernel for only for mvebu, activate earlyprintk and see
what happened. With your proposal we won't have any more this feature, and
it will make the debug or the bug report for the user much harder. As we still
in active development I really want to avoid it.

> 
> 	Arnd
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Gregory CLEMENT wrote:
> Loosing the earlyprintk would be a big regression. Having DEBUG_LL disabled
> in multiplatform case is fine.

EARLY_PRINTK depends on DEBUG_LL, so that makes no sense.

> If the kernel seems to not run at all we
> can still build a kernel for only for mvebu, activate earlyprintk and see
> what happened. With your proposal we won't have any more this feature, and
> it will make the debug or the bug report for the user much harder. As we still
> in active development I really want to avoid it.

I don't understand. You should still be able to turn on DEBUG_LL when you need
it. It will just work only on the machines it is configured for, as it always
does. You will need a separate Kconfig option for each physical address though,
but that is also nothing new.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Andrew Lunn]

> > I don't see how this can fix the problem. Which internal register base
> > address do we put in our Device Tree source in arch/arm/boot/dts? The
> > old one? The new one?
> 
> The one matching the boot loader for that board, i.e. the new address
> for all boards but Mirabox and OpenBlocks.

Hi Thomas

Maybe i'm missunderstand Arnd, but i think he is suggesting a property
somewhere in DT which says where the bootloader left the register
space before jumping to the kernel. You can then look at this property
and then decide if you need to remap or not. Only this one property
needs to differ between old and new bootloader. You could even say, if
the property is not in DT, a remap is needed.

However, i don't know if this can actually work. How early can you
parse the DT in order to know where the serial port is for
earlyprintk()? It also gets messy keeping track of the two different
DT binary blobs and somehow having the probe the uboot version in
order to install the right one. I don't know how i would include this
into the debian flash-kernel for example.

I prefer the CP15 bit.

	Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Gregory CLEMENT]

On 05/22/2013 05:43 PM, Arnd Bergmann wrote:
> On Wednesday 22 May 2013, Gregory CLEMENT wrote:
>> Loosing the earlyprintk would be a big regression. Having DEBUG_LL disabled
>> in multiplatform case is fine.
> 
> EARLY_PRINTK depends on DEBUG_LL, so that makes no sense.
> 
>> If the kernel seems to not run at all we
>> can still build a kernel for only for mvebu, activate earlyprintk and see
>> what happened. With your proposal we won't have any more this feature, and
>> it will make the debug or the bug report for the user much harder. As we still
>> in active development I really want to avoid it.
> 
> I don't understand. You should still be able to turn on DEBUG_LL when you need
> it. It will just work only on the machines it is configured for, as it always
> does. You will need a separate Kconfig option for each physical address though,
> but that is also nothing new.

But you proposed to use device tree instead of CP15. In this case we won't be
able to use earlyprintk that was my point. You justified the fact to not use
earlyprintk because we can't use it for multiplatform. So I am fine to not being
able to use earlyprintk in multiplatform, but it sill important to be able to
continue to use in non multiplatform case, hence the need of the use of CP15
bit.



> 
> 	Arnd
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Wed, 22 May 2013 17:35:11 +0200, Arnd Bergmann wrote:

> Ok, but going back to the discussion about remapping physical memory
> addresses, would you actually be able to map more of the available
> RAM when the registers are moved out of the way?

Yes, we can.

> IIRC the translation is rather coarse-grained, and the new location
> is still in the same 1GB window from 0xc0000000-0xffffffff.

We have four different CS for RAM, and therefore when we have 8 GB of
RAM installed, if the internal registers are at 0xD0000000, we loose
768 MB of RAM. When they are mapped at 0xF1000000, we loose only 256 MB
of RAM. A win of 512 MB of RAM, which is not negligible.

And the new bootloaders that are remapping to 0xf1000000 on Armada
370/XP are already being shipped by Marvell, so it's not like we have
much choice here.

> > > > So, the new generation of bootloaders that are shipped on new Marvell
> > > > Armada 370/XP platforms are doing the remapping at 0xF1000000 prior to
> > > > starting Linux. The current kernel cannot boot on such platforms.
> > > 
> > > Does it boot if you disable the DEBUG_LL code and apply your
> > > patches 1-8?
> > 
> > Patches 1-8 do not change anything in terms of the internal register
> > window base address. So just like the kernel was working with and
> > without DEBUG_LL before patches 1-8, it continues to work after those
> > cleanups/improvements.
> 
> I mean with the new boot loader which breaks unmodified kernels.

If you boot the kernel with, or without 1-8, it will not boot with a
new bootloader. Only if you apply the 9th patch will it work. And the
kernel will continue to work with both an old and a new bootloader.

> > I'm sorry, but I don't follow you here. When you have patches 1-8 (or
> > no patch applied at all), the UART for mach-mvebu is *always* at
> > 0xd0012000.
> 
> No. UART0 is always at 0xd0012000 but there are also UARTs at 0xd0012100,
> 0xd0012200 and 0xd0012300. You don't currently support using them for
> early output, but you might need to add that, similar to how other
> SoC platforms handle this.

Right.

> More importantly, you can build a kernel with any number of other SoCs
> already, and if you have a combined ArmadaXP+OMAP+Highbank kernel,
> there is no way to get a working early debug output.

So you're proposing to add two Kconfig options, one to have the UART at
0xd0012000 and the other one to have the UART at 0xf1012000. The user
would have to know whether he is going to boot with an old bootloader
or new bootloader, and select the appropriate option. This means that
the mvebu_defconfig would no longer be able to have earlyprintk enabled
by default, otherwise the resulting kernel may not boot for some
people, but maybe it's a reasonable tradeoff.

> > > As you already admit, using the CP15 register is a hack. It sounds
> > > to me that a cleaner approach would be to put the correct address
> > > into the device tree and use that value everywhere, rather than
> > > hardcoding one or more addresses.
> > 
> > I don't see how this can fix the problem. Which internal register base
> > address do we put in our Device Tree source in arch/arm/boot/dts? The
> > old one? The new one?
> 
> The one matching the boot loader for that board, i.e. the new address
> for all boards but Mirabox and OpenBlocks.

And what happens when Globalscale (Mirabox manufacturer) or Plathome
(manufacturer of OpenBlocks) release a new bootloader version, based on
the new bootloader from Marvell, which remaps things at 0xf1000000 ? Or
when those people boot from Barebox, which remaps at 0xf1000000 ?

Conclusion: you can't classify on one side boards that are at 0xd0 and
boards that are at 0xf1. It depends on *which* bootloader each
particular instance of those boards will be using. Will it be a
bootloader that complies with the "old" protocol (CP15 bit cleared and
registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
0xf1).

So we can't encode into the boards Device Tree whether the registers
are at 0xd0 or 0xf1, because it's not a board-related information, but
a bootloader-related information.

> > > > Unfortunately, tweaking ->map_io() is not sufficient: we also want
> > > > earlyprintk to work. And earlyprintk is used *before* ->map_io() is
> > > > called, and *after* ->map_io() is called.
> > > 
> > > Note that by the time map_io is called, we no longer care about
> > > the physical address, since the uart is only accessed through the
> > > virtual mapping after __enable_mmu.
> > 
> > That's not correct. ->map_io() calls debug_ll_io_init(), which calls
> > the addruart code of earlyprintk, from which it gets the virtual
> > address *AND* physical address of the UART, and sets up a mapping with
> > create_mapping(). See the implementation of debug_ll_io_init().
> 
> Ah right, but that is also something that could easily be changed.

I must admit I'm not sure why we have this debug_ll_io_init() function.
When DEBUG_LL is enabled, there is already a call to addruart from
arch/arm/kernel/head.S, which does create a virtual -> physical mapping
that covers the UART area. Since debug_ll_io_init() is also only
enabled under DEBUG_LL, I am not sure to understand what it does that
the assembly code in arch/arm/kernel/head.S hasn't done already. Adding
Rob in Cc, as he's the one who added debug_ll_io_init() in the first
place.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Andrew Lunn,

On Wed, 22 May 2013 17:51:34 +0200, Andrew Lunn wrote:

> Maybe i'm missunderstand Arnd, but i think he is suggesting a property
> somewhere in DT which says where the bootloader left the register
> space before jumping to the kernel. You can then look at this property
> and then decide if you need to remap or not. Only this one property
> needs to differ between old and new bootloader. You could even say, if
> the property is not in DT, a remap is needed.

The problem is that neither the old nor the new bootloaders are setting
such property in the DT. Old bootloaders shipped by Marvell don't have
DT support at all. New bootloaders shipped by Marvell do have DT
support, but they are not adding any specific DT property that allows
us to tell whether the registers are mapped here or there.

We could maybe find a way of detecting whether if the bootloader is DT
capable, and assume that if it's DT capable, the registers are already
mapped at 0xf1. But that's very fragile: nothing prevents an user to
use a DT-capable bootloader to boot a kernel in the old style way
(appended DTB). In this case, we would assume that the registers are at
0xd0, because we're being booted old-style, and therefore it must be an
old bootloader. Very fragile, clearly not better than the CP15 trick.

> However, i don't know if this can actually work. How early can you
> parse the DT in order to know where the serial port is for
> earlyprintk()? It also gets messy keeping track of the two different
> DT binary blobs and somehow having the probe the uboot version in
> order to install the right one. I don't know how i would include this
> into the debian flash-kernel for example.

For earlyprintk, we can "solve" it as Arnd suggested: just have two
Kconfig options, one for 0xd0, one for 0xf1, and it's up to the user to
choose the right one. It's not entirely nice because it's no longer
automatic, and I'm sure our users will have a hard time figuring out
whether they have an "old" or a "new" bootloader. But let's assume we
do this. earlyprintk knows the physical address of the UART.

Then, we enter ->map_io(). We have access to the Device Tree. But the
Device Tree doesn't tell us whether the bootloader has mapped the
registers. As I explained to Arnd, it's not a per-board criteria: when
I'm booting my OpenBlocks with the manufacturer U-Boot, registers are
mapped at 0xd0. When I boot the same OpenBlocks with the (admittedly
work-in-progress) Barebox bootloader, registers are mapped at 0xf1.
Same board, different register location.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Willy Tarreau]

Hi Thomas,

On Wed, May 22, 2013 at 06:08:42PM +0200, Thomas Petazzoni wrote:
> And the new bootloaders that are remapping to 0xf1000000 on Armada
> 370/XP are already being shipped by Marvell, so it's not like we have
> much choice here.

Which also means we can't request to modify new boot loaders to pass
any specific information.

(...)
> And what happens when Globalscale (Mirabox manufacturer) or Plathome
> (manufacturer of OpenBlocks) release a new bootloader version, based on
> the new bootloader from Marvell, which remaps things at 0xf1000000 ? Or
> when those people boot from Barebox, which remaps at 0xf1000000 ?

We experienced a similar issue some time ago when something changed
in the boot loader spec. Kernels >= 3.2 would randomly hang at boot
when using old boot loaders, and many boot loaders had to be upgraded.
It was quite painful because there was no information reported with
this hang but I guess most people managed to upgrade the boot loaders.
I don't know for how long devices have been shipped with boot loaders
mapping at 0xd0, nor if all these platforms do have a new boot loader,
but maybe the same forced upgrade could be reasonable, I have no idea.
On the other hand, if we consider the forced upgrade as mostly acceptable,
then the CP15 trick already is an acceptable workaround in that it is
used as an indicator to detect the old boot loader. So even if for any
reason it fails on some users, they'll have to upgrade anyway.

> Conclusion: you can't classify on one side boards that are at 0xd0 and
> boards that are at 0xf1. It depends on *which* bootloader each
> particular instance of those boards will be using. Will it be a
> bootloader that complies with the "old" protocol (CP15 bit cleared and
> registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> 0xf1).

Just a point I'm thinking about, is it possible to detect the boot loader's
mode via ATAGs ? ATAGs are present very early as well (but maybe too late
already).

> So we can't encode into the boards Device Tree whether the registers
> are at 0xd0 or 0xf1, because it's not a board-related information, but
> a bootloader-related information.

I totally agree with you. The issue is only the boot loader.

Best regards,
Willy

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Willy Tarreau,

On Wed, 22 May 2013 18:35:57 +0200, Willy Tarreau wrote:

> On Wed, May 22, 2013 at 06:08:42PM +0200, Thomas Petazzoni wrote:
> > And the new bootloaders that are remapping to 0xf1000000 on Armada
> > 370/XP are already being shipped by Marvell, so it's not like we have
> > much choice here.
> 
> Which also means we can't request to modify new boot loaders to pass
> any specific information.

Correct.

> > And what happens when Globalscale (Mirabox manufacturer) or Plathome
> > (manufacturer of OpenBlocks) release a new bootloader version, based on
> > the new bootloader from Marvell, which remaps things at 0xf1000000 ? Or
> > when those people boot from Barebox, which remaps at 0xf1000000 ?
> 
> We experienced a similar issue some time ago when something changed
> in the boot loader spec. Kernels >= 3.2 would randomly hang at boot
> when using old boot loaders, and many boot loaders had to be upgraded.
> It was quite painful because there was no information reported with
> this hang but I guess most people managed to upgrade the boot loaders.

Agreed. I already receive quite a number of e-mails from users trying
to do this or that with the Mirabox, and I'm not sure I would receive
many more e-mails if the kernel would not "just" work without having to
do some bizarre configuration. Especially if the kernel just hangs with
no useful information.

> I don't know for how long devices have been shipped with boot loaders
> mapping at 0xd0, nor if all these platforms do have a new boot loader,
> but maybe the same forced upgrade could be reasonable, I have no idea.

I guess the Mirabox and OpenBlocks have been shipping for about 9-10
months now. Judging by the amount of e-mails are received about Mirabox
kernel (asking when PCIe support will be available, when NAND will be
available), there is definitely a certain number of people who have
those platforms in their hands.

> On the other hand, if we consider the forced upgrade as mostly acceptable,
> then the CP15 trick already is an acceptable workaround in that it is
> used as an indicator to detect the old boot loader. So even if for any
> reason it fails on some users, they'll have to upgrade anyway.

My plan would be to have this CP15 trick for now. In two release
cycles, show a big fat warning to users that are booting with old
bootloaders, which would encourage them to update their bootloader. By
this time, hopefully, the manufacturers of OpenBlocks and Mirabox will
have released updated bootloader versions, and we can tell users to
upgrade. The upgrade process is easy and safe, because it takes place
through the UART, as you know.

Then, another two release cycles later, we could remove the CP15 trick
entirely.

So it's just a matter of living with this for about 4 release cycles,
approximately a year.

> > Conclusion: you can't classify on one side boards that are at 0xd0 and
> > boards that are at 0xf1. It depends on *which* bootloader each
> > particular instance of those boards will be using. Will it be a
> > bootloader that complies with the "old" protocol (CP15 bit cleared and
> > registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> > 0xf1).
> 
> Just a point I'm thinking about, is it possible to detect the boot loader's
> mode via ATAGs ? ATAGs are present very early as well (but maybe too late
> already).

As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
ARM register that was used to pass the pointer to the ATAGS is now used
to pass the pointer to the DTB in memory.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Willy Tarreau]

On Wed, May 22, 2013 at 06:42:50PM +0200, Thomas Petazzoni wrote:
> > I don't know for how long devices have been shipped with boot loaders
> > mapping at 0xd0, nor if all these platforms do have a new boot loader,
> > but maybe the same forced upgrade could be reasonable, I have no idea.
> 
> I guess the Mirabox and OpenBlocks have been shipping for about 9-10
> months now. Judging by the amount of e-mails are received about Mirabox
> kernel (asking when PCIe support will be available, when NAND will be
> available), there is definitely a certain number of people who have
> those platforms in their hands.

Indeed, but either these people are running stock kernel on them and don't
care, or they're ready to hack their box and should not be much frightened
by a boot loader upgrade (especially since it's safe from the UART as we
have found).

> > On the other hand, if we consider the forced upgrade as mostly acceptable,
> > then the CP15 trick already is an acceptable workaround in that it is
> > used as an indicator to detect the old boot loader. So even if for any
> > reason it fails on some users, they'll have to upgrade anyway.
> 
> My plan would be to have this CP15 trick for now. In two release
> cycles, show a big fat warning to users that are booting with old
> bootloaders, which would encourage them to update their bootloader. By
> this time, hopefully, the manufacturers of OpenBlocks and Mirabox will
> have released updated bootloader versions, and we can tell users to
> upgrade. The upgrade process is easy and safe, because it takes place
> through the UART, as you know.
> 
> Then, another two release cycles later, we could remove the CP15 trick
> entirely.
> 
> So it's just a matter of living with this for about 4 release cycles,
> approximately a year.

This seems reasonable to me indeed!

> > > Conclusion: you can't classify on one side boards that are at 0xd0 and
> > > boards that are at 0xf1. It depends on *which* bootloader each
> > > particular instance of those boards will be using. Will it be a
> > > bootloader that complies with the "old" protocol (CP15 bit cleared and
> > > registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> > > 0xf1).
> > 
> > Just a point I'm thinking about, is it possible to detect the boot loader's
> > mode via ATAGs ? ATAGs are present very early as well (but maybe too late
> > already).
> 
> As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> ARM register that was used to pass the pointer to the ATAGS is now used
> to pass the pointer to the DTB in memory.

OK so one possibility could be to rely on this as well if we're certain there
is a 1:1 relation between DT support and new mapping. But at least you have
working code now with CP15, so let's not complexify the situation !

Best regards,
Willy

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Cooper]

Thomas,

On Wed, May 22, 2013 at 06:42:50PM +0200, Thomas Petazzoni wrote:
> On Wed, 22 May 2013 18:35:57 +0200, Willy Tarreau wrote:
> > On Wed, May 22, 2013 at 06:08:42PM +0200, Thomas Petazzoni wrote:
> > > Conclusion: you can't classify on one side boards that are at 0xd0 and
> > > boards that are at 0xf1. It depends on *which* bootloader each
> > > particular instance of those boards will be using. Will it be a
> > > bootloader that complies with the "old" protocol (CP15 bit cleared and
> > > registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> > > 0xf1).
> > 
> > Just a point I'm thinking about, is it possible to detect the boot loader's
> > mode via ATAGs ? ATAGs are present very early as well (but maybe too late
> > already).
> 
> As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> ARM register that was used to pass the pointer to the ATAGS is now used
> to pass the pointer to the DTB in memory.

So we could look for the ATAG magic or the dtb magic at that address,
then we know if we have an old or new bootloader...

thx,

Jason.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Jason Cooper,

On Wed, 22 May 2013 12:49:36 -0400, Jason Cooper wrote:

> > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > ARM register that was used to pass the pointer to the ATAGS is now used
> > to pass the pointer to the DTB in memory.
> 
> So we could look for the ATAG magic or the dtb magic at that address,
> then we know if we have an old or new bootloader...

No, because you can use an old bootloader, and still do some old-style
appended-DTB booting, in which case you have a new bootloader
(registers mapped at 0xf1), but you see the ATAG magic, which will make
you think you booted from an old bootloader (registers mapped at 0xd0).

For example, I'm currently booting alternatively with an old and a new
bootloader (to test that things work properly), and in both cases I'm
booting old style, DTB-appended, with ATAGs.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Willy Tarreau,

On Wed, 22 May 2013 18:49:28 +0200, Willy Tarreau wrote:

> > I guess the Mirabox and OpenBlocks have been shipping for about 9-10
> > months now. Judging by the amount of e-mails are received about Mirabox
> > kernel (asking when PCIe support will be available, when NAND will be
> > available), there is definitely a certain number of people who have
> > those platforms in their hands.
> 
> Indeed, but either these people are running stock kernel on them and don't
> care, or they're ready to hack their box and should not be much frightened
> by a boot loader upgrade (especially since it's safe from the UART as we
> have found).

Yes, they could upgrade. But as of today, such upgrades do not even
exist for the Mirabox and OpenBlocks AX3 boards. So what do we tell to
those users?

Of course, one option is simply to have a kconfig option "I am booting
from an old bootloader", but with the CP15 trick, we've tried to make
this automatic so that users don't have to understand the kind of gory
details we are discussing right now.


> > 
> > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > ARM register that was used to pass the pointer to the ATAGS is now used
> > to pass the pointer to the DTB in memory.
> 
> OK so one possibility could be to rely on this as well if we're certain there
> is a 1:1 relation between DT support and new mapping. But at least you have
> working code now with CP15, so let's not complexify the situation !

There is absolutely no 1:1 relation between DT support and new mapping.

As I've just written in a reply to Jason Cooper, I'm currently using a
new bootloader (which uses the new mapping at 0xf1), but I'm booting
the kernel old style (appended DTB).

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Willy Tarreau]

On Wed, May 22, 2013 at 07:00:04PM +0200, Thomas Petazzoni wrote:
> Yes, they could upgrade. But as of today, such upgrades do not even
> exist for the Mirabox and OpenBlocks AX3 boards. So what do we tell to
> those users?

I was assuming these boot loaders do already exist if they're shipping
on some devices, but maybe not all yet.

> Of course, one option is simply to have a kconfig option "I am booting
> from an old bootloader", but with the CP15 trick, we've tried to make
> this automatic so that users don't have to understand the kind of gory
> details we are discussing right now.

Same as for ATAGS right now which are not necessarily the most obvious
setting for end users. Better keep the trick on by default if it's safe
enough. In fact I'm mostly concerned about the risk that it later breaks
for even newer boot loaders. But then we'll have more feedback to decide.

> > > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > > ARM register that was used to pass the pointer to the ATAGS is now used
> > > to pass the pointer to the DTB in memory.
> > 
> > OK so one possibility could be to rely on this as well if we're certain there
> > is a 1:1 relation between DT support and new mapping. But at least you have
> > working code now with CP15, so let's not complexify the situation !
> 
> There is absolutely no 1:1 relation between DT support and new mapping.
>
> As I've just written in a reply to Jason Cooper, I'm currently using a
> new bootloader (which uses the new mapping at 0xf1), but I'm booting
> the kernel old style (appended DTB).

OK, that was what I was wondering about. Sorry for having bothere you with
my questions but at least not it's much clearer that CP15 probably is the
only viable solution.

Best regards,
Willy

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Cooper]

On Wed, May 22, 2013 at 06:57:57PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Wed, 22 May 2013 12:49:36 -0400, Jason Cooper wrote:
> 
> > > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > > ARM register that was used to pass the pointer to the ATAGS is now used
> > > to pass the pointer to the DTB in memory.
> > 
> > So we could look for the ATAG magic or the dtb magic at that address,
> > then we know if we have an old or new bootloader...
> 
> No, because you can use an old bootloader, and still do some old-style

Did you mean 'new bootloader'?

> appended-DTB booting, in which case you have a new bootloader
> (registers mapped at 0xf1), but you see the ATAG magic, which will make
> you think you booted from an old bootloader (registers mapped at 0xd0).
> 
> For example, I'm currently booting alternatively with an old and a new
> bootloader (to test that things work properly), and in both cases I'm
> booting old style, DTB-appended, with ATAGs.

Is this something users would experience?  I think it is fairly safe to
say that once dt-able bootloaders are shipped, they will provide a dtb.
So, OF_DT_MAGIC == new bootloader might hold true for users.

thx,

Jason.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Willy Tarreau,

On Wed, 22 May 2013 19:08:07 +0200, Willy Tarreau wrote:
> On Wed, May 22, 2013 at 07:00:04PM +0200, Thomas Petazzoni wrote:
> > Yes, they could upgrade. But as of today, such upgrades do not even
> > exist for the Mirabox and OpenBlocks AX3 boards. So what do we tell to
> > those users?
> 
> I was assuming these boot loaders do already exist if they're shipping
> on some devices, but maybe not all yet.

The new bootloaders have started being offered by Marvell to
board/system manufacturers since only a few weeks or so, so besides
evaluation boards, there are not yet public products that use the new
bootloaders. But the vast majority of Armada 370 and Armada XP
platforms are still to be coming, and this vast majority will be using
the new bootloader.

> > Of course, one option is simply to have a kconfig option "I am booting
> > from an old bootloader", but with the CP15 trick, we've tried to make
> > this automatic so that users don't have to understand the kind of gory
> > details we are discussing right now.
> 
> Same as for ATAGS right now which are not necessarily the most obvious
> setting for end users. Better keep the trick on by default if it's safe
> enough. In fact I'm mostly concerned about the risk that it later breaks
> for even newer boot loaders. But then we'll have more feedback to decide.

As I suggested earlier, we can print a warning when people boot from an
old bootloader, and then down the road get rid of the trick. Or move it
under a conditional compilation so that the minority of remaining users
will have to enable this particular option. This will anyway be needed
if we want to go multiplatform with Dove, for example.

> OK, that was what I was wondering about. Sorry for having bothere you with
> my questions but at least not it's much clearer that CP15 probably is the
> only viable solution.

We've also gone through a lot of thinking about this problem, and we've
considered various options, and we came down to the following two
options:

 * Assume we're booted with a bootloader that has done the remapping at
   0xf1. Works for all new platforms, breaks all the older platforms,
   unless people upgrade their bootloaders.

 * Have a nicer way to handle the transition, and in this case, when
   you take into account all the complexities, the CP15 turned out to
   be the only really viable solution.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Andrew Lunn]

On Tue, May 21, 2013 at 12:33:34PM +0200, Thomas Petazzoni wrote:
> On Marvell platforms, the "internal registers" is a window of 1 MB of
> the physical address space that contains all the registers for the
> various peripherals of the SoC (timers, interrupt controllers, GPIO
> controllers, Ethernet, etc.). The base address of this 1 MB window is
> configurable, and this base address is configured using a register
> that is itself part of this window.
> 
> All earlier families of Marvell EBU SoCs supported by Linux used
> 0xf1000000 as the base address of the internal registers window. For
> some reason, early versions of the Marvell 370/XP platforms used
> 0xd0000000 as the base address. This base address is set up by the
> bootloader. However, in order to keep as much physical address space
> to address RAM below the 4 GB limit, we had to switch the base address
> of the internal registers window back to 0xf1000000, and we wanted to
> achieve that without breaking existing platforms, where the bootloader
> was initially setting the base address to 0xd0000000. So the kernel is
> responsible for switching to 0xf1000000 when needed.
> 
> One particular bit of a CP15 register has been chosen to indicate
> whether we use an old bootloader (setting the address at 0xd0000000)
> or a new bootloader (setting the address at 0xf1000000). In the
> ->map_io() function, when the bit is already set, there is nothing to
> do, we are already using a new bootloader and internal registers are
> at 0xf1000000.
> 
> However, when the bit is not set, we do a static mapping of the
> register area that allows to change the internal register address, we
> change this address, and set this bit.
> 
> Note that this mechanism also needs cooperation from the earlyprintk
> addruart handler (in arch/arm/include/debug/mvebu.S). This is where
> things get complex: the CP15 bit that has been chosen gets reset to
> zero at the first call to the 'wfi' instruction. So it cannot be used
> during the entire life of the system. Instead, whenever the ->map_io()
> function has switched to the internal register space to the new
> address, it sets a global variable 'mvebu_switched_regs', which the
> earlyprintk reads. If it's true, then the move has already been done
> and there is no need to read the CP15 register. If
> 'mvebu_switched_regs' is false, we are before the register move, so we
> need to read the CP15 bit to find out whether we're being booted from
> an old bootloader (which mapped the register space at 0xd0000000) or a
> new bootloader (which mapped the register space at 0xf1000000).
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi |    2 +-
>  arch/arm/boot/dts/armada-370.dtsi    |    2 +-
>  arch/arm/include/debug/mvebu.S       |   77 +++++++++++++++++++++++--
>  arch/arm/mach-mvebu/armada-370-xp.c  |  104 ++++++++++++++++++++++++++++++++++
>  arch/arm/mach-mvebu/armada-370-xp.h  |    2 +-
>  5 files changed, 178 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 96a6bcd..1e7561e 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -33,7 +33,7 @@
>  		#size-cells = <1>;
>  		compatible = "simple-bus";
>  		interrupt-parent = <&mpic>;
> -		ranges = <0 0 0xd0000000 0x100000>;
> +		ranges = <0 0 0xf1000000 0x100000>;
>  
>  		internal-regs {
>  			compatible = "simple-bus";
> diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
> index b2c1b5a..7ff9c1b 100644
> --- a/arch/arm/boot/dts/armada-370.dtsi
> +++ b/arch/arm/boot/dts/armada-370.dtsi
> @@ -29,7 +29,7 @@
>  	};
>  
>  	soc {
> -		ranges = <0 0xd0000000 0x100000>;
> +		ranges = <0 0xf1000000 0x100000>;
>  		internal-regs {
>  			system-controller at 18200 {
>  				compatible = "marvell,armada-370-xp-system-controller";
> diff --git a/arch/arm/include/debug/mvebu.S b/arch/arm/include/debug/mvebu.S
> index df191af..dba702a 100644
> --- a/arch/arm/include/debug/mvebu.S
> +++ b/arch/arm/include/debug/mvebu.S
> @@ -5,20 +5,85 @@
>   *
>   * Lior Amsalem <alior@marvell.com>
>   * Gregory Clement <gregory.clement@free-electrons.com>
> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>  */
>  
> -#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
> -#define ARMADA_370_XP_REGS_VIRT_BASE	0xfec00000
> +#define ARMADA_370_XP_UART_PHYS_BASE_OLD	0xd0012000
> +#define ARMADA_370_XP_UART_PHYS_BASE_NEW	0xf1012000
> +#define ARMADA_370_XP_UART_VIRT_BASE_OLD	0xfec12000
> +#define ARMADA_370_XP_UART_VIRT_BASE_NEW	0xfebff000
>  
>  	.macro	addruart, rp, rv, tmp
> -	ldr	\rp, =ARMADA_370_XP_REGS_PHYS_BASE
> -	ldr	\rv, =ARMADA_370_XP_REGS_VIRT_BASE
> -	orr	\rp, \rp, #0x00012000
> -	orr	\rv, \rv, #0x00012000
> +
> +#ifndef ZIMAGE
> +	/*
> +	 * This loads the value of mvebu_switched_regs. We need a
> +	 * little bit of gymnastic here to handle the fact that the
> +	 * addruart code is duplicated in different places, and called
> +	 * sometimes with the MMU disabled, sometimes the MMU enabled.
> +	 *
> +	 * Notice that when this earlyprintk code is linked into the
> +	 * kernel decompressor (i.e ZIMAGE is defined), this code isn't
> +	 * compiled. The reason is because the global mvebu_switched_regs
> +	 * variable is not available in the decompressor code. At this
> +	 * early stage, we simply test the CP15 bit (below) to know where
> +	 * the internal registers are.
> +	 */
> +	adr	\rp, addruart_switched_regs_\@
> +	ldr	\rv, [\rp]
> +	sub	\rv, \rv, \rp
> +	ldr	\rp, [\rp, #4]
> +	sub	\tmp, \rp, \rv
> +	ldr	\rp, [\tmp, #0]
> +	cmp	\rp, #0
> +
> +	/* If we have already switched, then use the new register
> +	   address */
> +	bne	addruart_new_\@
> +#endif
> +
> +	/*
> +	 * We haven't switched the register location, so test the CP15
> +	 * register bit to find whether we are:
> +	 * - with an old bootloader setting the base address to
> +	 *   0xd0000000
> +	 * - with a new bootloader that has already set the
> +	 *   base address to 0xf1000000
> +	 * We adapt the physical address returned depending on the
> +	 * value of this bit, but also the virtual address, because we
> +	 * can't remap the old physical address and the new physical
> +	 * address at the same location.
> +	 */
> +	mrc	p15, 0, \tmp, c5, c0, 0
> +	and	\tmp, \tmp, #(1 << 11)
> +	cmp	\tmp, #0
> +	bne	addruart_new_\@
> +
> +addruart_old_\@:
> +	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_OLD
> +	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_OLD
> +	b	addruart_out_\@
> +addruart_new_\@:
> +	ldr	\rp, =ARMADA_370_XP_UART_PHYS_BASE_NEW
> +	ldr	\rv, =ARMADA_370_XP_UART_VIRT_BASE_NEW
> +	b	addruart_out_\@
> +
> +#ifndef ZIMAGE
> +	/*
> +	 * Global variable mvebu_switched_regs not visible in the
> +	 * decompressor code
> +	 */
> +	.align
> +addruart_switched_regs_\@:
> +	.long	.
> +	.long	mvebu_switched_regs
> +#endif
> +
> +addruart_out_\@:
>  	.endm
>  
>  #define UART_SHIFT	2
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index 90cc0e8..c56875e 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -29,8 +29,112 @@
>  #include "common.h"
>  #include "coherency.h"
>  
> +/*
> + * Note on the internal register address.
> + *
> + * On Marvell platforms, the "internal registers" is a window of 1 MB
> + * of the physical address space that contains all the registers for
> + * the various peripherals of the SoC (timers, interrupt controllers,
> + * GPIO controllers, Ethernet, etc.). The base address of this 1 MB
> + * window is configurable, and this base address is configured using a
> + * register that is itself part of this window.
> + *
> + * All earlier families of Marvell EBU SoCs supported by Linux used
> + * 0xf1000000 as the base address of the internal registers
> + * window. For some reason, early versions of the Marvell 370/XP
> + * platforms used 0xd0000000 as the base address. This base address is
> + * set up by the bootloader. However, in order to keep as much
> + * physical address space to address RAM below the 4 GB limit, we had
> + * to switch the base address of the internal registers window back to
> + * 0xf1000000, and we wanted to achieve that without breaking existing
> + * platforms, where the bootloader was initially setting the base
> + * address to 0xd0000000. So the kernel is responsible for switching
> + * to 0xf1000000 when needed.
> + *
> + * One particular bit of a CP15 register has been chosen to indicate
> + * whether we use an old bootloader (setting the address at
> + * 0xd0000000) or a new bootloader (setting the address at
> + * 0xf1000000). In the ->map_io() function, when the bit is already
> + * set, there is nothing to do, we are already using a new bootloader
> + * and internal registers are at 0xf1000000.
> + *
> + * However, when the bit is not set, we do a static mapping of the
> + * register area that allows to change the internal register address,
> + * we change this address, and set this bit.

Hi Thomas

Please could you explain this "and set this bit".

If i understand the comment correctly, you are talking about the bit
in CP15. Why is it necessary to set it, since at the next WFI its
going to get cleared. I also don't actually see any code doing the
setting of this bit.

> + *
> + * Note that this mechanism also needs cooperation from the
> + * earlyprintk addruart handler (in arch/arm/include/debug/mvebu.S)
> + * and the early secondary CPU initialization code (in
> + * arch/arm/mach-mvebu/headsmp.S).
> + */
> +
> +#define NEW_INTERNAL_REGS_ADDR  0xf1000000
> +#define OLD_INTERNAL_REGS_ADDR  0xd0000000
> +#define MBUS_OLD_PHYS_ADDR      (OLD_INTERNAL_REGS_ADDR + 0x20000)
> +#define MBUS_OLD_VIRT_ADDR      0xfebfe000
> +#define  MBUS_INTERNAL_REGS_ADDR      0x80
> +
> +/*
> + * We really want this variable to be part of the .data section,
> + * because it gets accessed by the earlyprintk code before BSS gets
> + * initialized to zero. This variable indicates whether the switch to
> + * the new register has taken place or not. It is needed because the
> + * CP15 bit used by the bootloader to tell the kernel where the
> + * registers are mapped is cleared to 0 at the first 'wfi'
> + * instruction. So we can't use it for the entire life of the system,
> + * and we instead use this boolean to indicate to the earlyprintk code
> + * that the switch has occured and it doesn't need to look at the CP15
> + * bit anymore.
> + */
> +unsigned int __section(.data) mvebu_switched_regs = 0;
> +
> +/*
> + * Detect if we're running an old bootloader that has set the physical
> + * base address of internal registers to 0xd0000000
> + */
> +static int armada_370_xp_has_old_bootloader(void)
> +{
> +	u32 val;
> +	asm volatile("mrc p15, 0, %0, c5, c0, 0" : "=r"(val));
> +	return !(val & BIT(11));
> +}
> +
>  static void __init armada_370_xp_map_io(void)
>  {
> +	if (armada_370_xp_has_old_bootloader()) {
> +		struct map_desc desc;
> +		void __iomem *mbus = IOMEM(MBUS_OLD_VIRT_ADDR);
> +
> +		desc.virtual = MBUS_OLD_VIRT_ADDR;
> +		desc.pfn     = __phys_to_pfn(MBUS_OLD_PHYS_ADDR);
> +		desc.length  = PAGE_SIZE;
> +		desc.type    = MT_DEVICE;
> +
> +		/*
> +		 * Map the area that contains the registers that
> +		 * allows to change the base address of internal
> +		 * registers.
> +		 */
> +		iotable_init(&desc, 1);
> +
> +		/*
> +		 * Change the physical base address of the
> +		 * registers. From now on, and until
> +		 * debug_ll_io_init() is called below, it is not
> +		 * possible to do any print, because earlyprintk will
> +		 * not work.

Is its "not work" or "locks up the CPU as dead as a Dodo?".

Thanks
	Andrew

> +		 */
> +		writel(NEW_INTERNAL_REGS_ADDR, mbus + MBUS_INTERNAL_REGS_ADDR);
> +	}
> +
> +	/*
> +	 * Either the registers have been switched by the above code,
> +	 * or the bootloader had already set the register space at the
> +	 * right location. In both cases, tell the earlyprintk code it
> +	 * can now use the new location.
> +	 */
> +	mvebu_switched_regs = 1;
> +
>  	debug_ll_io_init();
>  }
>  
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.h b/arch/arm/mach-mvebu/armada-370-xp.h
> index 585e147..ff9adc4 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.h
> +++ b/arch/arm/mach-mvebu/armada-370-xp.h
> @@ -15,7 +15,7 @@
>  #ifndef __MACH_ARMADA_370_XP_H
>  #define __MACH_ARMADA_370_XP_H
>  
> -#define ARMADA_370_XP_REGS_PHYS_BASE	0xd0000000
> +#define ARMADA_370_XP_REGS_PHYS_BASE	0xf1000000
>  
>  /* These defines can go away once mvebu-mbus has a DT binding */
>  #define ARMADA_370_XP_MBUS_WINS_BASE    (ARMADA_370_XP_REGS_PHYS_BASE + 0x20000)
> -- 
> 1.7.9.5
> 

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Thomas Petazzoni]

Dear Andrew Lunn,

On Wed, 22 May 2013 19:42:58 +0200, Andrew Lunn wrote:

> > + * However, when the bit is not set, we do a static mapping of the
> > + * register area that allows to change the internal register address,
> > + * we change this address, and set this bit.
> 
> Hi Thomas
> 
> Please could you explain this "and set this bit".
> 
> If i understand the comment correctly, you are talking about the bit
> in CP15. Why is it necessary to set it, since at the next WFI its
> going to get cleared. I also don't actually see any code doing the
> setting of this bit.

Nice catch. This is a left-over from a previous implementation. For
experimentation purposes, I have been using a different CP15 bit that
was not getting cleared on WFIs, so once the register address switch was
done, I was setting this bit to indicate to further invocations of
earlyprintk that the switch has occurred.

In the mean time, the code has been changed to use the final bit used
by Marvell bootloaders, and this one gets cleared at the next WFI. So
I'm now using a global variable instead to indicate that the switch has
occurred making this comment inaccurate.

I'll fix that in the next revision of this patch set.


> > +		/*
> > +		 * Change the physical base address of the
> > +		 * registers. From now on, and until
> > +		 * debug_ll_io_init() is called below, it is not
> > +		 * possible to do any print, because earlyprintk will
> > +		 * not work.
> 
> Is its "not work" or "locks up the CPU as dead as a Dodo?".

locks up the CPU as a Dodo: you have switched the registers from 0xd0
to 0xf1, but the earlyprintk virtual mapping still points to 0xd0. So
if you try to do a printk, it will call earlyprintk, which will use the
old virtual mapping (since mvebu_switched_regs isn't set yet, it will
be at the next line), and that will make it use the old physical
address of the UART. So you'll write to 0xd0012000, and you're dead.

Thanks for the review!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Jason Cooper,

On Wed, 22 May 2013 13:13:59 -0400, Jason Cooper wrote:

> > On Wed, 22 May 2013 12:49:36 -0400, Jason Cooper wrote:
> > 
> > > > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > > > ARM register that was used to pass the pointer to the ATAGS is now used
> > > > to pass the pointer to the DTB in memory.
> > > 
> > > So we could look for the ATAG magic or the dtb magic at that address,
> > > then we know if we have an old or new bootloader...
> > 
> > No, because you can use an old bootloader, and still do some old-style
> 
> Did you mean 'new bootloader'?

Gaah, yes, of course. Getting myself confused with all this old/new
discussion :)

"No, because you can use a new bootloader, and still do some
old-style..."

> > appended-DTB booting, in which case you have a new bootloader
> > (registers mapped at 0xf1), but you see the ATAG magic, which will make
> > you think you booted from an old bootloader (registers mapped at 0xd0).
> > 
> > For example, I'm currently booting alternatively with an old and a new
> > bootloader (to test that things work properly), and in both cases I'm
> > booting old style, DTB-appended, with ATAGs.
> 
> Is this something users would experience?  I think it is fairly safe to
> say that once dt-able bootloaders are shipped, they will provide a dtb.
> So, OF_DT_MAGIC == new bootloader might hold true for users.

We could be tempted to say that, but since what users trying to do this
would experience is a completely silent kernel, no message, nothing,
I'm not sure I like this too much.

Especially since users of the mainline kernel for Marvell platforms
have become used to do the appended DTB gymnastic, it's pretty likely
that they will do the same gymnastic when they'll move to more recent
Marvell platforms or bootloaders, not necessarily knowing that the
bootloader is now capable of doing DT-based booting.

Therefore, I continue to believe that making the assumptions that being
booted from DT == new bootloader == new mapping, being booted from ATAGS
== old boot == old mapping, should not be made.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Cooper]

On Wed, May 22, 2013 at 08:05:13PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Wed, 22 May 2013 13:13:59 -0400, Jason Cooper wrote:
> 
> > > On Wed, 22 May 2013 12:49:36 -0400, Jason Cooper wrote:
> > > 
> > > > > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > > > > ARM register that was used to pass the pointer to the ATAGS is now used
> > > > > to pass the pointer to the DTB in memory.
> > > > 
> > > > So we could look for the ATAG magic or the dtb magic at that address,
> > > > then we know if we have an old or new bootloader...
> > > 
> > > No, because you can use an old bootloader, and still do some old-style
> > 
> > Did you mean 'new bootloader'?
> 
> Gaah, yes, of course. Getting myself confused with all this old/new
> discussion :)
> 
> "No, because you can use a new bootloader, and still do some
> old-style..."
> 
> > > appended-DTB booting, in which case you have a new bootloader
> > > (registers mapped at 0xf1), but you see the ATAG magic, which will make
> > > you think you booted from an old bootloader (registers mapped at 0xd0).
> > > 
> > > For example, I'm currently booting alternatively with an old and a new
> > > bootloader (to test that things work properly), and in both cases I'm
> > > booting old style, DTB-appended, with ATAGs.
> > 
> > Is this something users would experience?  I think it is fairly safe to
> > say that once dt-able bootloaders are shipped, they will provide a dtb.
> > So, OF_DT_MAGIC == new bootloader might hold true for users.
> 
> We could be tempted to say that, but since what users trying to do this
> would experience is a completely silent kernel, no message, nothing,
> I'm not sure I like this too much.

Ok, I buy that.  Having stared at blank terminals many times, that's
not cool. :(  It really hurts to hear the HD spin down too.  It just
sounds like something died a horrible agonizing death.

thx,

Jason.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Jason Cooper,

On Wed, 22 May 2013 14:09:35 -0400, Jason Cooper wrote:

> > > Is this something users would experience?  I think it is fairly safe to
> > > say that once dt-able bootloaders are shipped, they will provide a dtb.
> > > So, OF_DT_MAGIC == new bootloader might hold true for users.
> > 
> > We could be tempted to say that, but since what users trying to do this
> > would experience is a completely silent kernel, no message, nothing,
> > I'm not sure I like this too much.
> 
> Ok, I buy that.  Having stared at blank terminals many times, that's
> not cool. :(  It really hurts to hear the HD spin down too.  It just
> sounds like something died a horrible agonizing death.

The other day, I was precisely trying to boot a kernel in a DT-way from
a bootloader that was in fact not DT capable (but I hadn't realized
that). And the kernel had no earlyprintk support. It was really
frustrating. No message at all, nothing. If we can avoid that
experience to the users of the Marvell platforms, I'd prefer. Something
that fails later on during boot is less problematic because at least
you get an error message that you can copy/paste in Google, or that
gives you some initial clue of what's going on. But a completely silent
kernel is terrifying.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Gunthorpe]

On Wed, May 22, 2013 at 06:57:57PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Wed, 22 May 2013 12:49:36 -0400, Jason Cooper wrote:
> 
> > > As far as I know, a DT-capable bootloader doesn't pass any ATAG. The
> > > ARM register that was used to pass the pointer to the ATAGS is now used
> > > to pass the pointer to the DTB in memory.
> > 
> > So we could look for the ATAG magic or the dtb magic at that address,
> > then we know if we have an old or new bootloader...
> 
> No, because you can use an old bootloader, and still do some old-style
> appended-DTB booting, in which case you have a new bootloader
> (registers mapped at 0xf1), but you see the ATAG magic, which will make
> you think you booted from an old bootloader (registers mapped at 0xd0).

Just to chime in a bit here.. We also hit this problem on Kirkwood,
and fixed it in the bootloader. The power on default for Kirkwood
chips is 0xd0000000 and Linux has long required the bootloader to move
it.. Not sure why 370/XP dropped this.

Also, can you talk abit more about how this works to give more low DDR
memory? The docs say you can't overlap windows, and DDR CS's have a
power of two size/alignment requirement.

Does this mean the desire is to place a DDR CS from 3G -> 3.5G?

> For example, I'm currently booting alternatively with an old and a new
> bootloader (to test that things work properly), and in both cases I'm
> booting old style, DTB-appended, with ATAGs.

IMHO the DTB must match both the hardware *and* the bootloader. The
bootloader is setting the address map, the DTB contains that address
map, they must all match together.

Using a DTB property really is the right way to go.

Yes, I realize this means you now need to vary the DTB you will use
depending on boot loader version (see below)

To use a DTB property, the approach would be to make the internal regs
base address inside the kernel dynamic instead of hard coded. Do not
remap the internal regs, just use whatever the bootloader setup
directly. The kernel can early parse the FDT to find the correct
address, instead of hardwiring an arbitary base address.

Also, the blind remapping approach in these patches is sketchy. If the
bootloader has placed some other mapping at the 0xf1.. target address
this will blow up. The mbus driver gets it right, to change the window
layout you have to clear out and disable all window registers except
for internal, then rebuild the entire mapping from scratch to avoid
address overlap conflicts. That approach doesn't seem possible from
early assembly.. :(

All things considered, I think it is much safer and cleaner to make
the physical base of internal regs dynamic within the kernel, than to
try and relocate it during early boot.

So, please consider this instead. The kernel has functions already to
parse the fdt directly, they can be called before anything attempts to
map internal regs based stuff.

You can combine this idea with the CP15 signalling mechanism. Check
CP15 and use it to alter the DTB, similar to how ATAGs are used to
fixup the DTB. Don't relocate the registers.

Jason

[PATCH 9/9] arm: mvebu: switch internal register address at runtime if needed

[Andrew Lunn]

> > > +		/*
> > > +		 * Change the physical base address of the
> > > +		 * registers. From now on, and until
> > > +		 * debug_ll_io_init() is called below, it is not
> > > +		 * possible to do any print, because earlyprintk will
> > > +		 * not work.
> > 
> > Is its "not work" or "locks up the CPU as dead as a Dodo?".
> 
> locks up the CPU as a Dodo

Then i would suggest something a bit strong than "will not work".

     Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Andrew Lunn]

> Just to chime in a bit here.. We also hit this problem on Kirkwood,
> and fixed it in the bootloader. The power on default for Kirkwood
> chips is 0xd0000000 and Linux has long required the bootloader to move
> it.. Not sure why 370/XP dropped this.
> 
> Also, can you talk abit more about how this works to give more low DDR
> memory? The docs say you can't overlap windows, and DDR CS's have a
> power of two size/alignment requirement.
> 
> Does this mean the desire is to place a DDR CS from 3G -> 3.5G?
> 
> > For example, I'm currently booting alternatively with an old and a new
> > bootloader (to test that things work properly), and in both cases I'm
> > booting old style, DTB-appended, with ATAGs.
> 
> IMHO the DTB must match both the hardware *and* the bootloader. The
> bootloader is setting the address map, the DTB contains that address
> map, they must all match together.
> 
> Using a DTB property really is the right way to go.

Interesting you said that, yet for your kirkwood, you hacked your
bootloader rather than Linux.

I think in practice, it is not going to be easy to match the DTB to
the hardware and the bootloader. Look at debians flash-kernel, used to
prepare the kernel for an embedded system. For each target it has a
database entry:

Machine: Globalscale Technologies Dreamplug
Kernel-Flavors: kirkwood
DTB-Id: kirkwood-dreamplug.dtb
DTB-Append: yes
U-Boot-Kernel-Address: 0x00008000
U-Boot-Initrd-Address: 0x0
Boot-Device: /dev/sda1
Boot-Kernel-Path: uImage
Boot-Initrd-Path: uInitrd
Boot-DTB-Path: dtb
Required-Packages: u-boot-tools
Bootloader-sets-root: no

So it appends the kirkwood-dreamplug.dtb blob to the kernel.  What you
are saying is that it also needs to somehow query the version of uboot
running on the hardware so it can pick the correct dtb blob from a
collection of kirkwood-dreamplug.dtb blobs and append it to the
kernel.

Is it even possible to query the uboot version from Linux?

To keep the problems tractable, we should not really be dependent on
the bootloader version.

    Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Gunthorpe]

On Wed, May 22, 2013 at 08:55:21PM +0200, Andrew Lunn wrote:
> > Just to chime in a bit here.. We also hit this problem on Kirkwood,
> > and fixed it in the bootloader. The power on default for Kirkwood
> > chips is 0xd0000000 and Linux has long required the bootloader to move
> > it.. Not sure why 370/XP dropped this.
> > 
> > Also, can you talk abit more about how this works to give more low DDR
> > memory? The docs say you can't overlap windows, and DDR CS's have a
> > power of two size/alignment requirement.
> > 
> > Does this mean the desire is to place a DDR CS from 3G -> 3.5G?
> > 
> > > For example, I'm currently booting alternatively with an old and a new
> > > bootloader (to test that things work properly), and in both cases I'm
> > > booting old style, DTB-appended, with ATAGs.
> > 
> > IMHO the DTB must match both the hardware *and* the bootloader. The
> > bootloader is setting the address map, the DTB contains that address
> > map, they must all match together.
> > 
> > Using a DTB property really is the right way to go.
> 
> Interesting you said that, yet for your kirkwood, you hacked your
> bootloader rather than Linux.

Not sure what you mean?

I don't care where internal-regs is on my box, so least-work is to
have the bootloader produce a DTB and memory map that matches the
hardwired expectations in Linux.

Keep in mind, the internal regs base is already encoded in the DTB,
but it must be 0xf1.. otherwise Linux blows up. That is a Linux 'bug',
IMHO.

> I think in practice, it is not going to be easy to match the DTB to
> the hardware and the bootloader. Look at debians flash-kernel, used to
> prepare the kernel for an embedded system. For each target it has a
> database entry:

Right, that is why I suggested using the CP15 indication to fix the
DTB on-the-fly. Similar to how ATAGs fixes the DTB. This makes the
meaning of the CP15 bit much cleaner: 'If set, override the internal
regs base in the DTB to be XYZ, if clear do not mangle the DTB'

Obviously this mangling should migrate into the DTB aware bootloader.

> So it appends the kirkwood-dreamplug.dtb blob to the kernel.  What
> you are saying is that it also needs to somehow query the version of
> uboot running on the hardware so it can pick the correct dtb blob
> from a collection of kirkwood-dreamplug.dtb blobs and append it to
> the kernel.

IMHO, if you are already booted and running in Linux you can just look
at the DTB Linux is already using to find a match in the available
DTBs. A DTB database is only needed for the OS install.

> To keep the problems tractable, we should not really be dependent on
> the bootloader version.

So, as bootloaders get updated to support native DTB, how do you
envision supporting that through flash-kernel? The way to set the DTB
is bootloader version dependent..

Jason

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Gregory CLEMENT wrote:
> But you proposed to use device tree instead of CP15. In this case we won't be
> able to use earlyprintk that was my point. You justified the fact to not use
> earlyprintk because we can't use it for multiplatform. So I am fine to not being
> able to use earlyprintk in multiplatform, but it sill important to be able to
> continue to use in non multiplatform case, hence the need of the use of CP15
> bit.

The use of devicetree would only be for the regualar kernel operation, not
for DEBUG_LL. DEBUG_LL can easily be hardcoded.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Andrew Lunn]

> So, as bootloaders get updated to support native DTB, how do you
> envision supporting that through flash-kernel? The way to set the DTB
> is bootloader version dependent..

You need to ask the Debian people. But my guess is, they will keep
appending the DTB blob to the kernel. They then have control of it in
an easy way. I've no idea what happens if there is both an appended
blob and one passed by the boot loader. I've not yet actually seen a
kirkwood u-boot which supports DT on COTS hardware. So it may never
actually come to pass, except for machines in the hands of kernel
hackers who update the bootloader.

	Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Thomas Petazzoni wrote:
> On Wed, 22 May 2013 17:51:34 +0200, Andrew Lunn wrote:
> 
> > Maybe i'm missunderstand Arnd, but i think he is suggesting a property
> > somewhere in DT which says where the bootloader left the register
> > space before jumping to the kernel. You can then look at this property
> > and then decide if you need to remap or not. Only this one property
> > needs to differ between old and new bootloader. You could even say, if
> > the property is not in DT, a remap is needed.

No, the idea was that we don't need to remap if we know where the registers
are. The mbus driver might remap them if it is really careful about not
breaking any existing mappings, but it's probably easier to just leave
them alone.

> The problem is that neither the old nor the new bootloaders are setting
> such property in the DT. Old bootloaders shipped by Marvell don't have
> DT support at all. New bootloaders shipped by Marvell do have DT
> support, but they are not adding any specific DT property that allows
> us to tell whether the registers are mapped here or there.
> 
> We could maybe find a way of detecting whether if the bootloader is DT
> capable, and assume that if it's DT capable, the registers are already
> mapped at 0xf1. But that's very fragile: nothing prevents an user to
> use a DT-capable bootloader to boot a kernel in the old style way
> (appended DTB). In this case, we would assume that the registers are at
> 0xd0, because we're being booted old-style, and therefore it must be an
> old bootloader. Very fragile, clearly not better than the CP15 trick.

Yes, that would not be an improvement. However, since we know what boot
loader we have when we pass the devicetree, that devicetree can just
contain the correct 'reg' property for the internal registers.

> > However, i don't know if this can actually work. How early can you
> > parse the DT in order to know where the serial port is for
> > earlyprintk()? It also gets messy keeping track of the two different
> > DT binary blobs and somehow having the probe the uboot version in
> > order to install the right one. I don't know how i would include this
> > into the debian flash-kernel for example.
> 
> For earlyprintk, we can "solve" it as Arnd suggested: just have two
> Kconfig options, one for 0xd0, one for 0xf1, and it's up to the user to
> choose the right one. It's not entirely nice because it's no longer
> automatic, and I'm sure our users will have a hard time figuring out
> whether they have an "old" or a "new" bootloader. But let's assume we
> do this. earlyprintk knows the physical address of the UART.
> 
> Then, we enter ->map_io(). We have access to the Device Tree. But the
> Device Tree doesn't tell us whether the bootloader has mapped the
> registers. As I explained to Arnd, it's not a per-board criteria: when
> I'm booting my OpenBlocks with the manufacturer U-Boot, registers are
> mapped at 0xd0. When I boot the same OpenBlocks with the (admittedly
> work-in-progress) Barebox bootloader, registers are mapped at 0xf1.
> Same board, different register location.

But since you control barebox, you can find the "regs" property of the
mbus node in it and set it up to the same address you use when you
map the internal registers.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Wednesday 22 May 2013, Thomas Petazzoni wrote:
> > More importantly, you can build a kernel with any number of other SoCs
> > already, and if you have a combined ArmadaXP+OMAP+Highbank kernel,
> > there is no way to get a working early debug output.
> 
> So you're proposing to add two Kconfig options, one to have the UART at
> 0xd0012000 and the other one to have the UART at 0xf1012000. The user
> would have to know whether he is going to boot with an old bootloader
> or new bootloader, and select the appropriate option. This means that
> the mvebu_defconfig would no longer be able to have earlyprintk enabled
> by default, otherwise the resulting kernel may not boot for some
> people, but maybe it's a reasonable tradeoff.

Yes. It may also be reasonable to use the CP15 hack for DEBUG_LL, but
I would not like to see that information getting used anywhere in the
kernel outside of DEBUG_LL. That should really come from the DT.

> > > > As you already admit, using the CP15 register is a hack. It sounds
> > > > to me that a cleaner approach would be to put the correct address
> > > > into the device tree and use that value everywhere, rather than
> > > > hardcoding one or more addresses.
> > > 
> > > I don't see how this can fix the problem. Which internal register base
> > > address do we put in our Device Tree source in arch/arm/boot/dts? The
> > > old one? The new one?
> > 
> > The one matching the boot loader for that board, i.e. the new address
> > for all boards but Mirabox and OpenBlocks.
> 
> And what happens when Globalscale (Mirabox manufacturer) or Plathome
> (manufacturer of OpenBlocks) release a new bootloader version, based on
> the new bootloader from Marvell, which remaps things at 0xf1000000 ? Or
> when those people boot from Barebox, which remaps at 0xf1000000 ?
> 
> Conclusion: you can't classify on one side boards that are at 0xd0 and
> boards that are at 0xf1. It depends on which bootloader each
> particular instance of those boards will be using. Will it be a
> bootloader that complies with the "old" protocol (CP15 bit cleared and
> registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> 0xf1).
> 
> So we can't encode into the boards Device Tree whether the registers
> are at 0xd0 or 0xf1, because it's not a board-related information, but
> a bootloader-related information.

The new boot loaders will of course know that they are remapping the
registers, so they are also able to put the correct information into the
DT.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Sebastian Hesselbarth]

On 05/22/2013 07:14 PM, Thomas Petazzoni wrote:
> On Wed, 22 May 2013 19:08:07 +0200, Willy Tarreau wrote:
>> On Wed, May 22, 2013 at 07:00:04PM +0200, Thomas Petazzoni wrote:
>>> Of course, one option is simply to have a kconfig option "I am booting
>>> from an old bootloader", but with the CP15 trick, we've tried to make
>>> this automatic so that users don't have to understand the kind of gory
>>> details we are discussing right now.
>>
>> Same as for ATAGS right now which are not necessarily the most obvious
>> setting for end users. Better keep the trick on by default if it's safe
>> enough. In fact I'm mostly concerned about the risk that it later breaks
>> for even newer boot loaders. But then we'll have more feedback to decide.
>
> As I suggested earlier, we can print a warning when people boot from an
> old bootloader, and then down the road get rid of the trick. Or move it
> under a conditional compilation so that the minority of remaining users
> will have to enable this particular option. This will anyway be needed
> if we want to go multiplatform with Dove, for example.

I am not so sure Kconfig for Dove is really required, if it moves to
mach-mvebu. As I already stated before, Dove is happily accessing
0xd0000000 even if internal registers mapped on 0xf1000000. So maybe
the enforced remap will do no harm and at max write in some RAM mapped
there.

Although, I doubt that there will ever be a Dove board with RAM at 3GB
addresses with only two DDR CS available. Currently, for DDR3 DRAMs
8Gbit is the largest you can get, DDR2 ends at 4Gbit but >3GB RAM would 
require 16Gbit DRAMs.

Also I guess Dove is about to be replaced by Armada 1000/1500 for new
products.

Sebastian

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Wed, 22 May 2013 22:40:04 +0200, Arnd Bergmann wrote:

> > So you're proposing to add two Kconfig options, one to have the UART at
> > 0xd0012000 and the other one to have the UART at 0xf1012000. The user
> > would have to know whether he is going to boot with an old bootloader
> > or new bootloader, and select the appropriate option. This means that
> > the mvebu_defconfig would no longer be able to have earlyprintk enabled
> > by default, otherwise the resulting kernel may not boot for some
> > people, but maybe it's a reasonable tradeoff.
> 
> Yes. It may also be reasonable to use the CP15 hack for DEBUG_LL, but
> I would not like to see that information getting used anywhere in the
> kernel outside of DEBUG_LL. That should really come from the DT.

Ok.


> > Conclusion: you can't classify on one side boards that are at 0xd0 and
> > boards that are at 0xf1. It depends on which bootloader each
> > particular instance of those boards will be using. Will it be a
> > bootloader that complies with the "old" protocol (CP15 bit cleared and
> > registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> > 0xf1).
> > 
> > So we can't encode into the boards Device Tree whether the registers
> > are at 0xd0 or 0xf1, because it's not a board-related information, but
> > a bootloader-related information.
> 
> The new boot loaders will of course know that they are remapping the
> registers, so they are also able to put the correct information into the
> DT.

The thing that I believe you don't realize is that those new bootloaders
are already shipping *today*. And they are not putting whatever
"correct" information into the DT you believe they should be putting.

So what do we do, today, with those new bootloaders, that are not the
future, but the present. Again, they do *not* add any information in
the DT that allows us to know that they have remapped the registers to
0xf1.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Andrew Lunn]

> > The new boot loaders will of course know that they are remapping the
> > registers, so they are also able to put the correct information into the
> > DT.
> 
> The thing that I believe you don't realize is that those new bootloaders
> are already shipping *today*. And they are not putting whatever
> "correct" information into the DT you believe they should be putting.
> 
> So what do we do, today, with those new bootloaders, that are not the
> future, but the present. Again, they do *not* add any information in
> the DT that allows us to know that they have remapped the registers to
> 0xf1.

That is a key point. Compared to the kernel, the bootloaders a very
slow to evolve new features, even slower to get deployed, and have a
very long life without ever getting upgraded. The kernel has to deal
with legacy bootloaders.

What we seem to be heading towards is a bootloader which actually uses
the DT itself, can modify it dynamically to reflect the running system,
and pass it to the kernel.

What we have today, on Marvell boards, is mostly no support for DT at
all with blobs appended to the kernel with tools like debians
flash-kernel. Maybe this new Marvell uboot can load the blob in the
same way it can load the kernel, or initrd, from flash, a file on a
disk, tftpboot, etc, but i expect it is just an opaque binary blob to
uboot.

I guess your bootloader which will put the correct information into
the DT is a few years away.

    Andrew

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Wed, May 22, 2013 at 11:31:08PM +0200, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> The thing that I believe you don't realize is that those new bootloaders
> are already shipping *today*. And they are not putting whatever
> "correct" information into the DT you believe they should be putting.

Well, if they're not putting addresses of registers into the DT, then
that DT information is surely pretty much useless to the kernel - or
anything.

If it's putting _wrong_ addresses in the DT, then the same thing applies.

Either way, it says "you need to change the DT blob passed to the kernel."

Luckily, as has already been agreed, DT blobs will not be hard-coded into
boot loaders, so that's easily sorted - you just tell people in the
installation instructions "if you have X versions of the boot loader, you
need to use DT blob X, otherwise you need to use blob Y."

>From what you're saying, that's the best that can be done - the kernel
can't do much else.

I'd also hasten to point out that even if we could detect the base of the
registers, with DT, what matters is what's in the DT blob itself, which
should override any detection.  So the DT blob would need to be updated
in any case.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Wed, 22 May 2013 12:19:53 -0600, Jason Gunthorpe wrote:

> Just to chime in a bit here.. We also hit this problem on Kirkwood,
> and fixed it in the bootloader. The power on default for Kirkwood
> chips is 0xd0000000 and Linux has long required the bootloader to move
> it.. Not sure why 370/XP dropped this.

This is a mistake from the original support of Armada 370/XP. What we
want to do now is to fix this mistake by moving everybody to 0xf1 and
align with the other Marvell EBU platforms.

What we're proposing here is only a *temporary* solution to provide a
smooth upgrade path for users of platforms that have an old bootloader.
This temporary solution is meant to go away when we have a good
confidence that new bootloaders for OpenBlocks and Mirabox are
available, and that enough users will have seen the warning that we
plan to add in two release cycles.

> Also, can you talk abit more about how this works to give more low DDR
> memory? The docs say you can't overlap windows, and DDR CS's have a
> power of two size/alignment requirement.
> 
> Does this mean the desire is to place a DDR CS from 3G -> 3.5G?

The MBus windows can overlap the DRAM chip selects. On Armada 370/XP,
there is a tuple of registers, called the MBus Bridge Window Base
Register and the MBus Bridge Window Control Register, that provide a
range of addresses for which the CPU should go look at MBus windows
instead of looking at the DRAM.

So, on a board with 8 GB of RAM, you have one DRAM chip select for the
first 4 GB of RAM, and a second chip select for the last 4 GB of RAM.
This is currently visible with the Armada XP GP board:

# cat /sys/kernel/debug/mvebu-mbus/sdram 
[0] 0000000000000000 - 0000000100000000 : cs0
[1] 0000000100000000 - 0000000200000000 : cs1
[2] disabled
[3] disabled

And the internal registers (be they mapped at 0xd0000000 of 0xf1000000)
and all the other windows (for PCIe and al) are overlapping the DRAM CS.

This is not a problem, because the MBus Bridge Window {Base,Control}
Register tell the CPU that memory accesses between addresses X and Y
are for MBus windows, and the other accesses are for the DRAM.

With the old bootloader, the MBus Bridge Window Base is set to
0xC0000000 (3G), so you cannot benefit from this additional memory
between 3G and 3.5G. But the only two consumers platforms available
that are using the old bootloaders have 3 GB of RAM or less (the
OpenBlocks has an expansion slot, but I believe it's reasonable to ask
the user to upgrade the bootloader if he wants to use a larger RAM on
this specific board).

With the new bootloader, the MBus Bridge Window Base is set to
0xE0000000 (3.5 GB). So any access in [ 0 ; 3.5 GB ] and [ 4 GB; ... ]
will go to the DRAM, and access between [ 3.5 GB ; 4 GB ] will go to
the MBus windows, including the internal registers one.

Does that make sense to explain how we can actually save 512 MB of RAM ?

> > For example, I'm currently booting alternatively with an old and a new
> > bootloader (to test that things work properly), and in both cases I'm
> > booting old style, DTB-appended, with ATAGs.
> 
> IMHO the DTB must match both the hardware *and* the bootloader. The
> bootloader is setting the address map, the DTB contains that address
> map, they must all match together.
> 
> Using a DTB property really is the right way to go.
> 
> Yes, I realize this means you now need to vary the DTB you will use
> depending on boot loader version (see below)
> 
> To use a DTB property, the approach would be to make the internal regs
> base address inside the kernel dynamic instead of hard coded. Do not
> remap the internal regs, just use whatever the bootloader setup
> directly. The kernel can early parse the FDT to find the correct
> address, instead of hardwiring an arbitary base address.
> 
> Also, the blind remapping approach in these patches is sketchy. If the
> bootloader has placed some other mapping at the 0xf1.. target address
> this will blow up. The mbus driver gets it right, to change the window
> layout you have to clear out and disable all window registers except
> for internal, then rebuild the entire mapping from scratch to avoid
> address overlap conflicts. That approach doesn't seem possible from
> early assembly.. :(
> 
> All things considered, I think it is much safer and cleaner to make
> the physical base of internal regs dynamic within the kernel, than to
> try and relocate it during early boot.

We believe it is not safer to make the physical base of internal regs
dynamic within the kernel. Really, the 0xd0 usage should be seen as a
mistake, and the goal is to align everybody to use 0xf1 for all Marvell
platforms.

> So, please consider this instead. The kernel has functions already to
> parse the fdt directly, they can be called before anything attempts to
> map internal regs based stuff.
> 
> You can combine this idea with the CP15 signalling mechanism. Check
> CP15 and use it to alter the DTB, similar to how ATAGs are used to
> fixup the DTB. Don't relocate the registers.

Where would the code doing this would sit? Remember, this is a
temporary solution that we want to get rid of after some time. So I've
been very careful in this proposal to only touch mvebu-specific code.

With what you're proposing, we would have to add something like:

	if (of_machine_is_compatible("marvell,armada-370-xp")) {
		/* do our crap */
	}

in some ARM generic code. That's something we would definitely like to
avoid, and I'm pretty the ARM maintainers would also like to avoid such
SoC-specific code spreading into generic kernel places.

Could you be more specific about where such early code would be
located, and what it would look like? I'm pretty sure once we will see
it, we'll clearly see that it's not acceptable to put such SoC-specific
code in a generic place of the kernel.

Thanks for your feedback,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Wed, 22 May 2013 22:36:12 +0200, Arnd Bergmann wrote:

> > > Maybe i'm missunderstand Arnd, but i think he is suggesting a property
> > > somewhere in DT which says where the bootloader left the register
> > > space before jumping to the kernel. You can then look at this property
> > > and then decide if you need to remap or not. Only this one property
> > > needs to differ between old and new bootloader. You could even say, if
> > > the property is not in DT, a remap is needed.
> 
> No, the idea was that we don't need to remap if we know where the registers
> are. The mbus driver might remap them if it is really careful about not
> breaking any existing mappings, but it's probably easier to just leave
> them alone.

Again, our aim is to get everybody to run kernels with registers
remapped at 0xf1, so that we're back to what should have been done
originally.

Ideally, we'd like to completely forget about supporting old
bootloaders, and assume we only have good bootloaders that remap things
to 0xf1. We're only proposing this *temporary* solution to make the
transition easier, give some time to the board manufacturers to release
updated bootloaders, to users to upgrade their bootloaders, and finally
remove this temporary solution.

We clearly don't want to go down the road of supporting an
arbitrarily-defined base address for the internal registers. This has
never been the case for Kirkwood, Orion5x, Dove, and there is no
compelling reason to make it a requirement for Armada 370/XP.

> > The problem is that neither the old nor the new bootloaders are setting
> > such property in the DT. Old bootloaders shipped by Marvell don't have
> > DT support at all. New bootloaders shipped by Marvell do have DT
> > support, but they are not adding any specific DT property that allows
> > us to tell whether the registers are mapped here or there.
> > 
> > We could maybe find a way of detecting whether if the bootloader is DT
> > capable, and assume that if it's DT capable, the registers are already
> > mapped at 0xf1. But that's very fragile: nothing prevents an user to
> > use a DT-capable bootloader to boot a kernel in the old style way
> > (appended DTB). In this case, we would assume that the registers are at
> > 0xd0, because we're being booted old-style, and therefore it must be an
> > old bootloader. Very fragile, clearly not better than the CP15 trick.
> 
> Yes, that would not be an improvement. However, since we know what boot
> loader we have when we pass the devicetree, that devicetree can just
> contain the correct 'reg' property for the internal registers.

What do you mean by "we know what boot loader we have when we pass the
devicetree" ?

The mere user of the kernel just does:

	make ARCH=arm mvebu_defconfig
	make ARCH=arm CROSS_COMPILE=...
	cat arch/arm/boot/zImage arch/arm/boot/dts/armada-370-mirabox.dtb > zImage.mirabox
	... prepare uImage ...

and he certainly doesn't want to understand the gory details of which
internal register base address to chose depending on which bootloader
version he is using. This is really asking normal users to have a too
much intimate knowledge of the hardware details.

The temporary solution we're proposing doesn't have this drawback: an
user can boot the kernel on either an old or new bootloader, without
having to worry about checking/changing the Device Tree source about
this bizarre internal register address thing.

As a developer of the Armada 370/XP, I am frequently contacted by users
to help them getting the mainline kernel booting on their Mirabox or
OpenBlocks. And trust me: even getting the appended DTB gymnastic right
is not easy. So getting this internal register story right is going to
be a nightmare in terms of support.

Please do not inflict to us such a support pain :-)

> > > However, i don't know if this can actually work. How early can you
> > > parse the DT in order to know where the serial port is for
> > > earlyprintk()? It also gets messy keeping track of the two different
> > > DT binary blobs and somehow having the probe the uboot version in
> > > order to install the right one. I don't know how i would include this
> > > into the debian flash-kernel for example.
> > 
> > For earlyprintk, we can "solve" it as Arnd suggested: just have two
> > Kconfig options, one for 0xd0, one for 0xf1, and it's up to the user to
> > choose the right one. It's not entirely nice because it's no longer
> > automatic, and I'm sure our users will have a hard time figuring out
> > whether they have an "old" or a "new" bootloader. But let's assume we
> > do this. earlyprintk knows the physical address of the UART.
> > 
> > Then, we enter ->map_io(). We have access to the Device Tree. But the
> > Device Tree doesn't tell us whether the bootloader has mapped the
> > registers. As I explained to Arnd, it's not a per-board criteria: when
> > I'm booting my OpenBlocks with the manufacturer U-Boot, registers are
> > mapped at 0xd0. When I boot the same OpenBlocks with the (admittedly
> > work-in-progress) Barebox bootloader, registers are mapped at 0xf1.
> > Same board, different register location.
> 
> But since you control barebox, you can find the "regs" property of the
> mbus node in it and set it up to the same address you use when you
> map the internal registers.

We may control Barebox because the support is not yet written, but we
clearly do not have much control over what Marvell is doing with its
own U-Boot. And let's be pragmatic: 99.99% of the Marvell 370/XP
platforms will be running the U-Boot from Marvell.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Wed, 22 May 2013 22:40:04 +0200, Arnd Bergmann wrote:

> > So you're proposing to add two Kconfig options, one to have the UART at
> > 0xd0012000 and the other one to have the UART at 0xf1012000. The user
> > would have to know whether he is going to boot with an old bootloader
> > or new bootloader, and select the appropriate option. This means that
> > the mvebu_defconfig would no longer be able to have earlyprintk enabled
> > by default, otherwise the resulting kernel may not boot for some
> > people, but maybe it's a reasonable tradeoff.
> 
> Yes. It may also be reasonable to use the CP15 hack for DEBUG_LL, but
> I would not like to see that information getting used anywhere in the
> kernel outside of DEBUG_LL. That should really come from the DT.

Ok.

> > And what happens when Globalscale (Mirabox manufacturer) or Plathome
> > (manufacturer of OpenBlocks) release a new bootloader version, based on
> > the new bootloader from Marvell, which remaps things at 0xf1000000 ? Or
> > when those people boot from Barebox, which remaps at 0xf1000000 ?
> > 
> > Conclusion: you can't classify on one side boards that are at 0xd0 and
> > boards that are at 0xf1. It depends on which bootloader each
> > particular instance of those boards will be using. Will it be a
> > bootloader that complies with the "old" protocol (CP15 bit cleared and
> > registers at 0xd0) or the "new" protocol (CP15 bit set and registers at
> > 0xf1).
> > 
> > So we can't encode into the boards Device Tree whether the registers
> > are at 0xd0 or 0xf1, because it's not a board-related information, but
> > a bootloader-related information.
> 
> The new boot loaders will of course know that they are remapping the
> registers, so they are also able to put the correct information into the
> DT.

But that's *not* the case. Neither the old nor the new bootloaders are
changing the Device Tree in any way that allows us to know whether
registers are mapped at 0xd0 and 0xf1. Those bootloaders are already in
the field, and we have little control over that. Even if we had
control, what we would tell Marvell to implement in their U-Boot,
today? The DT binding of the mbus driver is not complete, and therefore
the mechanism to specify the location of the internal registers is
going to change when we introduce the DT binding for the mbus driver.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Thursday 23 May 2013, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> On Wed, 22 May 2013 22:36:12 +0200, Arnd Bergmann wrote:
> 
> > > > Maybe i'm missunderstand Arnd, but i think he is suggesting a property
> > > > somewhere in DT which says where the bootloader left the register
> > > > space before jumping to the kernel. You can then look at this property
> > > > and then decide if you need to remap or not. Only this one property
> > > > needs to differ between old and new bootloader. You could even say, if
> > > > the property is not in DT, a remap is needed.
> > 
> > No, the idea was that we don't need to remap if we know where the registers
> > are. The mbus driver might remap them if it is really careful about not
> > breaking any existing mappings, but it's probably easier to just leave
> > them alone.
> 
> Again, our aim is to get everybody to run kernels with registers
> remapped at 0xf1, so that we're back to what should have been done
> originally.
> 
> Ideally, we'd like to completely forget about supporting old
> bootloaders, and assume we only have good bootloaders that remap things
> to 0xf1. We're only proposing this *temporary* solution to make the
> transition easier, give some time to the board manufacturers to release
> updated bootloaders, to users to upgrade their bootloaders, and finally
> remove this temporary solution.
> 
> We clearly don't want to go down the road of supporting an
> arbitrarily-defined base address for the internal registers. This has
> never been the case for Kirkwood, Orion5x, Dove, and there is no
> compelling reason to make it a requirement for Armada 370/XP.

Well, the main reason I see now is that Marvell screwed it up by having
two incompatible versions of u-boot. Using a different base address on
new machines would have been no problem at all, but supporting the
same machine with different addresses depending on the boot loader
version is madness. As you say, it's too late to change that now, so
maybe the best solution is to make the registers always get reassigned.
5 trick.
> > 
> > Yes, that would not be an improvement. However, since we know what boot
> > loader we have when we pass the devicetree, that devicetree can just
> > contain the correct 'reg' property for the internal registers.
> 
> What do you mean by "we know what boot loader we have when we pass the
> devicetree" ?
> 
> The mere user of the kernel just does:
> 
> 	make ARCH=arm mvebu_defconfig
> 	make ARCH=arm CROSS_COMPILE=...
> 	cat arch/arm/boot/zImage arch/arm/boot/dts/armada-370-mirabox.dtb > zImage.mirabox
> 	... prepare uImage ...
> 
> and he certainly doesn't want to understand the gory details of which
> internal register base address to chose depending on which bootloader
> version he is using. This is really asking normal users to have a too
> much intimate knowledge of the hardware details.

Right, appended device tree is a problem, but if a user does that, I think
we can rightfully expect them to know what they are doing.

> The temporary solution we're proposing doesn't have this drawback: an
> user can boot the kernel on either an old or new bootloader, without
> having to worry about checking/changing the Device Tree source about
> this bizarre internal register address thing.
> 
> As a developer of the Armada 370/XP, I am frequently contacted by users
> to help them getting the mainline kernel booting on their Mirabox or
> OpenBlocks. And trust me: even getting the appended DTB gymnastic right
> is not easy. So getting this internal register story right is going to
> be a nightmare in terms of support.
> 
> Please do not inflict to us such a support pain :-)

I think we should consequently make sure we never get a production
u-boot for Mirabox or OpenBlocks that changes the internal register
address and also allows ATAGS based booting. Either those machines
continue to use the 0xd0 address for the internal registers, or they
must ensure to fix up the device tree properly.

Since you explained that we want to move the internal registers to get
more available memory, I think what you also want to do is to dynamically
reassign the internal register mapping when the mbus driver gets loaded,
just like all the other mbus mappings. That will let you move it even
higher to free up more memory, and it means we can tell the Mirabox
and OpenBlocks people to leave the internal register base address alone
in their u-boot, so we don't have two conflicting versions of those
machines.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Thursday 23 May 2013, Thomas Petazzoni wrote:
> But that's not the case. Neither the old nor the new bootloaders are
> changing the Device Tree in any way that allows us to know whether
> registers are mapped at 0xd0 and 0xf1. Those bootloaders are already in
> the field, and we have little control over that. Even if we had
> control, what we would tell Marvell to implement in their U-Boot,
> today?

We would tell Marvell that changing the base address was a horrible
mistake and that they have to revert to 0xd0 in order to make sure
the mistake does not spread any further and all users would update.

> The DT binding of the mbus driver is not complete, and therefore
> the mechanism to specify the location of the internal registers is
> going to change when we introduce the DT binding for the mbus driver.

Right. It should not be hard to agree on the binding at least, but
you are correct that we don't have that today and we shouldn't rush
things here.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Thu, 23 May 2013 16:12:42 +0200, Arnd Bergmann wrote:

> > Ideally, we'd like to completely forget about supporting old
> > bootloaders, and assume we only have good bootloaders that remap
> > things to 0xf1. We're only proposing this *temporary* solution to
> > make the transition easier, give some time to the board
> > manufacturers to release updated bootloaders, to users to upgrade
> > their bootloaders, and finally remove this temporary solution.
> > 
> > We clearly don't want to go down the road of supporting an
> > arbitrarily-defined base address for the internal registers. This
> > has never been the case for Kirkwood, Orion5x, Dove, and there is no
> > compelling reason to make it a requirement for Armada 370/XP.
> 
> Well, the main reason I see now is that Marvell screwed it up by
> having two incompatible versions of u-boot. Using a different base
> address on new machines would have been no problem at all, but
> supporting the same machine with different addresses depending on the
> boot loader version is madness. As you say, it's too late to change
> that now, so maybe the best solution is to make the registers always
> get reassigned. 5 trick.

Not sure what you mean by '5 trick', and what I should understand from
this :(

> > What do you mean by "we know what boot loader we have when we pass
> > the devicetree" ?
> > 
> > The mere user of the kernel just does:
> > 
> > 	make ARCH=arm mvebu_defconfig
> > 	make ARCH=arm CROSS_COMPILE=...
> > 	cat arch/arm/boot/zImage
> > arch/arm/boot/dts/armada-370-mirabox.dtb > zImage.mirabox ...
> > prepare uImage ...
> > 
> > and he certainly doesn't want to understand the gory details of
> > which internal register base address to chose depending on which
> > bootloader version he is using. This is really asking normal users
> > to have a too much intimate knowledge of the hardware details.
> 
> Right, appended device tree is a problem, but if a user does that, I
> think we can rightfully expect them to know what they are doing.

Unfortunately, not really. Appended Device Tree is the only way to go
for old bootloaders, and we don't want users of such bootloaders to
have to modify their Device Tree to change the base address of the
internal registers.

> > The temporary solution we're proposing doesn't have this drawback:
> > an user can boot the kernel on either an old or new bootloader,
> > without having to worry about checking/changing the Device Tree
> > source about this bizarre internal register address thing.
> > 
> > As a developer of the Armada 370/XP, I am frequently contacted by
> > users to help them getting the mainline kernel booting on their
> > Mirabox or OpenBlocks. And trust me: even getting the appended DTB
> > gymnastic right is not easy. So getting this internal register
> > story right is going to be a nightmare in terms of support.
> > 
> > Please do not inflict to us such a support pain :-)
> 
> I think we should consequently make sure we never get a production
> u-boot for Mirabox or OpenBlocks that changes the internal register
> address and also allows ATAGS based booting.

It is impossible to prevent that. U-Boot always supports ATAGs booting
for ARM, and optionally Device Tree based booting.

And once again, you believe we have an enormous control over what is
happening at the bootloader level. So when you say "we should
consequently make sure ..", this is simply not possible: we have no
control over this.

> Either those machines continue to use the 0xd0 address for the
> internal registers, or they must ensure to fix up the device tree
> properly.

We don't have such control over bootloaders. I believe Marvell has done
a great achievement by pushing all this Armada 370/XP code to the
mainline kernel. However, they are not (yet) at the same level of
open-source involvement as far as bootloaders are concerned, so we have
little control over that. I'm hoping that the situation will evolve
over time, but for now, we have to live with the assumption that the
control we have over what the bootloader is doing is minimal.

So any solution you propose with "the bootloader should do X or Y" will
unfortunately not work practically for us.

> Since you explained that we want to move the internal registers to get
> more available memory, I think what you also want to do is to
> dynamically reassign the internal register mapping when the mbus
> driver gets loaded, just like all the other mbus mappings.

Ideally, it should be like this. Unfortunately, moving the internal
register mapping is really a painful operation: you have to know where
is the address to change the internal register base address (which
itself changes when the internal register base address changes), and all
code interacting with peripherals before the change of the address must
be synchronized so that after the switch it uses the new address.

Switching the internal registers address at runtime after a lot of
things have booted is *painful* and we really don't want to do that.
Sebastian Hesselbarth, who has worked on the topic, will confirm this.

So, just like it has been the case for years in Kirkwood, Dove and
Orion5x, Armada 370/XP will have their registers mapped at 0xf1000000
when the kernel is booted. As far as Linux is concerned, it should
assume that the base address of the register is *not* configurable,
just like it is the case for many other ARM SoCs.

What we're only asking is to have a temporary, SoC-specific, workaround
to ease the transition towards this goal.

We've gone through great lengths to find a solution that does not
touch *any* ARM generic code (even though it would have been much
simpler!). At least one Marvell maintainer (Andrew Lunn) is fine with
this temporary solution, and since the issue is very SoC-specific, and
completely self-contained in the SoC-specific code, would it be
possible to be pragmatic and let us merge such a workaround?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Thu, 23 May 2013 16:14:53 +0200, Arnd Bergmann wrote:
> On Thursday 23 May 2013, Thomas Petazzoni wrote:
> > But that's not the case. Neither the old nor the new bootloaders are
> > changing the Device Tree in any way that allows us to know whether
> > registers are mapped at 0xd0 and 0xf1. Those bootloaders are
> > already in the field, and we have little control over that. Even if
> > we had control, what we would tell Marvell to implement in their
> > U-Boot, today?
> 
> We would tell Marvell that changing the base address was a horrible
> mistake and that they have to revert to 0xd0 in order to make sure
> the mistake does not spread any further and all users would update.

The longer we wait to switch from 0xd0 to 0xf1, the more complex and
painful the migration will be.

Honestly, from Marvell perspective, switching to 0xf1 and dropping
support for the old platforms is not a problem. I insisted quite a lot
to be given time to find a reasonable solution, that is entirely
self-contained in SoC-specific code, to provide a transition path for
people who are using old bootloaders.

I'm humbly asking you to be pragmatic on this one, and understand that
the goal is to have the kernel assumes it's being booted with registers
configured at 0xf1 and that all the rest is a temporary workaround.

> > The DT binding of the mbus driver is not complete, and therefore
> > the mechanism to specify the location of the internal registers is
> > going to change when we introduce the DT binding for the mbus
> > driver.
> 
> Right. It should not be hard to agree on the binding at least, but
> you are correct that we don't have that today and we shouldn't rush
> things here.

Agreed. We are currently working on this, but it will take some time to
get sorted out properly.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Jason Gunthorpe]

On Thu, May 23, 2013 at 11:53:21AM +0200, Thomas Petazzoni wrote:

> This is not a problem, because the MBus Bridge Window {Base,Control}
> Register tell the CPU that memory accesses between addresses X and Y
> are for MBus windows, and the other accesses are for the DRAM.

Okay, I see, you are minimizing the size of the memory hole, sounds
good.

> > You can combine this idea with the CP15 signalling mechanism. Check
> > CP15 and use it to alter the DTB, similar to how ATAGs are used to
> > fixup the DTB. Don't relocate the registers.
> 
> Where would the code doing this would sit? Remember, this is a
> temporary solution that we want to get rid of after some time. So I've
> been very careful in this proposal to only touch mvebu-specific code.

Well, there are three elements here:
 1) Remove ARMADA_370_XP_REGS_PHYS_BASE and related (looks nearly done now)
 2) Adjust the FDT/DT before it is used based on CP15
 3) Support early printk

#1 looks like it is done when the mbus driver is merged, ie
   we will soon have full dynamic support for the internal regs base
#2 can be done in the machine's init_early, nothing will touch
   internal regs prior to init_early
#3 is for expert use, so the rules can be a bit different:
   - Patch the XP/370 specific early printk code to look in CP15
     to compute the base address
   - .. or rely on a CONFIG_XXX to set the serial base address
   - .. or parse the FDT to find the serial port, requiring
     that the developer has made the FDT match the bootloader.

So, I'm not sure what common code would need to be touched, do you see
something else?

Jason

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Thursday 23 May 2013, Thomas Petazzoni wrote:
> 
> On Thu, 23 May 2013 16:12:42 +0200, Arnd Bergmann wrote:
>
> > Well, the main reason I see now is that Marvell screwed it up by
> > having two incompatible versions of u-boot. Using a different base
> > address on new machines would have been no problem at all, but
> > supporting the same machine with different addresses depending on the
> > boot loader version is madness. As you say, it's too late to change
> > that now, so maybe the best solution is to make the registers always
> > get reassigned. 5 trick.
> 
> Not sure what you mean by '5 trick', and what I should understand from
> this :(

I have no idea myself how that got there, don't remember typing it.

Very odd.

> > > What do you mean by "we know what boot loader we have when we pass
> > > the devicetree" ?
> > > 
> > > The mere user of the kernel just does:
> > > 
> > > 	make ARCH=arm mvebu_defconfig
> > > 	make ARCH=arm CROSS_COMPILE=...
> > > 	cat arch/arm/boot/zImage
> > > arch/arm/boot/dts/armada-370-mirabox.dtb > zImage.mirabox ...
> > > prepare uImage ...
> > > 
> > > and he certainly doesn't want to understand the gory details of
> > > which internal register base address to chose depending on which
> > > bootloader version he is using. This is really asking normal users
> > > to have a too much intimate knowledge of the hardware details.
> > 
> > Right, appended device tree is a problem, but if a user does that, I
> > think we can rightfully expect them to know what they are doing.
> 
> Unfortunately, not really. Appended Device Tree is the only way to go
> for old bootloaders, and we don't want users of such bootloaders to
> have to modify their Device Tree to change the base address of the
> internal registers.

But then the old boot loaders surely wouldn't use the new address either.

> > I think we should consequently make sure we never get a production
> > u-boot for Mirabox or OpenBlocks that changes the internal register
> > address and also allows ATAGS based booting.
> 
> It is impossible to prevent that. U-Boot always supports ATAGs booting
> for ARM, and optionally Device Tree based booting.
> 
> And once again, you believe we have an enormous control over what is
> happening at the bootloader level. So when you say "we should
> consequently make sure ..", this is simply not possible: we have no
> control over this.

The control you have over it is to ensure that it's impossible to
boot an upstream kernel using a broken boot loader. This seems to have
worked rather well to get all Kirkwood/Orion5x/mv78xx0/Dove boards
to use an address that is different from the power-on default.

If we see Mirabox or OpenBlocks machines (or any new ones) get out
in the wild with different addresses and no DT support, that would
be rather unfortunate, but it's not the end of the world, we could
call add a trivial .dts file that includes the original one and puts
in the correct address into the ranges property.

> > Since you explained that we want to move the internal registers to get
> > more available memory, I think what you also want to do is to
> > dynamically reassign the internal register mapping when the mbus
> > driver gets loaded, just like all the other mbus mappings.
> 
> Ideally, it should be like this. Unfortunately, moving the internal
> register mapping is really a painful operation: you have to know where
> is the address to change the internal register base address (which
> itself changes when the internal register base address changes), and all
> code interacting with peripherals before the change of the address must
> be synchronized so that after the switch it uses the new address.

But you are proposing a patch set that does exactly this, so it's
clearly possible, right?

> Switching the internal registers address at runtime after a lot of
> things have booted is *painful* and we really don't want to do that.
> Sebastian Hesselbarth, who has worked on the topic, will confirm this.

It can still be done really early, e.g. from map_io(), since that is
run at a time when we have access to the DT and are not using any
devices other than the debug_ll code that is already a hack.

> We've gone through great lengths to find a solution that does not
> touch *any* ARM generic code (even though it would have been much
> simpler!). At least one Marvell maintainer (Andrew Lunn) is fine with
> this temporary solution, and since the issue is very SoC-specific, and
> completely self-contained in the SoC-specific code, would it be
> possible to be pragmatic and let us merge such a workaround?

Maybe you can put the hack into a separate board file that gets used
only for the two boards that need it and is enabled optionally?
The part that is really worrying about your patch is that you essentially
make an undocumented extension to the boot protocol to silently ignore
the information from the official interface and use a hardcoded value
instead, for all boards in that platform.

If you make a hack there, at least make it blatently obvious that
something is wrong when you work around it:

* allow the hack only for known broken machines (e.g. OpenBlocks
  with the internal registers at 0xf1), on all other machines
  trust the DT information.

* When you detect a mismatch between DT and the hardware setup
  as passed by the boot loader, print a fat warning.

* Fix it up by modifying the DT in memory to match the actual
  hardware configuration, not by making the hardware match the DT.

* If you ever want to get rid of the hack, put a time-bomb it in
  right away, like "BUG_ON(LINUX_VERSION_CODE > KERNEL_VERSION(3,14,0))"

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Thu, May 23, 2013 at 02:23:01PM +0200, Thomas Petazzoni wrote:
> But that's *not* the case. Neither the old nor the new bootloaders are
> changing the Device Tree in any way that allows us to know whether
> registers are mapped at 0xd0 and 0xf1.

Who provides this DT blob?  Is it already provided with the board, or
is it something which is provided by others?

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Thursday 23 May 2013, Russell King - ARM Linux wrote:
> On Thu, May 23, 2013 at 02:23:01PM +0200, Thomas Petazzoni wrote:
> > But that's not the case. Neither the old nor the new bootloaders are
> > changing the Device Tree in any way that allows us to know whether
> > registers are mapped at 0xd0 and 0xf1.
> 
> Who provides this DT blob?  Is it already provided with the board, or
> is it something which is provided by others?

In theory, the board could provide the DT, but we would always be able
to provide an updated DT blob if necessary. In practice, the DT blob at
the moment is always shipped alongside the kernel, as some interfaces
(e.g. the mbus or pci in case of mvebu) are not final yet.

The boot loader has ways to modify that blob in memory, in order to
pass the information that traditionally is in the ATAGS and other things
like a board specific MAC address. It is straightforward for the bootloader
to update properties like the location of the internal registers, once
we have specified the binding for the mbus, but not until then.

For older boot loaders that only support ATAGS, the update of the DT
blob happens inside of the kernel in arch/arm/boot/compressed/atags_to_fdt.c
on the blob that is appended to the zImage.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Thu, May 23, 2013 at 10:40:03PM +0200, Arnd Bergmann wrote:
> On Thursday 23 May 2013, Russell King - ARM Linux wrote:
> > On Thu, May 23, 2013 at 02:23:01PM +0200, Thomas Petazzoni wrote:
> > > But that's not the case. Neither the old nor the new bootloaders are
> > > changing the Device Tree in any way that allows us to know whether
> > > registers are mapped at 0xd0 and 0xf1.
> > 
> > Who provides this DT blob?  Is it already provided with the board, or
> > is it something which is provided by others?
> 
> In theory, the board could provide the DT, but we would always be able
> to provide an updated DT blob if necessary. In practice, the DT blob at
> the moment is always shipped alongside the kernel, as some interfaces
> (e.g. the mbus or pci in case of mvebu) are not final yet.
> 
> The boot loader has ways to modify that blob in memory, in order to
> pass the information that traditionally is in the ATAGS and other things
> like a board specific MAC address. It is straightforward for the bootloader
> to update properties like the location of the internal registers, once
> we have specified the binding for the mbus, but not until then.
> 
> For older boot loaders that only support ATAGS, the update of the DT
> blob happens inside of the kernel in arch/arm/boot/compressed/atags_to_fdt.c
> on the blob that is appended to the zImage.

*Sigh*

No, I'm not asking about the principle.  I'm asking _in this particular
problem case_ who provides the DT blob?

So the person to answer that question is Thomas or someone who is using
the boards concerned.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Russell,

On Fri, 24 May 2013 00:09:11 +0100, Russell King - ARM Linux wrote:

> No, I'm not asking about the principle.  I'm asking _in this particular
> problem case_ who provides the DT blob?
> 
> So the person to answer that question is Thomas or someone who is using
> the boards concerned.

The DTB is built from arch/arm/boot/dts/, together with the kernel
image itself. As far as I'm aware of, there is for the moment no plan
to "burn" the DTB on the board and therefore make the Device Tree
really separated from the kernel.

Does that answer your question? If not, do not hesitate to ask for
further details.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Fri, May 24, 2013 at 01:17:24AM +0200, Thomas Petazzoni wrote:
> Russell,
> 
> On Fri, 24 May 2013 00:09:11 +0100, Russell King - ARM Linux wrote:
> 
> > No, I'm not asking about the principle.  I'm asking _in this particular
> > problem case_ who provides the DT blob?
> > 
> > So the person to answer that question is Thomas or someone who is using
> > the boards concerned.
> 
> The DTB is built from arch/arm/boot/dts/, together with the kernel
> image itself. As far as I'm aware of, there is for the moment no plan
> to "burn" the DTB on the board and therefore make the Device Tree
> really separated from the kernel.

Right, so we're still in the position where we aren't dealing with
vendor provided DT blobs... this also means of course that the DT blob
isn't going to get updated by the boot loader depending on where it
decides to set the registers.

I suspect as far as the boot loaders on these boards go, the DT blob is
"just another file" to be loaded into memory by the boot loader, and it
is completely untouched by the boot loader.

Unfortunately, it also means that it's _completely_ our problem to select
the appropriate DT file for the boot loader behaviour, _or_ just not
bother with the problematical devices in DT and stick to "the old way"
of doing this if we can detect where they are.

For the latter, it really is not worth the effort of stuffing the DT file
full of definitions which are either (a) possibly wrong on the hardware
and (b) aren't being used because we're having to override them in the
kernel.

Another solution to this is to document the problem on a website, and
provide two DT blobs, and tell people to try one, if that fails to boot,
try the other, and only if that also fails to ask for help.  Yea, I know,
people don't bother reading websites though, it's loads easier to send
an email to a mailing list or someone else. :P

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Friday 24 May 2013, Russell King - ARM Linux wrote:
> No, I'm not asking about the principle.  I'm asking _in this particular
> problem case_ who provides the DT blob?
> 
> So the person to answer that question is Thomas or someone who is using
> the boards concerned.

But there is no problem on those boards /yet/. So far, the two boards
always come with the known 0xd0 configuration that match the DTs
we have, independent of where they come from.

Thomas wants to be prepared for them to do something stupid, and he
anticipates that they are going to do it in a particular way, but
he says that he has no control over them, so it's all speculation
at this point, unless we do something stupid and change the dts
files in the kernel to no longer match the current hardware.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Fri, 24 May 2013 09:15:27 +0200, Arnd Bergmann wrote:

> But there is no problem on those boards /yet/. So far, the two boards
> always come with the known 0xd0 configuration that match the DTs
> we have, independent of where they come from.
> 
> Thomas wants to be prepared for them to do something stupid, and he
> anticipates that they are going to do it in a particular way, but
> he says that he has no control over them, so it's all speculation
> at this point,

Speculation? The new bootloaders that change the address to 0xf1000000
are already shipping on all new Marvell evaluation platforms, they are
the ones currently available to Marvell customers for their
developments, and I'm running such a new bootloader on an evaluation
board.

I wouldn't call speculation something that I have in front of my eyes
today. But maybe we don't have the same definition of speculation.

> unless we do something stupid and change the dts
> files in the kernel to no longer match the current hardware.

You are again making the same mistake again. You make the association
between a given hardware platform and the location of the internal
registers, but this is completely false. Current OpenBlocks are shipped
with a old bootloader, but new OpenBlocks will most likely be shipped
with a new bootloader, or existing users of OpenBlocks may upgrade
their bootloader. So you can't make the assumption that OpenBlocks ==
old register address.

Again, we do *NOT* want to make this address configurable. All boards
should use 0xf1000000, and the kernel should assume that this a fixed
location for the internal registers.

All what we're trying to achieve here is to provide some temporary
workaround to help people using old bootloaders move smoothly to newer
bootloaders version without having a completely silent kernel at boot
time in the mean time.

So, while we are seeking for a workaround for a mistake of the past,
you are trying to force us to generalize the mechanism of customizing
the internal register address, which we do not want to do.

If all you're willing to accept is a very complex generalization of the
internal register base address handling, then I believe what we're
going to do is just move all Device Tree sources to use 0xf1, and we'll
tell users to upgrade their bootloader.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Russell King - ARM Linux]

On Fri, May 24, 2013 at 09:15:27AM +0200, Arnd Bergmann wrote:
> On Friday 24 May 2013, Russell King - ARM Linux wrote:
> > No, I'm not asking about the principle.  I'm asking _in this particular
> > problem case_ who provides the DT blob?
> > 
> > So the person to answer that question is Thomas or someone who is using
> > the boards concerned.
> 
> But there is no problem on those boards /yet/.

Wrong, again.  Thomas has already said there are boot loaders shipping
on boards which have the registers programmed to be at 0xf1xxxxxx _and_
there's boards with boot loaders programmed for 0xd0xxxxxx.

The problem is here TODAY.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Fri, 24 May 2013 08:46:16 +0100, Russell King - ARM Linux wrote:

> Wrong, again.  Thomas has already said there are boot loaders shipping
> on boards which have the registers programmed to be at 0xf1xxxxxx _and_
> there's boards with boot loaders programmed for 0xd0xxxxxx.
> 
> The problem is here TODAY.

Right. And again, don't expect something like:

 * Board A = 0xd0xxxxxx
 * Board B = 0xf1xxxxxx

Depending on which bootloader, and which version of the bootloader you
run, on a given board, the kernel may be booted with 0xd0 or 0xf1.

And again, again. This 0xd0 was a mistake. We don't want to support
moving forward. The only thing we want to support is 0xf1, nothing else.

All the proposal is about a temporary workaround, just to make the
transition smoother.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Friday 24 May 2013, Thomas Petazzoni wrote:
> On Fri, 24 May 2013 09:15:27 +0200, Arnd Bergmann wrote:
> 
> > But there is no problem on those boards /yet/. So far, the two boards
> > always come with the known 0xd0 configuration that match the DTs
> > we have, independent of where they come from.
> > 
> > Thomas wants to be prepared for them to do something stupid, and he
> > anticipates that they are going to do it in a particular way, but
> > he says that he has no control over them, so it's all speculation
> > at this point,
> 
> Speculation? The new bootloaders that change the address to 0xf1000000
> are already shipping on all new Marvell evaluation platforms, they are
> the ones currently available to Marvell customers for their
> developments, and I'm running such a new bootloader on an evaluation
> board.
> 
> I wouldn't call speculation something that I have in front of my eyes
> today. But maybe we don't have the same definition of speculation.

You said earlier yourself that you think the evaluation platforms are
under control because there are only small quantities of them and they
will eventually all move to the new address. I understand that Marvell
screwed up here (actually repeatedly it seems)

> > unless we do something stupid and change the dts
> > files in the kernel to no longer match the current hardware.
> 
> You are again making the same mistake again. You make the association
> between a given hardware platform and the location of the internal
> registers, but this is completely false. Current OpenBlocks are shipped
> with a old bootloader, but new OpenBlocks will most likely be shipped
> with a new bootloader, or existing users of OpenBlocks may upgrade
> their bootloader. So you can't make the assumption that OpenBlocks ==
> old register address.

That is what I meant with speculation: You don't actually know if
OpenBlocks are going to change their boot loader incompatibly, or
in case they do, whether they do it in the same way that Marvell
did or in a different way.

I think it's rather unlikely that a company that has shipped a ton
of machines would force an update on their users that breaks every
single kernel they have running today.

> Again, we do *NOT* want to make this address configurable. All boards
> should use 0xf1000000, and the kernel should assume that this a fixed
> location for the internal registers.

You also said that it didn't matter what you *want* because you have
no control over the boot loaders.

> All what we're trying to achieve here is to provide some temporary
> workaround to help people using old bootloaders move smoothly to newer
> bootloaders version without having a completely silent kernel at boot
> time in the mean time.

I understand that you are trying to do what's best here, but I fear
your workaround is going to make it much worse than it already is.

> So, while we are seeking for a workaround for a mistake of the past,
> you are trying to force us to generalize the mechanism of customizing
> the internal register address, which we do not want to do.

But your approach forces OpenBlocks and others to make the same mistake
that Marvell did by changing their boot loaders in a way that is not
backwards compatible and not detectable in a reliable way (see below).

> If all you're willing to accept is a very complex generalization of the
> internal register base address handling, then I believe what we're
> going to do is just move all Device Tree sources to use 0xf1, and we'll
> tell users to upgrade their bootloader.

Those are two options that would both be better than your current proposal,
but you can probably come up with others. The one thing you must not do
is override the address passed in the boot protocol for all boards based
on an arbitrary non-architecture register setting.

We know that boot loaders are broken all the time. Being able to describe
the breakage correctly in the device tree so the kernel can work around
it is our last line of defence, we have to trust that data.

In your patch, you are effectively encoding the policy that whatever we
find as the internal register address in the DT is meaningless and that
we instead use one of two hardcoded values depending on a bit that
Marvell (who are the ones that screwed this up before) tells you to use
as the interface. You also assume that by some magic you can introduce
the interface now and get everybody to do what you think they should in
the future (i.e. set the internal register to 0xf1 and enable that bit)
so you can remove that hack. I would call that rather optimistic.

What I would expect to see instead is:

* As the interface is now there, some board developers randomly mix
  boot loaders with 0xd0 and 0xf1 and rely on the kernel to use that
  bit for all eternity.

* some other board developers will get the updated u-boot from Marvell
  that sets the bit, but they revert the address back to 0xd0 because
  they want to keep running their old in-house kernels.

* some (slightly crazy) board developers will use their own boot loader
  with the 0xf1 address since Marvell is telling them to use that as
  the new default, but will not set the bit because their in-house kernel
  does not require it.

* at least one (rather crazy) board developers decides that neither 0xd0
  nor 0xf1 is the best base address for their uses and pick something
  completely different. They put the address into their DT following
  the binding and expect the kernel to handle that.

If any of the above happen, your hack will fail and you will have to
build additional hacks on top, instead of getting rid of it.

	Arnd

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Greg]

Le 24/05/2013 01:40, Russell King - ARM Linux a ?crit :
> On Fri, May 24, 2013 at 01:17:24AM +0200, Thomas Petazzoni wrote:
>> Russell,
>>
>> On Fri, 24 May 2013 00:09:11 +0100, Russell King - ARM Linux wrote:
>>
>>> No, I'm not asking about the principle.  I'm asking _in this particular
>>> problem case_ who provides the DT blob?
>>>
>>> So the person to answer that question is Thomas or someone who is using
>>> the boards concerned.
>> The DTB is built from arch/arm/boot/dts/, together with the kernel
>> image itself. As far as I'm aware of, there is for the moment no plan
>> to "burn" the DTB on the board and therefore make the Device Tree
>> really separated from the kernel.
> Right, so we're still in the position where we aren't dealing with
> vendor provided DT blobs... this also means of course that the DT blob
> isn't going to get updated by the boot loader depending on where it
> decides to set the registers.
>
> I suspect as far as the boot loaders on these boards go, the DT blob is
> "just another file" to be loaded into memory by the boot loader, and it
> is completely untouched by the boot loader.
>
Russell,

this is a little more complicated, the bootloader does touch the DT 
blob, for example it sets the MAC addresses of the network interfaces, I 
believe it also sets UART0 parameters. It does this silently if it 
founds entries it knows about in the DT blob.

I guess the good idea would have been to ask Marvell to also update an 
MBUS entry in the DT blob instead of doing the CP15 trick. This would 
not have solved the earlyprintk problem however.
I also guess this is not feasible anymore since it would imply to have 3 
different bootloader possibilities and 2 already is a pain to deal with.

Does this raise any new idea to someone ?

Cheers,

PS: I also got Marvell's confirmation, there is no way to avoid CPU hang 
when accessing an unmapped memory address, this triggers an external 
abort exception which is not recoverable in kernel mode.

[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Arnd Bergmann]

On Friday 24 May 2013, Thomas Petazzoni wrote:
> On Fri, 24 May 2013 08:46:16 +0100, Russell King - ARM Linux wrote:
> 
> > Wrong, again.  Thomas has already said there are boot loaders shipping
> > on boards which have the registers programmed to be at 0xf1xxxxxx and
> > there's boards with boot loaders programmed for 0xd0xxxxxx.
> > 
> > The problem is here TODAY.

The problem so far only concerns the development boards if I understood
Thomas correctly, not the Mirabox/OpenBlocks systems that this thread was
about, if you read up how we got here.
 
> Right. And again, don't expect something like:
> 
>  * Board A = 0xd0xxxxxx
>  * Board B = 0xf1xxxxxx
> 
> Depending on which bootloader, and which version of the bootloader you
> run, on a given board, the kernel may be booted with 0xd0 or 0xf1.

This is fine, the only thing we have to ensure is that the device tree
correctly describes what the boot loader has set up:

* For development boards and any future products you said we can ignore
  the old boot loaders and happily put the new address in the device
  tree. This seems reasonable.
* For Mirabox/OpenBlocks, it's too late to change it now, as you say,
  so they should stay at 0xd0.
* If someone wants to update their OpenBlocks to a new boot loader that
  uses the 0xf1 address, that's also fine, as long as they ensure they
  have a matching device tree. This is possible because there are no
  boot loaders for those devices yet using the new address. If the
  device tree gets loaded by u-boot, it can update the address in both
  DT and in hardware. If it uses ATAGS, it has to assume it is booting
  an older kernel so it cannot reprogram the hardware.
* If they screw up and change the address without updating the DT in
  memory, the only reasonable option left is to have a different DT
  blob for the new u-boot version. Hopefully we can avoid that case.

> And again, again. This 0xd0 was a mistake. We don't want to support
> moving forward. The only thing we want to support is 0xf1, nothing else.
>
> All the proposal is about a temporary workaround, just to make the
> transition smoother.

The 0xd0 wasn't the only mistake, it was just one in a series of
mistakes, each trying to fix the previous one but making things
worse in the process. Changing the boot loader from one address to
another is a much more serious mistake and we should not encourage
that from spreading to other boards.

	Arnd