Re: [PATCH v2] powerpc: mpc: Put U-Boot version string at correct place by linker script

["Pali Rohár" <pali@kernel.org>]

On Tuesday 24 August 2021 07:28:07 Christophe Leroy wrote:
> Le 08/08/2021 à 13:20, Pali Rohár a écrit :
> > It is unknown why version string is placed at specific position on these
> > powerpc mpc platforms. But there is no need to overload version_string
> > symbol. Just use common definition of version_string and modify linker
> > script to put it at "correct place".
> 
> What's the benefit of this change ?

Removal of the _weak_ property of version_string symbol to cleanup
U-Boot code, which allows U-Boot build system on second build run to
recompile only files which were really changed.

> I see more insertions than deletions, it
> doesn't remove any redundancy either it seems. And it provides a strange
> look to u-boot.lds

It is only to ensure that version_string is defined only at one place in
U-Boot sources. And for this kind of things it is required to use linker
hacks.

> I think we should probably investigate the reason for the version at that
> specific place if other architectures don't do the same.

I agree. This is really the only platform which has version_string at
specific place.

> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > ---
> > Changes in v2:
> > * Put explicit ".section" keyword before declaring ".text_pre" section as
> >    some gcc versions cannot recognize specifying custom section without it.
> > * Tested compilation for:
> >    $ make CROSS_COMPILE=powerpc-linux-gnu- MCR3000_defconfig u-boot.bin
> >    and checked that u-boot.bin header is same with and without this patch
> > ---
> >   arch/powerpc/cpu/mpc83xx/start.S         | 10 +++-------
> >   arch/powerpc/cpu/mpc83xx/u-boot.lds      |  3 +++
> >   arch/powerpc/cpu/mpc85xx/start.S         | 10 ++++------
> >   arch/powerpc/cpu/mpc85xx/u-boot-nand.lds |  4 ++++
> >   arch/powerpc/cpu/mpc85xx/u-boot-spl.lds  |  4 ++++
> >   arch/powerpc/cpu/mpc85xx/u-boot.lds      |  4 ++++
> >   arch/powerpc/cpu/mpc8xx/start.S          |  9 ++++-----
> >   board/cssi/MCR3000/u-boot.lds            |  2 ++
> >   8 files changed, 28 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/powerpc/cpu/mpc8xx/start.S b/arch/powerpc/cpu/mpc8xx/start.S
> > index ed735cdee005..09628a0d60f0 100644
> > --- a/arch/powerpc/cpu/mpc8xx/start.S
> > +++ b/arch/powerpc/cpu/mpc8xx/start.S
> > @@ -23,7 +23,6 @@
> >   #include <asm-offsets.h>
> >   #include <config.h>
> >   #include <mpc8xx.h>
> > -#include <version.h>
> >   #include <ppc_asm.tmpl>
> >   #include <ppc_defs.h>
> > @@ -60,12 +59,12 @@
> >    * r3 - 1st arg to board_init(): IMMP pointer
> >    * r4 - 2nd arg to board_init(): boot flag
> >    */
> > -	.text
> > +	.section .text_pre
> >   	.long	0x27051956		/* U-Boot Magic Number			*/
> 
> Instead of creating a new artificial section, maybe you can add the magic
> number in front of the version string directly in u-boot.lds ?

Well, this patch series is about version_string. I do not want to touch
other parts, like magic numbers in this patch series as it is not
related to version string in build system.

> > -	.globl	version_string
> > -version_string:
> > -	.ascii U_BOOT_VERSION_STRING, "\0"
> > +/* U-Boot version string is filled at this place by linker script */
> > +
> > +	.text
> >   	. = EXC_OFF_SYS_RESET
> >   	.globl	_start
> >   _start:
> > diff --git a/board/cssi/MCR3000/u-boot.lds b/board/cssi/MCR3000/u-boot.lds
> > index 70aef3241c8e..9b2ead29b4a5 100644
> > --- a/board/cssi/MCR3000/u-boot.lds
> > +++ b/board/cssi/MCR3000/u-boot.lds
> > @@ -16,6 +16,8 @@ SECTIONS
> >   	. = + SIZEOF_HEADERS;
> >   	.text          :
> >   	{
> > +		arch/powerpc/cpu/mpc8xx/start.o	(.text_pre)
> > +		*(.text_version_string)
> >   		arch/powerpc/cpu/mpc8xx/start.o	(.text)
> >   		arch/powerpc/cpu/mpc8xx/traps.o	(.text*)
> >   		arch/powerpc/lib/built-in.o		(.text*)
> >