[U-Boot] [PATCH v3 13/28] mtd: ensure MTD is compiled when ENV_IS_IN_FLASH is selected

[Tom Rini]

On Thu, Dec 06, 2018 at 04:23:00PM +0100, Miquel Raynal wrote:
> Hi Wolfgang,
> 
> Wolfgang Denk <wd at denx.de> wrote on Thu, 06 Dec 2018 10:32:14 +0100:
> 
> > Dear Boris,
> > 
> > In message <20181206100016.706330ba at bbrezillon> you wrote:
> > >  
> > > > > I took a rather small configuration: stm32f429-discovery_defconfig
> > > > > which uses CONFIG_MTD_NOR_FLASH. Without CONFIG_MTD, u-boot.bin is
> > > > > 209416 bytes. With CONFIG_MTD, u-boot.bin is 214540 bytes. This is an
> > > > > additional 5124 bytes which represent about 2% of the entire size.    
> > > > 
> > > > So it's a 5 k code increase for zero benefit.  
> > >
> > > There's no short term benefit, but the long term benefit is ease better
> > > maintainability.  
> > 
> > For MTD, yes.  But a user of environment in NOR flash does not
> > necessarily want MTD,
> > 
> > Parallel NOR flash is a traditional memory device - you find it
> > frequently on older, resource-restricted systems.  It is not so
> > polular any more today - SPI NOR is much cheaper, comea with a
> > smaller footprint of thr PCB and is much easier to route.
> > 
> > And on these older systems a 5 k code increase is often critical.
> > 
> > Especially if this should also affect the SPL.
> 
> Please, stop repeating the SPL is a problem. I am not forcing MTD
> to be compiled in the SPL. You overlooked that point in our last
> three e-mails: MTD is *not* built in the SPL unless specifically
> requested. CONFIG_MTD has absolutely no impact on the SPL.

Right, and this is the important part that you are indeed getting right.
We must be careful about the SPL case.  You are, so good, lets move on.

> Now, let's talk about U-Boot size. I went through U-Boot MTD
> implementation to see where this 5k overhead was coming from. The
> explanation is short: nothing. My observation was wrong because of
> commands being automatically compiled ("default y"). By the way this is
> the kind of approach I want to get rid of because it is fundamentally
> wrong.
> 
> Here is a brief summary, still with stm32f429-discovery_defconfig:
> * before my series (MTD_NOR_FLASH + CMD_FLASH)
> * after my series without MTD (CONFIG_MTD_NOR_FLASH + CMD_FLASH)
> * after my series with MTD (MTD_NOR_FLASH + CMD_FLASH + MTD)
> 
> In all cases, u-boot.bin is 214540 bytes. I suppose if you don't use
> anything from MTD, there is no additional symbol, the binary will not be
> enlarged. And when you use it, it is hard to tell how much bytes were
> added by MTD and how much bytes have been added by the feature using
> MTD.

So when you aren't calling into the new code it gets discarded as
expected, good.  Thanks for digging.

> > > I think you missed the *no*. There's no overhead at all for the SPL.
> > > Either the platform was already enabling CONFIG_SPL_MTD_SUPPORT and the
> > > MTD code was already compiled in before Miquel's patch, or this option
> > > is disabled, and the SPL still does *not* embed the MTD layer.  
> > 
> > So parallel NOR is still supported in the SPL _without_ MTD?
> > Then why cannot we have the same support in U-Boot, too?
> 
> Code duplication, maintainability, single interface, (hopefully) single
> command, etc

Right.  And to be clear, in the main U-Boot case while we don't grow the
binary for fun, we do grow the overall binary (for some board or
another) with nearly every push because we're quite often doing "lets
fix $X" or "let's add $Y" and rarely "Drop $X as it's linked but
unused".  Just like DM is resulting in an overall larger binary that's a
more consistent and maintainable codebase, this too will result in some
things growing in binary size.  And we should review the framework with
an eye towards "can we do this in a more simple or size-courteous way?"
size growth in and of itself isn't a fatal problem.

> > > > Please understand that there is a fundamental difference between
> > > > parallel NOR flash and things like NAND, SPI NOR etc. - the latter
> > > > are storage devices, which are usually handled as block device or
> > > > similar, i. e. you always need a driver to read data.  Parallel NOR
> > > > is not storage, it is _memory_, which is directly mapped int o
> > > > memory. You can execute code from it.  
> > >
> > > That's only partially true. Yes, you can read from a parallel NORs like
> > > if it was memory because memory controllers embedded in SoCs provide
> > > your a direct mmio mapping. That's not true for write accesses,  
> > 
> > Right, but especially in the SPL read access is sufficient.
> 
> 100% agreed, the SPL just needs read access. But we are talking about
> U-Boot itself here.

So, new but related question.  Are we dropping the write path in the
case of SPL?  The only case I can think of where SPL might be doing an
MTD write would be bootcount stored in env, so we should be able to drop
those paths in the normal case.

> [...]
> 
> > > > Please do not understand me wrong: I fully agree with the MTD rework
> > > > in general, and I'm happy to see it happen.  But please do it in a
> > > > way not to break existing basic use cases.  
> > >
> > > Okay, maybe we can put aside the parallel NOR case for now, but I do
> > > think even parallel NOR accesses would benefit from the MTD layer.  
> > 
> > I agree that it benefits form an architectural and maintenance point
> > of view.  But it adds a new dependency with more than insignificant
> > impact to systems where this has not been before, and where
> > resources are tight.

Frankly I'm concerned that we must have a lot of these boards that
aren't actually functional today, or are overwriting something else in
memory.  Using buildman and the stm32f429-discovery with buildman, I see
our growth from v2018.01 to v2018.11 is:
all +48781 bss -628 data +5592 rodata +3079 text +40738

Of course, that's not all inertia growth, there's lots of intentional
new board-specific code.  So lets try taurus (slightly reformatted
output):
all +42136 bss +13344 data +4512 rodata +936 text +23344
spl/u-boot-spl:all +126 spl/u-boot-spl:bss +24 spl/u-boot-spl:rodata +38
spl/u-boot-spl:text +64

Here the board hasn't opted out of the EFI subsystem which has seen a
good deal of desired growth (fixes and features).

So, if we're going to start actively worrying about resource constrained
boards not fitting within resource _now_ step 1 is to identify them and
step 2 is to make sure they're making use of the hooks we have to make
exceeding a specific size a link time error.  Step 3 will be dealing
with however many boards we have that are failing today.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181206/9daef51c/attachment.sig>