[U-Boot] [PATCH v3 0/3] Generic file system firmware loader DM
From: Tien Fong Chee tien.fong.chee@intel.com
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@lists.denx.de/msg286979.html [v1]: https://www.mail-archive.com/u-boot@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
From: Tien Fong Chee tien.fong.chee@intel.com
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
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com --- doc/driver-model/fs_firmware_loader.txt | 129 ++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 doc/driver-model/fs_firmware_loader.txt
diff --git a/doc/driver-model/fs_firmware_loader.txt b/doc/driver-model/fs_firmware_loader.txt new file mode 100644 index 0000000..102e14b --- /dev/null +++ b/doc/driver-model/fs_firmware_loader.txt @@ -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@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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Example for usb: + fs_loader1: fs_loader@1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader@2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader@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); + }
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com --- 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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci@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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; +}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + }; + + Example for usb: + fs_loader1: fs_loader@1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + }; + + Example for sata: + fs_loader2: fs_loader@2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + }; + + Example for ubi: + fs_loader3: fs_loader@3 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "ubi"; + mtdpart = "UBI", + ubivol = "ubi0"; + };
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci@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@0 {
This should be :
fs_loader0: fs-loader@0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
u-boot,dm-pre-reloc;compatible = "fs_loader";
How about u-boot,fs-loader since this is U-Boot specific I think.
storage_device = "mmc";
storage-device
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.
};+}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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@0 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "mmc";devpart = "0:1";};Example for usb:fs_loader1: fs_loader@1 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "usb";devpart = "0:1";};Example for sata:fs_loader2: fs_loader@2 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "sata";devpart = "0:1";};Example for ubi:fs_loader3: fs_loader@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?
};-- 2.2.0
Regards, Simon
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree- bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci@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@0 {
This should be :
+ fs_loader0: fs-loader@0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
+ u-boot,dm-pre-reloc; + compatible = "fs_loader";
How about u-boot,fs-loader since this is U-Boot specific I think.
+ storage_device = "mmc";
storage-device
Okay
+ 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.
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).
+ }; +}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + };
+ Example for usb: + fs_loader1: fs_loader@1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + };
+ Example for sata: + fs_loader2: fs_loader@2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + };
+ Example for ubi: + fs_loader3: fs_loader@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.
+ };
2.2.0
Regards, Simon
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree- bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci@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@0 {This should be :
fs_loader0: fs-loader@0 {since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
u-boot,dm-pre-reloc;compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
storage_device = "mmc";storage-device
Okay
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.
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.
};+}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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 formmc, usb,
sata and ubi as shown in below:Example for mmc:fs_loader0: fs_loader@0 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "mmc";devpart = "0:1";};Example for usb:fs_loader1: fs_loader@1 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "usb";devpart = "0:1";};Example for sata:fs_loader2: fs_loader@2 {u-boot,dm-pre-reloc;compatible = "fs_loader";storage_device = "sata";devpart = "0:1";};Example for ubi:fs_loader3: fs_loader@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.
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
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device- tree- bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci@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@0 {
This should be :
+ fs_loader0: fs-loader@0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
+ u-boot,dm-pre-reloc; + compatible = "fs_loader";
How about u-boot,fs-loader since this is U-Boot specific I think.
+ storage_device = "mmc";
storage-device
Okay
+ 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.
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.
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?
+ }; +}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + };
+ Example for usb: + fs_loader1: fs_loader@1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + };
+ Example for sata: + fs_loader2: fs_loader@2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + };
+ Example for ubi: + fs_loader3: fs_loader@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.
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
On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com
wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device- tree- bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci@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@0 {
This should be :
+ fs_loader0: fs-loader@0 {
since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
+ u-boot,dm-pre-reloc; + compatible = "fs_loader";
How about u-boot,fs-loader since this is U-Boot specific I think.
+ storage_device = "mmc";
storage-device
Okay
+ 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.
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.
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?
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?
+ }; +}; diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt new file mode 100644 index 0000000..78bea66 --- /dev/null +++ b/doc/device-tree-bindings/misc/fs_loader.txt @@ -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@0 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "mmc"; + devpart = "0:1"; + };
+ Example for usb: + fs_loader1: fs_loader@1 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "usb"; + devpart = "0:1"; + };
+ Example for sata: + fs_loader2: fs_loader@2 { + u-boot,dm-pre-reloc; + compatible = "fs_loader"; + storage_device = "sata"; + devpart = "0:1"; + };
+ Example for ubi: + fs_loader3: fs_loader@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.
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
Hi Tien Fong,
On 28 June 2018 at 01:58, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com
wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
Add a document to describe file system firmware loader binding information.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device- tree- bindings/chosen.txt index c96b8f7..738673c 100644 --- a/doc/device-tree-bindings/chosen.txt +++ b/doc/device-tree-bindings/chosen.txt @@ -73,3 +73,25 @@ Example u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd hci@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@0 {This should be :
fs_loader0: fs-loader@0 {since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
u-boot,dm-pre-reloc;compatible = "fs_loader";How about u-boot,fs-loader since this is U-Boot specific I think.
storage_device = "mmc";storage-device
Okay
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.
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.
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?
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,
- storage-interface:
block device - "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)?
non block device "ubi"
- 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?
Sounds right to me.
[...]
Regards, Simon
On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
Hi Tien Fong,
On 28 June 2018 at 01:58, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel .com
wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote: > > > > > From: Tien Fong Chee tien.fong.chee@intel.com > > Add a document to describe file system firmware loader > binding > information. > > Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com > --- > 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 > > diff --git a/doc/device-tree-bindings/chosen.txt > b/doc/device- > tree- > bindings/chosen.txt > index c96b8f7..738673c 100644 > --- a/doc/device-tree-bindings/chosen.txt > +++ b/doc/device-tree-bindings/chosen.txt > @@ -73,3 +73,25 @@ Example > u-boot,spl-boot-order = "same-as-spl", > &sdmmc, > "/sd > hci@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@0 { This should be :
> > > > > + fs_loader0: fs-loader@0 { since hyphen is used for node and property names. Underscore is used for phandles (so you have that correct).
Okay.
> > > > > + u-boot,dm-pre-reloc; > + compatible = "fs_loader"; How about u-boot,fs-loader since this is U-Boot specific I think.
> > > > > + storage_device = "mmc"; storage-device
Okay
> > > > > + 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.
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.
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?
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,
- storage-interface:
block device - "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)?
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?
non block device "ubi"
- 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?
Sounds right to me.
Is that possible one parent device(like MMC) contains multiple child devices in UCLASS_BLK?
[...]
Regards, Simon
Hi Tien Fong,
On 2 July 2018 at 01:26, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
Hi Tien Fong,
On 28 June 2018 at 01:58, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel .com
wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote: > > > > Hi, > > On 25 June 2018 at 07:28, tien.fong.chee@intel.com > wrote: > > > > > > > > > > From: Tien Fong Chee tien.fong.chee@intel.com > > > > Add a document to describe file system firmware loader > > binding > > information. > > > > Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com > > --- > > 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 > > > > diff --git a/doc/device-tree-bindings/chosen.txt > > b/doc/device- > > tree- > > bindings/chosen.txt > > index c96b8f7..738673c 100644 > > --- a/doc/device-tree-bindings/chosen.txt > > +++ b/doc/device-tree-bindings/chosen.txt > > @@ -73,3 +73,25 @@ Example > > u-boot,spl-boot-order = "same-as-spl", > > &sdmmc, > > "/sd > > hci@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@0 { > This should be : > > > > > > > > > > > + fs_loader0: fs-loader@0 { > since hyphen is used for node and property names. > Underscore is > used > for phandles (so you have that correct). > Okay. > > > > > > > > > > > > > + u-boot,dm-pre-reloc; > > + compatible = "fs_loader"; > How about u-boot,fs-loader since this is U-Boot specific I > think. > > > > > > > > > > > > + storage_device = "mmc"; > storage-device > Okay > > > > > > > > > > > > > + 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. > 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.
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?
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,
- storage-interface:
block device - "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)?
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), };
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.
non block device "ubi"
- 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?
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
On Sun, 2018-07-08 at 20:39 -0600, Simon Glass wrote:
Hi Tien Fong,
On 2 July 2018 at 01:26, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
Hi Tien Fong,
On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.c om> wrote:
On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@i ntel .com > > > wrote: > > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote: > > > > > > > > > > Hi, > > > > On 25 June 2018 at 07:28, tien.fong.chee@intel.com > > wrote: > > > > > > > > > > > > > > > > > > From: Tien Fong Chee tien.fong.chee@intel.com > > > > > > Add a document to describe file system firmware > > > loader > > > binding > > > information. > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.c > > > 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.txt > > > > > > diff --git a/doc/device-tree-bindings/chosen.txt > > > b/doc/device- > > > tree- > > > bindings/chosen.txt > > > index c96b8f7..738673c 100644 > > > --- a/doc/device-tree-bindings/chosen.txt > > > +++ b/doc/device-tree-bindings/chosen.txt > > > @@ -73,3 +73,25 @@ Example > > > u-boot,spl-boot-order = "same-as- > > > spl", > > > &sdmmc, > > > "/sd > > > hci@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@0 { > > This should be : > > > > > > > > > > > > > > > > > > > > + fs_loader0: fs-loader@0 { > > since hyphen is used for node and property names. > > Underscore is > > used > > for phandles (so you have that correct). > > > Okay. > > > > > > > > > > > > > > > > > > > > > > > > > > + u-boot,dm-pre-reloc; > > > + compatible = "fs_loader"; > > How about u-boot,fs-loader since this is U-Boot > > specific I > > think. > > > > > > > > > > > > > > > > > > > > > > + storage_device = "mmc"; > > storage-device > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > + 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. > > > 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.
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?
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,
- storage-interface:
block device - "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)?
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), };
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.
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.
non block device "ubi"
- 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?
Sounds right to me.
Is that possible one parent device(like MMC) contains multiple child devices in UCLASS_BLK?
No, only one is supported.
Noted.
Regards, Simon
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com --- drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 17b3a80..4163b4f 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL depends on MISC help Support gdsys FPGA's RXAUI control. + +config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be used to load + the file image from the storage into target such as memory. + + The consumer driver would then use this loader to program whatever, + ie. the FPGA device. + endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 4ce9d21..67a36f8 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o +obj-$(CONFIG_FS_LOADER) += fs_loader.o diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c new file mode 100644 index 0000000..0dc385f --- /dev/null +++ b/drivers/misc/fs_loader.c @@ -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); + } 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); + } 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. + * + * @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) { + 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. + * + * Return: Size of total read, negative value when error. + */ +static int fw_get_filesystem_firmware(struct device_platdata *plat, + struct firmware *firmware_p) +{ + 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); + + 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, + 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", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index d7f9df3..39e88ac 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -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 */ diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..e3e5eeb --- /dev/null +++ b/include/fs_loader.h @@ -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
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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?
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 17b3a80..4163b4f 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL depends on MISC help Support gdsys FPGA's RXAUI control.
+config FS_LOADER
bool "Enable loader driver for file system"helpThis is file system generic loader which can be used to loadthe file image from the storage into target such as memory.The consumer driver would then use this loader to program whatever,ie. the FPGA device.endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 4ce9d21..67a36f8 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o +obj-$(CONFIG_FS_LOADER) += fs_loader.o diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c new file mode 100644 index 0000000..0dc385f --- /dev/null +++ b/drivers/misc/fs_loader.c @@ -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);} 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.
} else if (!strcmp("ubi", plat->name)) {if (plat->ubivol)ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);elseret = -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.
- @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?
const char *name, void *dbuf,device to ubisize_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.
- @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/
+{
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?
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)
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",+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index d7f9df3..39e88ac 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -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 */diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..e3e5eeb --- /dev/null +++ b/include/fs_loader.h @@ -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);
Please can you add a full function comment here?
+#endif
2.2.0
Regards, Simon
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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?
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.
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 17b3a80..4163b4f 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL depends on MISC help Support gdsys FPGA's RXAUI control.
+config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be used to load + the file image from the storage into target such as memory.
+ The consumer driver would then use this loader to program whatever, + ie. the FPGA device.
endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 4ce9d21..67a36f8 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o +obj-$(CONFIG_FS_LOADER) += fs_loader.o diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c new file mode 100644 index 0000000..0dc385f --- /dev/null +++ b/drivers/misc/fs_loader.c @@ -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); + } 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.
+ } 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.
- @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?
Okay.
+ 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; +}
+/**
- 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/
Okay.
+{ + 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?
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.
+ 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)
Okay.
+ 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", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index d7f9df3..39e88ac 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -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 */ diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..e3e5eeb --- /dev/null +++ b/include/fs_loader.h @@ -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);
Please can you add a full function comment here?
Okay.
+#endif
2.2.0
Regards, Simon
Hi Tien Fong,
On 27 June 2018 at 01:41, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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?
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.
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 17b3a80..4163b4f 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL depends on MISC help Support gdsys FPGA's RXAUI control.
+config FS_LOADER
bool "Enable loader driver for file system"helpThis is file system generic loader which can be used toload
the file image from the storage into target such asmemory.
The consumer driver would then use this loader to programwhatever,
ie. the FPGA device.endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 4ce9d21..67a36f8 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o +obj-$(CONFIG_FS_LOADER) += fs_loader.o diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c new file mode 100644 index 0000000..0dc385f --- /dev/null +++ b/drivers/misc/fs_loader.c @@ -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);
} 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.
See my other email on this.
} else if (!strcmp("ubi", plat->name)) {if (plat->ubivol)ret = fs_set_blk_dev("ubi", NULL,FS_TYPE_UBIFS);
elseret = -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.
- @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?
Okay.
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;+}
+/**
- 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/
Okay.
+{
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?
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
On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
Hi Tien Fong,
On 27 June 2018 at 01:41, Chee, Tien Fong tien.fong.chee@intel.com wrote:
On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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?
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.
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 17b3a80..4163b4f 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL depends on MISC help Support gdsys FPGA's RXAUI control.
+config FS_LOADER + bool "Enable loader driver for file system" + help + This is file system generic loader which can be used to load + the file image from the storage into target such as memory.
+ The consumer driver would then use this loader to program whatever, + ie. the FPGA device.
endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 4ce9d21..67a36f8 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o +obj-$(CONFIG_FS_LOADER) += fs_loader.o diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c new file mode 100644 index 0000000..0dc385f --- /dev/null +++ b/drivers/misc/fs_loader.c @@ -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); + } 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.
See my other email on this.
Okay.
+ } 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.
- @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?
Okay.
+ 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; +}
+/**
- 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/
Okay.
+{ + 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?
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.
[..]
Regards, Simon
On Mon, Jun 25, 2018 at 09:58:51PM -0600, Simon Glass wrote:
Hi,
On 25 June 2018 at 07:28, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
drivers/misc/Kconfig | 10 ++ drivers/misc/Makefile | 1 + drivers/misc/fs_loader.c | 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?
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 :)
Hi,
please see some comments below.
On Mon, 25 Jun 2018 21:28:58 +0800 tien.fong.chee@intel.com tien.fong.chee@intel.com wrote:
+/**
- _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) {
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.
- fw_priv->name = name;
- fw_priv->offset = offset;
- firmware->data = dbuf;
- firmware->size = size;
- firmware->priv = fw_priv;
- *firmware_p = firmware;
- return 0;
+}
-- Anatolij
On Fri, 2018-06-29 at 14:13 +0200, Anatolij Gustschin wrote:
Hi,
please see some comments below.
On Mon, 25 Jun 2018 21:28:58 +0800 tien.fong.chee@intel.com tien.fong.chee@intel.com wrote:
+/**
- _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) {
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.
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.
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.
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.
- fw_priv->name = name;
- fw_priv->offset = offset;
- firmware->data = dbuf;
- firmware->size = size;
- firmware->priv = fw_priv;
- *firmware_p = firmware;
- return 0;
+}
-- Anatolij
participants (5)
-
Anatolij Gustschin -
Chee, Tien Fong -
Simon Glass -
tien.fong.chee＠intel.com
-
Tom Rini