[PATCH 1/4] common/board_f: remove XTRN_DECLARE_GLOBAL_DATA_PTR dead code

[PATCH 1/4] common/board_f: remove XTRN_DECLARE_GLOBAL_DATA_PTR dead code

[Ovidiu Panait]

The XTRN_DECLARE_GLOBAL_DATA_PTR declarations in ppc code are permanently
commented out, so there are no users for this macro:
  #if 1
    #define DECLARE_GLOBAL_DATA_PTR   register volatile gd_t *gd asm ("r2")
  #else
    #define XTRN_DECLARE_GLOBAL_DATA_PTR   extern
    #define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
                                        gd_t *gd
  #endif

Remove all references.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 arch/powerpc/include/asm/global_data.h |  6 ------
 common/board_f.c                       | 11 -----------
 2 files changed, 17 deletions(-)

diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h
index 6709e692e6..6ed21c781f 100644
--- a/arch/powerpc/include/asm/global_data.h
+++ b/arch/powerpc/include/asm/global_data.h
@@ -92,12 +92,6 @@ struct arch_global_data {
 
 #include <asm-generic/global_data.h>
 
-#if 1
 #define DECLARE_GLOBAL_DATA_PTR     register volatile gd_t *gd asm ("r2")
-#else /* We could use plain global data, but the resulting code is bigger */
-#define XTRN_DECLARE_GLOBAL_DATA_PTR	extern
-#define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
-				    gd_t *gd
-#endif
 
 #endif /* __ASM_GBL_DATA_H */
diff --git a/common/board_f.c b/common/board_f.c
index 18e2246733..f4238d4c90 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -57,18 +57,7 @@
 #include <dm/root.h>
 #include <linux/errno.h>
 
-/*
- * Pointer to initial global data area
- *
- * Here we initialize it if needed.
- */
-#ifdef XTRN_DECLARE_GLOBAL_DATA_PTR
-#undef	XTRN_DECLARE_GLOBAL_DATA_PTR
-#define XTRN_DECLARE_GLOBAL_DATA_PTR	/* empty = allocate here */
-DECLARE_GLOBAL_DATA_PTR = (gd_t *)(CONFIG_SYS_INIT_GD_ADDR);
-#else
 DECLARE_GLOBAL_DATA_PTR;
-#endif
 
 /*
  * TODO(sjg@chromium.org): IMO this code should be
-- 
2.25.1

[PATCH 2/4] common/board_f: move CONFIG_MACH_TYPE logic to arch/arm/lib/bdinfo.c

[Ovidiu Panait]

asm/mach_type.h header and CONFIG_MACH_TYPE macro are arm-specific, so move
related bdinfo logic to arch_setup_bdinfo() in arch/arm/lib/bdinfo.c.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 arch/arm/lib/bdinfo.c | 12 ++++++++++++
 common/board_f.c      |  7 -------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/arm/lib/bdinfo.c b/arch/arm/lib/bdinfo.c
index b22ee07b85..826e09e72c 100644
--- a/arch/arm/lib/bdinfo.c
+++ b/arch/arm/lib/bdinfo.c
@@ -9,9 +9,21 @@
 #include <common.h>
 #include <init.h>
 #include <asm/global_data.h>
+#include <asm/mach-types.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+int arch_setup_bdinfo(void)
+{
+#ifdef CONFIG_MACH_TYPE
+	struct bd_info *bd = gd->bd;
+
+	bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
+#endif
+
+	return 0;
+}
+
 void arch_print_bdinfo(void)
 {
 	struct bd_info *bd = gd->bd;
diff --git a/common/board_f.c b/common/board_f.c
index f4238d4c90..3789708a30 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -45,9 +45,6 @@
 #include <video.h>
 #include <watchdog.h>
 #include <asm/cache.h>
-#ifdef CONFIG_MACH_TYPE
-#include <asm/mach-types.h>
-#endif
 #if defined(CONFIG_MP) && defined(CONFIG_PPC)
 #include <asm/mp.h>
 #endif
@@ -588,10 +585,6 @@ int setup_bdinfo(void)
 		bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE;  /* size  of SRAM */
 	}
 
-#ifdef CONFIG_MACH_TYPE
-	bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
-#endif
-
 	return arch_setup_bdinfo();
 }
 
-- 
2.25.1

[PATCH 3/4] common/board_f: introduce arch_setup_dest_addr()

[Ovidiu Panait]

In order to move ppc-specific code out of setup_dest_addr(), provide an
arch-specific variant arch_setup_dest_addr(), that can be used by
architecture code to fix up the initial reloc address.

