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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Thu, 2018-07-26 at 12:29 +0200, Michal Simek wrote:
> 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?
This is proper error, mounting is required before running
fs_set_blk_dev(). Hence, both arguments mtd partition and ubi volume
are required for mounting.
> 
> Thanks,
> Michal
> 
>