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

[Simon Goldschmidt]

On 23.01.2018 07:28, Chee, Tien Fong wrote:
> On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
>> 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?
>>
> Okay, i got you.
>
> Yeah, you are right for use case if you need to load the file from mmc
> during SPL boot and SPL is not loaded from mmc.
>
> Since the spl_boot_device is platform dependent, you may try to add
> some state machine so that this function can return correct device
> type.
>
> Secondly, you can use this function in U-Boot instead of SPL.

You're right on that. Thinking about it, I would probably use it from 
U-Boot, not from SPL...

> Lastly, i can declare __Weak to init_mmc, so that user can define their
> own implementation.

At least somehow allowing to override which device is passed to 
'spl_mmc_find_device' might be good, instead of hardcoding it to 
'spl_boot_device'...

Regards,
Simon