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

[Simon Goldschmidt]

On 22.01.2018 09:08, Chee, Tien Fong wrote:
> On Thu, 2018-01-18 at 06:57 +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
>> <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?
>>
> Yeah, you are right, i should add the check on the return value. I
> think that would better to initialize NULL to mmc, because there is no
> checking on the mmc in spl_mmc_find_device(), so that is possible
> memory access violation/abort can be happended if unknown value in mmc
> pointer.
>
>> 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.
>>
> The main purpose here is to initialize the mmc driver. So which storage
> user wants to load the file is totally depend what storage such as mmc
> user defines in location->name. Loader would init the storage based on
> the storage defined in location->name before accessing it. Since the
> loader only support file system at this moment, i would suggest FAT fs
> for mmc and ubi fs for qspi.

What I meant to say is this: at least on mach-socfpga, from reading the 
code, I cannot load a file from mmc when booting from qspi, as 
'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I 
need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I wrong 
here?

Regards,
Simon