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

[Simon Goldschmidt]

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.

>> 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