[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
> On 23.01.2018 09:31, Chee, Tien Fong wrote:
> > 
> > On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
> > > 
> > > On 27.12.2017 06:04, tien.fong.chee at intel.com wrote:
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > 
> > > > This is file system generic loader which can be used to load
> > > > the file image from the storage into target such as memory.
> > > > The consumer driver would then use this loader to program
> > > > whatever,
> > > > ie. the FPGA device.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > ---
> > > >    common/Makefile            |   1 +
> > > >    common/fs_loader.c         | 309
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >    doc/README.firmware_loader |  76 +++++++++++
> > > >    include/fs_loader.h        |  28 ++++
> > > >    4 files changed, 414 insertions(+)
> > > >    create mode 100644 common/fs_loader.c
> > > >    create mode 100644 doc/README.firmware_loader
> > > >    create mode 100644 include/fs_loader.h
> > > > 
> > > > diff --git a/common/Makefile b/common/Makefile
> > > > index cec506f..2934221 100644
> > > > --- a/common/Makefile
> > > > +++ b/common/Makefile
> > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> > > <snip>
> > > 
> > > > 
> > > > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > > > new file mode 100644
> > > > index 0000000..83a8b80
> > > > --- /dev/null
> > > > +++ b/include/fs_loader.h
> > > > @@ -0,0 +1,28 @@
> > > > +/*
> > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com>
> > > > + *
> > > > + * SPDX-License-Identifier:    GPL-2.0
> > > > + */
> > > > +#ifndef _FS_LOADER_H_
> > > > +#define _FS_LOADER_H_
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct firmware {
> > > > +	size_t size;		/* Size of a file */
> > > > +	const u8 *data;		/* Buffer for file */
> > > > +	void *priv;		/* Firmware loader private
> > > > fields */
> > > > +};
> > > > +
> > > > +struct device_location {
> > > > +	char *name;	/* Such as mmc, usb,and sata. */
> > > Would it make sense to use 'enum if_type' from blk.h here instead
> > > of
> > > a
> > > "magic" name? Or are the constants passed here "well-known"
> > > enough
> > > to
> > > hide them?
> > > 
> > This structure is declared such way so that it can be compatible
> > with
> > common/splash.c. It also much more easy to port fs_loader into
> > common/splash_source.c in later.
> OK, but reading splash_source.c, it seems to me that 'enum 
> splash_storage' is used to detect the device to load from, not 'char
> *name'.
> 
> However, since these constant strings already seem to be "common 
> knowledge" for callers of fs.h functions, it might be OK to use them
> in 
> the firmware loader, too. I just wanted to point this out while this
> is 
> in the reviewing process.
> 
Actually this is common interface name used in any fs related. You can
refer more details regarding @ifname in include/fs.h and include part.h
.

So, why this interface name must be in characters string?
It's actually serving in 2 main purposes, and 1 minor purpose:
1. console environment, most commands using those interface name as
their arguments in characters string format such as FPGA loadfs
command.

2. These command interface name is one of the characters string
argument of fs_set_blk_dev too.

3. As identity of default location and its dev_part configuration.

> > 
> > > 
> > > Regards,
> > > Simon
> > > 
> > > > 
> > > > +	char *devpart;  /* Use the load command dev:part
> > > > conventions */
> > > > +	char *mtdpart;	/* MTD partition for ubi part */
> > > > +	char *ubivol;	/* UBI volume-name for ubifsmount
> > > > */
> > > > +};
> > > > +
> > > > +int request_firmware_into_buf(struct firmware **firmware_p,
> > > > +			      const char *name,
> > > > +			      struct device_location
> > > > *location,
> > > > +			      void *buf, size_t size, u32
> > > > offset);
> > > > +#endif