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.c...@intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.c...@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.c...@intel.com>
>>> Reviewed-by: Lothar Waßmann <l...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
