Generic file system firmware loader DM
v3 cover Generic file system firmware loader DM
From: Tien Fong Chee <[email protected]>
This patchset contains generic file system loader DM which is very close to Linux firmware loader but for U-Boot framework. Generic file system firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Simon and Marek in [v2].
Patch set for sandbox will be sent out separately.
This series is working on top of u-boot-socfpga.git -
http://git.denx.de/u-boot.git .
[v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg286979.html [v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg286294.html
v2 -> v3 changes
----------------
- Node for this driver go in /chosen
- Update firmware doc and device tree binding doc.
Tien Fong Chee (3):
doc: Add new doc for file system firmware loader driver model
doc: dtbinding: Add file system firmware loader binding document
common: Generic loader for file system
doc/device-tree-bindings/chosen.txt | 22 +++ doc/device-tree-bindings/misc/fs_loader.txt | 52 ++++++ doc/driver-model/fs_firmware_loader.txt | 129 +++++++++++++++ drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 238 ++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fs_loader.h | 28 ++++ 8 files changed, 481 insertions(+) create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt create mode 100644 doc/driver-model/fs_firmware_loader.txt create mode 100644 drivers/misc/fs_loader.c create mode 100644 include/fs_loader.h
Signature · 1 line
2.2.0
PATCH v3 1/3 doc: Add new doc for file system firmware loader driver model
From: Tien Fong Chee <[email protected]>
Provide information about
- overview of file system firmware loader driver model
- describe storage device and partition in device tree source
- describe fie system firmware loader API
--- doc/driver-model/fs_firmware_loader.txt | 129 ++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 doc/driver-model/fs_firmware_loader.txt
doc/driver-model/fs_firmware_loader.txt new file +129 -0
@@ -0,0 +1,129 @@ +# Copyright (C) 2018 Intel Corporation <www.intel.com> +# +# SPDX-License-Identifier: GPL-2.0 + +Introduction +============ + +This is file system firmware loader for U-Boot framework, which has very close +to some Linux Firmware API. For the details of Linux Firmware API, you can refer +to https://01.org/linuxgraphics/gfx-docs/drm/driver-api/firmware/index.html. + +File system firmware loader can be used to load whatever(firmware, image, +and binary) from the storage device in file system format into target location +such as memory, then consumer driver such as FPGA driver can program FPGA image +from the target location into FPGA. + +To enable firmware loader, CONFIG_FS_LOADER need to be set at +<board_name>_defconfig such as "CONFIG_FS_LOADER=y". + +Firmware Loader API core features +--------------------------------- + +Firmware storage device described in device tree source +------------------------------------------------------- + For passing data like storage device and partition where the firmware + loading from to the firmware loader driver, those data could be + defined in fs_loader node as shown in below: + + fs_loader0: fs_loader at 0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Then, firmware_loader property would be set with the path of fs_loader + node under /chosen node such as: + /{ + chosen { + firmware_loader = &fs_loader0; + }; + }; + + Example of storage type and device partition search set for mmc, usb, + sata and ubi as shown in below: + Example for mmc: + fs_loader0: fs_loader at 0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader at 3 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "ubi"; + mtdpart = "UBI", + ubivol = "ubi0"; + }; + + + However, the property of devpart, mtdpart, and ubivol values from + fs_loader node can be overwritten with value which is defined in the + environment variable "fw_dev_part", "fw_ubi_mtdpart", and + "fw_ubi_volume" respectively. + For example: env_set("fw_dev_part", "0:2"); + +File system firmware Loader API +------------------------------- + +int request_firmware_into_buf(struct device_platdata *plat, + struct firmware **firmware_p, + const char *name, + void *buf, size_t size, u32 offset) +-------------------------------------------------------------------- +Load firmware into a previously allocated buffer + +Parameters: + +1. struct device_platdata *plat + Platform data such as storage and partition firmware loading from + +2. struct firmware **firmware_p + pointer to firmware image + +3. const char *name + name of firmware file + +4. void *buf + address of buffer to load firmware into + +5. size_t size + size of buffer + +6. u32 offset + offset of a file for start reading into buffer + +return: + size of total read + -ve when error + +Description: + The firmware is loaded directly into the buffer pointed to by buf and + the @firmware_p data member is pointed at buf + +Example of creating firmware loader instance and calling +request_firmware_into_buf API: + if (uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &dev)) { + request_firmware_into_buf(dev->plat, &fw, filename, + buffer_location, buffer_size, offset_ofreading); + }
Signature · 1 line
2.2.0
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
In reply to tien.fong.chee at intel.comFrom: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding information.
--- doc/device-tree-bindings/chosen.txt | 22 ++++++++++++ doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
doc/device-tree-bindings/chosen.txt +22 -0
@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci at fe330000"; }; }; + +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be defined in device trees for +multiple storage type and their default partition, then a property +"firmware-loader" can be used to pass default firmware loader +node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; +};
doc/device-tree-bindings/misc/fs_loader.txt new file +52 -0
52 lines not on this page · raw message
Signature · 1 line
2.2.0
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Hi,
Quoted 26 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding information.
Signed-off-by Tien Fong Chee <[email protected]>--- doc/device-tree-bindings/chosen.txt | 22 ++++++++++++ doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 doc/device-tree-bindings/misc/fs_loader.txtdoc/device-tree-bindings/chosen.txt +6
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci at fe330000"; }; }; + +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be defined in device trees for +multiple storage type and their default partition, then a property +"firmware-loader" can be used to pass default firmware loader
doc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {
This should be :
doc/device-tree-bindings/chosen.txt Example · line 92
Quoted 1 line
doc/device-tree-bindings/chosen.txt +1
-0+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
doc/device-tree-bindings/chosen.txt Example · line 94
Quoted 2 lines
doc/device-tree-bindings/chosen.txt +2
-0+ u-boot,dm-pre-reloc; + compatible = "fs_loader";
How about u-boot,fs-loader since this is U-Boot specific I think.
doc/device-tree-bindings/chosen.txt Example · line 95
Quoted 1 line
doc/device-tree-bindings/chosen.txt +1
-0+ storage_device = "mmc";
storage-device
doc/device-tree-bindings/chosen.txt Example · line 96
Quoted 1 line
doc/device-tree-bindings/chosen.txt +1
-0+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
storage-device = <&mmc_3 1>;
for example (meaning use that MMC, partition 1.
Quoted 49 lines
doc/device-tree-bindings/chosen.txt +2
-0+ }; +};doc/device-tree-bindings/misc/fs_loader.txt new file +41
-0@@ -0,0 +1,52 @@ +* File system firmware loader + +Required properties: +-------------------- + +- compatible: should contain "fs_loader" +- storage_device: which storage device loading from, could be: + - mmc, usb, sata, and ubi. +- devpart: which storage device and partition the image loading from, + this property is required for mmc, usb and sata. +- mdtpart: which partition of ubi the image loading from, this property is + required for ubi. +- ubivol: which volume of ubi the image loading from, this proprety is required + for ubi. + +Example of storage device and partition search set for mmc, usb, sata and +ubi in device tree source as shown in below: + + Example of storage type and device partition search set for mmc, usb, + sata and ubi as shown in below: + Example for mmc: + fs_loader0: fs_loader at 0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata";
doc/device-tree-bindings/misc/fs_loader.txt line 51
Quoted 10 lines
doc/device-tree-bindings/misc/fs_loader.txt new file +10
-0+ devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader at 3 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "ubi"; + mtdpart = "UBI", + ubivol = "ubi0";
I'm not sure about ubi. It needs to refer to a particular device I suppose. How do we know the mapping from ubi to device?
Quoted 4 lines
doc/device-tree-bindings/misc/fs_loader.txt new file +1
-1+ }; --2.2.0
Regards,
Simon
Chee
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Quoted 47 lines · Simon Glass
Hi,
Quoted 31 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding information.
Signed-off-by Tien Fong Chee <[email protected]>---
doc/device-tree-bindings/chosen.txt | 22 ++++++++++++doc/device-tree-bindings/misc/fs_loader.txt| 52
+++++++++++++++++++++++++++++2 files changed, 74 insertions(+)create mode 100644
doc/device-tree-bindings/misc/fs_loader.txtdoc/device-tree- renamed from doc/device-tree-bindings/chosen.txt +0
-0
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Exampleu-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci at
fe330000";
};
};doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be defined indevice trees for
doc/device-tree-bindings/chosen.txt +2
-0+multiple storage type and their default partition, then a property +"firmware-loader" can be used to pass default firmware loaderdoc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 1 line
Quoted 4 lines
Quoted 1 line
+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
Quoted 10 lines
Quoted 3 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 2 lines
+ storage_device = "mmc";
storage-device
Okay
Quoted 9 lines
Quoted 2 lines
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
storage-device = <
&mmc_31>;for example (meaning use that MMC, partition 1.
How to get device number based on node of a storage?
This could make design more complicated because this driver is designed to support both fdt and without fdt(U-boot env and hard coding in driver).
Quoted 60 lines
Quoted 60 lines
+ };
doc/device-tree-bindings/chosen.txt +1
-0+};doc/device-tree-bindings/misc/fs_loader.txt new file +9
-0@@ -0,0 +1,52 @@ +* File system firmware loader + +Required properties: +-------------------- + +- compatible: should contain "fs_loader" +- storage_device: which storage device loading from, could be: + - mmc, usb, sata, and ubi. +- devpart: which storage device and partition the image loadingfrom,
+ this property is required for mmc, usb and sata. +- mdtpart: which partition of ubi the image loading from, this property is
+ required for ubi.
+- ubivol: which volume of ubi the image loading from, this proprety is required
+ for ubi.doc/device-tree-bindings/misc/fs_loader.txt new file +2
-0+ +Example of storage device and partition search set for mmc, usb,sata and
doc/device-tree-bindings/misc/fs_loader.txt new file +3
-0+ubi in device tree source as shown in below: + + Example of storage type and device partition search set formmc, usb,
+ sata and ubi as shown in below:
+ Example for mmc:
+ fs_loader0:fs_loaderat 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";
+ storage_device = "mmc";
+ devpart = "0:1";
+ };doc/device-tree-bindings/misc/fs_loader.txt new file +18
-0+ + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi:
Quoted 9 lines
Quoted 6 lines
+ fs_loader3:
fs_loaderat 3 {
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";
+ storage_device = "ubi";
+ mtdpart = "UBI",
+ ubivol = "ubi0";I'm not sure about ubi. It needs to refer to a particular device I suppose. How do we know the mapping from ubi to device?
Actually this design is based on file system implementation which is abstract from physical device, storage_device is used as interface of file system, so nand and qspi may having the same ubi interface.
May be i can change the storage_device into storage_interface to avoid confusing.
Quoted 7 lines
Quoted 5 lines
+ };
--
2.2.0Regards,
Simon
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Hi Tien Fong,
Quoted 69 lines · Tien Fong
Quoted 47 lines · Simon Glass
Hi,
Quoted 31 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding information.
Signed-off-by Tien Fong Chee <[email protected]>--- doc/device-tree-bindings/chosen.txt | 22 ++++++++++++ doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 doc/device-tree-bindings/misc/fs_loader.txtdoc/device-tree- renamed from doc/device-tree-bindings/chosen.txt +0
-0
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +4
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci at fe330000"; }; }; + +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be defined indevice trees for
doc/device-tree-bindings/chosen.txt +2
-0+multiple storage type and their default partition, then a property +"firmware-loader" can be used to pass default firmware loaderdoc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 1 line
Quoted 4 lines
Quoted 1 line
+
fs_loader0: fs-loader at 0 {since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
Quoted 10 lines
doc/device-tree-bindings/chosen.txt
Quoted 3 lines
doc/device-tree-bindings/chosen.txt +2
-0+ u-boot,dm-pre-reloc; + compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 2 lines
+
storage_device= "mmc";storage-device
Okay
Quoted 5 lines
Quoted 2 lines
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
Quoted 8 lines
Quoted 4 lines
storage-device = <&mmc_3 1>;for example (meaning use that MMC, partition 1.
How to get device number based on node of a storage?
This could make design more complicated because this driver is designed to support both fdt and without fdt(U-boot env and hard coding in driver).
I think it is fine to support the environment as a way of providing this info, but there is no need to support non-DM. Everything should be converting to DM now.
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()
I think we need to create a function called
device_get_global_by_phandle_id()
which can be used to obtain the device based on a phandle.
Quoted 69 lines
Quoted 60 lines
Quoted 60 lines
doc/device-tree-bindings/chosen.txt +2
-0+ }; +};doc/device-tree-bindings/misc/fs_loader.txt new file +9
-0@@ -0,0 +1,52 @@ +* File system firmware loader + +Required properties: +-------------------- + +- compatible: should contain "fs_loader" +- storage_device: which storage device loading from, could be: + - mmc, usb, sata, and ubi. +- devpart: which storage device and partition the image loadingfrom,
+ this property is required for mmc, usb and sata.
+- mdtpart: which partition of ubi the image loading from, this property is
+ required for ubi.
+- ubivol: which volume of ubi the image loading from, this proprety is requireddoc/device-tree-bindings/misc/fs_loader.txt new file +3
-0+ for ubi. + +Example of storage device and partition search set for mmc, usb,sata and
doc/device-tree-bindings/misc/fs_loader.txt new file +3
-0+ubi in device tree source as shown in below: + + Example of storage type and device partition search set formmc, usb,
doc/device-tree-bindings/misc/fs_loader.txt new file +26
-0+ sata and ubi as shown in below: + Example for mmc: + fs_loader0: fs_loader at 0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi:Quoted 9 lines
doc/device-tree-bindings/misc/fs_loader.txt line 51
Quoted 6 lines
doc/device-tree-bindings/misc/fs_loader.txt new file +6
-0+ fs_loader3: fs_loader at 3 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "ubi"; + mtdpart = "UBI", + ubivol = "ubi0";I'm not sure about ubi. It needs to refer to a particular device I suppose. How do we know the mapping from ubi to device?
Quoted 6 lines
Actually this design is based on file system implementation which is abstract from physical device,
storage_deviceis used as interface of file system, so nand and qspi may having the same ubi interface.May be i can change the
storage_deviceintostorage_interfaceto avoid confusing.
OK I see. Well I suppose this is OK then, at least for now. I'm not have any great ideas on how to specify which ubi storage device to use. Let's assume we only have one for now.
Regards,
Simon
Chee
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Quoted 90 lines · Simon Glass
Hi Tien Fong,
Quoted 86 lines · Tien Fong
Quoted 53 lines · Simon Glass
Hi,
Quoted 35 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding information.
Signed-off-by Tien Fong Chee <[email protected]>---
doc/device-tree-bindings/chosen.txt | 22 ++++++++++++doc/device-tree-bindings/misc/fs_loader.txt| 52
+++++++++++++++++++++++++++++2 files changed, 74 insertions(+)create mode 100644
doc/device-tree-bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc,"/sd
hci atfe330000";
};
};doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be defined indevice trees for
+multiple storage type and their default partition, then a property
+"firmware-loader" can be used to pass default firmware loaderdoc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 2 lines
Quoted 5 lines
Quoted 1 line
+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names. Underscore is used
for phandles (so you have that correct).Okay.
Quoted 5 lines
Quoted 5 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";Quoted 8 lines
How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 3 lines
+ storage_device = "mmc";
storage-device
Okay
Quoted 11 lines
Quoted 4 lines
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
storage-device = <
&mmc_31>;for example (meaning use that MMC, partition 1.
Quoted 9 lines
Quoted 5 lines
How to get device number based on node of a storage? This could make design more complicated because this driver is designed
to support both fdt and without fdt(U-boot env and hard coding in driver).I think it is fine to support the environment as a way of providing this info, but there is no need to support non-DM. Everything should be converting to DM now.
Ok,i would keep the DM and environment support.
Quoted 11 lines
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
device_get_global_by_phandle_id()which can be used to obtain the device based on a phandle.
I means the device number in the struct blk_desc, block device. I don't know how the device number is built up during driver model init. Is there a function to retrive the device number?
Quoted 91 lines
Quoted 82 lines
Quoted 67 lines
Quoted 67 lines
+ };
doc/device-tree-bindings/chosen.txt +1
-0+};doc/device-tree-bindings/misc/fs_loader.txt new file +9
-0@@ -0,0 +1,52 @@ +* File system firmware loader + +Required properties: +-------------------- + +- compatible: should contain "fs_loader" +- storage_device: which storage device loading from, could be: + - mmc, usb, sata, and ubi. +- devpart: which storage device and partition the imageloading
from,
+ this property is required for mmc, usb and sata. +- mdtpart: which partition of ubi the image loading from, this property is
+ required for ubi.
+- ubivol: which volume of ubi the image loading from, this proprety is required
+ for ubi.doc/device-tree-bindings/misc/fs_loader.txt new file +2
-0+ +Example of storage device and partition search set for mmc,usb,
sata anddoc/device-tree-bindings/misc/fs_loader.txt new file +3
-0+ubi in device tree source as shown in below: + + Example of storage type and device partition search setfor
mmc, usb,
+ sata and ubi as shown in below:
+ Example for mmc:
+ fs_loader0:fs_loaderat 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";
+ storage_device = "mmc";
+ devpart = "0:1";
+ };doc/device-tree-bindings/misc/fs_loader.txt new file +19
-0+ + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader at 3 {Quoted 9 lines
Quoted 5 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";
+ storage_device = "ubi";
+ mtdpart = "UBI",
+ ubivol = "ubi0";I'm not sure about ubi. It needs to refer to a particular device I
suppose. How do we know the mapping from ubi to device?Actually this design is based on file system implementation which is
abstract from physical device,storage_deviceis used as interface of
file system, so nand and qspi may having the same ubi interface.Quoted 3 lines
May be i can change the
storage_deviceintostorage_interfaceto avoid
confusing.OK I see. Well I suppose this is OK then, at least for now. I'm not have any great ideas on how to specify which ubi storage device to use. Let's assume we only have one for now.
Regards,
Simon
Chee
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Quoted 130 lines · Tien Fong
Quoted 114 lines · Simon Glass
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <[email protected]
Quoted 1 line
Quoted 101 lines · wrote:
Quoted 63 lines · Simon Glass
Hi,
Quoted 43 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding
information.Signed-off-by Tien Fong Chee <[email protected]>---
doc/device-tree-bindings/chosen.txt | 22
++++++++++++
doc/device-tree-bindings/misc/fs_loader.txt | 52
+++++++++++++++++++++++++++++2 files changed, 74 insertions(+)create mode 100644 doc/device-tree-
bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc,"/sd
hci atfe330000";
};
};doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be definedin
device trees for
+multiple storage type and their default partition, then a property
+"firmware-loader" can be used to pass default firmware loaderdoc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 3 lines
Quoted 5 lines
Quoted 1 line
+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names. Underscore is used
for phandles (so you have that correct).Okay.
Quoted 14 lines
Quoted 7 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
+ storage_device = "mmc";
storage-device
Okay
Quoted 11 lines
Quoted 6 lines
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
storage-device = <
&mmc_31>;Quoted 7 lines
Quoted 2 lines
for example (meaning use that MMC, partition 1.
How to get device number based on node of a storage? This could make design more complicated because this driver is designed
to support both fdt and without fdt(U-boot env and hard coding in driver).Quoted 5 lines
I think it is fine to support the environment as a way of providing this info, but there is no need to support non-DM. Everything should
be converting to DM now.Ok,i would keep the DM and environment support.
Quoted 8 lines
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
Quoted 8 lines
Quoted 4 lines
device_get_global_by_phandle_id()which can be used to obtain the device based on a phandle.
I means the device number in the struct
blk_desc, block device. I don't
know how the device number is built up during driver model init. Is there a function to retrive the device number?
After roughly checking the blk-uclass.c, i think we can get the device number for blok device and also mainstain the consistent interface with UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device -
"ide" "scsi" "atapi" "usb" "doc" "mmc" "sd" "sata" "host" "nvme" "efi"
non block device
"ubi"
2) phandle for physical storage node(parent node) which contain the uclass support as listed below for block devices:
UCLASS_IDE
UCLASS_SCSI
UCLASS_INVALID
UCLASS_MASS_STORAGE
UCLASS_INVALID
UCLASS_MMC
UCLASS_INVALID
UCLASS_AHCI
UCLASS_ROOT
UCLASS_NVME
UCLASS_EFI
Above nodes must contain the child node UCLASS_BLK(this node contains device number)
No node required for UBI interface.
I can create a new function, which is specific for getting the device number from the parent node(block device) which contains the child node UCLASS_BLK. May be that function can be put into fs.c.
I'm not sure would this work as expected, any ideas/thoughts on my proposal?
Quoted 100 lines
Quoted 100 lines
Quoted 91 lines
Quoted 75 lines
Quoted 75 lines
+ };
doc/device-tree-bindings/chosen.txt +1
-0+};doc/device-tree-bindings/misc/fs_loader.txt new file +7
-0@@ -0,0 +1,52 @@ +* File system firmware loader + +Required properties: +-------------------- + +- compatible: should contain "fs_loader" +- storage_device: which storage device loading from, couldbe:
+ - mmc, usb, sata, and ubi.
+- devpart: which storage device and partition the image loading
from,
+ this property is required for mmc, usb and sata. +- mdtpart: which partition of ubi the image loading from, this
property is
+ required for ubi.
+- ubivol: which volume of ubi the image loading from, this proprety is required
+ for ubi.doc/device-tree-bindings/misc/fs_loader.txt new file +2
-0+ +Example of storage device and partition search set for mmc,usb,
sata anddoc/device-tree-bindings/misc/fs_loader.txt new file +3
-0+ubi in device tree source as shown in below: + + Example of storage type and device partition searchset
for
mmc, usb,
+ sata and ubi as shown in below:
+ Example for mmc:
+ fs_loader0:fs_loaderat 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";
+ storage_device = "mmc";
+ devpart = "0:1";
+ };doc/device-tree-bindings/misc/fs_loader.txt new file +20
-0+ + Example for usb: + fs_loader1: fs_loader at 1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader at 2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader at 3 { + u-boot,dm-pre-reloc;Quoted 9 lines
Quoted 4 lines
+ compatible = "
fs_loader";
+ storage_device = "ubi";
+ mtdpart = "UBI",
+ ubivol = "ubi0";I'm not sure about ubi. It needs to refer to a particular device
I
suppose. How do we know the mapping from ubi to device?Actually this design is based on file system implementation which is
abstract from physical device,storage_deviceis used as interface
of
file system, so nand and qspi may having the same ubi interface.Quoted 3 lines
May be i can change the
storage_deviceintostorage_interfaceto avoid
confusing.OK I see. Well I suppose this is OK then, at least for now. I'm not have any great ideas on how to specify which ubi storage device to use. Let's assume we only have one for now.
Regards,
Simon
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Hi Tien Fong,
Quoted 144 lines · Tien Fong
Quoted 130 lines · Tien Fong
Quoted 114 lines · Simon Glass
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <[email protected]
Quoted 1 line
Quoted 101 lines · wrote:
Quoted 63 lines · Simon Glass
Hi,
Quoted 43 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding
information.Signed-off-by Tien Fong Chee <[email protected]>--- doc/device-tree-bindings/chosen.txt | 22 ++++++++++++ doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 doc/device-tree-
bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc,"/sd
hci at fe330000"; };};
doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could be definedin
device trees for
+multiple storage type and their default partition, then a property
+"firmware-loader" can be used to pass default firmware loaderdoc/device-tree-bindings/chosen.txt Example · line 91
Quoted 10 lines
doc/device-tree-bindings/chosen.txt +10
-0+node(default storage type) to the firmware loader driver. + +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 3 lines
Quoted 5 lines
Quoted 1 line
+
fs_loader0: fs-loader at 0 {since hyphen is used for node and property names. Underscore is used
for phandles (so you have that correct).Okay.
Quoted 14 lines
doc/device-tree-bindings/chosen.txt
Quoted 7 lines
doc/device-tree-bindings/chosen.txt +2
-0+ u-boot,dm-pre-reloc; + compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
+
storage_device= "mmc";storage-device
Okay
Quoted 11 lines
Quoted 6 lines
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc device to use.
storage-device = <&mmc_3 1>;Quoted 7 lines
Quoted 2 lines
for example (meaning use that MMC, partition 1.
How to get device number based on node of a storage? This could make design more complicated because this driver is designed
to support both fdt and without fdt(U-boot env and hard coding in driver).Quoted 5 lines
I think it is fine to support the environment as a way of providing this info, but there is no need to support non-DM. Everything should
be converting to DM now.Ok,i would keep the DM and environment support.
Quoted 8 lines
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
Quoted 8 lines
Quoted 4 lines
device_get_global_by_phandle_id()which can be used to obtain the device based on a phandle.
I means the device number in the struct
blk_desc, block device. I don't
know how the device number is built up during driver model init. Is
there a function to retrive the device number?After roughly checking the
blk-uclass.c, i think we can get the device number for blok device and also mainstain the consistent interface with UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device -
Quoted 11 lines
"ide"
"scsi"
"atapi"
"usb"
"doc"
"mmc"
"sd"
"sata"
"host"
"nvme"
"efi"Do you need this? Can it not be obtained from uclass_get_name() using the phandle (below)?
Quoted 20 lines
non block device
"ubi"2) phandle for physical storage node(parent node) which contain the uclass support as listed below for block devices:
UCLASS_IDE
UCLASS_SCSI
UCLASS_INVALID
UCLASS_MASS_STORAGE
UCLASS_INVALID
UCLASS_MMC
UCLASS_INVALID
UCLASS_AHCI
UCLASS_ROOT
UCLASS_NVME
UCLASS_EFI
Above nodes must contain the child nodeUCLASS_BLK(this node contains device number)
Quoted 8 lines
No node required for UBI interface.
I can create a new function, which is specific for getting the device number from the parent node(block device) which contains the child node
UCLASS_BLK. May be that function can be put intofs.c.I'm not sure would this work as expected, any ideas/thoughts on my proposal?
Sounds right to me.
[...]
Regards,
Simon
Chee
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Quoted 186 lines · Simon Glass
Hi Tien Fong,
Quoted 174 lines · Tien Fong
Quoted 155 lines · Tien Fong
Quoted 134 lines · Simon Glass
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel .com
Quoted 2 lines
Quoted 118 lines · wrote:
Quoted 70 lines · Simon Glass
Hi,
Quoted 48 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding
information.Signed-off-by Tien Fong Chee <[email protected]>---
doc/device-tree-bindings/chosen.txt | 22
++++++++++++
doc/device-tree-bindings/misc/fs_loader.txt | 52
+++++++++++++++++++++++++++++2 files changed, 74 insertions(+)create mode 100644 doc/device-tree-
bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc,"/sd
hci atfe330000";
};
};doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could bedefined
in
device trees for
+multiple storage type and their default partition, then a
property
+"firmware-loader" can be used to pass default firmware loader
+node(default storage type) to the firmware loader driver.doc/device-tree-bindings/chosen.txt
Quoted 9 lines
doc/device-tree-bindings/chosen.txt +9
-0+ +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 4 lines
Quoted 6 lines
Quoted 1 line
+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names.
Underscore is
used
for phandles (so you have that correct).Okay.
Quoted 17 lines
Quoted 9 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+ storage_device = "mmc";
storage-device
Okay
Quoted 7 lines
Quoted 7 lines
Quoted 9 lines
Quoted 1 line
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc
device to use.storage-device = <
&mmc_31>;for example (meaning use that MMC, partition 1.
Quoted 7 lines
How to get device number based on node of a storage? This could make design more complicated because this driver is
designed
to support both fdt and without fdt(U-boot env and hard coding in
driver).Quoted 6 lines
I think it is fine to support the environment as a way of providing
this info, but there is no need to support non-DM. Everything should
be converting to DM now.Ok,i would keep the DM and environment support.
Quoted 11 lines
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
device_get_global_by_phandle_id()Quoted 7 lines
Quoted 2 lines
which can be used to obtain the device based on a phandle.
I means the device number in the struct
blk_desc, block device. I don't
know how the device number is built up during driver model init. Is
there a function to retrive the device number?After roughly checking the
blk-uclass.c, i think we can get the device
number for blok device and also mainstain the consistent interface with
UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device -
"ide"
"scsi"
"atapi"Quoted 8 lines
"usb"
"doc"
"mmc"
"sd"
"sata"
"host"
"nvme"
"efi"
Quoted 3 lines
Do you need this? Can it not be obtained from
uclass_get_name()using the phandle (below)?
The purpose of above parameter is used to differentiate the block devices and non block devices(like UBI). This function would be called if it is block device.
Is the name is standard defined as above block devices string?
Quoted 30 lines
Quoted 30 lines
non block device
"ubi"2) phandle for physical storage node(parent node) which contain the uclass support as listed below for block devices:
UCLASS_IDE
UCLASS_SCSI
UCLASS_INVALID
UCLASS_MASS_STORAGE
UCLASS_INVALID
UCLASS_MMC
UCLASS_INVALID
UCLASS_AHCI
UCLASS_ROOT
UCLASS_NVME
UCLASS_EFI
Above nodes must contain the child node UCLASS_BLK(this node contains
device number)No node required for UBI interface.
I can create a new function, which is specific for getting the device
number from the parent node(block device) which contains the child nodeUCLASS_BLK. May be that function can be put intofs.c.
Quoted 4 lines
Quoted 2 lines
I'm not sure would this work as expected, any ideas/thoughts on my proposal?
Sounds right to me.
Is that possible one parent device(like MMC) contains multiple child devices in UCLASS_BLK?
Quoted 4 lines
[...]
Regards,
Simon
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Hi Tien Fong,
Quoted 187 lines · Tien Fong
Quoted 186 lines · Simon Glass
Hi Tien Fong,
Quoted 182 lines · Tien Fong
Quoted 155 lines · Tien Fong
Quoted 134 lines · Simon Glass
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel .com
Quoted 2 lines
Quoted 118 lines · wrote:
Quoted 70 lines · Simon Glass
Hi,
Quoted 48 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware loader binding
information.Signed-off-by Tien Fong Chee <[email protected]>--- doc/device-tree-bindings/chosen.txt | 22 ++++++++++++ doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 doc/device-tree-
bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc,"/sd
hci at fe330000"; };};
doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could bedefined
in
device trees for
+multiple storage type and their default partition, then a
property
+"firmware-loader" can be used to pass default firmware loader
+node(default storage type) to the firmware loader driver.doc/device-tree-bindings/chosen.txt
Quoted 9 lines
doc/device-tree-bindings/chosen.txt +9
-0+ +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 4 lines
Quoted 6 lines
Quoted 1 line
+
fs_loader0: fs-loader at 0 {since hyphen is used for node and property names.
Underscore is
used
for phandles (so you have that correct).Okay.
Quoted 17 lines
doc/device-tree-bindings/chosen.txt
Quoted 9 lines
doc/device-tree-bindings/chosen.txt +2
-0+ u-boot,dm-pre-reloc; + compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+
storage_device= "mmc";storage-device
Okay
Quoted 7 lines
Quoted 7 lines
Quoted 9 lines
Quoted 1 line
+ devpart = "0:1";
I think you should use a phandle to the node containing the mmc
device to use.storage-device = <&mmc_3 1>;for example (meaning use that MMC, partition 1.
Quoted 7 lines
How to get device number based on node of a storage? This could make design more complicated because this driver is
designed
to support both fdt and without fdt(U-boot env and hard coding in
driver).Quoted 6 lines
I think it is fine to support the environment as a way of providing
this info, but there is no need to support non-DM. Everything should
be converting to DM now.Ok,i would keep the DM and environment support.
Quoted 11 lines
We have:
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
device_get_global_by_phandle_id()Quoted 7 lines
Quoted 2 lines
which can be used to obtain the device based on a phandle.
I means the device number in the struct
blk_desc, block device. I don't
know how the device number is built up during driver model init.
Is
there a function to retrive the device number?After roughly checking the
blk-uclass.c, i think we can get the device
number for blok device and also mainstain the consistent interface with
UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device -"ide" "scsi" "atapi" "usb" "doc" "mmc" "sd" "sata" "host" "nvme" "efi"
Quoted 6 lines
Quoted 3 lines
Do you need this? Can it not be obtained from
uclass_get_name()using the phandle (below)?The purpose of above parameter is used to differentiate the block devices and non block devices(like UBI). This function would be called if it is block device.
Did you see the strings in the uclass drivers? E.g.:
UCLASS_DRIVER(mmc) = {
.id = UCLASS_MMC,
.name = "mmc",
.flags = DM_UC_FLAG_SEQ_ALIAS, .per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
};
Quoted 2 lines
Is the name is standard defined as above block devices string?
I don't understand this. The same is defined for all uclasses that can have a block device as a child.
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
non block device
"ubi"2) phandle for physical storage node(parent node) which contain the uclass support as listed below for block devices:
UCLASS_IDE
UCLASS_SCSI
UCLASS_INVALID
UCLASS_MASS_STORAGE
UCLASS_INVALID
UCLASS_MMC
UCLASS_INVALID
UCLASS_AHCI
UCLASS_ROOT
UCLASS_NVME
UCLASS_EFI
Above nodes must contain the child node UCLASS_BLK(this node contains
device number)No node required for UBI interface.
I can create a new function, which is specific for getting the device
number from the parent node(block device) which contains the child nodeUCLASS_BLK. May be that function can be put intofs.c.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
I'm not sure would this work as expected, any ideas/thoughts on my proposal?
Sounds right to me.
Is that possible one parent device(like MMC) contains multiple child devices in
UCLASS_BLK?
No, only one is supported.
Regards,
Simon
Chee
PATCH v3 2/3 doc: dtbinding: Add file system firmware loader binding document
Quoted 229 lines · Simon Glass
Hi Tien Fong,
Quoted 217 lines · Tien Fong
Quoted 215 lines · Simon Glass
Hi Tien Fong,
Quoted 209 lines · Tien Fong
Quoted 180 lines · Tien Fong
Quoted 154 lines · Simon Glass
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@i ntel
.comQuoted 3 lines
Quoted 134 lines · wrote:
Quoted 78 lines · Simon Glass
Hi,
Quoted 54 lines · On 25 June 2018 at 07:28, <[email protected]> wrote:
From: Tien Fong Chee <[email protected]>
Add a document to describe file system firmware
loader
binding
information.Signed-off-by Tien Fong Chee <[email protected]om>
---
doc/device-tree-bindings/chosen.txt | 22
++++++++++++
doc/device-tree-bindings/misc/fs_loader.txt | 52
+++++++++++++++++++++++++++++2 files changed, 74 insertions(+)create mode 100644 doc/device-tree-
bindings/misc/fs_loader.txtdoc/device- renamed from doc/device-tree-bindings/chosen.txt +0
-0tree-
bindings/chosen.txt
index c96b8f7..738673c 100644doc/device-tree-bindings/chosen.txt +0
-0@@ -73,3 +73,25 @@ Exampleu-boot,spl-boot-order = "same-as-
spl",
&sdmmc,
"/sd
hci atfe330000";
};
};doc/device-tree-bindings/chosen.txt +4
-0+ +firmware-loader property +------------------------ +Multiple file system firmware loader nodes could bedefined
in
device trees for
+multiple storage type and their default partition, then
a
property
+"firmware-loader" can be used to pass default firmware
loader
+node(default storage type) to the firmware loader driver.doc/device-tree-bindings/chosen.txt
Quoted 9 lines
doc/device-tree-bindings/chosen.txt +9
-0+ +Example +------- +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs_loader at 0 {This should be :
Quoted 5 lines
Quoted 6 lines
Quoted 1 line
+ fs_loader0: fs-loader at 0 {
since hyphen is used for node and property names.
Underscore is
used
for phandles (so you have that correct).Okay.
Quoted 21 lines
Quoted 11 lines
+ u-boot,dm-pre-reloc;
+ compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot
specific I
think.Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+ storage_device = "mmc";
storage-device
Okay
Quoted 10 lines
Quoted 10 lines
+ devpart = "0:1";
Quoted 9 lines
I think you should use a phandle to the node containing the
mmc
device to use.storage-device = <
&mmc_31>;for example (meaning use that MMC, partition 1.
Quoted 8 lines
How to get device number based on node of a storage? This could make design more complicated because this driver
is
designed
to support both fdt and without fdt(U-boot env and hard coding in
driver).Quoted 7 lines
I think it is fine to support the environment as a way of providing
this info, but there is no need to support non-DM. Everything
should
be converting to DM now.Ok,i would keep the DM and environment support.
Quoted 5 lines
We have:
Quoted 9 lines
uclass_get_device_by_phandle_id()device_get_global_by_of_offset()I think we need to create a function called
device_get_global_by_phandle_id()which can be used to obtain the device based on a phandle.
Quoted 7 lines
I means the device number in the struct
blk_desc, block device. I
don't
know how the device number is built up during driver model init.
Is
there a function to retrive the device number?After roughly checking the
blk-uclass.c, i think we can get the device
number for blok device and also mainstain the consistent interface
with
UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device -
"ide"
"scsi"
"atapi"
"usb"
"doc"
"mmc"
"sd"
"sata"
"host"
"nvme"
"efi"Quoted 8 lines
Quoted 4 lines
Do you need this? Can it not be obtained from
uclass_get_name()using
the phandle (below)?The purpose of above parameter is used to differentiate the block devices and non block devices(like UBI). This function would be called
if it is block device.
Quoted 9 lines
Did you see the strings in the uclass drivers? E.g.:
UCLASS_DRIVER(mmc)= {
.id =UCLASS_MMC,
.name = "mmc",
.flags =DM_UC_FLAG_SEQ_ALIAS, .per_device_auto_alloc_size =sizeof(struct mmc_uclass_priv), };
Yeah, but i'm not sure all uclass drivers having same string as above interface string.
Anyway, i found alternate way using blk structure instead of the interface string. You can check the v[4] patchset i have submitted from last week.
Quoted 45 lines
Quoted 3 lines
Is the name is standard defined as above block devices string?
I don't understand this. The same is defined for all uclasses that can
have a block device as a child.Quoted 38 lines
Quoted 38 lines
Quoted 38 lines
non block device
"ubi"2) phandle for physical storage node(parent node) which contain the
uclass support as listed below for block devices:
UCLASS_IDE
UCLASS_SCSI
UCLASS_INVALID
UCLASS_MASS_STORAGE
UCLASS_INVALID
UCLASS_MMC
UCLASS_INVALID
UCLASS_AHCI
UCLASS_ROOT
UCLASS_NVME
UCLASS_EFI
Above nodes must contain the child node UCLASS_BLK(this node contains
device number)No node required for UBI interface.
I can create a new function, which is specific for getting the device
number from the parent node(block device) which contains the child
nodeUCLASS_BLK. May be that function can be put intofs.c.I'm not sure would this work as expected, any ideas/thoughts on my
proposal?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Sounds right to me.
Is that possible one parent device(like MMC) contains multiple child
devices inUCLASS_BLK?No, only one is supported.
Noted.
Quoted 2 lines
Regards,
Simon
PATCH v3 3/3 common: Generic loader for file system
In reply to tien.fong.chee at intel.comFrom: 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 | 238 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fs_loader.h | 28 ++++++ 5 files changed, 278 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 +238 -0
238 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 +28 -0
@@ -0,0 +1,28 @@ +/* +* 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 { +size_t size;/* Size of a file */ +const u8 *data;/* Buffer for file */ +void *priv;/* Firmware loader private fields */ +}; + +struct device_platdata { +char *name;/* Such as mmc, usb,and sata. */ +char *devpart;/* Use the load command dev:part conventions */ +char *mtdpart;/* MTD partition for ubi part */ +char *ubivol;/* UBI volume-name for ubifsmount */ +}; + +int request_firmware_into_buf (struct device_platdata *plat, +struct firmware **firmware_p, +const char *name, +void *buf,size_t size,u32 offset); +#endif
Signature · 1 line
2.2.0
PATCH v3 3/3 common: Generic loader for file system
Hi,
Quoted 8 lines · On 25 June 2018 at 07: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 | 238 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fs_loader.h | 28 ++++++ 5 files changed, 278 insertions(+) create mode 100644 drivers/misc/fs_loader.c create mode 100644 include/fs_loader.h
This is definitely looking good. I think we need to figure out the binding to devices, to use phandles instead of strings. Also we need a sandbox driver and test.
I'm a little worried that you are blazing a trail here, but it is a valuable trail :-)
Tom, do you have any ideas / thoughts on the design?
Quoted 56 lines
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. + 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 +21
-0@@ -0,0 +1,238 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <linux/string.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 line 31
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+static int select_fs_dev (struct device_platdata *plat) +{ +int ret; + +if (!strcmp ("mmc" , plat->name)) { + ret =fs_set_blk_dev ("mmc" , plat->devpart,FS_TYPE_ANY ); + }else if (!strcmp ("usb" , plat->name)) { + ret =fs_set_blk_dev ("usb" , plat->devpart,FS_TYPE_ANY ); + }else if (!strcmp ("sata" , plat->name)) { + ret =fs_set_blk_dev ("sata" , plat->devpart,FS_TYPE_ANY );
For these I think you can look up the phandle to obtain the device, then check its uclass to find out its type.
Quoted 33 lines
drivers/misc/fs_loader.c new file +33
-0+ }else if (!strcmp ("ubi" , plat->name)) { +if (plat->ubivol) + ret =fs_set_blk_dev ("ubi" ,NULL ,FS_TYPE_UBIFS ); +else + ret = -ENODEV ; + }else { +debug ("Error: unsupported storage device.\n" ); +return -ENODEV ; + } + +if (ret) +debug ("Error: could not access storage.\n" ); + +return ret; +} + +static void set_storage_devpart (struct device_platdata *plat,char *devpart) +{ + plat->devpart = devpart; +} + +static void set_storage_mtdpart (struct device_platdata *plat,char *mtdpart) +{ + plat->mtdpart = mtdpart; +} + +static void set_storage_ubivol (struct device_platdata *plat,char *ubivol) +{ + plat->ubivol = ubivol; +} + +/** +* _request_firmware_prepare - Prepare firmware struct.
drivers/misc/fs_loader.c line 74
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into. +* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* +* Return: Negative value if fail, 0 for successful. +*/ +static int _request_firmware_prepare(struct firmware **firmware_p,
Can you please move this to be the last arg and rename to firmwarep?
Quoted 30 lines
drivers/misc/fs_loader.c new file +30
-0+ const char *name, void *dbuf,device to ubi + size_t size, u32 offset) +{ + struct firmware *firmware; +struct firmware_priv *fw_priv; + + *firmware_p =NULL ; + +if (!name || name[0 ] =='\0' ) +return -EINVAL ; + + firmware =calloc (1 ,sizeof (*firmware)); +if (!firmware) +return -ENOMEM ; + + fw_priv =calloc (1 ,sizeof (*fw_priv)); +if (!fw_priv) { +free (firmware); +return -ENOMEM ; + } + + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + +return 0 ; +}
drivers/misc/fs_loader.c line 114
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ +/** +* fw_get_filesystem_firmware - load firmware into an allocated buffer. +* @plat: Platform data such as storage and partition firmware loading from. +* @firmware_p: 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_p)
s/firmware_p/firmware/
drivers/misc/fs_loader.c line 127
Quoted 13 lines
drivers/misc/fs_loader.c new file +13
-0+{ +struct firmware_priv *fw_priv =NULL ; +loff_t actread; +char *dev_part, *ubi_mtdpart, *ubi_volume; +int ret; + + dev_part =env_get ("fw_dev_part" ); +if (dev_part) +set_storage_devpart (plat, dev_part); + + ubi_mtdpart =env_get ("fw_ubi_mtdpart" ); +if (ubi_mtdpart) +set_storage_mtdpart (plat, ubi_mtdpart);
Do you mean that the storage partition comes from the environment? I thought it came from the FDT?
drivers/misc/fs_loader.c line 139
Quoted 12 lines
drivers/misc/fs_loader.c new file +12
-0+ + ubi_volume =env_get ("fw_ubi_volume" ); +if (ubi_volume) +set_storage_ubivol (plat, ubi_volume); + + ret =select_fs_dev (plat); +if (ret) + goto out; + + fw_priv = firmware_p->priv; + + ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
map_to_sysmem(firmware_p->data)
(so that it works on sandbox)
Quoted 134 lines
drivers/misc/fs_loader.c new file +99
-0+ firmware_p->size, &actread); +if (ret) { +debug ("Error: %d Failed to read %s from flash %lld != %d.\n" , + ret, fw_priv->name, actread, firmware_p->size); + }else { + ret = actread; + } + +out: +return ret; +} + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer. +* @plat: Platform data such as storage and partition firmware loading from. +* @firmware_p: Pointer to firmware image. +* @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. +* +* The firmware is loaded directly into the buffer pointed to by @buf and +* the @firmware_p data member is pointed at @buf. +* +* Return: Size of total read, negative value when error. +*/ +int request_firmware_into_buf (struct device_platdata *plat, +struct firmware **firmware_p, +const char *name, +void *buf,size_t size,u32 offset) +{ +int ret; + +if (!plat) +return -EINVAL ; + + ret =_request_firmware_prepare (firmware_p, name, buf, size, offset); +if (ret <0 )/* error */ +return ret; + + ret =fw_get_filesystem_firmware (plat, *firmware_p); + +return ret; +} + +static int fs_loader_ofdata_to_platdata (struct udevice *dev) +{ +const char *fs_loader_path; + + 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; + + plat->name = (char *)ofnode_read_string (fs_loader_node, +"storage_device" ); + + plat->devpart = (char *)ofnode_read_string ( + fs_loader_node,"devpart" ); + + 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 ="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 +17
-0@@ -0,0 +1,28 @@ +/* +* 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 { +size_t size;/* Size of a file */ +const u8 *data;/* Buffer for file */ +void *priv;/* Firmware loader private fields */ +}; + +struct device_platdata {
include/fs_loader.h line 27
Quoted 10 lines
include/fs_loader.h new file +10
-0+char *name;/* Such as mmc, usb,and sata. */ +char *devpart;/* Use the load command dev:part conventions */ +char *mtdpart;/* MTD partition for ubi part */ +char *ubivol;/* UBI volume-name for ubifsmount */ +}; + +int request_firmware_into_buf (struct device_platdata *plat, +struct firmware **firmware_p, +const char *name, +void *buf,size_t size,u32 offset);
Please can you add a full function comment here?
Quoted 4 lines
include/fs_loader.h new file +1
-1+#endif --2.2.0
Regards,
Simon
Chee
PATCH v3 3/3 common: Generic loader for file system
Quoted 27 lines · Simon Glass
Hi,
Quoted 16 lines · On 25 June 2018 at 07: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 | 238
+++++++++++++++++++++++++++++++++++++++++++++++
include/dm/uclass-id.h | 1 +
include/fs_loader.h | 28 ++++++Quoted 3 lines
5 files changed, 278 insertions(+)create mode 100644
drivers/misc/fs_loader.c
create mode 100644include/fs_loader.hThis is definitely looking good. I think we need to figure out the binding to devices, to use phandles instead of strings. Also we need a
sandbox driver and test.
Quoted 5 lines
I'm a little worried that you are blazing a trail here, but it is a valuable trail :-)
Tom, do you have any ideas / thoughts on the design?
yeah, this part is most challenging, because this driver is designed based on file system implementation, which is abstract from physical device. So, it requires data like interface type, device number and partition to work. Wonder how we can get those data if based on physical storage node.
Quoted 67 lines
Quoted 67 lines
drivers/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
+ 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 +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 +27
-0@@ -0,0 +1,238 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <linux/string.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 */ +}; + +static int select_fs_dev (struct device_platdata *plat) +{ + int ret; + + if (!strcmp("mmc", plat->name)) { + ret = fs_set_blk_dev("mmc", plat->devpart,
FS_TYPE_ANY);
Quoted 9 lines
Quoted 6 lines
+ } else if (!strcmp("usb",
plat->name)) {
+ ret =fs_set_blk_dev("usb", plat->devpart, FS_TYPE_ANY);
+ } else if (!strcmp("sata",plat->name)) { + ret =fs_set_blk_dev("sata", plat->devpart, FS_TYPE_ANY);For these I think you can look up the phandle to obtain the device, then check its uclass to find out its type.
How to find the interface type of the file system based on the physical device node? Some devices like NAND and QSPI could share the similar interface type like UBI.
Quoted 41 lines
Quoted 41 lines
+ } else if (!strcmp("ubi",
plat->name)) {
+ if (plat->ubivol)
+ ret =fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+ else
+ ret =-ENODEV;
+ } else {
+ debug("Error: unsupported storage device.\n"); + return-ENODEV;
+ }drivers/misc/fs_loader.c new file +8
-0+ + if (ret) + debug("Error: could not access storage.\n"); + + return ret; +} + +static void set_storage_devpart(struct device_platdata *plat, char*devpart)
drivers/misc/fs_loader.c new file +5
-0+{ + plat->devpart = devpart; +} + +static void set_storage_mtdpart(struct device_platdata *plat, char*mtdpart)
drivers/misc/fs_loader.c new file +5
-0+{ + plat->mtdpart = mtdpart; +} + +static void set_storage_ubivol(struct device_platdata *plat, char*ubivol)
drivers/misc/fs_loader.c new file +9
-0+{ + plat->ubivol = ubivol; +} + +/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file.
Quoted 9 lines
Quoted 7 lines
+ * @dbuf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * Return: Negative value if fail, 0 for successful.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,Can you please move this to be the last arg and rename to firmwarep?
Okay.
Quoted 38 lines
Quoted 38 lines
+ const char *name, void *dbuf,device to ubi
+ size_t size, u32 offset)drivers/misc/fs_loader.c new file +31
-0+{ + struct firmware *firmware; + struct firmware_priv *fw_priv; + + *firmware_p = NULL; + + if (!name || name[0] == '\0') + return -EINVAL; + + firmware = calloc(1, sizeof(*firmware)); + if (!firmware) + return -ENOMEM; + + fw_priv = calloc(1, sizeof(*fw_priv)); + if (!fw_priv) { + free(firmware); + return -ENOMEM; + } + + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + + return 0; +} + +/** +* fw_get_filesystem_firmware - load firmware into an allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
Quoted 9 lines
Quoted 7 lines
+ * @firmware_p: 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_p)s/firmware_p/firmware/
Okay.
Quoted 17 lines
Quoted 4 lines
drivers/misc/fs_loader.c new file +3
-0+{ + struct firmware_priv *fw_priv = NULL; + loff_t actread;drivers/misc/fs_loader.c line 127
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ char *dev_part, *ubi_mtdpart, *ubi_volume; + int ret; + + dev_part = env_get("fw_dev_part"); + if (dev_part) + set_storage_devpart(plat, dev_part); + + ubi_mtdpart = env_get("fw_ubi_mtdpart"); + if (ubi_mtdpart) + set_storage_mtdpart(plat, ubi_mtdpart);Do you mean that the storage partition comes from the environment? I thought it came from the FDT?
This driver is designed not only supporting the FDT, it also work with U-boot env, U-boot script and without FDT.
For example, fpga loadfs commands from U-boot console can call this driver to load bitstream file from the storage.
Quoted 16 lines
Quoted 13 lines
drivers/misc/fs_loader.c new file +12
-0+ + ubi_volume = env_get("fw_ubi_volume"); + if (ubi_volume) + set_storage_ubivol(plat, ubi_volume); + + ret = select_fs_dev(plat); + if (ret) + goto out; + + fw_priv = firmware_p->priv; + + ret = fs_read(fw_priv->name, (ulong)firmware_p->data,Quoted 1 line
fw_priv->offset,
map_to_sysmem(firmware_p->data)
Quoted 2 lines
(so that it works on sandbox)
Okay.
Quoted 152 lines
Quoted 152 lines
+ firmware_p->size, &actread);
+ if (ret) {
+ debug("Error: %d Failed to read %s from flash %lld != %d.\n",
+ ret,fw_priv->name, actread, firmware_p-Quoted 1 line
size);
+ } else {
+ ret = actread;
+ }drivers/misc/fs_loader.c new file +7
-0+ +out: + return ret; +} + +/** +* request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @firmware_p: Pointer to firmware image.
+ * @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.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmware_p 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, + struct firmware **firmware_p, + const char *name, + void *buf, size_t size, u32 offset) +{ + int ret; + + if (!plat) + return -EINVAL; + + ret = _request_firmware_prepare(firmware_p, name, buf,size, offset);
+ if (ret < 0) /* error */
+ return ret;drivers/misc/fs_loader.c new file +21
-0+ + ret = fw_get_filesystem_firmware(plat, *firmware_p); + + return ret; +} + +static int fs_loader_ofdata_to_platdata (struct udevice *dev) +{ + const char *fs_loader_path; + + 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; + + plat->name = (char*)ofnode_read_string(fs_loader_node,
+ "storage_device"); +
+ plat->devpart = (char *)ofnode_read_string( + fs_loader_node, "devpart");
+
+ 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 = "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 +14
-0@@ -0,0 +1,28 @@ +/* +* 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 { + size_t size; /* Size of a file */ + const u8 *data; /* Buffer for file */ + void *priv; /* Firmware loader private fields*/
include/fs_loader.h new file +5
-0+}; + +struct device_platdata { + char *name; /* Such as mmc, usb,and sata. */ + char *devpart; /* Use the load command dev:partconventions */
+ char *mtdpart; /* MTD partition for ubi part */
Quoted 9 lines
include/fs_loader.h line 27
Quoted 7 lines
+ char *ubivol; /* UBI volume-name for ubifsmount */
include/fs_loader.h new file +6
-0+}; + +int request_firmware_into_buf (struct device_platdata *plat, + struct firmware **firmware_p, + const char *name, + void *buf, size_t size, u32 offset);Please can you add a full function comment here?
Okay.
Quoted 7 lines
Quoted 5 lines
+#endif
--
2.2.0Regards,
Simon
PATCH v3 3/3 common: Generic loader for file system
Hi Tien Fong,
Quoted 114 lines · Tien Fong
Quoted 27 lines · Simon Glass
Hi,
Quoted 9 lines · On 25 June 2018 at 07: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 10 lines
--- drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 238 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fs_loader.h | 28 ++++++ 5 files changed, 278 insertions(+) create mode 100644 drivers/misc/fs_loader.c create mode 100644 include/fs_loader.hThis is definitely looking good. I think we need to figure out the binding to devices, to use phandles instead of strings. Also we need a
sandbox driver and test.Quoted 5 lines
I'm a little worried that you are blazing a trail here, but it is a valuable trail :-)
Tom, do you have any ideas / thoughts on the design?
yeah, this part is most challenging, because this driver is designed based on file system implementation, which is abstract from physical device. So, it requires data like interface type, device number and partition to work. Wonder how we can get those data if based on physical storage node.
Quoted 67 lines
Quoted 67 lines
drivers/misc/Kconfig +5
-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 toload
+ the file image from the storage into target such as
memory.drivers/misc/Kconfig +2
-0+ + The consumer driver would then use this loader to programwhatever,
drivers/misc/Kconfig +2
-0+ 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 +27
-0@@ -0,0 +1,238 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <linux/string.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 */ +}; + +static int select_fs_dev (struct device_platdata *plat) +{ +int ret; + +if (!strcmp ("mmc" , plat->name)) { + ret = fs_set_blk_dev("mmc", plat->devpart,
FS_TYPE_ANY);Quoted 9 lines
Quoted 6 lines
drivers/misc/fs_loader.c new file +2
-0+ }else if (!strcmp ("usb" , plat->name)) { + ret = fs_set_blk_dev("usb", plat->devpart,
FS_TYPE_ANY);drivers/misc/fs_loader.c new file +2
-0+ }else if (!strcmp ("sata" , plat->name)) { + ret = fs_set_blk_dev("sata", plat->devpart,
FS_TYPE_ANY);For these I think you can look up the phandle to obtain the device, then check its uclass to find out its type.
Quoted 3 lines
How to find the interface type of the file system based on the physical device node? Some devices like NAND and QSPI could share the similar interface type like UBI.
See my other email on this.
Quoted 116 lines
Quoted 41 lines
Quoted 41 lines
drivers/misc/fs_loader.c new file +3
-0+ }else if (!strcmp ("ubi" , plat->name)) { +if (plat->ubivol) + ret = fs_set_blk_dev("ubi", NULL,
FS_TYPE_UBIFS);drivers/misc/fs_loader.c new file +14
-0+else + ret = -ENODEV ; + }else { +debug ("Error: unsupported storage device.\n" ); +return -ENODEV ; + } + +if (ret) +debug ("Error: could not access storage.\n" ); + +return ret; +} + +static void set_storage_devpart(struct device_platdata *plat, char*devpart)
drivers/misc/fs_loader.c new file +5
-0+{ + plat->devpart = devpart; +} + +static void set_storage_mtdpart(struct device_platdata *plat, char*mtdpart)
drivers/misc/fs_loader.c new file +5
-0+{ + plat->mtdpart = mtdpart; +} + +static void set_storage_ubivol(struct device_platdata *plat, char*ubivol)
drivers/misc/fs_loader.c new file +9
-0+{ + plat->ubivol = ubivol; +} + +/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file. Quoted 9 lines
Quoted 7 lines
+ * @dbuf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * Return: Negative value if fail, 0 for successful.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,Can you please move this to be the last arg and rename to firmwarep?
Okay.
Quoted 38 lines
Quoted 38 lines
+ const char *name, void
*dbuf,device to ubidrivers/misc/fs_loader.c new file +32
-0+ size_t size, u32 offset) +{ +struct firmware *firmware; +struct firmware_priv *fw_priv; + + *firmware_p =NULL ; + +if (!name || name[0 ] =='\0' ) +return -EINVAL ; + + firmware =calloc (1 ,sizeof (*firmware)); +if (!firmware) +return -ENOMEM ; + + fw_priv =calloc (1 ,sizeof (*fw_priv)); +if (!fw_priv) { +free (firmware); +return -ENOMEM ; + } + + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + +return 0 ; +} + +/** +* fw_get_filesystem_firmware - load firmware into an allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.Quoted 9 lines
Quoted 7 lines
+ * @firmware_p: 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_p)s/firmware_p/firmware/
Okay.
Quoted 17 lines
Quoted 4 lines
drivers/misc/fs_loader.c new file +3
-0+{ +struct firmware_priv *fw_priv =NULL ; +loff_t actread;drivers/misc/fs_loader.c line 127
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+char *dev_part, *ubi_mtdpart, *ubi_volume; +int ret; + + dev_part =env_get ("fw_dev_part" ); +if (dev_part) +set_storage_devpart (plat, dev_part); + + ubi_mtdpart =env_get ("fw_ubi_mtdpart" ); +if (ubi_mtdpart) +set_storage_mtdpart (plat, ubi_mtdpart);Do you mean that the storage partition comes from the environment? I thought it came from the FDT?
Quoted 5 lines
This driver is designed not only supporting the FDT, it also work with U-boot env, U-boot script and without FDT.
For example, fpga loadfs commands from U-boot console can call this driver to load bitstream file from the storage.
OK, but in that case why use environment? Can't you just put the parameters in the command?
[..]
Regards,
Simon
Chee
PATCH v3 3/3 common: Generic loader for file system
Quoted 133 lines · Simon Glass
Hi Tien Fong,
Quoted 129 lines · Tien Fong
Quoted 32 lines · Simon Glass
Hi,
Quoted 18 lines · On 25 June 2018 at 07: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 | 238
+++++++++++++++++++++++++++++++++++++++++++++++
include/dm/uclass-id.h | 1 +
include/fs_loader.h | 28 ++++++Quoted 3 lines
5 files changed, 278 insertions(+)create mode 100644
drivers/misc/fs_loader.c
create mode 100644include/fs_loader.hThis is definitely looking good. I think we need to figure out the
binding to devices, to use phandles instead of strings. Also we need
a
sandbox driver and test.Quoted 6 lines
I'm a little worried that you are blazing a trail here, but it is a
valuable trail :-)Tom, do you have any ideas / thoughts on the design?
yeah, this part is most challenging, because this driver is designed
based on file system implementation, which is abstract from physical
device. So, it requires data like interface type, device number and partition to work. Wonder how we can get those data if based on physical storage node.Quoted 82 lines
Quoted 71 lines
drivers/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
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 +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 +27
-0@@ -0,0 +1,238 @@ +/* +* Copyright (C) 2018 Intel Corporation <www.intel.com> +* +* SPDX-License-Identifier: GPL-2.0 +*/ +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <linux/string.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 */ +}; + +static int select_fs_dev (struct device_platdata *plat) +{ + int ret; + + if (!strcmp("mmc", plat->name)) { + ret = fs_set_blk_dev("mmc", plat->devpart,Quoted 7 lines
FS_TYPE_ANY);
+ } else if (!strcmp("usb",plat->name)) {
+ ret =fs_set_blk_dev("usb", plat->devpart, FS_TYPE_ANY);
+ } else if (!strcmp("sata",plat->name)) { + ret =fs_set_blk_dev("sata", plat->devpart, FS_TYPE_ANY);For these I think you can look up the phandle to obtain the device,
then check its uclass to find out its type.
Quoted 7 lines
Quoted 5 lines
How to find the interface type of the file system based on the physical
device node? Some devices like NAND and QSPI could share the similar
interface type like UBI.See my other email on this.
Okay.
Quoted 131 lines
Quoted 131 lines
Quoted 49 lines
Quoted 49 lines
+ } else if (!strcmp("ubi",
plat->name)) {
+ if (plat->ubivol)
+ ret =fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+ else
+ ret =-ENODEV;
+ } else {
+ debug("Error: unsupported storage device.\n"); + return-ENODEV;
+ }drivers/misc/fs_loader.c new file +8
-0+ + if (ret) + debug("Error: could not access storage.\n"); + + return ret; +} + +static void set_storage_devpart(struct device_platdata *plat,char
*devpart)drivers/misc/fs_loader.c new file +5
-0+{ + plat->devpart = devpart; +} + +static void set_storage_mtdpart(struct device_platdata *plat,char
*mtdpart)drivers/misc/fs_loader.c new file +5
-0+{ + plat->mtdpart = mtdpart; +} + +static void set_storage_ubivol(struct device_platdata *plat,char
*ubivol)drivers/misc/fs_loader.c new file +11
-0+{ + plat->ubivol = ubivol; +} + +/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into. +* @size: Size of buffer. Quoted 9 lines
Quoted 6 lines
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * Return: Negative value if fail, 0 for successful.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,Can you please move this to be the last arg and rename to firmwarep?
Okay.
Quoted 42 lines
Quoted 42 lines
+ const char *name, void *dbuf,device to ubi
+ size_t size, u32 offset)drivers/misc/fs_loader.c new file +31
-0+{ + struct firmware *firmware; + struct firmware_priv *fw_priv; + + *firmware_p = NULL; + + if (!name || name[0] == '\0') + return -EINVAL; + + firmware = calloc(1, sizeof(*firmware)); + if (!firmware) + return -ENOMEM; + + fw_priv = calloc(1, sizeof(*fw_priv)); + if (!fw_priv) { + free(firmware); + return -ENOMEM; + } + + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + + return 0; +} + +/** +* fw_get_filesystem_firmware - load firmware into an allocated
buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @firmware_p: pointer to firmware image.Quoted 9 lines
Quoted 7 lines
+ *
+ * Return: Size of total read, negative value when error.
+ */
+static int fw_get_filesystem_firmware(structdevice_platdata*plat,
+ struct firmware*firmware_p)s/firmware_p/firmware/
Okay.
Quoted 20 lines
Quoted 6 lines
drivers/misc/fs_loader.c new file +3
-0+{ + struct firmware_priv *fw_priv = NULL; + loff_t actread;drivers/misc/fs_loader.c line 127
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ char *dev_part, *ubi_mtdpart, *ubi_volume; + int ret; + + dev_part = env_get("fw_dev_part"); + if (dev_part) + set_storage_devpart(plat, dev_part); + + ubi_mtdpart = env_get("fw_ubi_mtdpart"); + if (ubi_mtdpart) + set_storage_mtdpart(plat, ubi_mtdpart);Do you mean that the storage partition comes from the environment? I
thought it came from the FDT?
Quoted 9 lines
Quoted 6 lines
This driver is designed not only supporting the FDT, it also work with
U-boot env, U-boot script and without FDT.For example, fpga loadfs commands from U-boot console can call this driver to load bitstream file from the storage.
OK, but in that case why use environment? Can't you just put the parameters in the command?
User can save the value like partition into environment and saving in the storage as new default env value. So, this env value would be used for every power on.
Quoted 4 lines
[..]
Regards,
Simon
PATCH v3 3/3 common: Generic loader for file system
In reply to Simon GlassQuoted 21 lines · Simon Glass
Hi,
Quoted 8 lines · On 25 June 2018 at 07: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 | 238 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fs_loader.h | 28 ++++++ 5 files changed, 278 insertions(+) create mode 100644 drivers/misc/fs_loader.c create mode 100644 include/fs_loader.h
Quoted 8 lines
This is definitely looking good. I think we need to figure out the binding to devices, to use phandles instead of strings. Also we need a sandbox driver and test.
I'm a little worried that you are blazing a trail here, but it is a valuable trail :-)
Tom, do you have any ideas / thoughts on the design?
If we're still able to support the various use cases, and everyone is happier than before, that's good enough for me for now. We can always evolve things as time goes on. Once there's tests of course :)
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/20180627/ba1fddcc/attachment.sig>
PATCH v3 3/3 common: Generic loader for file system
In reply to tien.fong.chee at intel.comHi,
please see some comments below.
Quoted 22 lines · On Mon, 25 Jun 2018 21:28:58 +0800 tien.fong.chee at intel.com tien.fong.chee…
drivers/misc/fs_loader.c new file +22
-0+/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into. +* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* +* Return: Negative value if fail, 0 for successful. +*/ +static int _request_firmware_prepare (struct firmware **firmware_p, +const char *name,void *dbuf, +size_t size,u32 offset) +{ +struct firmware *firmware; +struct firmware_priv *fw_priv; + + *firmware_p =NULL ; + +if (!name || name[0 ] =='\0' ) +return -EINVAL ;
drivers/misc/fs_loader.c line 94
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ + firmware =calloc (1 ,sizeof (*firmware)); +if (!firmware) +return -ENOMEM ; + + fw_priv =calloc (1 ,sizeof (*fw_priv)); +if (!fw_priv) { +free (firmware); +return -ENOMEM ; + }
please add a note to API description that the API user should free() *firmware_p and *firmware_p->priv structs after usage of request_firmware_into_buf(), otherwise it will always leak memory while subsequent calls of request_firmware_into_buf() with the same *firmware_p argument.
Or probably we should better allow request_firmware_into_buf() to be called multiple times with prepared *firmware_p stuct for reloading the same firmware image when needed.
Then request_firmware_into_buf() should check if the given *firmware_p stuct is already initialized and must not call _request_firmware_prepare() in this case.
drivers/misc/fs_loader.c line 104
Quoted 10 lines
drivers/misc/fs_loader.c new file +10
-0+ + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + +return 0 ; +}
--
Anatolij
Chee
PATCH v3 3/3 common: Generic loader for file system
Quoted 37 lines · Anatolij Gustschin
Hi,
please see some comments below.
Quoted 30 lines · On Mon, 25 Jun 2018 21:28:58 +0800 tien.fong.chee at intel.com tien.fong.chee…
drivers/misc/fs_loader.c new file +29
-0+/** +* _request_firmware_prepare - Prepare firmware struct. +* +* @firmware_p: Pointer to pointer to firmware image. +* @name: Name of firmware file. +* @dbuf: Address of buffer to load firmware into. +* @size: Size of buffer. +* @offset: Offset of a file for start reading into buffer. +* +* Return: Negative value if fail, 0 for successful. +*/ +static int _request_firmware_prepare (struct firmware **firmware_p, + const char *name, void *dbuf, + size_t size, u32 offset) +{ +struct firmware *firmware; +struct firmware_priv *fw_priv; + + *firmware_p =NULL ; + +if (!name || name[0 ] =='\0' ) +return -EINVAL ; + + firmware =calloc (1 ,sizeof (*firmware)); +if (!firmware) +return -ENOMEM ; + + fw_priv =calloc (1 ,sizeof (*fw_priv)); +if (!fw_priv) {
Quoted 9 lines
drivers/misc/fs_loader.c line 94
Quoted 3 lines
drivers/misc/fs_loader.c new file +3
-0+free (firmware); +return -ENOMEM ; + }please add a note to API description that the API user should
free()*firmware_pand*firmware_p->privstructs after usage ofrequest_firmware_into_buf(), otherwise it will always leak memory while subsequent calls ofrequest_firmware_into_buf()with the same*firmware_pargument.
Okay, i will add the description into doc. I can create a function to release memory allocation and setting both *firmware_p and fw_priv to null for user.
Quoted 7 lines
Or probably we should better allow
request_firmware_into_buf()to be called multiple times with prepared*firmware_pstuct for reloading the same firmware image when needed.
Thenrequest_firmware_into_buf()should check if the given*firmware_pstuct is already initialized and must not call_request_firmware_prepare()in this case.
Okay, this should work in this way by checking both *firmware_p and fw_priv, if they are not null, memory allocation would be skipped for better performance. However, *firmware_p always need to get updated with function arguments, because it could be chunk by chunk transferring if image is larger than available buffer.
Quoted 14 lines
drivers/misc/fs_loader.c line 104
Quoted 11 lines
drivers/misc/fs_loader.c new file +10
-0+ + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + +return 0 ; +}--
Anatolij