[PATCH 1/4] firmware: Add relocatable FW_JUMP_ADDR and FW_JUMP_FDT_ADDR

[Inochi Amaoto]

On Fri, Feb 23, 2024 at 12:59:30PM +0530, Anup Patel wrote:
> On Fri, Feb 23, 2024 at 12:51 PM Inochi Amaoto <inochiama at outlook.com> wrote:
> >
> > On Fri, Feb 23, 2024 at 10:28:49AM +0530, Anup Patel wrote:
> > > On Fri, Feb 2, 2024 at 9:35 AM Inochi Amaoto <inochiama at outlook.com> wrote:
> > > >
> > > > If FW_PIC=y is defined, the fw_jump.bin will be broken if
> > > > FW_TEXT_START is wrong. This is not the desired behavior.
> > >
> > > We should support FW_JUMP_OFFSET, irrespective of whether
> > > FW_PIC is defined or not.
> > >
> >
> > In fact, this patch allows to use FW_JUMP_OFFSET when FW_PIC=n.
> > But It will expand to FW_JUMP_ADDR to use old path.
> > I think your meaning is to drop this expansion and using
> > FW_JUMP_OFFSET directly, right?
> 
> Yes, FW_JUMP_ADDR should not be derived from FW_JUMP_OFFSET
> so that platforms can specify any one.
> 

Thanks. I will support this.

> > I use this code path because of the binary size, but I am not
> > sure which way is better. Is using OFFSET code path better?
> >
> > > >
> > > > Add two new variables to identify relocatable jump address:
> > > > FW_JUMP_OFFSET and FW_JUMP_FDT_ADDR. To keep the existing
> > > > ABI, FW_JUMP_ADDR and FW_JUMP_FDT_ADDR is prefered if they
> > > > are defined. And these two new variables will convert to
> > > > the old one if compiler does not support PIC.
> > > >
> > > > Signed-off-by: Inochi Amaoto <inochiama at outlook.com>
> > > > ---
> > > >  firmware/fw_jump.S  | 20 ++++++++++++++++----
> > > >  firmware/objects.mk | 15 +++++++++++++++
> > > >  2 files changed, 31 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/firmware/fw_jump.S b/firmware/fw_jump.S
> > > > index ac74dc6..0ad2398 100644
> > > > --- a/firmware/fw_jump.S
> > > > +++ b/firmware/fw_jump.S
> > > > @@ -46,6 +46,10 @@ fw_save_info:
> > > >  fw_next_arg1:
> > > >  #ifdef FW_JUMP_FDT_ADDR
> > > >         li      a0, FW_JUMP_FDT_ADDR
> > > > +#elif defined(FW_JUMP_FDT_OFFSET)
> > > > +       lla     a0, _fw_start
> > > > +       li      a1, FW_JUMP_FDT_OFFSET
> > > > +       add     a0, a0, a1
> > > >  #else
> > > >         add     a0, a1, zero
> > > >  #endif
> > > > @@ -59,7 +63,13 @@ fw_next_arg1:
> > > >          * The next address should be returned in 'a0'.
> > > >          */
> > > >  fw_next_addr:
> > > > +#ifdef FW_JUMP_ADDR
> > > >         lla     a0, _jump_addr
> > > > +#elif defined(FW_JUMP_OFFSET)
> > > > +       lla     a0, _fw_start
> > > > +       li      a1, FW_JUMP_OFFSET
> > > > +       add     a0, a0, a1
> > >
> > > What if both FW_JUMP_ADDR and FW_JUMP_OFFSET are not available ?
> > >
> >
> > The build will failed, see the change in the below.
> >
> > > > +#endif
> > > >         REG_L   a0, (a0)
> > > >         ret
> > > >
> > > > @@ -86,11 +96,13 @@ fw_options:
> > > >         add     a0, zero, zero
> > > >         ret
> > > >
> > > > -#ifndef FW_JUMP_ADDR
> > > > -#error "Must define FW_JUMP_ADDR"
> > > > -#endif
> > > > -
> > > > +#ifdef FW_JUMP_ADDR
> > > >         .section .rodata
> > > >         .align 3
> > > >  _jump_addr:
> > > >         RISCV_PTR FW_JUMP_ADDR
> > > > +#else
> > > > +#ifndef FW_JUMP_OFFSET
> > > > +#error "Must define at least FW_JUMP_ADDR or FW_JUMP_OFFSET"
> > > > +#endif
> > > > +#endif
> > > > diff --git a/firmware/objects.mk b/firmware/objects.mk
> > > > index a1704c4..248706d 100644
> > > > --- a/firmware/objects.mk
> > > > +++ b/firmware/objects.mk
> > > > @@ -35,12 +35,27 @@ firmware-genflags-y += -DFW_FDT_PADDING=$(FW_FDT_PADDING)
> > > >  endif
> > > >  endif
> > > >
> > > > +ifeq ($(FW_PIC),n)
> > >
> > > Let's not condition this upon FW_PIC.
> > >
> > > Instead this should be:
> > >
> > > ifdef FW_JUMP_OFFSET
> > >
> >
> > This is already included in the change below, If it supports
> > FW_PIC=n, this should be removed.
> 
> Lets just remove this ifeq block so that FW_JUMP_ADDR is
> totally independent of FW_JUMP_OFFSET.
> 
> >
> > > > +  ifndef FW_JUMP_ADDR
> > > > +    FW_JUMP_ADDR=$(shell printf "0x%X" $$(($(FW_TEXT_START) + $(FW_JUMP_OFFSET))))
> > > > +  endif
> > > > +  ifndef FW_JUMP_FDT_ADDR
> > > > +    FW_JUMP_FDT_ADDR=$(shell printf "0x%X" $$(($(FW_TEXT_START) + $(FW_JUMP_FDT_OFFSET))))
> > > > +  endif
> > > > +endif
> > > > +
> > > >  firmware-bins-$(FW_DYNAMIC) += fw_dynamic.bin
> > > >
> > > >  firmware-bins-$(FW_JUMP) += fw_jump.bin
> > > > +ifdef FW_JUMP_OFFSET
> > > > +firmware-genflags-$(FW_JUMP) += -DFW_JUMP_OFFSET=$(FW_JUMP_OFFSET)
> > > > +endif
> > > >  ifdef FW_JUMP_ADDR
> > > >  firmware-genflags-$(FW_JUMP) += -DFW_JUMP_ADDR=$(FW_JUMP_ADDR)
> > > >  endif
> > > > +ifdef FW_JUMP_FDT_OFFSET
> > > > +firmware-genflags-$(FW_JUMP) += -DFW_JUMP_FDT_OFFSET=$(FW_JUMP_FDT_OFFSET)
> > > > +endif
> > > >  ifdef FW_JUMP_FDT_ADDR
> > > >  firmware-genflags-$(FW_JUMP) += -DFW_JUMP_FDT_ADDR=$(FW_JUMP_FDT_ADDR)
> > > >  endif
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > Regards,
> > > Anup
> 
> Regards,
> Anup