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

Thu Dec 6 16:19:13 UTC 2018

Am 06.12.2018 um 16:59 schrieb 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.

Based on my tests with Vignesh's series to port SF to MTD, I don't think 
we drop the write part in SPL. I also brought that up in that thread.

However, my (preliminary) test results of his work showed that SPL size 
is actually reduced when using his new 'spi-nor-tiny' code. And that's 
without trying to reduce MTD code size.

Regards,
Simon

> 
>> [...]
>>
>>>>> 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.
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 