[U-Boot] [PATCH 04/13] arm: K3: Introduce System Firmware loader framework

Thu May 16 15:47:30 UTC 2019

Hi Tom,

On Wed, May 15, 2019 at 05:50:31PM -0400, Tom Rini wrote:
> On Wed, May 15, 2019 at 04:39:22PM -0500, Andreas Dannenberg wrote:
> > Hi Tom,
> > 
> > On Wed, May 15, 2019 at 11:17:22AM -0400, Tom Rini wrote:
> > > On Tue, May 07, 2019 at 12:25:33PM -0500, Andreas Dannenberg wrote:
> > > 
> > > > Introduce a framework that allows loading the System Firmware (SYSFW)
> > > > binary as well as the associated configuration data from an image tree
> > > > blob named "sysfw.itb" from an FS-based MMC boot media or from an MMC
> > > > RAW mode partition or sector.
> > > > 
> > > > To simplify the handling of and loading from the different boot media
> > > > we tap into the existing U-Boot SPL framework usually used for loading
> > > > U-Boot by building on an earlier commit that exposes some of that
> > > > functionality.
> > > > 
> > > > Note that this initial implementation only supports FS and RAW-based
> > > > eMMC/SD card boot.
> > > [snip]
> > > > diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> > > > index e677a2e01b..f1731dda58 100644
> > > > --- a/arch/arm/mach-k3/Kconfig
> > > > +++ b/arch/arm/mach-k3/Kconfig
> > > > @@ -58,6 +58,46 @@ config SYS_K3_BOOT_CORE_ID
> > > >  	int
> > > >  	default 16
> > > >  
> > > > +config K3_LOAD_SYSFW
> > > > +	bool
> > > > +	depends on SPL
> > > > +	default n
> > > 
> > > 'n' is already default, you can drop this.
> > 
> > Ok sure.
> > 
> > > 
> > > [snip]
> > > > +config K3_SYSFW_IMAGE_SIZE_MAX
> > > > +	int "Amount of memory dynamically allocated for loading SYSFW blob"
> > > > +	depends on K3_LOAD_SYSFW
> > > > +	default	269000
> > > > +	help
> > > > +	  Amount of memory reserved through dynamic allocation at runtime for
> > > > +	  loading the combined System Firmware and configuration image tree
> > > > +	  blob. Keep it as tight as possible, as this directly affects the
> > > > +	  overall SPL memory footprint.
> > > 
> > > This is missing a unit, and is 'int' really the best choice here (and
> > > really, I guess, 262.6KiB as a default) ?

I think 'int' is a more natural fit as this is to reserve memory for the
'sysfw.itb' blob we load from media. This blob is build outside U-Boot
using the "System Firmware Image Generator" project [1], and if after
build somebody does an 'll' in the build directory the file size would
show up as decimal, and that's what needs to fit inside
K3_SYSFW_IMAGE_SIZE_MAX.

> > Will add a unit.
> > 
> > As for the specific value, in our R5 SPL memory is very very tight. The
> > value used here is basically used for a malloc(), and any extra byte
> > allocated will be "wasted" and not available for stack etc. Also our
> > SYSFW image that is loaded is fixed size (except some +/-100 odd bytes
> > if the configuration data is changed that's part of the same SYSFW.ITB
> > blob), so a rather tailored size makes sense here.
> 
> Right, that makes sense.  But how did you get to that
> not-exactly-"round" number rather than say 0x41A00 or some other
> slightly smaller value in hex?  If 0x41AC8 is right, then, OK, it's
> right.  It just seems strange at first.

The sysfw.itb blob consumed by the SYSFW loader comprises a fixed
component (the SYSFW itself), plus a few smaller chunks of variable data
containing user configuration data. During initial development the
combined size of this ITB was about 264,900 bytes, and 4KB was added as
slack to accommodate configuration changes, resulting in what looks like
the arbitrary number under discussion. The sysfw.itb blob has actually
since grown in size but there is still plenty of space for it to further
grow if needed. Since we use this value already in production since last
year with no issues I'd rather leave it alone (due to using stack based
malloc in R5 SPL increasing that value would directly decrease available
stack space with our R5 SPL memory layout where the stack is growing
down towards the R5 SPL image itself).

> > > [snip]
> > > > diff --git a/arch/arm/mach-k3/Makefile b/arch/arm/mach-k3/Makefile
> > > > index 0c3a4f7db1..6c895400c2 100644
> > > > --- a/arch/arm/mach-k3/Makefile
> > > > +++ b/arch/arm/mach-k3/Makefile
> > > > @@ -7,4 +7,5 @@ obj-$(CONFIG_SOC_K3_AM6) += am6_init.o
> > > >  obj-$(CONFIG_ARM64) += arm64-mmu.o
> > > >  obj-$(CONFIG_CPU_V7R) += r5_mpu.o lowlevel_init.o
> > > >  obj-$(CONFIG_TI_SECURE_DEVICE) += security.o
> > > > +obj-$(CONFIG_K3_LOAD_SYSFW) += sysfw-loader.o
> > > >  obj-y += common.o
> > > [snip]
> > > > diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c
> > > > new file mode 100644
> > > > index 0000000000..a222266c27
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-k3/sysfw-loader.c
> > > > @@ -0,0 +1,263 @@
> > > [snip]
> > > > +#ifdef CONFIG_SPL_BUILD
> > > [snip of the whole body of the file]
> > > > +#endif
> > > 
> > > We should be using something else in the Makefile, typically:
> > > obj-$(CONFIG_SPL_BUILD) += sysfw-loader.o
> > > should work.
> > 
> > Well, it won't work like this. We need to make the building of the SYSFW
> > loader code depending on a CONFIG. This is because for K3 family devices
> > we build the U-Boot tree into three distinct images [1]:
> 
> Then you do:
> ifeq ($(CONFIG_SPL_BUILD),y)
> obj-$(CONFIG_K3_LOAD_SYSFW) += sysfs-loader.o
> endif
> 
> As seen in other places, as we do not whole-body ifdef code.  Thanks!

Great suggestion, works like a champ.

--
Andreas Dannenberg
Texas Instruments Inc

[1] http://git.ti.com/gitweb/?p=processor-firmware/system-firmware-image-gen.git;a=summary