Re: [PATCH] sandbox: Remove OF_HOSTFILE

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon, 

[...]
> > -INPUTS-$(CONFIG_OF_HOSTFILE) += u-boot.dtb
> > +INPUTS-$(CONFIG_SANDBOX) += u-boot.dtb
> >  ifneq ($(CONFIG_SPL_TARGET),)
> >  INPUTS-$(CONFIG_SPL) += $(CONFIG_SPL_TARGET:"%"=%)
> >  endif
> > @@ -1423,7 +1423,7 @@ u-boot-lzma.img: u-boot.bin.lzma FORCE
> >
> >  u-boot-dtb.img u-boot.img u-boot.kwb u-boot.pbl u-boot-ivt.img: \
> >                 $(if $(CONFIG_SPL_LOAD_FIT),u-boot-nodtb.bin \
> > -                       $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE)$(CONFIG_BINMAN_STANDALONE_FDT),dts/dt.dtb) \
> > +                       $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_SANDBOX)$(CONFIG_BINMAN_STANDALONE_FDT),dts/dt.dtb) \
> >                 ,$(UBOOT_BIN)) FORCE
> >         $(call if_changed,mkimage)
> >         $(BOARD_SIZE_CHECK)
> > @@ -1437,7 +1437,7 @@ MKIMAGEFLAGS_u-boot.itb += -B 0x8
> >
> >  ifdef U_BOOT_ITS
> >  u-boot.itb: u-boot-nodtb.bin \
> > -               $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),dts/dt.dtb) \
> > +               $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_SANDBOX),dts/dt.dtb) \
> >                 $(U_BOOT_ITS) FORCE
> >         $(call if_changed,mkfitimage)
> >         $(BOARD_SIZE_CHECK)
> > diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> > index 48636ab63919..bc67a5a5a10b 100644
> > --- a/arch/sandbox/cpu/cpu.c
> > +++ b/arch/sandbox/cpu/cpu.c
> > @@ -291,44 +291,47 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop)
> >  {
> >  }
> >
> > -int sandbox_read_fdt_from_file(void)
> > +void *board_fdt_blob_setup(void)
> 
> Can you instead keep the function and call it from your new
> board_fdt_blob_setup() function? That way the error path goes through
> one place and we don't need to add goto.

Not without changing board_fdt_blob_setup() itself.  The function returns a
ptr and not an int.  We had a different #ifdef up to now, so the existing
function was returning an int and was setting the gd->fdt_blob internally.
In any case the goto either remains of gets converted to 'return NULL'.

> 
> The other problem is that we need to know whether a DT is required
> (i.e. -d, -D or -T). So it must fail (and exit) if it is required but
> cannot be loaded.

So there's two things we could do here.  Either explicitly check for NULL
if sandbox is enabled or change board_fdt_blob_setup and put an error
return code in an argument.  I'll go with the return code in v2

[...]
> > - *
> > - * Read a device tree file from a host file and set it up for use as the
> > - * control FDT.
> > - */
> > -int sandbox_read_fdt_from_file(void);
> > -
> >  /**
> >   * sandbox_reset() - reset sandbox
> >   *
> > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > index f7098b496983..358a6c168259 100644
> > --- a/configs/sandbox64_defconfig
> > +++ b/configs/sandbox64_defconfig
> > @@ -86,7 +86,7 @@ CONFIG_MAC_PARTITION=y
> >  CONFIG_AMIGA_PARTITION=y
> >  CONFIG_OF_CONTROL=y
> >  CONFIG_OF_LIVE=y
> > -CONFIG_OF_HOSTFILE=y
> > +CONFIG_OF_BOARD=y
> 
> Can we put this in Kconfig instead, so it is enabled for all sandbox boards?

Sure

[...]
> > --- a/scripts/Makefile.spl
> > +++ b/scripts/Makefile.spl
> > @@ -298,11 +298,11 @@ endif
> >  # Build the .dtb file if:
> >  #   - we are not using OF_PLATDATA
> >  #   - we are using OF_CONTROL
> > -#   - we have either OF_SEPARATE or OF_HOSTFILE
> > +#   - we have either OF_SEPARATE or we are compiling for sandbox
> >  build_dtb :=
> >  ifeq ($(CONFIG_$(SPL_TPL_)OF_PLATDATA),)
> >  ifneq ($(CONFIG_$(SPL_TPL_)OF_CONTROL),)
> > -ifeq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_HOSTFILE),y)
> > +ifeq ($(CONFIG_OF_SEPARATE)$(CONFIG_SANDBOX_SPL),y)
> 
> Do you mean CONFIG_SANDBOX ?

I thought we could use either.  I assumed since this is an SPL makefile
CONFIG_SANDBOX_SPL would be defined.  If it's not I can switch to CONFIG_SANDBOX

> 
> >  build_dtb := y
> >  endif
> >  endif
> > --
> > 2.33.0
> >
> 
> Regards,
> Simon

Thanks
/Ilias