[PATCH v2 0/3] common/board_f: Make reserve_mmu generic

[PATCH v2 0/3] common/board_f: Make reserve_mmu generic

[Ovidiu Panait]

Changes in v2:
- split the original patch in 3 parts
- add function comments
- drop #ifdefs around asm/cache.h includes
- drop reserve_mmu function and leave only arch_reserve_mmu

For the following suggestion:
"Can you please try to use if() instead of #if as much as possible?"

I tried replacing in reserve_mmu
#if !(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
with
if (!(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))) {
...
}
but it caused compile issues on different arm platforms such as:
...
arch/arm/lib/cache.c: In function 'arm_reserve_mmu':
arch/arm/lib/cache.c:134:11: error: 'volatile struct arch_global_data' has no member named 'tlb_size'
   gd->arch.tlb_size = PGTABLE_SIZE;
           ^
arch/arm/lib/cache.c:135:28: error: 'volatile struct arch_global_data' has no member named 'tlb_size'
   gd->relocaddr -= gd->arch.tlb_size;
                            ^
arch/arm/lib/cache.c:140:11: error: 'volatile struct arch_global_data' has no member named 'tlb_addr'
   gd->arch.tlb_addr = gd->relocaddr;
In file included from include/common.h:34:0,
                 from arch/arm/lib/cache.c:9:
arch/arm/lib/cache.c:141:52: error: 'volatile struct arch_global_data' has no member named 'tlb_addr'
...

Ovidiu Panait (3):
  common/board_f: Move arm-specific reserve_mmu to arch/arm/lib/cache.c
  arm: asm/cache.c: Introduce arm_reserve_mmu
  common/board_f: Make reserve_mmu generic

 arch/arm/include/asm/cache.h | 11 +++++++++++
 arch/arm/lib/cache.c         | 33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-versal/cpu.c   |  3 ++-
 arch/arm/mach-zynqmp/cpu.c   |  3 ++-
 common/board_f.c             | 29 ++---------------------------
 include/init.h               | 13 ++++++++++++-
 6 files changed, 62 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] common/board_f: Move arm-specific reserve_mmu to arch/arm/lib/cache.c

[Ovidiu Panait]

Move the ARM-specific reserve_mmu definition from common/board_f.c
to arch/arm/lib/cache.c.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---
 arch/arm/lib/cache.c | 28 ++++++++++++++++++++++++++++
 common/board_f.c     | 28 ----------------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 007d4ebc49..b8e1e340a1 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -10,6 +10,8 @@
 #include <cpu_func.h>
 #include <malloc.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /*
  * Flush range from all levels of d-cache/unified-cache.
  * Affects the range [start, start + size - 1].
@@ -118,3 +120,29 @@ void invalidate_l2_cache(void)
 	isb();
 }
 #endif
+
+__weak int reserve_mmu(void)
+{
+#if !(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
+	/* reserve TLB table */
+	gd->arch.tlb_size = PGTABLE_SIZE;
+	gd->relocaddr -= gd->arch.tlb_size;
+
+	/* round down to next 64 kB limit */
+	gd->relocaddr &= ~(0x10000 - 1);
+
+	gd->arch.tlb_addr = gd->relocaddr;
+	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
+	      gd->arch.tlb_addr + gd->arch.tlb_size);
+
+#ifdef CONFIG_SYS_MEM_RESERVE_SECURE
+	/*
+	 * Record allocated tlb_addr in case gd->tlb_addr to be overwritten
+	 * with location within secure ram.
+	 */
+	gd->arch.tlb_allocated = gd->arch.tlb_addr;
+#endif
+#endif
+
+	return 0;
+}
diff --git a/common/board_f.c b/common/board_f.c
index 82a164752a..a88bd64630 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -385,34 +385,6 @@ static int reserve_round_4k(void)
 	return 0;
 }
 
