common: Generic loader for file system
PATCH v4 6/6 common: Generic loader for file system
From: Tien Fong Chee <[email protected]>
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.
--- 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
drivers/misc/Kconfig +10 -0
@@ -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
drivers/misc/Makefile +1 -0
@@ -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
drivers/misc/fs_loader.c new file +295 -0
295 lines not on this page · raw message
include/dm/uclass-id.h +1 -0
@@ -36,6 +36,7 @@ enum uclass_id {UCLASS_DMA ,/* Direct Memory Access */ UCLASS_EFI ,/* EFI managed devices */ UCLASS_ETH ,/* Ethernet device */ +UCLASS_FS_FIRMWARE_LOADER ,/* Generic loader */ UCLASS_GPIO ,/* Bank of general-purpose I/O pins */ UCLASS_FIRMWARE ,/* Firmware */ UCLASS_I2C, /* I2C bus */
include/fs_loader.h new file +79 -0
79 lines not on this page · raw message
Signature · 1 line
2.2.0
PATCH v4 6/6 common: Generic loader for file system
Hi Tien,
Quoted 8 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>
Quoted 9 lines
--- 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
I don't see a sandbox test for this. We need to add a test for every new uclass. Please let me know if you want some pointers to ideas.
[..]
Regards,
Simon
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 22 lines · Simon Glass
Hi Tien,
Quoted 16 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 +++++++++++++Quoted 3 lines
5 files changed, 386 insertions(+)create mode 100644
drivers/misc/fs_loader.c
create mode 100644include/fs_loader.h
Quoted 3 lines
I don't see a sandbox test for this. We need to add a test for every new uclass. Please let me know if you want some pointers to ideas.
Yeah, i will add the sandbox for this once we finalize the design. Sandbox is new to me, i would appreciate it very much if you can share some ideas.
Quoted 4 lines
[..]
Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
In reply to tien.fong.chee at intel.comHi Tien,
Quoted 23 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>--- 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.hdrivers/misc/Kconfig +0
-0
drivers/misc/Kconfig config GDSYS_RXAUI_CTRL · line 285
Quoted 9 lines
drivers/misc/Kconfig +6
-0@@ -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.
*a* file image?
drivers/misc/Kconfig config GDSYS_RXAUI_CTRL · line 288
Quoted 3 lines
drivers/misc/Kconfig +3
-0+ + The consumer driver would then use this loader to program whatever, + ie. the FPGA device.
e.g. an FPGA device
Quoted 28 lines
drivers/misc/Kconfig +1
-0+ endmenudrivers/misc/Makefile +1
-0@@ -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.odrivers/misc/fs_loader.c new file +11
-0@@ -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>
drivers/misc/fs_loader.c line 21
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+#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 */
I don't understand that comment. Is it the offset within the file to read from? Please can you expand it a bit?
Quoted 13 lines
drivers/misc/fs_loader.c new file +13
-0+}; + +#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);
drivers/misc/fs_loader.c line 44
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+} + +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" );
blank line here
Quoted 8 lines
drivers/misc/fs_loader.c new file +8
-0+ return -ENOSYS; +} +#endif + +static int select_fs_dev (struct device_platdata *plat) +{ +int ret; +
drivers/misc/fs_loader.c line 62
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+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);
Can you please create device_get_global_by_ofnode() and use that instead? We should not use offsets in new code.
Quoted 32 lines
drivers/misc/fs_loader.c new file +32
-0+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 ); + }else { +debug ("Error: unsupported storage device.\n" ); +return -ENODEV ; + } + +if (ret) +debug ("Error: could not access storage.\n" ); + +return ret; +} + +/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into.
drivers/misc/fs_loader.c line 104
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* @firmwarep: Pointer to pointer to firmware image. +* +* Return: Negative value if fail, 0 for successful. +*/ +static int _request_firmware_prepare (const char *name,void *dbuf, +size_t size,u32 offset, +struct firmware **firmwarep) +{
struct firmware *firmware = *firmwarep;
and use that instead of the clumsy *firmwarep in this function
drivers/misc/fs_loader.c line 109
Quoted 5 lines
drivers/misc/fs_loader.c new file +5
-0+if (!name || name[0 ] =='\0' ) +return -EINVAL ; + +/* No memory allocation is required if *firmwarep is allocated */ +if (!(*firmwarep)) {
Can this not be allocated automatically by the uclass or driver?
drivers/misc/fs_loader.c line 119
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ (*firmwarep) =calloc (1 ,sizeof (struct firmware )); +if (!(*firmwarep)) +return -ENOMEM ; + + (*firmwarep)->priv =calloc (1 ,sizeof (struct firmware_priv )); +if (!(*firmwarep)->priv) { +free (*firmwarep); +return -ENOMEM ; + } + }else if (!(*firmwarep)->priv) {
Can you not use the driver's priv_auto_alloc_size to allocate this? Or the uclass?
drivers/misc/fs_loader.c line 127
Quoted 8 lines
drivers/misc/fs_loader.c new file +8
-0+ (*firmwarep)->priv =calloc (1 ,sizeof (struct firmware_priv )); +if (!(*firmwarep)->priv) { +free (*firmwarep); +return -ENOMEM ; + } + } + + ((struct firmware_priv *)((*firmwarep)->priv))->name = name;
Again this needs a local variable.
Quoted 66 lines
drivers/misc/fs_loader.c new file +66
-0+ ((struct firmware_priv *)((*firmwarep)->priv))->offset = offset; + (*firmwarep)->data = dbuf; + (*firmwarep)->size = size; + +return 0 ; +} + +/** +* release_firmware - Release the resource associated with a firmware image +* @firmware: Firmware resource to release +*/ +void release_firmware (struct firmware *firmware) +{ +if (firmware) { +if (firmware->priv) { +free (firmware->priv); + firmware->priv =NULL ; + } +free (firmware); + } +} + +/** +* fw_get_filesystem_firmware - load firmware into an allocated buffer. +* @plat: Platform data such as storage and partition firmware loading from. +* @firmware: pointer to firmware image. +* +* Return: Size of total read, negative value when error. +*/ +static int fw_get_filesystem_firmware (struct device_platdata *plat, +struct firmware *firmware) +{ +struct firmware_priv *fw_priv =NULL ; +loff_t actread; +char *storage_interface, *dev_part, *ubi_mtdpart, *ubi_volume; +int ret; + + storage_interface =env_get ("storage_interface" ); + dev_part =env_get ("fw_dev_part" ); + ubi_mtdpart =env_get ("fw_ubi_mtdpart" ); + ubi_volume =env_get ("fw_ubi_volume" ); + +if (storage_interface && dev_part) { + ret =fs_set_blk_dev (storage_interface, dev_part,FS_TYPE_ANY ); + }else if (storage_interface && ubi_mtdpart && ubi_volume) { + ret =mount_ubifs (ubi_mtdpart, ubi_volume); +if (ret) +return ret; + +if (!strcmp ("ubi" , storage_interface)) + ret =fs_set_blk_dev (storage_interface,NULL , +FS_TYPE_UBIFS ); +else + ret = -ENODEV ; + }else { + ret =select_fs_dev (plat); + } + +if (ret) + goto out; + + fw_priv = firmware->priv; + + ret =fs_read (fw_priv->name, (ulong )map_to_sysmem (firmware->data), + fw_priv->offset, firmware->size, &actread); +if (ret) {
drivers/misc/fs_loader.c line 203
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+debug ("Error: %d Failed to read %s from flash %lld != %d.\n" , + ret, fw_priv->name, actread, firmware->size); + }else { + ret = actread; + } + +out: +#ifdef CONFIG_CMD_UBIFS +umount_ubifs (); +#endif
debug() here to report failure
Quoted 124 lines
drivers/misc/fs_loader.c new file +92
-0+return ret; +} + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer. +* @plat: Platform data such as storage and partition firmware loading from. +* @name: Name of firmware file. +* @buf: Address of buffer to load firmware into. +* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* @firmwarep: Pointer to firmware image. +* +* The firmware is loaded directly into the buffer pointed to by @buf and +* the @firmwarep data member is pointed at @buf. +* +* Return: Size of total read, negative value when error. +*/ +int request_firmware_into_buf (struct device_platdata *plat, +const char *name, +void *buf,size_t size,u32 offset, +struct firmware **firmwarep) +{ +int ret; + +if (!plat) +return -EINVAL ; + + ret =_request_firmware_prepare (name, buf, size, offset, firmwarep); +if (ret <0 )/* error */ +return ret; + + ret =fw_get_filesystem_firmware (plat, *firmwarep); + +return ret; +} + +static int fs_loader_ofdata_to_platdata (struct udevice *dev) +{ +const char *fs_loader_path; +u32 phandlepart[2 ]; + + fs_loader_path =ofnode_get_chosen_prop ("firmware-loader" ); + +if (fs_loader_path) { +ofnode fs_loader_node; + + fs_loader_node =ofnode_path (fs_loader_path); +if (ofnode_valid (fs_loader_node)) { +struct device_platdata *plat; + plat = dev->platdata; + +if (!ofnode_read_u32_array (fs_loader_node, +"phandlepart" , + phandlepart,2 )) { + plat->phandlepart.phandle = phandlepart[0 ]; + plat->phandlepart.partition = phandlepart[1 ]; + } + + plat->mtdpart = (char *)ofnode_read_string ( + fs_loader_node,"mtdpart" ); + + plat->ubivol = (char *)ofnode_read_string ( + fs_loader_node,"ubivol" ); + } + } + +return 0 ; +} + +static int fs_loader_probe (struct udevice *dev) +{ +return 0 ; +}; + +static const struct udevice_id fs_loader_ids[] = { + { .compatible ="u-boot,fs-loader" }, + { } +}; + +U_BOOT_DRIVER (fs_loader) = { + .name ="fs-loader" , + .id =UCLASS_FS_FIRMWARE_LOADER , + .of_match = fs_loader_ids, + .probe = fs_loader_probe, + .ofdata_to_platdata = fs_loader_ofdata_to_platdata, + .platdata_auto_alloc_size =sizeof (struct device_platdata ), +}; + +UCLASS_DRIVER (fs_loader) = { + .id =UCLASS_FS_FIRMWARE_LOADER , + .name ="fs-loader" , +};include/dm/uclass-id.h +1
-0@@ -36,6 +36,7 @@ enum uclass_id {UCLASS_DMA ,/* Direct Memory Access */ UCLASS_EFI ,/* EFI managed devices */ UCLASS_ETH ,/* Ethernet device */ +UCLASS_FS_FIRMWARE_LOADER ,/* Generic loader */ UCLASS_GPIO ,/* Bank of general-purpose I/O pins */ UCLASS_FIRMWARE ,/* Firmware */ UCLASS_I2C, /* I2C bus */include/fs_loader.h new file +14
-0@@ -0,0 +1,79 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_ + +#include <dm.h> + +/** +* struct firmware - A place for storing firmware and its attribute data. +* +* This holds information about a firmware and its content.
include/fs_loader.h line 24
Quoted 10 lines
include/fs_loader.h new file +10
-0+* +* @size: Size of a file +* @data: Buffer for file +* @priv: Firmware loader private fields +*/ +struct firmware { +size_t size; +const u8 *data; +void *priv; +};
Let's try to get rid of priv by using driver model.
include/fs_loader.h line 35
Quoted 11 lines
include/fs_loader.h new file +11
-0+ +/** +* struct phandle_part - A place for storing phandle of node and its partition +* +* This holds information about a phandle of the block device, and its +* partition where the firmware would be loaded from. +* +* @phandle: Phandle of storage device node +* @partition: Partition of block device +*/ +struct phandle_part {
struct device_part ?
A phandle is a device-tree concept.
include/fs_loader.h line 45
Quoted 10 lines
include/fs_loader.h new file +10
-0+u32 phandle; +u32 partition; +}; + +/** +* struct phandle_part - A place for storing all supported storage devices +* +* This holds information about all supported storage devices for driver use. +* +* @phandlepart: Attribute data for block device.
nit: please drop your periods at the end of these lines - they are not sentences.
Quoted 23 lines
include/fs_loader.h new file +23
-0+* @mtdpart: MTD partition for ubi partition. +* @ubivol: UBI volume-name for ubifsmount. +*/ +struct device_platdata { +struct phandle_part phandlepart; +char *mtdpart; +char *ubivol; +}; + +/** +* release_firmware - Release the resource associated with a firmware image +* @firmware: Firmware resource to release +*/ +void release_firmware (struct firmware *firmware); + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer. +* @plat: Platform data such as storage and partition firmware loading from. +* @name: Name of firmware file. +* @buf: Address of buffer to load firmware into. +* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* @firmwarep: Pointer to firmware image.
include/fs_loader.h line 78
Quoted 10 lines
include/fs_loader.h new file +10
-0+* +* The firmware is loaded directly into the buffer pointed to by @buf and +* the @firmwarep data member is pointed at @buf. +* +* Return: Size of total read, negative value when error. +*/ +int request_firmware_into_buf (struct device_platdata *plat, +const char *name, +void *buf,size_t size,u32 offset, +struct firmware **firmwarep);
Without a test it's hard to see how this is used, but I'm not keen on the struct firmware ** here.
Can you just use struct firmware * instead?
Or maybe just return the fields individually since you only have start address and size, I think.
Quoted 4 lines
include/fs_loader.h new file +1
-1+#endif --2.2.0
Regards,
Simon
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 32 lines · Simon Glass
Hi Tien,
Quoted 29 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.
+
Quoted 8 lines
Quoted 7 lines
+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.*a* file image?
Okay.
Quoted 7 lines
Quoted 6 lines
+
+ The consumer driver would then use this loader to program whatever,
+ ie. the FPGA device.e.g. an FPGA device
Okay.
Quoted 34 lines
Quoted 34 lines
+
endmenudrivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +15
-0@@ -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>
Quoted 8 lines
drivers/misc/fs_loader.c line 21
Quoted 6 lines
drivers/misc/fs_loader.c new file +6
-0+ +DECLARE_GLOBAL_DATA_PTR ; + +struct firmware_priv { + const char *name; /* Filename */ + u32 offset; /* Offset of reading a file */I don't understand that comment. Is it the offset within the file to read from? Please can you expand it a bit?
Sure. This is offset within the file, can be used in limited memory buffer, chunk by chunk transfer from device storage to memory.
Quoted 18 lines
Quoted 18 lines
drivers/misc/fs_loader.c new file +16
-0+}; + +#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 )
Quoted 8 lines
drivers/misc/fs_loader.c line 44
Quoted 7 lines
drivers/misc/fs_loader.c new file +7
-0+{ + return cmd_ubifs_umount(); +} +#else +static int mount_ubifs (char *mtdpart,char *ubivol) +{ + debug("Error: Cannot load image: no UBIFS support\n");blank line here
Okay.
Quoted 15 lines
Quoted 15 lines
drivers/misc/fs_loader.c new file +12
-0+ 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-Quoted 1 line
phandlepart.phandle);
Quoted 9 lines
Quoted 7 lines
drivers/misc/fs_loader.c new file +6
-0+ + int of_offset = ofnode_to_offset(node); + + struct udevice *dev; + + ret = device_get_global_by_of_offset(of_offset,&dev);
Can you please create
device_get_global_by_ofnode()and use that instead? We should not use offsets in new code.
okay.
Quoted 50 lines
Quoted 38 lines
+ if (!ret) {
+ structblk_desc*desc =blk_get_by_device(dev);
+ if (desc) {
+ ret =
fs_set_blk_dev_with_part(desc,
+ plat-Quoted 1 line
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;drivers/misc/fs_loader.c new file +18
-0+ + ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS); + } else { + debug("Error: unsupported storage device.\n"); + return -ENODEV; + } + + if (ret) + debug("Error: could not access storage.\n"); + + return ret; +} + +/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into. drivers/misc/fs_loader.c line 104
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* @firmwarep: Pointer to pointer to firmware image. +* +* Return: Negative value if fail, 0 for successful. +*/ +static int _request_firmware_prepare (const char *name,void *dbuf, + size_t size, u32 offset, + struct firmware **firmwarep) +{struct firmware *firmware = *firmwarep;
Quoted 1 line
and use that instead of the clumsy *firmwarep in this function
Okay.
Quoted 9 lines
Quoted 8 lines
+ if (!name || name[0] == '\0')
+ return-EINVAL;
+
+ /* No memory allocation is required if *firmwarep is allocated */
+ if (!(*firmwarep)) {Can this not be allocated automatically by the uclass or driver?
Okay, i will try it out.
Quoted 13 lines
Quoted 13 lines
+ (*firmwarep) =
calloc(1, sizeof(struct firmware)); + if (!(*firmwarep))
+ return-ENOMEM;
+
+ (*firmwarep)->priv =calloc(1, sizeof(struct firmware_priv));
+ if (!(*firmwarep)->priv) {
+ free(*firmwarep);
+ return-ENOMEM;
+ }
+ } else if (!(*firmwarep)->priv) {
Quoted 3 lines
Can you not use the driver's
priv_auto_alloc_sizeto allocate this? Or
the uclass?
Okay, i will try it out.
Quoted 12 lines
Quoted 12 lines
+ (*firmwarep)->priv =
calloc(1, sizeof(struct firmware_priv));
+ if (!(*firmwarep)->priv) {
+ free(*firmwarep);
+ return-ENOMEM;
+ }
+ }
+
+ ((structfirmware_priv*)((*firmwarep)->priv))->name = name;
Quoted 1 line
Again this needs a local variable.
Why?
Quoted 76 lines
Quoted 76 lines
+ ((struct
firmware_priv*)((*firmwarep)->priv))->offset = offset;
+ (*firmwarep)->data = dbuf;
+ (*firmwarep)->size = size;drivers/misc/fs_loader.c new file +6
-0+ + return 0; +} + +/** +* release_firmware - Release the resource associated with a firmware image
+ * @firmware: Firmware resource to release
+ */drivers/misc/fs_loader.c new file +13
-0+void release_firmware (struct firmware *firmware) +{ + if (firmware) { + if (firmware->priv) { + free(firmware->priv); + firmware->priv = NULL; + } + free(firmware); + } +} + +/** +* fw_get_filesystem_firmware - load firmware into an allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @firmware: pointer to firmware image.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+static int fw_get_filesystem_firmware(structdevice_platdata*plat,
+ struct firmware *firmware) +{
+ structfirmware_priv*fw_priv= NULL;
+ loff_t actread;
+ char*storage_interface,*dev_part,*ubi_mtdpart,*ubi_volume;
+ int ret;drivers/misc/fs_loader.c new file +8
-0+ + storage_interface = env_get("storage_interface"); + dev_part = env_get("fw_dev_part"); + ubi_mtdpart = env_get("fw_ubi_mtdpart"); + ubi_volume = env_get("fw_ubi_volume"); + + if (storage_interface && dev_part) { + ret = fs_set_blk_dev(storage_interface, dev_part,
FS_TYPE_ANY);
+ } else if (storage_interface&&ubi_mtdpart&&ubi_volume) {
+ ret =mount_ubifs(ubi_mtdpart, ubi_volume); + if (ret)
+ return ret;
+
+ if (!strcmp("ubi",storage_interface)) + ret = fs_set_blk_dev(storage_interface, NULL,
+ FS_TYPE_UBIFS);
+ else
+ ret =-ENODEV;
+ } else {
+ ret =select_fs_dev(plat);
+ }drivers/misc/fs_loader.c new file +7
-0+ + if (ret) + goto out; + + fw_priv = firmware->priv; + + ret = fs_read(fw_priv->name, (ulong)map_to_sysmem(firmware-Quoted 1 line
data),
Quoted 14 lines
drivers/misc/fs_loader.c line 203
Quoted 13 lines
+ fw_priv->offset,
firmware->size, &actread); + if (ret) {
+ debug("Error: %d Failed to read %s from flash %lld != %d.\n",
+ ret,fw_priv->name, actread,firmware->size); + } else {
+ ret = actread;
+ }drivers/misc/fs_loader.c new file +5
-0+ +out: +#ifdef CONFIG_CMD_UBIFS + umount_ubifs(); +#endif
debug()here to report failure
Okay.
Quoted 141 lines
Quoted 141 lines
+ return ret;
drivers/misc/fs_loader.c new file +4
-0+} + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ * @firmwarep: Pointer to firmware image.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmwarep data member is pointed at @buf.
+ *
+ * Return: Size of total read, negative value when error.
+ */drivers/misc/fs_loader.c new file +11
-0+int request_firmware_into_buf (struct device_platdata *plat, + const char *name, + void *buf, size_t size, u32 offset, + struct firmware **firmwarep) +{ + int ret; + + if (!plat) + return -EINVAL; + + ret = _request_firmware_prepare(name, buf, size, offset,firmwarep);
+ if (ret < 0) /* error */
+ return ret;drivers/misc/fs_loader.c new file +24
-0+ + ret = fw_get_filesystem_firmware(plat, *firmwarep); + + return ret; +} + +static int fs_loader_ofdata_to_platdata (struct udevice *dev) +{ + const char *fs_loader_path; + u32 phandlepart[2]; + + fs_loader_path = ofnode_get_chosen_prop("firmware-loader"); + + if (fs_loader_path) { + ofnode fs_loader_node; + + fs_loader_node = ofnode_path(fs_loader_path); + if (ofnode_valid(fs_loader_node)) { + struct device_platdata *plat; + plat = dev->platdata; + + if (!ofnode_read_u32_array(fs_loader_node, + "phandlepart", + phandlepart, 2)){
+ plat->phandlepart.phandle = phandlepart[0];
+ plat->phandlepart.partition = phandlepart[1];
+ }
+
+ plat->mtdpart = (char *)ofnode_read_string( + fs_loader_node, "mtdpart");drivers/misc/fs_loader.c new file +26
-0+ + plat->ubivol = (char *)ofnode_read_string( + fs_loader_node, "ubivol"); + } + } + + return 0; +} + +static int fs_loader_probe (struct udevice *dev) +{ + return 0; +}; + +static const struct udevice_id fs_loader_ids[] = { + { .compatible = "u-boot,fs-loader"}, + { } +}; + +U_BOOT_DRIVER (fs_loader) = { + .name = "fs-loader", + .id = UCLASS_FS_FIRMWARE_LOADER, + .of_match = fs_loader_ids, + .probe = fs_loader_probe, + .ofdata_to_platdata = fs_loader_ofdata_to_platdata, + .platdata_auto_alloc_size = sizeof(struct
device_platdata),drivers/misc/fs_loader.c new file +6
-0+}; + +UCLASS_DRIVER (fs_loader) = { + .id = UCLASS_FS_FIRMWARE_LOADER, + .name = "fs-loader", +};include/dm/uclass-id.h +0
-0@@ -36,6 +36,7 @@ enum uclass_id {UCLASS_DMA, /* Direct Memory Access */ UCLASS_EFI, /* EFI managed devices */ UCLASS_ETH, /* Ethernet device */
+ UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */
UCLASS_GPIO, /* Bank of general-purpose I/O pins */
UCLASS_FIRMWARE, /* Firmware */ UCLASS_I2C, /* I2C bus */include/fs_loader.h new file +12
-0@@ -0,0 +1,79 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_ + +#include <dm.h> + +/** +* struct firmware - A place for storing firmware and its attribute data.
+ *
+ * This holds information about a firmware and its content.
+ *
+ * @size: Size of a file
+ * @data: Buffer for file
Quoted 8 lines
include/fs_loader.h line 24
Quoted 7 lines
+ * @priv: Firmware loader private fields
+ */include/fs_loader.h new file +5
-0+struct firmware { + size_t size; + const u8 *data; + void *priv; +};Let's try to get rid of priv by using driver model.
Okay.
Quoted 17 lines
Quoted 7 lines
include/fs_loader.h new file +3
-0+ +/** +* struct phandle_part - A place for storing phandle of node and its partition
+ *Quoted 8 lines
+ * This holds information about a phandle of the block device, and its
+ * partition where the firmware would be loaded from.
+ *
+ * @phandle: Phandle of storage device node
+ * @partition: Partition of block device
+ */
+structphandle_part{struct
device_part?
Quoted 1 line
A phandle is a device-tree concept.
Okay.
Quoted 18 lines
Quoted 8 lines
+ u32 phandle;
+ u32 partition;include/fs_loader.h new file +4
-0+}; + +/** +* struct phandle_part - A place for storing all supported storage Quoted 6 lines
devices
+ *
+ * This holds information about all supported storage devices for driver use.
+ *
+ * @phandlepart: Attribute data for block device.nit: please drop your periods at the end of these lines - they are not
sentences.
Okay.
Quoted 41 lines
Quoted 30 lines
+ * @mtdpart: MTD partition for ubi partition.
+ * @ubivol: UBI volume-name for ubifsmount.
+ */include/fs_loader.h new file +8
-0+struct device_platdata { + struct phandle_part phandlepart; + char *mtdpart; + char *ubivol; +}; + +/** +* release_firmware - Release the resource associated with a firmware image
+ * @firmware: Firmware resource to release
+ */include/fs_loader.h new file +4
-0+void release_firmware (struct firmware *firmware); + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ * @firmwarep: Pointer to firmware image.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf andQuoted 8 lines
+ * the @firmwarep data member is pointed at @buf.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+int request_firmware_into_buf(structdevice_platdata*plat, + const char *name,
+ void *buf,size_tsize, u32 offset, + struct firmware **firmwarep);Without a test it's hard to see how this is used, but I'm not keen on the struct firmware ** here.
Quoted 5 lines
Can you just use struct firmware * instead?
Or maybe just return the fields individually since you only have start
address and size, I think.
I can try to remove struct firmware, let driver model handles all memory allocation, and then struct device_platdata *plat will change to struct udevice *dev. So, it would become like this
int request_firmware_into_buf(struct udevice *dev,
const char *name, void *buf, size_t size, u32 offset);
I will remove release_firmware() after letting driver model to handle all memory deallocation.
So, the API interface would looks a bit different compare to Linux firmware API interface. Does this change OK for you?
Quoted 8 lines
Quoted 6 lines
+#endif
--
2.2.0Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
Hi Tien Fong,
Quoted 24 lines · Tien Fong
Quoted 23 lines · Simon Glass
Hi Tien,
Quoted 20 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>--- 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
[..]
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
+ (*firmwarep)->priv = calloc(1, sizeof(struct
firmware_priv));drivers/misc/fs_loader.c new file +7
-0+if (!(*firmwarep)->priv) { +free (*firmwarep); +return -ENOMEM ; + } + } + + ((struct firmware_priv *)((*firmwarep)->priv))->name =name;
Quoted 2 lines
Quoted 1 line
Again this needs a local variable.
Why?
Because these casts are really ugly and repetitive, particularly when used for assignments :-)
[..]
Quoted 35 lines
Quoted 30 lines
Quoted 23 lines
+ *
release_firmware- Release the resource associated with a firmware image
+ * @firmware: Firmware resource to release
+ */include/fs_loader.h new file +4
-0+void release_firmware (struct firmware *firmware); + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ * @firmwarep: Pointer to firmware image.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmwarep data member is pointed at @buf.
+ *
+ * Return: Size of total read, negative value when error.
+ */include/fs_loader.h line 78
Quoted 4 lines
include/fs_loader.h new file +4
-0+int request_firmware_into_buf (struct device_platdata *plat, +const char *name, +void *buf,size_t size,u32 offset, +struct firmware **firmwarep);Without a test it's hard to see how this is used, but I'm not keen on the struct firmware ** here.
Quoted 5 lines
Can you just use struct firmware * instead?
Or maybe just return the fields individually since you only have start
address and size, I think.
Quoted 10 lines
I can try to remove struct firmware, let driver model handles all memory allocation, and then struct
device_platdata*plat will change to struct udevice *dev. So, it would become like this
int request_firmware_into_buf(struct udevice *dev,
const char *name,
void *buf,size_tsize, u32 offset);
I will removerelease_firmware()after letting driver model to handle all memory deallocation.
So, the API interface would looks a bit different compare to Linux firmware API interface. Does this change OK for you?
I believe you would need:
Quoted 3 lines
int request_firmware_into_buf(struct udevice *dev,
const char *name,
void **bufp,size_t*sizep, u32 offset);
since you need to return the buffer and size?
What exactly is 'dev'? Is it the device in the FS_LOADER uclass?
Regards,
Simon
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 49 lines · Simon Glass
Hi Tien Fong,
Quoted 28 lines · Tien Fong
Quoted 26 lines · Simon Glass
Hi Tien,
Quoted 22 lines · On 6 July 2018 at 02:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.h[..]
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
+ (*firmwarep)->priv =
calloc(1, sizeof(struct firmware_priv));
+ if (!(*firmwarep)->priv) {
+ free(*firmwarep);
+ return-ENOMEM;
+ }
+ }
+
+ ((structfirmware_priv*)((*firmwarep)->priv))->name = name;
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
Again this needs a local variable.
Why?
Because these casts are really ugly and repetitive, particularly when used for assignments :-)
Okay.
Quoted 56 lines
[..]
Quoted 41 lines
Quoted 36 lines
Quoted 24 lines
+ *
release_firmware- Release the resource associated with a firmware image
+ * @firmware: Firmware resource to release
+ */include/fs_loader.h new file +4
-0+void release_firmware (struct firmware *firmware); + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ * @firmwarep: Pointer to firmware image.
+ *
+ * The firmware is loaded directly into the buffer pointed to by
@buf and
+ * the @firmwarep data member is pointed at @buf.Quoted 8 lines
+ *
+ * Return: Size of total read, negative value when error.
+ */
+int request_firmware_into_buf(structdevice_platdata*plat, + const char *name,
+ void *buf,size_tsize, u32 offset,
+ struct firmware **firmwarep);Without a test it's hard to see how this is used, but I'm not keen on
the struct firmware ** here.Quoted 5 lines
Can you just use struct firmware * instead?
Or maybe just return the fields individually since you only have start
address and size, I think.Quoted 12 lines
I can try to remove struct firmware, let driver model handles all memory allocation, and then struct
device_platdata*plat will change
to struct udevice *dev. So, it would become like this int request_firmware_into_buf(struct udevice *dev, const char *name,
void *buf,size_tsize, u32 offset); I will removerelease_firmware()after letting driver model to handle
all memory deallocation.
So, the API interface would looks a bit different compare to Linux firmware API interface. Does this change OK for you?
Quoted 8 lines
I believe you would need:
Quoted 5 lines
int
request_firmware_into_buf(struct udevice *dev, const char *name, void **bufp, size_t *sizep, u32 offset);since you need to return the buffer and size?
Why need to return the buffer and size? There is no modification required on noth buffer and size arguments by request_firmware_into_buf().
Both buffer and size are allocated and defined by user, then request_firmware_into_buf() would load the file into buffer based on the size exactly defined by user.
Quoted 2 lines
What exactly is 'dev'? Is it the device in the
FS_LOADERuclass?
Yes, contains FDT HW data.
Quoted 3 lines
Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
In reply to tien.fong.chee at intel.comQuoted 120 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>--- 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.hdrivers/misc/Kconfig +10
-0@@ -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. + endmenudrivers/misc/Makefile +1
-0@@ -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.odrivers/misc/fs_loader.c new file +68
-0@@ -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 {
drivers/misc/fs_loader.c line 78
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+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.
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.
Also I can't see any usage of firmware_loader from chosen which you have in 4/6.
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.
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.
Thanks,
Michal
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 130 lines · Michal Simek
Quoted 129 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.drivers/misc/Kconfig +5
-0+ +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be used toload
drivers/misc/Kconfig +1
-0+ the file image from the storage into target such asmemory.
drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loader toprogram whatever,
drivers/misc/Kconfig +2
-0+ ie. the FPGA device. +endmenu
drivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +56
-0@@ -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-Quoted 1 line
phandlepart.phandle);
drivers/misc/fs_loader.c new file +6
-0+ +int of_offset =ofnode_to_offset (node); + +struct udevice *dev; + + ret = device_get_global_by_of_offset(of_offset,&dev);
drivers/misc/fs_loader.c new file +2
-0+if (!ret) { + struct blk_desc *desc =
blk_get_by_device(dev);drivers/misc/fs_loader.c new file +2
-0+if (desc) { + ret =fs_set_blk_dev_with_part(desc,
drivers/misc/fs_loader.c new file +1
-0+ plat-Quoted 1 line
phandlepart.partition);
Quoted 14 lines
drivers/misc/fs_loader.c line 78
Quoted 12 lines
drivers/misc/fs_loader.c new file +2
-0+ }else { + debug("%s: No device found\n",
__func__);drivers/misc/fs_loader.c new file +9
-0+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.
Quoted 5 lines
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.
Quoted 4 lines
Also I can't see any usage of
firmware_loaderfrom 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.
Quoted 5 lines
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.
Quoted 6 lines
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?
Quoted 3 lines
Thanks,
Michal
PATCH v4 6/6 common: Generic loader for file system
Quoted 145 lines · Tien Fong
Quoted 130 lines · Michal Simek
Quoted 129 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.drivers/misc/Kconfig +5
-0+ +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be used toload
drivers/misc/Kconfig +1
-0+ the file image from the storage into target such asmemory.
drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loader toprogram whatever,
drivers/misc/Kconfig +2
-0+ ie. the FPGA device. +endmenu
drivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +56
-0@@ -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-Quoted 1 line
phandlepart.phandle);
drivers/misc/fs_loader.c new file +6
-0+ +int of_offset =ofnode_to_offset (node); + +struct udevice *dev; + + ret = device_get_global_by_of_offset(of_offset,&dev);
drivers/misc/fs_loader.c new file +2
-0+if (!ret) { + struct blk_desc *desc =
blk_get_by_device(dev);drivers/misc/fs_loader.c new file +2
-0+if (desc) { + ret =fs_set_blk_dev_with_part(desc,
drivers/misc/fs_loader.c new file +1
-0+ plat-Quoted 1 line
phandlepart.partition);
Quoted 14 lines
drivers/misc/fs_loader.c line 78
Quoted 12 lines
drivers/misc/fs_loader.c new file +2
-0+ }else { + debug("%s: No device found\n",
__func__);drivers/misc/fs_loader.c new file +9
-0+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.
Quoted 3 lines
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.
Quoted 9 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 4 lines
Also I can't see any usage of
firmware_loaderfrom 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_toplatdata()is used to parse the configuration in chosen.Quoted 1 line
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
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.
Quoted 6 lines
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
PATCH v4 6/6 common: Generic loader for file system
Hi,
Quoted 7 lines · Michal Simek
Quoted 5 lines · Tien Fong
Quoted 4 lines · Michal Simek
Quoted 3 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
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.
Quoted 8 lines
It is software configuration in file which should mostly describe hardware and state for hardware configuration.
Your
fs_loadernode 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.
I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get its firmware from somewhere. We use the chosen node to specify the UART to use, and this is no different. Again, we don't have user-space config files in U-Boot.
This argument comes up from time to time and I'd really like to put it to bed for U-Boot. I understand that Linux has its own approach and rules, but in some cases they serve U-Boot poorly.
Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
Quoted 43 lines · Simon Glass
Hi,
Quoted 7 lines · Michal Simek
Quoted 5 lines · Tien Fong
Quoted 4 lines · Michal Simek
Quoted 3 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
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.
Quoted 8 lines
It is software configuration in file which should mostly describe hardware and state for hardware configuration.
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get its firmware from somewhere. We use the chosen node to specify the UART to use, and this is no different. Again, we don't have user-space config files in U-Boot.
Quoted 3 lines
This argument comes up from time to time and I'd really like to put it to bed for U-Boot. I understand that Linux has its own approach and rules, but in some cases they serve U-Boot poorly.
I want to second this as well. So long as we're using our prefix and we've thought through and discussed what we're trying to do here, it's OK to do things that might not be accepted for Linux.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180725/64974b81/attachment.sig>
PATCH v4 6/6 common: Generic loader for file system
Quoted 44 lines · Tom Rini
Quoted 43 lines · Simon Glass
Hi,
Quoted 7 lines · Michal Simek
Quoted 5 lines · Tien Fong
Quoted 4 lines · Michal Simek
Quoted 3 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
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.
Quoted 8 lines
It is software configuration in file which should mostly describe hardware and state for hardware configuration.
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get its firmware from somewhere. We use the chosen node to specify the UART to use, and this is no different. Again, we don't have user-space config files in U-Boot.
Quoted 8 lines
Quoted 3 lines
This argument comes up from time to time and I'd really like to put it to bed for U-Boot. I understand that Linux has its own approach and rules, but in some cases they serve U-Boot poorly.
I want to second this as well. So long as we're using our prefix and we've thought through and discussed what we're trying to do here, it's OK to do things that might not be accepted for Linux.
I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current solution has also problem with ordering).
In this loader case doc is saying that you can rewrite it with variables on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb which setup u-boot this can be problematic and using DT is could be considered as easier for use.
In this case this is what was proposed:
+ fs_loader0: fs-loader at 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ phandlepart = <&mmc 1>;
+ };
+ fs_loader1: fs-loader at 1 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ mtdpart = "UBI",
+ ubivol = "ubi0";
+ };
u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup for this driver - it means this should be here.
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why this is only used for filling platdata but driver has no user that's why this is unused till someone calls that functions.
phandlepart/mtdpart/ubivol is just for setup.
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;
And for UBIfs. I have never played with that but I expect it shouldn't be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;
Then this driver/interface can stay in DT where it should stay. The only thing is how this should be initialized because there is no compatible string. But you can do that via platdata for platforms which want to use this.
Thanks,
Michal
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };
Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.
You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.
Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;
Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing, fs_loader can act the pointer to the chosen.
Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
Quoted 12 lines
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.Thanks,
Michal
PATCH v4 6/6 common: Generic loader for file system
Quoted 132 lines · Tien Fong
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;
Quoted 8 lines
Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing,
fs_loadercan act the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
The driver is still using DM. It is only about way how driver is binded. Take a look for example on board/hisilicon/hikey/hikey.c
Thanks,
Michal
PATCH v4 6/6 common: Generic loader for file system
In reply to CheeHi,
Quoted 133 lines · Tien Fong
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;
Quoted 16 lines
Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing,
fs_loadercan act the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
Quoted 8 lines
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.
We should add a compatible string then :-)
Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
Quoted 153 lines · Simon Glass
Hi,
Quoted 133 lines · Tien Fong
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;Quoted 16 lines
Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing,
fs_loadercan act the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
Quoted 8 lines
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
M
PATCH v4 6/6 common: Generic loader for file system
Hi Michal,
Quoted 154 lines · Michal Simek
Quoted 153 lines · Simon Glass
Hi,
Quoted 149 lines · Tien Fong
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing,
fs_loadercan act the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
Quoted 8 lines
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.
Quoted 3 lines
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be bound automatically. Manually binding a device is really just a fallback for particular situations (e.g. buses like PCI where we often rely on probing to find out what is on the bus).
Regards,
Simon
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 219 lines · Simon Glass
Hi Michal,
Quoted 215 lines · Michal Simek
Quoted 213 lines · Simon Glass
Hi,
Quoted 208 lines · Tien Fong
Quoted 146 lines · Michal Simek
Quoted 101 lines · Tom Rini
Quoted 90 lines · Simon Glass
Hi,
Quoted 16 lines · Michal Simek
Quoted 12 lines · Tien Fong
Quoted 8 lines · Michal Simek
Quoted 5 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 31 lines
Quoted 16 lines
Quoted 7 lines
Quoted 9 lines
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.Quoted 10 lines
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.Quoted 11 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the
'chosen' node is a good place to configure these common features.While you can argue that the partition or filesystem where an
image
comes from is a software config, it is something that has to be
configured. It has impact on hardware too, since the FPGA has to
get
its firmware from somewhere. We use the chosen node to specify
the
UART to use, and this is no different. Again, we don't have user-
space
config files in U-Boot.Quoted 7 lines
This argument comes up from time to time and I'd really like to
put it
to bed for U-Boot. I understand that Linux has its own approach
and
rules, but in some cases they serve U-Boot poorly.Quoted 8 lines
I want to second this as well. So long as we're using our prefix
and
we've thought through and discussed what we're trying to do here,
it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix
properties and my colleague hopefully with finish work about moving
u-boot,dm-pre-reloc; to chosen node where it should be (because
current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with
variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct
dtb
which setup u-boot this can be problematic and using DT is could be
considered as easier for use.In this case this is what was proposed:
+ fs_loader0: fs-loader at 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ phandlepart = <&mmc 1>;
+ };+ fs_loader1: fs-loader at 1 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ mtdpart = "UBI",
+ ubivol = "ubi0";
+ };Quoted 4 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup
for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user
to call any loader with default storage through the sequence number if
fs loader is not defined in chosen. For example, there is a case where
system loading the file from SDMMC, NAND and QSPI.Quoted 11 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's
why
this is only used for filling platdata but driver has no user that's
why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 10 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like)
u-boot,fs-loader = <0 ubi0>;Need consider description for UBIFS, using fs-loader seems not working
for UBIFS, since more arguments such as mtdpartition and mtd volume
need passing into driver. In order to avoid messing,fs_loadercan act
the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or
driver model if we can resolve the problem for UBIFS and maintainers
agree with it.Quoted 11 lines
Then this driver/interface can stay in DT where it should stay. The
only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to
use
this.
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be bound automatically. Manually binding a device is really just a fallback for particular situations (e.g. buses like PCI where we often
rely on probing to find out what is on the bus).
So, is this still the same with current implementation?
/ {
chosen {
firmware-loader = &fs_loader0;
};
fs_loader0: fs-loader at 0 {
u-boot,dm-pre-reloc;
compatible = "u-boot,fs-loader";
source-partition = <&mmc 1>;
};
};Quoted 3 lines
Regards,
Simon
PATCH v4 6/6 common: Generic loader for file system
In reply to Simon GlassQuoted 157 lines · Simon Glass
Hi Michal,
Quoted 154 lines · Michal Simek
Quoted 153 lines · Simon Glass
Hi,
Quoted 149 lines · Tien Fong
Quoted 105 lines · Michal Simek
Quoted 69 lines · Tom Rini
Quoted 62 lines · Simon Glass
Hi,
Quoted 11 lines · Michal Simek
Quoted 8 lines · Tien Fong
Quoted 6 lines · Michal Simek
Quoted 4 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
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.Quoted 6 lines
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.Quoted 8 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the 'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user- space
config files in U-Boot.Quoted 5 lines
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.Quoted 6 lines
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix properties and my colleague hopefully with finish work about moving u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be considered as easier for use.In this case this is what was proposed:
(quoted diff) +11
-0+ fs_loader0: fs-loader at 0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; + fs_loader1: fs-loader at 1 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + mtdpart = "UBI", + ubivol = "ubi0"; + };Quoted 3 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user to call any loader with default storage through the sequence number if fs loader is not defined in chosen. For example, there is a case where system loading the file from SDMMC, NAND and QSPI.
Quoted 8 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 8 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like) u-boot,fs-loader = <0 ubi0>;Need consider description for UBIFS, using fs-loader seems not working for UBIFS, since more arguments such as mtdpartition and mtd volume need passing into driver. In order to avoid messing,
fs_loadercan act the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or driver model if we can resolve the problem for UBIFS and maintainers agree with it.
Quoted 8 lines
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be bound automatically. Manually binding a device is really just a fallback for particular situations (e.g. buses like PCI where we often rely on probing to find out what is on the bus).
up2you guys. I just have different opinion how this should be done.
Thanks,
Michal
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 230 lines · Michal Simek
Quoted 222 lines · Simon Glass
Hi Michal,
Quoted 217 lines · Michal Simek
Quoted 215 lines · Simon Glass
Hi,
Quoted 210 lines · Tien Fong
Quoted 147 lines · Michal Simek
Quoted 102 lines · Tom Rini
Quoted 91 lines · Simon Glass
Hi,
Quoted 17 lines · Michal Simek
Quoted 13 lines · Tien Fong
Quoted 9 lines · Michal Simek
Quoted 5 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
[...]
Quoted 31 lines
Quoted 16 lines
Quoted 7 lines
Quoted 9 lines
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.Quoted 10 lines
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.Quoted 11 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the
'chosen' node is a good place to configure these common features.While you can argue that the partition or filesystem where an
image
comes from is a software config, it is something that has to be
configured. It has impact on hardware too, since the FPGA has to
get
its firmware from somewhere. We use the chosen node to specify
the
UART to use, and this is no different. Again, we don't have user-
space
config files in U-Boot.Quoted 7 lines
This argument comes up from time to time and I'd really like to
put it
to bed for U-Boot. I understand that Linux has its own approach
and
rules, but in some cases they serve U-Boot poorly.Quoted 8 lines
I want to second this as well. So long as we're using our prefix
and
we've thought through and discussed what we're trying to do here,
it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix
properties and my colleague hopefully with finish work about moving
u-boot,dm-pre-reloc; to chosen node where it should be (because
current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with
variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct
dtb
which setup u-boot this can be problematic and using DT is could be
considered as easier for use.In this case this is what was proposed:
+ fs_loader0: fs-loader at 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ phandlepart = <&mmc 1>;
+ };+ fs_loader1: fs-loader at 1 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ mtdpart = "UBI",
+ ubivol = "ubi0";
+ };Quoted 4 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup
for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user
to call any loader with default storage through the sequence number if
fs loader is not defined in chosen. For example, there is a case where
system loading the file from SDMMC, NAND and QSPI.Quoted 11 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty that's
why
this is only used for filling platdata but driver has no user that's
why
this is unused till someone calls that functions.phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 11 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;And for UBIfs. I have never played with that but I expect it
shouldn't
be big problem to describe it differently too (something like)
u-boot,fs-loader = <0 ubi0>;Need consider description for UBIFS, using fs-loader seems not working
for UBIFS, since more arguments such as mtdpartition and mtd volume
need passing into driver. In order to avoid messing,fs_loadercan act
the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or
driver model if we can resolve the problem for UBIFS and maintainers
agree with it.Quoted 11 lines
Then this driver/interface can stay in DT where it should stay. The
only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to
use
this.Quoted 7 lines
Quoted 2 lines
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be bound automatically. Manually binding a device is really just a fallback for particular situations (e.g. buses like PCI where we often
rely on probing to find out what is on the bus).
Quoted 4 lines
up2you guys. I just have different opinion how this should be done.
Thanks,
Michal
Sorry for late reply, because rushing the project.
So, are you guys OK with current implementation because we have no solution for the UBIFS setting with other solution than using DT?
Quoted 2 lines
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 1 line · Tien Fong
Quoted 1 line
If everybody agree with current framework, then the next version i will include the fixes:
1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the right fs_loader if the chosen node u-boot, fs-loader is not defined.
2. Let driver model handles all memory allocation
3. Using local variable instead of messy casts.
Thanks.
Quoted 275 lines · Michal Simek
Quoted 266 lines · Simon Glass
Quoted 257 lines
Hi Michal,
Quoted 251 lines · Michal Simek
Quoted 248 lines · Simon Glass
Hi,
Quoted 241 lines · Tien Fong
Quoted 170 lines · Michal Simek
Quoted 122 lines · Tom Rini
Quoted 108 lines · Simon Glass
Hi,
On 25 July 2018 at 03:48, Michal Simek <michal.simek@ xi
linx.com>Quoted 22 lines · wrote:
Quoted 17 lines · Tien Fong
Quoted 12 lines · Michal Simek
Quoted 7 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]
m>[...]
Quoted 35 lines
Quoted 19 lines
Quoted 10 lines
Quoted 9 lines
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.Quoted 11 lines
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.Quoted 13 lines
Your
fs_loadernode 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.I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy
decisions,
and the
'chosen' node is a good place to configure these
common
features.While you can argue that the partition or filesystem where an
image
comes from is a software config, it is something that has to be
configured. It has impact on hardware too, since the FPGA has to
get
its firmware from somewhere. We use the chosen node to
specify
the
UART to use, and this is no different. Again, we don't
have user-
space
config files in U-Boot.Quoted 9 lines
This argument comes up from time to time and I'd really
like to
put it
to bed for U-Boot. I understand that Linux has its own
approach
and
rules, but in some cases they serve U-Boot poorly.Quoted 9 lines
I want to second this as well. So long as we're using our prefix
and
we've thought through and discussed what we're trying to
do here,
it's
OK to do things that might not be accepted for Linux.I have not a problem with using chosen node with u-boot prefix
properties and my colleague hopefully with finish work about moving
u-boot,dm-pre-reloc; to chosen node where it should be (because
current
solution has also problem with ordering).In this loader case doc is saying that you can rewrite it with
variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct
dtb
which setup u-boot this can be problematic and using DT is
could be
considered as easier for use.In this case this is what was proposed:
+ fs_loader0: fs-loader at 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ phandlepart = <&mmc 1>;
+ };+ fs_loader1: fs-loader at 1 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ mtdpart = "UBI",
+ ubivol = "ubi0";
+ };Quoted 4 lines
u-boot,dm-pre-reloc; requires
DM_FLAG_PRE_RELOCwhich is not setup
for
this driver - it means this should be here.You are right, i missed this one. The intention of design enables user
to call any loader with default storage through the sequence
number if
fs loader is not defined in chosen. For example, there is a case where
system loading the file from SDMMC, NAND and QSPI.Quoted 12 lines
compatible = "u-boot,fs-loader"; - bind and probe are empty
that's
why
this is only used for filling platdata but driver has no user that's
why
this is unused till someone calls that functions.Quoted 1 line
phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.Quoted 6 lines
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;Quoted 6 lines
And for UBIfs. I have never played with that but I expect it
shouldn't
be big problem to describe it differently too (something like)
u-boot,fs-loader = <0 ubi0>;Need consider description for UBIFS, using fs-loader seems not working
for UBIFS, since more arguments such as mtdpartition and mtd
volume
need passing into driver. In order to avoid messing,fs_loadercan act
the pointer to the chosen.Anyway, i have no strong opinion with driver designed via platdata or
driver model if we can resolve the problem for UBIFS and maintainers
agree with it.Quoted 13 lines
Then this driver/interface can stay in DT where it should stay. The
only
thing is how this should be initialized because there is no
compatible
string. But you can do that via platdata for platforms which want to
use
this.Quoted 7 lines
Quoted 2 lines
Quoted 1 line
We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be bound automatically. Manually binding a device is really just a fallback for particular situations (e.g. buses like PCI where we often
rely on probing to find out what is on the bus).Quoted 4 lines
up2you guys. I just have different opinion how this should be done.
Thanks,
MichalSorry for late reply, because rushing the project.
So, are you guys OK with current implementation because we have no solution for the UBIFS setting with other solution than using DT?Quoted 1 line
PATCH v4 6/6 common: Generic loader for file system
Quoted 10 lines · Tien Fong
Quoted 1 line · Tien Fong
Quoted 1 line
If everybody agree with current framework, then the next version i will include the fixes:
1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the rightfs_loaderif the chosen node u-boot, fs-loader is not defined.2. Let driver model handles all memory allocation
3. Using local variable instead of messy casts.
A new version of this series, or a follow up series? Thanks again!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180925/9f94fa50/attachment.sig>
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 14 lines · Tom Rini
Quoted 12 lines · Tien Fong
Quoted 3 lines · Tien Fong
Quoted 3 lines
If everybody agree with current framework, then the next version i will
include the fixes:
1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the rightfs_loaderif the chosen node u-boot, fs-loader is not defined.
Quoted 5 lines
Quoted 3 lines
2. Let driver model handles all memory allocation
3. Using local variable instead of messy casts.
A new version of this series, or a follow up series? Thanks again!
This changes will be in follow up series to address feedback raised by Simon and michal on v4.
PATCH v4 6/6 common: Generic loader for file system
In reply to CheeHi,
Quoted 10 lines · Tien Fong
Quoted 1 line · Tien Fong
Quoted 1 line
If everybody agree with current framework, then the next version i will include the fixes:
1. AddingDM_FLAG_PRE_RELOC, this would allow runtime to choose the rightfs_loaderif the chosen node u-boot, fs-loader is not defined.2. Let driver model handles all memory allocation
3. Using local variable instead of messy casts.
Sounds good.
Can you please give an example of this? If you just mean within a function, that's fine. We try to avoid static variables with driver model.
Regards,
Simon
Chee
PATCH v4 6/6 common: Generic loader for file system
In reply to Michal SimekQuoted 175 lines · Michal Simek
Quoted 159 lines · Tien Fong
Quoted 149 lines · Michal Simek
Quoted 147 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.drivers/misc/Kconfig +5
-0+ +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be usedto
loaddrivers/misc/Kconfig +1
-0+ the file image from the storage into target such asmemory.
drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loader toprogram whatever,
drivers/misc/Kconfig +2
-0+ ie. the FPGA device. +endmenu
drivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +21
-0@@ -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 */
drivers/misc/fs_loader.c new file +9
-0+}; + +#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);
drivers/misc/fs_loader.c new file +26
-0+ 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-Quoted 2 lines
phandlepart.phandle);
drivers/misc/fs_loader.c new file +6
-0+ +int of_offset =ofnode_to_offset (node); + +struct udevice *dev; + + ret =device_get_global_by_of_offset(of_offset,
&dev);drivers/misc/fs_loader.c new file +2
-0+if (!ret) { + struct blk_desc *desc =
blk_get_by_device(dev);drivers/misc/fs_loader.c new file +2
-0+if (desc) { + ret =fs_set_blk_dev_with_part(desc,
drivers/misc/fs_loader.c new file +1
-0+ plat-Quoted 2 lines
phandlepart.partition);
drivers/misc/fs_loader.c new file +2
-0+ }else { + debug("%s: No device found\n",
__func__);drivers/misc/fs_loader.c new file +5
-0+return -ENODEV ; + } + } + }else if (plat->mtdpart && plat->ubivol) { + ret = mount_ubifs(plat->mtdpart, plat-Quoted 1 line
ubivol);
Quoted 8 lines
Quoted 5 lines
drivers/misc/fs_loader.c new file +4
-0+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.Quoted 5 lines
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_GENERICoption 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.
Quoted 3 lines
I didn't work with UBIFS but it is supported in
fs.cthat'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().
Regards,
TF
PATCH v4 6/6 common: Generic loader for file system
Quoted 176 lines · Tien Fong
Quoted 175 lines · Michal Simek
Quoted 159 lines · Tien Fong
Quoted 149 lines · Michal Simek
Quoted 147 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.drivers/misc/Kconfig +5
-0+ +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be usedto
loaddrivers/misc/Kconfig +1
-0+ the file image from the storage into target such asmemory.
drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loader toprogram whatever,
drivers/misc/Kconfig +2
-0+ ie. the FPGA device. +endmenu
drivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +21
-0@@ -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 */
drivers/misc/fs_loader.c new file +9
-0+}; + +#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);
drivers/misc/fs_loader.c new file +26
-0+ 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-Quoted 2 lines
phandlepart.phandle);
drivers/misc/fs_loader.c new file +6
-0+ +int of_offset =ofnode_to_offset (node); + +struct udevice *dev; + + ret =device_get_global_by_of_offset(of_offset,
&dev);drivers/misc/fs_loader.c new file +2
-0+if (!ret) { + struct blk_desc *desc =
blk_get_by_device(dev);drivers/misc/fs_loader.c new file +2
-0+if (desc) { + ret =fs_set_blk_dev_with_part(desc,
drivers/misc/fs_loader.c new file +1
-0+ plat-Quoted 2 lines
phandlepart.partition);
drivers/misc/fs_loader.c new file +2
-0+ }else { + debug("%s: No device found\n",
__func__);drivers/misc/fs_loader.c new file +5
-0+return -ENODEV ; + } + } + }else if (plat->mtdpart && plat->ubivol) { + ret = mount_ubifs(plat->mtdpart, plat-Quoted 1 line
ubivol);
Quoted 8 lines
Quoted 5 lines
drivers/misc/fs_loader.c new file +4
-0+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.Quoted 5 lines
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_GENERICoption 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.
Quoted 5 lines
Quoted 3 lines
I didn't work with UBIFS but it is supported in
fs.cthat's why I would
expect that this shouldn't be a problem to get it workIf
fs_set_blk_devis 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
Chee
PATCH v4 6/6 common: Generic loader for file system
Quoted 197 lines · Michal Simek
Quoted 196 lines · Tien Fong
Quoted 194 lines · Michal Simek
Quoted 173 lines · Tien Fong
Quoted 161 lines · Michal Simek
Quoted 158 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>---
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 100644include/fs_loader.hdrivers/misc/Kconfig +0
-0@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRLdepends on MISC
help
Support gdsys FPGA's RXAUI control.drivers/misc/Kconfig +5
-0+ +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can beused
to
loaddrivers/misc/Kconfig +1
-0+ the file image from the storage into target suchas
memory.drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loaderto
program whatever,drivers/misc/Kconfig +2
-0+ ie. the FPGA device. +endmenu
drivers/misc/Makefile +0
-0@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.oobj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
drivers/misc/Makefile +1
-0+obj-$(CONFIG_FS_LOADER) += fs_loader.odrivers/misc/fs_loader.c new file +21
-0@@ -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
*/drivers/misc/fs_loader.c new file +9
-0+}; + +#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);
drivers/misc/fs_loader.c new file +14
-0+ 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 UBIFSsupport\n");
drivers/misc/fs_loader.c new file +12
-0+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-Quoted 3 lines
phandlepart.phandle);
drivers/misc/fs_loader.c new file +6
-0+ +int of_offset =ofnode_to_offset (node); + +struct udevice *dev; + + ret =device_get_global_by_of_offset(of_offset,
&dev);drivers/misc/fs_loader.c new file +2
-0+if (!ret) { + struct blk_desc *desc =
blk_get_by_device(dev);drivers/misc/fs_loader.c new file +2
-0+if (desc) { + ret =fs_set_blk_dev_with_part(desc,
drivers/misc/fs_loader.c new file +1
-0+ plat-Quoted 3 lines
phandlepart.partition);
drivers/misc/fs_loader.c new file +2
-0+ }else { + debug("%s: No devicefound\n",
__func__);drivers/misc/fs_loader.c new file +5
-0+return -ENODEV ; + } + } + }else if (plat->mtdpart && plat->ubivol) { + ret = mount_ubifs(plat->mtdpart, plat-Quoted 2 lines
ubivol);
Quoted 9 lines
Quoted 5 lines
drivers/misc/fs_loader.c new file +4
-0+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.Quoted 6 lines
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_GENERICoption 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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
I didn't work with UBIFS but it is supported in
fs.cthat's why I would
expect that this shouldn't be a problem to get it workIf
fs_set_blk_devis 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.
Quoted 5 lines
Thanks,
Michal
PATCH v4 6/6 common: Generic loader for file system
In reply to tien.fong.chee at intel.comQuoted 8 lines · tien.fong.chee at intel.com
From: Tien Fong Chee <[email protected]>
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 <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180929/f5c7af28/attachment.sig>