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

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

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.
> 
> 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.
> 
> 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.
> 
> 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? 
> 
> Thanks,
> Michal