-#ifdef CONFIG_ARM
-__weak int reserve_mmu(void)
-{
-#if !(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
-	/* reserve TLB table */
-	gd->arch.tlb_size = PGTABLE_SIZE;
-	gd->relocaddr -= gd->arch.tlb_size;
-
-	/* round down to next 64 kB limit */
-	gd->relocaddr &= ~(0x10000 - 1);
-
-	gd->arch.tlb_addr = gd->relocaddr;
-	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
-	      gd->arch.tlb_addr + gd->arch.tlb_size);
-
-#ifdef CONFIG_SYS_MEM_RESERVE_SECURE
-	/*
-	 * Record allocated tlb_addr in case gd->tlb_addr to be overwritten
-	 * with location within secure ram.
-	 */
-	gd->arch.tlb_allocated = gd->arch.tlb_addr;
-#endif
-#endif
-
-	return 0;
-}
-#endif
-
 static int reserve_video(void)
 {
 #ifdef CONFIG_DM_VIDEO
-- 
2.17.1

[PATCH v2 2/3] arm: asm/cache.c: Introduce arm_reserve_mmu

[Ovidiu Panait]

As a preparation for turning reserve_mmu into an arch-specific variant,
introduce arm_reserve_mmu on ARM. It implements the default routine for
reserving memory for MMU TLB and needs to be weakly defined in order to allow
for machines to override it.

Without this decoupling, after introducing arch_reserve_mmu, there would be two
weak definitions for it, one in common/board_f.c and one in
arch/arm/lib/cache.c.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---
 arch/arm/include/asm/cache.h | 11 +++++++++++
 arch/arm/lib/cache.c         |  5 +++++
 arch/arm/mach-versal/cpu.c   |  3 ++-
 arch/arm/mach-zynqmp/cpu.c   |  3 ++-
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/cache.h b/arch/arm/include/asm/cache.h
index 950ec1e793..c20e05ec7f 100644
--- a/arch/arm/include/asm/cache.h
+++ b/arch/arm/include/asm/cache.h
@@ -49,4 +49,15 @@ void dram_bank_mmu_setup(int bank);
  */
 #define ARCH_DMA_MINALIGN	CONFIG_SYS_CACHELINE_SIZE
 
+/*
+ * arm_reserve_mmu() - Reserve memory for MMU TLB table
+ *
+ * Default implementation for reserving memory for MMU TLB table. It is used
+ * during generic board init sequence in common/board_f.c. Weakly defined, so
+ * that machines can override it if needed.
+ *
+ * Return: 0 if OK
+ */
+int arm_reserve_mmu(void);
+
 #endif /* _ASM_CACHE_H */
diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index b8e1e340a1..3cbed602eb 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -122,6 +122,11 @@ void invalidate_l2_cache(void)
 #endif
 
 __weak int reserve_mmu(void)
+{
+	return arm_reserve_mmu();
+}
+
+__weak int arm_reserve_mmu(void)
 {
 #if !(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
 	/* reserve TLB table */
diff --git a/arch/arm/mach-versal/cpu.c b/arch/arm/mach-versal/cpu.c
index 6ee6cd43ec..c14c5bb39c 100644
--- a/arch/arm/mach-versal/cpu.c
+++ b/arch/arm/mach-versal/cpu.c
@@ -9,6 +9,7 @@
 #include <asm/io.h>
 #include <asm/arch/hardware.h>
 #include <asm/arch/sys_proto.h>
+#include <asm/cache.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -98,7 +99,7 @@ u64 get_page_table_size(void)
 }
 
 #if defined(CONFIG_SYS_MEM_RSVD_FOR_MMU)
-int reserve_mmu(void)
+int arm_reserve_mmu(void)
 {
 	tcm_init(TCM_LOCK);
 	gd->arch.tlb_size = PGTABLE_SIZE;
diff --git a/arch/arm/mach-zynqmp/cpu.c b/arch/arm/mach-zynqmp/cpu.c
index 442427bc11..811684a9f8 100644
--- a/arch/arm/mach-zynqmp/cpu.c
+++ b/arch/arm/mach-zynqmp/cpu.c
@@ -11,6 +11,7 @@
 #include <asm/armv8/mmu.h>
 #include <asm/io.h>
 #include <zynqmp_firmware.h>
+#include <asm/cache.h>
 
 #define ZYNQ_SILICON_VER_MASK	0xF000
 #define ZYNQ_SILICON_VER_SHIFT	12
@@ -116,7 +117,7 @@ void tcm_init(u8 mode)
 #endif
 
 #ifdef CONFIG_SYS_MEM_RSVD_FOR_MMU
-int reserve_mmu(void)
+int arm_reserve_mmu(void)
 {
 	tcm_init(TCM_LOCK);
 	gd->arch.tlb_size = PGTABLE_SIZE;
-- 
2.17.1

[PATCH v2 3/3] common/board_f: Make reserve_mmu generic

[Ovidiu Panait]

Introduce arch_reserve_mmu to allow for architecture-specific reserve_mmu
routines. Also, define a weak nop stub for it.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---
 arch/arm/lib/cache.c |  2 +-
 common/board_f.c     |  9 ++++++---
 include/init.h       | 13 ++++++++++++-
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 3cbed602eb..44dde26065 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -121,7 +121,7 @@ void invalidate_l2_cache(void)
 }
 #endif
 
-__weak int reserve_mmu(void)
+int arch_reserve_mmu(void)
 {
 	return arm_reserve_mmu();
 }
diff --git a/common/board_f.c b/common/board_f.c
index a88bd64630..52750dd307 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -385,6 +385,11 @@ static int reserve_round_4k(void)
 	return 0;
 }
 
+__weak int arch_reserve_mmu(void)
+{
+	return 0;
+}
+
 static int reserve_video(void)
 {
 #ifdef CONFIG_DM_VIDEO
@@ -942,9 +947,7 @@ static const init_fnc_t init_sequence_f[] = {
 	reserve_pram,
 #endif
 	reserve_round_4k,
-#ifdef CONFIG_ARM
-	reserve_mmu,
-#endif
+	arch_reserve_mmu,
 	reserve_video,
 	reserve_trace,
 	reserve_uboot,
diff --git a/include/init.h b/include/init.h
index 2a33a3fd1e..9ef88c966b 100644
--- a/include/init.h
+++ b/include/init.h
@@ -129,6 +129,18 @@ int testdram(void);
  */
 int arch_reserve_stacks(void);
 
+/**
+ * arch_reserve_mmu() - Reserve memory for MMU TLB table
+ *
+ * Architecture-specific routine for reserving memory for the MMU TLB table.
+ * This is used in generic board init sequence in common/board_f.c.
+ *
+ * If an implementation is not provided, it will just be a nop stub.
+ *
+ * Return: 0 if OK
+ */
+int arch_reserve_mmu(void);
+
 /**
  * init_cache_f_r() - Turn on the cache in preparation for relocation
  *
@@ -145,7 +157,6 @@ int init_cache_f_r(void);
 int print_cpuinfo(void);
 #endif
 int timer_init(void);
-int reserve_mmu(void);
 int misc_init_f(void);
 
 #if defined(CONFIG_DTB_RESELECT)
-- 
2.17.1

[PATCH v2 1/3] common/board_f: Move arm-specific reserve_mmu to arch/arm/lib/cache.c

[Simon Glass]

On Sun, 29 Mar 2020 at 11:59, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> Move the ARM-specific reserve_mmu definition from common/board_f.c
> to arch/arm/lib/cache.c.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>  arch/arm/lib/cache.c | 28 ++++++++++++++++++++++++++++
>  common/board_f.c     | 28 ----------------------------
>  2 files changed, 28 insertions(+), 28 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 2/3] arm: asm/cache.c: Introduce arm_reserve_mmu

[Simon Glass]

Hi Ovidiu,

On Sun, 29 Mar 2020 at 11:59, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> As a preparation for turning reserve_mmu into an arch-specific variant,
> introduce arm_reserve_mmu on ARM. It implements the default routine for
> reserving memory for MMU TLB and needs to be weakly defined in order to allow
> for machines to override it.
>
> Without this decoupling, after introducing arch_reserve_mmu, there would be two
> weak definitions for it, one in common/board_f.c and one in
> arch/arm/lib/cache.c.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>  arch/arm/include/asm/cache.h | 11 +++++++++++
>  arch/arm/lib/cache.c         |  5 +++++
>  arch/arm/mach-versal/cpu.c   |  3 ++-
>  arch/arm/mach-zynqmp/cpu.c   |  3 ++-
>  4 files changed, 20 insertions(+), 2 deletions(-)
>

I'm not a fan of weak functions and here we have two layers of them.
But I'm not sure what else to suggest, other than a Kconfig to
enable/disable the generic arm_reserve_mmu().

Reviewed-by: Simon Glass <sjg@chromium.org>


> diff --git a/arch/arm/include/asm/cache.h b/arch/arm/include/asm/cache.h
> index 950ec1e793..c20e05ec7f 100644
> --- a/arch/arm/include/asm/cache.h
> +++ b/arch/arm/include/asm/cache.h
> @@ -49,4 +49,15 @@ void dram_bank_mmu_setup(int bank);
>   */
>  #define ARCH_DMA_MINALIGN      CONFIG_SYS_CACHELINE_SIZE
>
> +/*
> + * arm_reserve_mmu() - Reserve memory for MMU TLB table
> + *
> + * Default implementation for reserving memory for MMU TLB table. It is used
> + * during generic board init sequence in common/board_f.c. Weakly defined, so
> + * that machines can override it if needed.
> + *
> + * Return: 0 if OK
> + */
> +int arm_reserve_mmu(void);
> +
>  #endif /* _ASM_CACHE_H */
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> index b8e1e340a1..3cbed602eb 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -122,6 +122,11 @@ void invalidate_l2_cache(void)
>  #endif
>
>  __weak int reserve_mmu(void)
> +{
> +       return arm_reserve_mmu();
> +}
> +
> +__weak int arm_reserve_mmu(void)
>  {
>  #if !(CONFIG_IS_ENABLED(SYS_ICACHE_OFF) && CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
>         /* reserve TLB table */
> diff --git a/arch/arm/mach-versal/cpu.c b/arch/arm/mach-versal/cpu.c
> index 6ee6cd43ec..c14c5bb39c 100644
> --- a/arch/arm/mach-versal/cpu.c
> +++ b/arch/arm/mach-versal/cpu.c
> @@ -9,6 +9,7 @@
>  #include <asm/io.h>
>  #include <asm/arch/hardware.h>
>  #include <asm/arch/sys_proto.h>
> +#include <asm/cache.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -98,7 +99,7 @@ u64 get_page_table_size(void)
>  }
>
>  #if defined(CONFIG_SYS_MEM_RSVD_FOR_MMU)
> -int reserve_mmu(void)
> +int arm_reserve_mmu(void)
>  {
>         tcm_init(TCM_LOCK);
>         gd->arch.tlb_size = PGTABLE_SIZE;
> diff --git a/arch/arm/mach-zynqmp/cpu.c b/arch/arm/mach-zynqmp/cpu.c
> index 442427bc11..811684a9f8 100644
> --- a/arch/arm/mach-zynqmp/cpu.c
> +++ b/arch/arm/mach-zynqmp/cpu.c
> @@ -11,6 +11,7 @@
>  #include <asm/armv8/mmu.h>
>  #include <asm/io.h>
>  #include <zynqmp_firmware.h>
> +#include <asm/cache.h>
>
>  #define ZYNQ_SILICON_VER_MASK  0xF000
>  #define ZYNQ_SILICON_VER_SHIFT 12
> @@ -116,7 +117,7 @@ void tcm_init(u8 mode)
>  #endif
>
>  #ifdef CONFIG_SYS_MEM_RSVD_FOR_MMU
> -int reserve_mmu(void)
> +int arm_reserve_mmu(void)
>  {
>         tcm_init(TCM_LOCK);
>         gd->arch.tlb_size = PGTABLE_SIZE;
> --
> 2.17.1
>

[PATCH v2 3/3] common/board_f: Make reserve_mmu generic

[Simon Glass]

On Sun, 29 Mar 2020 at 11:59, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> Introduce arch_reserve_mmu to allow for architecture-specific reserve_mmu
> routines. Also, define a weak nop stub for it.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>  arch/arm/lib/cache.c |  2 +-
>  common/board_f.c     |  9 ++++++---
>  include/init.h       | 13 ++++++++++++-
>  3 files changed, 19 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

This series is very well put together, thank you.

[PATCH v2 1/3] common/board_f: Move arm-specific reserve_mmu to arch/arm/lib/cache.c

[Tom Rini]

On Sun, Mar 29, 2020 at 08:57:39PM +0300, Ovidiu Panait wrote:

> Move the ARM-specific reserve_mmu definition from common/board_f.c
> to arch/arm/lib/cache.c.
> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200426/69ddbb71/attachment.sig>

[PATCH v2 2/3] arm: asm/cache.c: Introduce arm_reserve_mmu

[Tom Rini]

On Sun, Mar 29, 2020 at 08:57:40PM +0300, Ovidiu Panait wrote:

> As a preparation for turning reserve_mmu into an arch-specific variant,
> introduce arm_reserve_mmu on ARM. It implements the default routine for
> reserving memory for MMU TLB and needs to be weakly defined in order to allow
> for machines to override it.
> 
> Without this decoupling, after introducing arch_reserve_mmu, there would be two
> weak definitions for it, one in common/board_f.c and one in
> arch/arm/lib/cache.c.
> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200426/b919f7c4/attachment.sig>

[PATCH v2 3/3] common/board_f: Make reserve_mmu generic

[Tom Rini]

On Sun, Mar 29, 2020 at 08:57:41PM +0300, Ovidiu Panait wrote:

> Introduce arch_reserve_mmu to allow for architecture-specific reserve_mmu
> routines. Also, define a weak nop stub for it.
> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200426/39a96825/attachment.sig>