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

[Andreas Dannenberg <dannenberg@ti.com>]

Hi Tom,

On Thu, May 16, 2019 at 10:47:30AM -0500, Andreas Dannenberg wrote:
> 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).

Small clarification, the size of the early malloc pool is of course
controlled by CONFIG_SYS_MALLOC_F_LEN with K3_SYSFW_IMAGE_SIZE_MAX using
the majority of it. However CONFIG_SYS_MALLOC_F_LEN is dimensioned such
to "tightly wrap" both K3_SYSFW_IMAGE_SIZE_MAX and other users of the
early malloc pool (fat fs is a major one), so the principle is the same.
I'd rather leave K3_SYSFW_IMAGE_SIZE_MAX as it stands.

Some more details on the memory usage to follow...

--
Andreas Dannenberg
Texas Instruments Inc