[U-Boot] [PATCH v4 6/6] common: Generic loader for file system

Wed Jul 25 09:48:21 UTC 2018

On 25.7.2018 08:31, Chee, Tien Fong wrote:
> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>> On 6.7.2018 10:28, 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>
>>> ---
>>>  drivers/misc/Kconfig     |  10 ++
>>>  drivers/misc/Makefile    |   1 +
>>>  drivers/misc/fs_loader.c | 295
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/dm/uclass-id.h   |   1 +
>>>  include/fs_loader.h      |  79 +++++++++++++
>>>  5 files changed, 386 insertions(+)
>>>  create mode 100644 drivers/misc/fs_loader.c
>>>  create mode 100644 include/fs_loader.h
>>>
>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>> index 17b3a80..4163b4f 100644
>>> --- a/drivers/misc/Kconfig
>>> +++ b/drivers/misc/Kconfig
>>> @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
>>>  	depends on MISC
>>>  	help
>>>  	  Support gdsys FPGA's RXAUI control.
>>> +
>>> +config FS_LOADER
>>> +	bool "Enable loader driver for file system"
>>> +	help
>>> +	  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.
>>> +
>>>  endmenu
>>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>>> index 4ce9d21..67a36f8 100644
>>> --- a/drivers/misc/Makefile
>>> +++ b/drivers/misc/Makefile
>>> @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
>>>  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
>>>  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
>>>  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
>>> +obj-$(CONFIG_FS_LOADER) += fs_loader.o
>>> diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
>>> new file mode 100644
>>> index 0000000..5fe642b
>>> --- /dev/null
>>> +++ b/drivers/misc/fs_loader.c
>>> @@ -0,0 +1,295 @@
>>> +/*
>>> + * Copyright (C) 2018 Intel Corporation <www.intel.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0
>>> + */
>>> +#include <common.h>
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <blk.h>
>>> +#include <fs.h>
>>> +#include <fs_loader.h>
>>> +#include <linux/string.h>
>>> +#include <mapmem.h>
>>> +#include <malloc.h>
>>> +#include <spl.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +struct firmware_priv {
>>> +	const char *name;	/* Filename */
>>> +	u32 offset;		/* Offset of reading a file */
>>> +};
>>> +
>>> +#ifdef CONFIG_CMD_UBIFS
>>> +static int mount_ubifs(char *mtdpart, char *ubivol)
>>> +{
>>> +	int ret = ubi_part(mtdpart, NULL);
>>> +
>>> +	if (ret) {
>>> +		debug("Cannot find mtd partition %s\n", mtdpart);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return cmd_ubifs_mount(ubivol);
>>> +}
>>> +
>>> +static int umount_ubifs(void)
>>> +{
>>> +	return cmd_ubifs_umount();
>>> +}
>>> +#else
>>> +static int mount_ubifs(char *mtdpart, char *ubivol)
>>> +{
>>> +	debug("Error: Cannot load image: no UBIFS support\n");
>>> +	return -ENOSYS;
>>> +}
>>> +#endif
>>> +
>>> +static int select_fs_dev(struct device_platdata *plat)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (plat->phandlepart.phandle) {
>>> +		ofnode node;
>>> +
>>> +		node = ofnode_get_by_phandle(plat-
>>>> phandlepart.phandle);
>>> +
>>> +		int of_offset = ofnode_to_offset(node);
>>> +
>>> +		struct udevice *dev;
>>> +
>>> +		ret = device_get_global_by_of_offset(of_offset,
>>> &dev);
>>> +		if (!ret) {
>>> +			struct blk_desc *desc =
>>> blk_get_by_device(dev);
>>> +			if (desc) {
>>> +				ret =
>>> fs_set_blk_dev_with_part(desc,
>>> +					plat-
>>>> phandlepart.partition);
>>> +			} else {
>>> +				debug("%s: No device found\n",
>>> __func__);
>>> +				return -ENODEV;
>>> +			}
>>> +		}
>>> +	} else if (plat->mtdpart && plat->ubivol) {
>>> +		ret = mount_ubifs(plat->mtdpart, plat->ubivol);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
>> I am curious why it is in generic FS loader any code which target any
>> filesystem. It should be filesystem independent.
> Because it supports our use case, and our preference using file system
> instead of RAW. As I agree with Tom, it can be evolved to support RAW
> in future.

It is not a problem that you have decided to support filesystems at
first place. I don't understand why you have UBIFS specific code here.

I would expect that this will done as CMD_FS_GENERIC option which is
based on code also able to work with UBIFS. It means this code will call
generic function and based on FS type proper functions will be chosen.

I didn't work with UBIFS but it is supported in fs.c that's why I would
expect that this shouldn't be a problem to get it work.

>>
>> Also that DT binding is quite weird and I don't think you will get
>> ACK
>> for this from device tree community at all. I think that calling via
>> platdata and avoid DT nodes would be better way to go.
> Why do you think DT binding is weird? The DT is designed based on Simon
> proposal, and i believe following the rules in DTS spec.
> There are some DT benefits with current design, i think someone may be
> maintainer need to made the final decision on the design.

It is software configuration in file which should mostly describe
hardware and state for hardware configuration.

Your fs_loader node is purely describe sw configuration which shouldn't
be here.
You have there run time configuration via variables. I think using only
this way is enough. Default variables will match what you would want to
add to DT.

>>
>> Also I can't see any usage of firmware_loader from chosen which you
>> have
>> in 4/6.
> https://patchwork.ozlabs.org/patch/940319/ This contains some
> explanation about chosen, default storage defined by user in DTS.
> fs_loader_ofdata_to platdata() is used to parse the configuration in
> chosen.
>>
>> Also based on discussion with Marek I am not quite sure if this can
>> be
>> used for his SPL FPGA A10 bitstream loading from FS by chuck from FIT
>> image as he mentioned.
> This design is currently not support RAW, but i think it can be
> enhanced in future. I'm still prefer to put the FPGA design in
> filesystem instead of RAW, thinking about the filesystem benefits.

I have no problem that you support filesystem first.

>>
>> And last but not least I am missing user of this loader. I think it
>> will
>> be the best to also send a user of this to see how exactly this will
>> be
>> called and used.
> https://patchwork.ozlabs.org/patch/940319/ , did you read the doc?
> I have setting up a test env for this design which is using FPGA
> manager to call this loader for chunk by chunk transfering the FPGA
> design from storage into FPGA manager. It would be a bit messy and
> complicated to understand from the codes but it would be helpful to
> understand the use of this loader. Do you want it? 

Yes please send this out to see how exactly you want to use this. We
have done this in zynq_loadfs by using generic fs_read functions.

Feel free to also check this series which is cleaning up fpga command.
https://lists.denx.de/pipermail/u-boot/2018-July/335193.html

Thanks,
Michal