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

[Michal Simek <michal.simek@xilinx.com>]

On 26.7.2018 11:23, Chee, Tien Fong wrote:
> On Wed, 2018-07-25 at 11:48 +0200, Michal Simek wrote:
>> 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@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@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
> If fs_set_blk_dev is called without mounting the ubi first, error would
> be returned from blk_get_device_part_str().

I am not working with ubifs but is this proper error. Or is this just
bug in the code that this error is shown?

Thanks,
Michal