It is called at the end of setup_dest_addr() initcall and the default
implementation is a nop stub.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 arch/powerpc/lib/stack.c | 17 +++++++++++++++++
 common/board_f.c         | 21 +++++++--------------
 include/init.h           | 13 +++++++++++++
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/lib/stack.c b/arch/powerpc/lib/stack.c
index f2a4652e08..2e731aa870 100644
--- a/arch/powerpc/lib/stack.c
+++ b/arch/powerpc/lib/stack.c
@@ -13,6 +13,7 @@
 #include <common.h>
 #include <init.h>
 #include <asm/global_data.h>
+#include <asm/mp.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -30,3 +31,19 @@ int arch_reserve_stacks(void)
 
 	return 0;
 }
+
+int arch_setup_dest_addr(void)
+{
+#if defined(CONFIG_MP) && (defined(CONFIG_MPC86xx) || defined(CONFIG_E500))
+	/*
+	 * We need to make sure the location we intend to put secondary core
+	 * boot code is reserved and not used by any part of u-boot
+	 */
+	if (gd->relocaddr > determine_mp_bootpg(NULL)) {
+		gd->relocaddr = determine_mp_bootpg(NULL);
+		debug("Reserving MP boot page to %08lx\n", gd->relocaddr);
+	}
+#endif
+
+	return 0;
+}
diff --git a/common/board_f.c b/common/board_f.c
index 3789708a30..96458c5151 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -45,9 +45,6 @@
 #include <video.h>
 #include <watchdog.h>
 #include <asm/cache.h>
-#if defined(CONFIG_MP) && defined(CONFIG_PPC)
-#include <asm/mp.h>
-#endif
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <asm/sections.h>
@@ -307,6 +304,11 @@ __weak ulong board_get_usable_ram_top(ulong total_size)
 	return gd->ram_top;
 }
 
+__weak int arch_setup_dest_addr(void)
+{
+	return 0;
+}
+
 static int setup_dest_addr(void)
 {
 	debug("Monitor len: %08lX\n", gd->mon_len);
@@ -334,17 +336,8 @@ static int setup_dest_addr(void)
 	gd->ram_top = board_get_usable_ram_top(gd->mon_len);
 	gd->relocaddr = gd->ram_top;
 	debug("Ram top: %08lX\n", (ulong)gd->ram_top);
-#if defined(CONFIG_MP) && (defined(CONFIG_MPC86xx) || defined(CONFIG_E500))
-	/*
-	 * We need to make sure the location we intend to put secondary core
-	 * boot code is reserved and not used by any part of u-boot
-	 */
-	if (gd->relocaddr > determine_mp_bootpg(NULL)) {
-		gd->relocaddr = determine_mp_bootpg(NULL);
-		debug("Reserving MP boot page to %08lx\n", gd->relocaddr);
-	}
-#endif
-	return 0;
+
+	return arch_setup_dest_addr();
 }
 
 #ifdef CONFIG_PRAM
diff --git a/include/init.h b/include/init.h
index 7b8f62c121..448da34b46 100644
--- a/include/init.h
+++ b/include/init.h
@@ -103,6 +103,19 @@ phys_size_t get_effective_memsize(void);
 
 int testdram(void);
 
+/**
+ * arch_setup_dest_addr() - Fix up initial reloc address
+ *
+ * This is called in generic board init sequence in common/board_f.c at the end
+ * of the setup_dest_addr() initcall. Each architecture could provide this
+ * function to make adjustments to the initial reloc address.
+ *
+ * If an implementation is not provided, it will just be a nop stub.
+ *
+ * Return: 0 if OK
+ */
+int arch_setup_dest_addr(void);
+
 /**
  * arch_reserve_stacks() - Reserve all necessary stacks
  *
-- 
2.25.1

[PATCH 4/4] common/board_f: drop ifdefs around header includes

[Ovidiu Panait]

Drop the remaining ifdef around spl.h include.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 common/board_f.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 96458c5151..4db1626c29 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -35,9 +35,7 @@
 #include <post.h>
 #include <relocate.h>
 #include <serial.h>
-#ifdef CONFIG_SPL
 #include <spl.h>
-#endif
 #include <status_led.h>
 #include <sysreset.h>
 #include <timer.h>
-- 
2.25.1

Re: [PATCH 1/4] common/board_f: remove XTRN_DECLARE_GLOBAL_DATA_PTR dead code

[Wolfgang Denk]

Dear Ovidiu,

In message <20220911161052.2986264-1-ovpanait@gmail.com> you wrote:
> The XTRN_DECLARE_GLOBAL_DATA_PTR declarations in ppc code are permanently
> commented out, so there are no users for this macro:
>   #if 1
>     #define DECLARE_GLOBAL_DATA_PTR   register volatile gd_t *gd asm ("r2")
>   #else
>     #define XTRN_DECLARE_GLOBAL_DATA_PTR   extern
>     #define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
>                                         gd_t *gd
>   #endif
>
> Remove all references.

Actually the commented out code contained some information, and I
feel it would be a pity if that got lost:

> -#if 1
>  #define DECLARE_GLOBAL_DATA_PTR     register volatile gd_t *gd asm ("r2")
> -#else /* We could use plain global data, but the resulting code is bigger */
> -#define XTRN_DECLARE_GLOBAL_DATA_PTR	extern
> -#define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
> -				    gd_t *gd
> -#endif

