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

[Miquel Raynal]

Hi Tom, Wolfgang,

Tom Rini <trini at konsulko.com> wrote on Thu, 6 Dec 2018 10:59:25 -0500:

> 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.
> 

So what's our next move? I think Wolfgang had two points:
* NOR (and everything related to NOR) should not depend on MTD.
* Same for NAND.

For NAND this is already too late, NAND code already relies on MTD so
I am just cleaning Makefiles/Kconfig/defconfigs to avoid the "xxx:
missing symbol" error. Now, what about NOR? We know that the overhead,
today, is null, so do you allow me to make NOR depend on MTD or are you
still opposed to that? I think U-Boot maintenance and future
changes would be really eased.


Thanks,
Miquèl