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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
> On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@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@intel.com>
> > Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> [...]
> 
> > 
> > +#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>
> > +#ifdef CONFIG_SPL
> Are the ifdefs needed ?
> 
Because spl.h contains some codes have its dependency with SPL. So, Tom
adviced to make this part of code depend on CONFIG_SPL.
However, only __weak int init_mmc() depend on the codes from spl.h, so
user can override their own init_mmc() if SPL is not used.

> > 
> > +#include <spl.h>
> > +#endif
> > +#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",
> > +	},
> How can this load from UBI if it's not listed here ?
> 
> > 
> > +};
> > +
> > +/* 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(1);
> mode = 0 I think, we don't need this verbose output.
> 
Okay so i will change to 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(void)
> > +{
> > +	return sata_probe(0);
> Shouldn't the sata device number be pulled from devpart in
> default_locations table ?
> 
This may need to add the logic for splitting the device number and
partition into integer from the string. Let me think how to do it, may
be i can leverage existing code.
> > 
> > +}
> > +#else
> > +static int init_storage_sata(void)
> > +{
> > +	debug("Error: Cannot load image: no SATA support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_CMD_UBIFS
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > +	int ret;
> > +	char cmd[32];
> > +
> > +	if (ret = ubi_part(location->mtdpart, NULL)) {
> > +		debug("Cannot find mtd partition %s\n", location-
> > >mtdpart);
> > +		return ret;
> > +	}
> > +
> > +	return cmd_ubifs_mount(location->ubivol);
> > +}
> > +
> > +static int umount_ubifs(void)
> > +{
> > +	return cmd_ubifs_umount();
> > +}
> > +#else
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > +	debug("Error: Cannot load image: no UBIFS support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> > +__weak int init_mmc(void)
> > +{
> > +	/* Just for the case MMC is not yet initialized */
> > +	struct mmc *mmc = NULL;
> > +	int err;
> > +
> > +#ifdef CONFIG_SPL
> > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > +#else
> > +	debug("#define CONFIG_SPL is required or overrriding
> > %s\n",
> > +	      __func__);
> This can be a compile-time error, maybe ?
> 
No compile error. When you open the file, "%s\n" is actually same line
with debug("..... .

> > 
> > +	return -ENOENT;
> > +#endif
> > +
> > +	err = mmc_init(mmc);
> > +	if (err) {
> > +		debug("spl: mmc init failed with error: %d\n",
> > err);
> > +		return err;
> > +	}
> > +
> > +	return err;
> > +}
> > +#else
> > +__weak int init_mmc(void)
> > +{
> > +	/* Expect somewhere already initialize MMC */
> > +	return 0;
> > +}
> > +#endif
> [...]
>