Maybe we can keep the information that using global data for the GD
pointer would be possible too (and simpler, as it does not require
the reservation of a specific register for it), but that the
implementation uses a register nevertheless because this results in
smaller code?

Maybe add such a comment instead ?

Thanks!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich,  Office: Kirchenstr. 5, 82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
All repairs tend to destroy the structure, to  increase  the  entropy
and  disorder  of the system. Less and less effort is spent on fixing
original design flaws; more and more is spent on fixing flaws  intro-
duced by earlier fixes.       - Fred Brooks, "The Mythical Man Month"

Re: [PATCH 2/4] common/board_f: move CONFIG_MACH_TYPE logic to arch/arm/lib/bdinfo.c

[Simon Glass]

On Sun, 11 Sept 2022 at 10:11, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> asm/mach_type.h header and CONFIG_MACH_TYPE macro are arm-specific, so move
> related bdinfo logic to arch_setup_bdinfo() in arch/arm/lib/bdinfo.c.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>
>  arch/arm/lib/bdinfo.c | 12 ++++++++++++
>  common/board_f.c      |  7 -------
>  2 files changed, 12 insertions(+), 7 deletions(-)

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

Re: [PATCH 3/4] common/board_f: introduce arch_setup_dest_addr()

[Simon Glass]

On Sun, 11 Sept 2022 at 10:11, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> In order to move ppc-specific code out of setup_dest_addr(), provide an
> arch-specific variant arch_setup_dest_addr(), that can be used by
> architecture code to fix up the initial reloc address.
>
> It is called at the end of setup_dest_addr() initcall and the default
> implementation is a nop stub.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>
>  arch/powerpc/lib/stack.c | 17 +++++++++++++++++
>  common/board_f.c         | 21 +++++++--------------
>  include/init.h           | 13 +++++++++++++
>  3 files changed, 37 insertions(+), 14 deletions(-)

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

Re: [PATCH 4/4] common/board_f: drop ifdefs around header includes

[Simon Glass]

On Sun, 11 Sept 2022 at 10:11, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> Drop the remaining ifdef around spl.h include.
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>
>  common/board_f.c | 2 --
>  1 file changed, 2 deletions(-)

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

Re: [PATCH 1/4] common/board_f: remove XTRN_DECLARE_GLOBAL_DATA_PTR dead code

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]

On Mon, Sep 12, 2022 at 11:22:53AM +0200, Wolfgang Denk wrote:
> Dear Ovidiu,
> 
> In message <20220911161052.2986264-1-ovpanait@gmail.com> you wrote:
> > The XTRN_DECLARE_GLOBAL_DATA_PTR declarations in ppc code are permanently
> > commented out, so there are no users for this macro:
> >   #if 1
> >     #define DECLARE_GLOBAL_DATA_PTR   register volatile gd_t *gd asm ("r2")
> >   #else
> >     #define XTRN_DECLARE_GLOBAL_DATA_PTR   extern
> >     #define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
> >                                         gd_t *gd
> >   #endif
> >
> > Remove all references.
> 
> Actually the commented out code contained some information, and I
> feel it would be a pity if that got lost:
> 
> > -#if 1
> >  #define DECLARE_GLOBAL_DATA_PTR     register volatile gd_t *gd asm ("r2")
> > -#else /* We could use plain global data, but the resulting code is bigger */
> > -#define XTRN_DECLARE_GLOBAL_DATA_PTR	extern
> > -#define DECLARE_GLOBAL_DATA_PTR     XTRN_DECLARE_GLOBAL_DATA_PTR \
> > -				    gd_t *gd
> > -#endif
> 
> Maybe we can keep the information that using global data for the GD
> pointer would be possible too (and simpler, as it does not require
> the reservation of a specific register for it), but that the
> implementation uses a register nevertheless because this results in
> smaller code?
> 
> Maybe add such a comment instead ?
> 

This is slightly mentioned in doc/develop/global_data.rst currently, but
a follow-up to expand on this would be good.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]