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

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hello Wolfgang,

On Thu, 06 Dec 2018 09:06:05 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Miquel,
> 
> In message <20181205153218.36f6ed4e@xps13> 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.

> 
> > I am talking about U-Boot only, there is a CONFIG_SPL_MTD_SUPPORT
> > option that must be enabled to compile MTD in the SPL so the change
> > I propose do not enlarge the SPL.  
> 
> This is even worse - the SPL is frequently at the size limits.

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.

> 
> 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,
as NORs need to be erased before you can write on it. The MTD layer is
here to abstract that, such that flash users only have to know how to
manipulate an MTD device instead of having one backend per flash-type
(actually it's even one per-flash-type+mem-bus).

> 
> Reading data from parallel NOR comes at zero overhead (except maybe
> for setting up the memory controller to map the NOR into the address
> space).

The overhead can be minimal if we want (we can have a tiny MTD layer)

current:
	flash_read()

proposed:
	mtd_read()
		mtd_to_flash_read_wrapper()
			flash_read()

and when I say minimal, I mean both in term of size and perfs (we're
talking about an indirect jump and a few extra instructions in
mtd_to_flash_read_wrapper()).

The benefit, well, a single entry point for all kind of mtd accesses.
That means we can have an MTD backend for env retrieval instead of one
per flash type. Same goes for DFU backends, and maybe even for SPL
boots.
To sum-up: less code to maintain. And I wouldn't be surprized if we
were actually reducing the image footprint in some cases (when support
for several flash types is enabled).

> 
> Adding a dependency on the MTD layer is broken.
> 
> > Today, there are multiple boards having more than one type of MTD
> > device (eg. raw NAND and SPI-NOR). In this case you need to compile two
> > commands which interface only with the subsystem they have been written
> > for. We propose to kill this approach and instead use an 'mtd' command
> > which shares the same code for all MTD devices: less duplication, more
> > users, and in the end, a reduced size. And I am not event talking about
> > all the code that has been added over the years to "avoid using MTD"
> > which enlarges the binary as well.  
> 
> This is perfectly fine.  But you must not break fundamental features
> that have been there ever since the beginning of the project.
> 
> Simple things - like reading the environment from parallel NOR
> (which boils down to calling ofa memcpy()) must not be bloated with
> unneeded layers like MTD - even if these are needed and useful
> elsewhere.
> 
> 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.

Regards,

Boris