[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-22 at 15:28 +0100, Marek Vasut wrote:
> On 02/22/2018 09:18 AM, Chee, Tien Fong wrote:
> > 
> > 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.
> You probably dont need those ifdefs around headers.
> 
> > 
> > > 
> > > > 
> > > > +#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 ?
> Well ?
> 
I can add ".name = ubi" and ".devpart = UBI" .
> [...]
> 
> > 
> > > 
> > > > 
> > > > +#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.
> strtok(), strtol() or something ?
> 
yes, plan to use that.
> [...]
> 
> > 
> > > 
> > > > 
> > > > +#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("..... .
> So what ? This missing macro can be detected at runtime, heck this
> code
> can even depend on CONFIG_SPL in Kconfig.
> 
Oh...I got you, i thought you means syntax error. So, i would remove it
and adding the depend on CONFIG_SPL in Kconfig.
> > 
> > > 
> > > > 
> > > > 
> > > > +	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
> > > [...]
>