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

[Simon Goldschmidt]

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

<snip>

> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +static int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err;
> +
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +
> +	err = mmc_init(mmc);
> +	if (err) {
> +		printf("spl: mmc init failed with error: %d\n", err);
> +		return err;
> +	}
> +
> +	return err;
> +}

I see two problems here: First, you're ignoring the return value of 
spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't it 
be better to let 'mmc' be uninitialized and return the error code 
returned by spl_mmc_find_device() if there is one?

Second, using spl_boot_device() would prevent making the loader work on 
mach-socfpga when spl is not loaded from mmc, right?

E.g. for the case I'm currently trying to fix (boot from qspi), this 
loader would not work although there's technically no reason since the 
platform only has one mmc. The call to spl_boot_device() could be 
replaced by the exact value here for platforms that only have one mmc. I 
don't know how to fix that, though.

Regards,
Simon