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

[Marek Vasut]

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 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>
>>> Reviewed-by: Lothar Waßmann <LW at 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 ?

[...]

>>> +#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 ?

[...]

>>> +#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.

>>>
>>> +	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
>> [...]


-- 
Best regards,
Marek Vasut