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

[Simon Goldschmidt]

On 24.01.2018 06:13, Chee, Tien Fong wrote:
> On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
>> On 23.01.2018 09:31, Chee, Tien Fong wrote:
>>> On Tue, 2018-01-23 at 08:58 +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
>>>>>
>>>>> diff --git a/common/Makefile b/common/Makefile
>>>>> index cec506f..2934221 100644
>>>>> --- a/common/Makefile
>>>>> +++ b/common/Makefile
>>>>> @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>>>> <snip>
>>>>
>>>>> diff --git a/include/fs_loader.h b/include/fs_loader.h
>>>>> new file mode 100644
>>>>> index 0000000..83a8b80
>>>>> --- /dev/null
>>>>> +++ b/include/fs_loader.h
>>>>> @@ -0,0 +1,28 @@
>>>>> +/*
>>>>> + * Copyright (C) 2017 Intel Corporation <www.intel.com>
>>>>> + *
>>>>> + * SPDX-License-Identifier:    GPL-2.0
>>>>> + */
>>>>> +#ifndef _FS_LOADER_H_
>>>>> +#define _FS_LOADER_H_
>>>>> +
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +struct firmware {
>>>>> +	size_t size;		/* Size of a file */
>>>>> +	const u8 *data;		/* Buffer for file */
>>>>> +	void *priv;		/* Firmware loader private
>>>>> fields */
>>>>> +};
>>>>> +
>>>>> +struct device_location {
>>>>> +	char *name;	/* Such as mmc, usb,and sata. */
>>>> Would it make sense to use 'enum if_type' from blk.h here instead
>>>> of
>>>> a
>>>> "magic" name? Or are the constants passed here "well-known"
>>>> enough
>>>> to
>>>> hide them?
>>>>
>>> This structure is declared such way so that it can be compatible
>>> with
>>> common/splash.c. It also much more easy to port fs_loader into
>>> common/splash_source.c in later.
>> OK, but reading splash_source.c, it seems to me that 'enum
>> splash_storage' is used to detect the device to load from, not 'char
>> *name'.
>>
>> However, since these constant strings already seem to be "common
>> knowledge" for callers of fs.h functions, it might be OK to use them
>> in
>> the firmware loader, too. I just wanted to point this out while this
>> is
>> in the reviewing process.
>>
> Actually this is common interface name used in any fs related. You can
> refer more details regarding @ifname in include/fs.h and include part.h
> .
>
> So, why this interface name must be in characters string?
> It's actually serving in 2 main purposes, and 1 minor purpose:
> 1. console environment, most commands using those interface name as
> their arguments in characters string format such as FPGA loadfs
> command.
>
> 2. These command interface name is one of the characters string
> argument of fs_set_blk_dev too.
>
> 3. As identity of default location and its dev_part configuration.

Right. I see that there does not seem to be a way to provide the 
interface type to fs.h without using a string. In most cases, this 
string is provided by the environment or via console, so I see where 
that comes from. It's probably not up to this patch to change it.

>>>> Regards,
>>>> Simon
>>>>
>>>>> +	char *devpart;  /* Use the load command dev:part
>>>>> conventions */
>>>>> +	char *mtdpart;	/* MTD partition for ubi part */
>>>>> +	char *ubivol;	/* UBI volume-name for ubifsmount
>>>>> */
>>>>> +};
>>>>> +
>>>>> +int request_firmware_into_buf(struct firmware **firmware_p,
>>>>> +			      const char *name,
>>>>> +			      struct device_location
>>>>> *location,
>>>>> +			      void *buf, size_t size, u32
>>>>> offset);
>>>>> +#endif