[U-Boot] [PATCH v9 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-02-27 at 11:04 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 27 Feb 2018 13:21:57 +0800 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>
> > Reviewed-by: Lothar Waßmann <LW at KARO-electronics.de>
> > ---
> >  common/Kconfig             |  10 ++
> >  common/Makefile            |   1 +
> >  common/fs_loader.c         | 365
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  doc/README.firmware_loader |  83 +++++++++++
> >  include/fs_loader.h        |  28 ++++
> >  5 files changed, 487 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/Kconfig b/common/Kconfig
> > index d5bc37e..ff88c6e 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -560,6 +560,16 @@ config DISPLAY_BOARDINFO
> >  	  when U-Boot starts up. The board function checkboard()
> > is called
> >  	  to do this.
> >  
> > +config GENERIC_FIRMWARE_LOADER
> > +	bool "Enable generic firmware loader driver for file
> > system"
> > +	depends on SPL
> > +	help
> > +	  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.
> > +
> >  menu "Start-up hooks"
> >  
> >  config ARCH_EARLY_INIT_R
> > diff --git a/common/Makefile b/common/Makefile
> > index c7bde23..92d8fb3 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -134,3 +134,4 @@ obj-$(CONFIG_$(SPL_)LOG) += log.o
> >  obj-$(CONFIG_$(SPL_)LOG_CONSOLE) += log_console.o
> >  obj-y += s_record.o
> >  obj-y += xyzModem.o
> > +obj-$(CONFIG_GENERIC_FIRMWARE_LOADER) += fs_loader.o
> > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > new file mode 100644
> > index 0000000..7fc318f
> > --- /dev/null
> > +++ b/common/fs_loader.c
> > @@ -0,0 +1,365 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +
> > +#include <common.h>
> > +#include <errno.h>
> > +#include <fs.h>
> > +#include <fs_loader.h>
> > +#include <nand.h>
> > +#include <sata.h>
> > +#include <spi.h>
> > +#include <spi_flash.h>
> > +#include <spl.h>
> > +#include <linux/string.h>
> > +#ifdef CONFIG_CMD_UBIFS
> > +#include <ubi_uboot.h>
> > +#include <ubifs_uboot.h>
> > +#endif
> > +#include <usb.h>
> > +
> > +struct firmware_priv {
> > +	const char *name;	/* Filename */
> > +	u32 offset;		/* Offset of reading a file */
> > +};
> > +
> > +static struct device_location default_locations[] = {
> > +	{
> > +		.name = "mmc",
> > +		.devpart = "0:1",
> > +	},
> > +	{
> > +		.name = "usb",
> > +		.devpart = "0:1",
> > +	},
> > +	{
> > +		.name = "sata",
> > +		.devpart = "0:1",
> > +	},
> > +	{
> > +		.name = "ubi",
> > +		.mtdpart = "UBI",
> > +		.ubivol = "ubi0",
> > +	},
> > +};
> > +
> > +/* USB build is not supported yet in SPL */
> > +#ifndef CONFIG_SPL_BUILD
> > +#ifdef CONFIG_USB_STORAGE
> > +static int init_usb(void)
> > +{
> > +	int err;
> > +
> > +	err = usb_init();
> > +	if (err)
> > +		return err;
> > +
> > +#ifndef CONFIG_DM_USB
> > +	err = usb_stor_scan(0);
> > +	if (err)
> > +		return err;
> > +#endif
> > +
> > +	return err;
> > +}
> > +#else
> > +static int init_usb(void)
> > +{
> > +	debug("Error: Cannot load flash image: no USB support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +#endif
> > +
> > +#ifdef CONFIG_SATA
> > +static int init_storage_sata(struct device_location *location)
> > +{
> > +	char *dup_str = NULL;
> > +	const char *dev_str, *part_str;
> > +	int dev;
> > +
> > +	/* Get device ID */
> > +	part_str = strchr(location->devpart, ':');
> > +	if (part_str) {
> > +		dup_str = strdup(location->devpart);
> > +		dup_str[part_str - location->devpart] = 0;
> > +		dev_str = dup_str;
> > +		part_str++;
> > +	} else {
> > +		dev_str = location->devpart;
> > +	}
> > +
> > +	dev = (int)simple_strtoul(dev_str, NULL, 10);
> > +
> > +	return sata_probe(dev);
> > 
> You are leaking the strdup() memory.
Good catch!!

> Furthermore, there is no need to mess around with the 'devpart'
> string
> like this. simple_strtoul() will convert the string up to the first
> non-digit character and return the position of this char in the
> second
> argument (if that's non-null). Thus you can easily check for a valid
> numerical argument.
> 
Okay, i would try with simple_strtoul().

> > 
> > +static void set_storage_devpart(char *name, char *devpart)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > +		if (!strcmp(default_locations[i].name, name))
> > +			default_locations[i].devpart = devpart;
> > +	}
> > +}
> > +
> > +static void set_storage_mtdpart(char *name, char *mtdpart)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > +		if (!strcmp(default_locations[i].name, name))
> > +			default_locations[i].mtdpart = mtdpart;
> > +	}
> > +}
> > +
> > +static void set_storage_ubivol(char *name, char *ubivol)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > +		if (!strcmp(default_locations[i].name, name))
> > +			default_locations[i].ubivol = ubivol;
> > +	}
> > +}
> > +
> The arguments to these functions could well be const.
> 
Okay. noted.
> 
> 
> Lothar Waßmann