[U-Boot-Users] LIBFDT: first version of fdt_find_compatible_node
Hello,
attached you can find a patch implementing fdt_find_compatible_node():
/* * Find a node based on its device type and one of the tokens in * its its "compatible" property. On success, the offset of that * node is returned or an error code: * * startoffset - the node to start searching from or 0, the node * you pass will not be searched, only the next one * will; typically, you pass 0 to start the search * and then what the previous call returned. * type - the device type string to match against * compat - the string to match to one of the tokens * in the "compatible" list. */
It should be used as shown below:
offset = 0; do { offset = fdt_find_compatible_node(fdt, offset, "type", "comp"); } while (offset >= 0);
This first hack also implements a cached version as alternative, because tag re-scanning might take quite long. In principle, the cache could also be used for other functions, like fdt_path_offset(), and could be invalidated in case the FDT gets updated.
What do you think?
Thanks.
Wolfgang.
Wolfgang Grandegger wrote:
Hello,
attached you can find a patch implementing fdt_find_compatible_node():
Yeeee-ha! :-)
/*
- Find a node based on its device type and one of the tokens in
- its its "compatible" property. On success, the offset of that
- node is returned or an error code:
- startoffset - the node to start searching from or 0, the node
you pass will not be searched, only the next onewill; typically, you pass 0 to start the searchand then what the previous call returned.- type - the device type string to match against
- compat - the string to match to one of the tokens
in the "compatible" list.*/
It should be used as shown below:
offset = 0; do { offset = fdt_find_compatible_node(fdt, offset, "type", "comp"); } while (offset >= 0);This first hack also implements a cached version as alternative, because tag re-scanning might take quite long. In principle, the cache could also be used for other functions, like fdt_path_offset(), and could be invalidated in case the FDT gets updated.
What do you think?
Thanks.
Wolfgang.
Looks good. In the real patch, I would like to see the cache addition split from the fdt_node_is_compatible() fdt_find_compatible_node() and addition.
diff --git a/include/libfdt.h b/include/libfdt.h index f8bac73..3fd7c9f 100644 --- a/include/libfdt.h +++ b/include/libfdt.h @@ -89,6 +89,12 @@ uint32_t fdt_next_tag(const void *fdt, int offset, int fdt_num_reservemap(void *fdt, int *used, int *total); int fdt_get_reservemap(void *fdt, int n, struct fdt_reserve_entry *re);
+int fdt_node_is_compatible(const void *fdt, int nodeoffset,
const char *compat);+int fdt_find_compatible_node(const void *fdt, int startoffset,
const char *type, const char *compat);/* Write-in-place functions */ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, const void *val, int len); diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c index 4e2c325..65ede88 100644 --- a/libfdt/fdt_ro.c +++ b/libfdt/fdt_ro.c @@ -55,6 +55,236 @@ char *fdt_string(const void *fdt, int stroffset) return (char *)fdt + fdt_off_dt_strings(fdt) + stroffset; }
+#define CONFIG_OF_LIBFDT_TABLE 64 +#if CONFIG_OF_LIBFDT_TABLE > 0
[snip]
+#endif /* CONFIG_OF_LIBFDT_TABLE > 0 */
+/*
- Check if the specified node is compatible by comparing the
- tokens in its "compatible" property with the specified string:
- nodeoffset - starting place of the node
- compat - the string to match to one of the tokens
in the "compatible" list.- */
+int fdt_node_is_compatible(const void *fdt, int nodeoffset,
const char *compat)+{
- const char* cp;
- int cplen, l;
- cp = fdt_getprop(fdt, nodeoffset, "compatible", &cplen);
- if (cp == NULL)
return 0;- while (cplen > 0) {
if (strncmp(cp, compat, strlen(compat)) == 0)return 1;l = strlen(cp) + 1;cp += l;cplen -= l;- }
- return 0;
+}
I see this came directly from arch/powerpc/kernel/prom.c, but using "l" for a variable is evil. For a minute, I was wondering how the compiler was compiling "1" (one) as a lvalue. I would prefer it to be "len" or something more descriptive.
+/*
- Find a node based on its device type and one of the tokens in
- its its "compatible" property. On success, the offset of that
- node is return or an error code:
- startoffset - the node to start searching from or 0, the node
you pass will not be searched, only the next one
will; typically, you pass 0 to start the search
and then what the previous call returned.
- type - the device type string to match against
- compat - the string to match to one of the tokens
in the "compatible" list.- */
+#if CONFIG_OF_LIBFDT_TABLE > 0
[snip]
+#else /* CONFIG_OF_LIBFDT_TABLE <= 0 */ +int fdt_find_compatible_node(const void *fdt, int startoffset,
const char *type, const char *compat)+{
- static int level, typefound;
- static int nodeoffset, nextoffset;
- int offset, namestroff;
- struct fdt_property *prop;
- uint32_t tag;
- CHECK_HEADER(fdt);
- if (startoffset == 0) {
level = 0;tag = fdt_next_tag(fdt, 0, &nextoffset, NULL);if (tag != FDT_BEGIN_NODE)return -FDT_ERR_BADOFFSET;- } else if (startoffset != nodeoffset)
return -FDT_ERR_BADOFFSET;- do {
offset = nextoffset;tag = fdt_next_tag(fdt, offset, &nextoffset, NULL);switch (tag) {case FDT_END:return -FDT_ERR_TRUNCATED;case FDT_BEGIN_NODE:level++;nodeoffset = offset;typefound = 0;break;case FDT_END_NODE:level--;break;case FDT_PROP:if (typefound)continue;prop = fdt_offset_ptr_typed(fdt, offset, prop);if (! prop)continue;namestroff = fdt32_to_cpu(prop->nameoff);if (streq(fdt_string(fdt, namestroff), "device_type")) {int len = fdt32_to_cpu(prop->len);typefound = 0;prop = fdt_offset_ptr(fdt, offset,sizeof(*prop)+len);if (! prop)continue;if (strncasecmp(prop->data, type, len - 1) == 0 &&fdt_node_is_compatible(fdt, nodeoffset, compat))return nodeoffset;}break;case FDT_NOP:break;default:return -FDT_ERR_BADSTRUCTURE;}- } while (level >= 0);
- return -FDT_ERR_NOTFOUND;
+} +#endif /* CONFIG_OF_LIBFDT_TABLE > 0 */
/*
- Return the node offset of the node specified by:
- parentoffset - starting place (0 to start at the root)
For the above version of fdt_find_compatible_node(), as well as the one that fills the cache table (snipped), I'm thinking it would be better to use the function I added:
uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char **namep)
I added this to step through the nodes and properties for dumping the tree. Rather than having Yet Another (slightly modified) Copy of the loop & switch, you should be able to use fdt_next_tag() to step through the nodes and properties, doing the if (streq(fdt_string(fdt, namestroff), "device_type")) on the **namep parameter on every call to find the device_type properties. Does this make sense?
Thanks, gvb
Jerry Van Baren wrote:
Wolfgang Grandegger wrote:
Hello,
attached you can find a patch implementing fdt_find_compatible_node():
Yeeee-ha! :-)
/*
- Find a node based on its device type and one of the tokens in
- its its "compatible" property. On success, the offset of that
- node is returned or an error code:
- startoffset - the node to start searching from or 0, the node
you pass will not be searched, only the next onewill; typically, you pass 0 to start the searchand then what the previous call returned.- type - the device type string to match against
- compat - the string to match to one of the tokens
in the "compatible" list.*/
It should be used as shown below:
offset = 0; do { offset = fdt_find_compatible_node(fdt, offset, "type", "comp"); } while (offset >= 0);This first hack also implements a cached version as alternative, because tag re-scanning might take quite long. In principle, the cache could also be used for other functions, like fdt_path_offset(), and could be invalidated in case the FDT gets updated.
What do you think?
Thanks.
Wolfgang.
Looks good. In the real patch, I would like to see the cache addition split from the fdt_node_is_compatible() fdt_find_compatible_node() and addition.
Yes, OK, the patch needs some cleanup and improvement.
[...]
+#endif /* CONFIG_OF_LIBFDT_TABLE > 0 */
+/*
- Check if the specified node is compatible by comparing the
- tokens in its "compatible" property with the specified string:
- nodeoffset - starting place of the node
- compat - the string to match to one of the tokens
in the "compatible" list.- */
+int fdt_node_is_compatible(const void *fdt, int nodeoffset,
const char *compat)+{
- const char* cp;
- int cplen, l;
- cp = fdt_getprop(fdt, nodeoffset, "compatible", &cplen);
- if (cp == NULL)
return 0;- while (cplen > 0) {
if (strncmp(cp, compat, strlen(compat)) == 0)return 1;l = strlen(cp) + 1;cp += l;cplen -= l;- }
- return 0;
+}
I see this came directly from arch/powerpc/kernel/prom.c, but using "l" for a variable is evil. For a minute, I was wondering how the compiler was compiling "1" (one) as a lvalue. I would prefer it to be "len" or something more descriptive.
The code looks strange indeed. Will use "len" instead of "1", aheee "l".
[...]
For the above version of fdt_find_compatible_node(), as well as the one that fills the cache table (snipped), I'm thinking it would be better to use the function I added:
uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char **namep)
I added this to step through the nodes and properties for dumping the tree. Rather than having Yet Another (slightly modified) Copy of the loop & switch, you should be able to use fdt_next_tag() to step through the nodes and properties, doing the if (streq(fdt_string(fdt, namestroff), "device_type")) on the **namep parameter on every call to find the device_type properties. Does this make sense?
Yes, my concern was the overhead due to looking up the location of each node name and property. But for fast scanning, the cached version is much better anyhow, I think.
What is your opinion on the cached version? Do we need it? Should we keep both?
Wolfgang.
Wolfgang Grandegger wrote:
Jerry Van Baren wrote:
Wolfgang Grandegger wrote:
Hello,
attached you can find a patch implementing fdt_find_compatible_node():
[snip]
For the above version of fdt_find_compatible_node(), as well as the one that fills the cache table (snipped), I'm thinking it would be better to use the function I added:
uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char **namep)
I added this to step through the nodes and properties for dumping the tree. Rather than having Yet Another (slightly modified) Copy of the loop & switch, you should be able to use fdt_next_tag() to step through the nodes and properties, doing the if (streq(fdt_string(fdt, namestroff), "device_type")) on the **namep parameter on every call to find the device_type properties. Does this make sense?
Yes, my concern was the overhead due to looking up the location of each node name and property. But for fast scanning, the cached version is much better anyhow, I think.
What is your opinion on the cached version? Do we need it? Should we keep both?
Wolfgang.
Hi wg,
Yes, blob parsing will be done from the start of the blob until an answer is found every time a question is asked of it. Not a paradigm of efficiency. :-/
WRT the cached version, I have doubts about how much time it will save since I expect the "find compatible" will only be used during initialization. Is it worth optimizing? Really slow memory - yes. Fast memory - I doubt it. a) I don't picture blobs being stored in really slow memory (no i2c memories). b) If the memory really is slow, it seems like it would be as good or better to copy the blob to RAM and use it out of RAM (but there may be chicken & egg problems with that - I don't know how deeply you are looking to embed this).
I don't know what board/processor/memory you are ultimately targeting with this, so my criticisms may not be valid. I know denx.de support(s|ed) some very slow to boot boards that lots of tricks were done WRT optimization of env variables because they were stored in i2c memory.
--------
Other puzzlements:
You are storing "level" in the cache table, I don't see why. That is only used while scanning the blob to keep track of node nesting and unnesting and exit when we've unnested back out to the original level. Nesting isn't really applicable when it is in the cache.
In fdt_find_compatible_node() you have local static variables: + static int level, typefound; + static int nodeoffset, nextoffset; and I don't understand why. I expected them to be auto variables, initialized on entry and discarded on exit.
Looking closer at your cached version, I see you really are using nodeoffset as a persistent variable there and I cringe a bit. That implementation presumes you ask questions sequentially or always reset to 0. This would prevent you from, for instance, searching for the "/soc8360@..." node using fdt_path_offset() and then looking for devices inside that node. Given what is being represented and how, perhaps I'm creating an unreasonable strawman.
Instead of using a static variable, you could scan the cache table for an element whose stored offset is greater than or equal to startoffset, and search the table forward from there (the offsets in the table will be monotonically increasing because of how you build the table). Slightly less efficient, but it will still be pretty good and much better than scanning the whole blob... or maybe I'm complaining based on an unreasonable strawman.
After writing all the above, it bothers me a bit that the hierarchical tree structure of the device tree is getting stuck into a one dimensional cache (the hierarchy is lost), but I cannot think at this point why that would bite us down the road.
Best regards, gvb
Jerry Van Baren wrote:
Wolfgang Grandegger wrote:
Jerry Van Baren wrote:
Wolfgang Grandegger wrote:
Hello,
attached you can find a patch implementing fdt_find_compatible_node():
[snip]
For the above version of fdt_find_compatible_node(), as well as the one that fills the cache table (snipped), I'm thinking it would be better to use the function I added:
uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char **namep)
I added this to step through the nodes and properties for dumping the tree. Rather than having Yet Another (slightly modified) Copy of the loop & switch, you should be able to use fdt_next_tag() to step through the nodes and properties, doing the if (streq(fdt_string(fdt, namestroff), "device_type")) on the **namep parameter on every call to find the device_type properties. Does this make sense?
Yes, my concern was the overhead due to looking up the location of each node name and property. But for fast scanning, the cached version is much better anyhow, I think.
What is your opinion on the cached version? Do we need it? Should we keep both?
Wolfgang.
Hi wg,
Yes, blob parsing will be done from the start of the blob until an answer is found every time a question is asked of it. Not a paradigm of efficiency. :-/
WRT the cached version, I have doubts about how much time it will save since I expect the "find compatible" will only be used during initialization. Is it worth optimizing? Really slow memory - yes. Fast memory - I doubt it. a) I don't picture blobs being stored in really slow memory (no i2c memories). b) If the memory really is slow, it seems like it would be as good or better to copy the blob to RAM and use it out of RAM (but there may be chicken & egg problems with that - I don't know how deeply you are looking to embed this).
I don't know what board/processor/memory you are ultimately targeting with this, so my criticisms may not be valid. I know denx.de support(s|ed) some very slow to boot boards that lots of tricks were done WRT optimization of env variables because they were stored in i2c memory.
I'm doing that for a MPC823 at 50 MHz, a very low-end system, and almost to slow for 2.6. I will do some real measurements when time permits to get a better feeling.
Other puzzlements:
You are storing "level" in the cache table, I don't see why. That is only used while scanning the blob to keep track of node nesting and unnesting and exit when we've unnested back out to the original level. Nesting isn't really applicable when it is in the cache.
In fdt_find_compatible_node() you have local static variables:
- static int level, typefound;
- static int nodeoffset, nextoffset;
and I don't understand why. I expected them to be auto variables, initialized on entry and discarded on exit.
This is to continue a scan started with startoffset=0.
* startoffset - the node to start searching from or 0, the node * you pass will not be searched, only the next one * will; typically, you pass 0 to start the search * and then what the previous call returned.
And it could be used in the following way:
offset = 0; do { offset = fdt_find_compatible_node(fdt, offset, "type", "comp"); } while (offset >= 0);
This is to be compatible with the Linux version (prom.c) and to scan for more than one compatible device efficiently (without re-scanning). For example, for the MPC5200 there are 8 compatible GPTs defined. To do so, we must preserve nextoffset and level. I also preserve typefound and nodeoffset for efficiency reasons and to check sub-sequent calls of fdt_find_compatible_node().
Looking closer at your cached version, I see you really are using nodeoffset as a persistent variable there and I cringe a bit. That implementation presumes you ask questions sequentially or always reset to 0. This would prevent you from, for instance, searching for the "/soc8360@..." node using fdt_path_offset() and then looking for devices inside that node. Given what is being represented and how, perhaps I'm creating an unreasonable strawman.
Yes that's the current behaviour for both versions. I thought a scan will always be started with 0. Nevertheless, as I see it, the Linux version does not use the hierarchy but just scans all device (nodes) sequentially. But this could be changed easily and I was already thinking to do so.
Instead of using a static variable, you could scan the cache table for an element whose stored offset is greater than or equal to startoffset, and search the table forward from there (the offsets in the table will be monotonically increasing because of how you build the table). Slightly less efficient, but it will still be pretty good and much better than scanning the whole blob... or maybe I'm complaining based on an unreasonable strawman.
Slightly less efficient, yes, that was my motivation. In U-Boot we access the libfdt always sequentially and therefore the trick with static variables for efficiency seems OK to me.
After writing all the above, it bothers me a bit that the hierarchical tree structure of the device tree is getting stuck into a one dimensional cache (the hierarchy is lost), but I cannot think at this point why that would bite us down the road.
See above. I have to check my assumption, though. Let's clarify first the intended behavior of fdt_find_compatible_node().
Wolfgang.
Hi Jerry,
Wolfgang Grandegger wrote:
Jerry Van Baren wrote:
[snip]
Yes, blob parsing will be done from the start of the blob until an answer is found every time a question is asked of it. Not a paradigm of efficiency. :-/
WRT the cached version, I have doubts about how much time it will save since I expect the "find compatible" will only be used during initialization. Is it worth optimizing? Really slow memory - yes. Fast memory - I doubt it. a) I don't picture blobs being stored in really slow memory (no i2c memories). b) If the memory really is slow, it seems like it would be as good or better to copy the blob to RAM and use it out of RAM (but there may be chicken & egg problems with that - I don't know how deeply you are looking to embed this).
I don't know what board/processor/memory you are ultimately targeting with this, so my criticisms may not be valid. I know denx.de support(s|ed) some very slow to boot boards that lots of tricks were done WRT optimization of env variables because they were stored in i2c memory.
I'm doing that for a MPC823 at 50 MHz, a very low-end system, and almost to slow for 2.6. I will do some real measurements when time permits to get a better feeling.
Here are the results of some quick measurements on my MPC855 at 80/40 MHz with the attached code example and my DTS test file:
from FLASH from Memory Non-cached: 11116 us 1703 us Cached : 2800 us 6226 us
Well, I think we can drop the cached version even if its 4 times faster, as it make life more difficult, especially in case the FDT gets updated.
Wolfgang.
/* * Lite5200 board Device Tree Source * * Copyright 2006-2007 Secret Lab Technologies Ltd. * Grant Likely grant.likely@secretlab.ca * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the * Free Software Foundation; either version 2 of the License, or (at your * option) any later version. */
/* * WARNING: Do not depend on this tree layout remaining static just yet. * The MPC5200 device tree conventions are still in flux * Keep an eye on the linuxppc-dev mailing list for more details */
/ { model = "fsl,lite5200"; // revision = "1.0"; compatible = "fsl,lite5200\0generic-mpc5200"; #address-cells = <1>; #size-cells = <1>;
cpus { #cpus = <1>; #address-cells = <1>; #size-cells = <0>;
PowerPC,5200@0 { device_type = "cpu"; reg = <0>; d-cache-line-size = <20>; i-cache-line-size = <20>; d-cache-size = <4000>; // L1, 16K i-cache-size = <4000>; // L1, 16K timebase-frequency = <0>; // from bootloader bus-frequency = <0>; // from bootloader clock-frequency = <0>; // from bootloader 32-bit; }; };
memory { device_type = "memory"; reg = <00000000 04000000>; // 64MB };
flash@ff800000 { device_type = "rom"; compatible = "direct-mapped"; probe-type = "CFI"; reg = <40000000 00800000>; bank-width = <4>; bank-count = <2>; partitions = <00000000 00040001 00040000 000c0001 00100000 00100000 00200000 00200000 00400000 00200000 00600000 00200000 00000000 00800001>; partition-names = "u-boot\0kernel\0user\0ramdisk\0cramfs\0jffs2\0all"; };
can@c0000000 { device_type = "can"; compatible = "mpc8xx-sja1000"; reg = <c0000000 00010000>;
can@c0000000 { compatible = "mpc8xx-sja1000"; reg = <000 100>; type = "upmb"; irq = <4>; }; };
soc5200@f0000000 { model = "fsl,mpc5200"; revision = ""; // from bootloader #interrupt-cells = <3>; device_type = "soc"; ranges = <0 f0000000 f0010000>; reg = <f0000000 00010000>; bus-frequency = <0>; // from bootloader system-frequency = <0>; // from bootloader
cdm@200 { compatible = "mpc5200-cdm"; reg = <200 38>; };
pic@500 { // 5200 interrupts are encoded into two levels; linux,phandle = <500>; interrupt-controller; #interrupt-cells = <3>; device_type = "interrupt-controller"; compatible = "mpc5200-pic"; reg = <500 80>; built-in; };
gpt@600 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <0>; reg = <600 10>; interrupts = <1 9 0>; interrupt-parent = <500>; has-wdt; };
gpt@610 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <1>; reg = <610 10>; interrupts = <1 a 0>; interrupt-parent = <500>; };
gpt@620 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <2>; reg = <620 10>; interrupts = <1 b 0>; interrupt-parent = <500>; };
gpt@630 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <3>; reg = <630 10>; interrupts = <1 c 0>; interrupt-parent = <500>; };
gpt@640 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <4>; reg = <640 10>; interrupts = <1 d 0>; interrupt-parent = <500>; };
gpt@650 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <5>; reg = <650 10>; interrupts = <1 e 0>; interrupt-parent = <500>; };
gpt@660 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <6>; reg = <660 10>; interrupts = <1 f 0>; interrupt-parent = <500>; };
gpt@670 { // General Purpose Timer compatible = "mpc5200-gpt"; device_type = "gpt"; cell-index = <7>; reg = <670 10>; interrupts = <1 10 0>; interrupt-parent = <500>; };
rtc@800 { // Real time clock compatible = "mpc5200-rtc"; device_type = "rtc"; reg = <800 100>; interrupts = <1 5 0 1 6 0>; interrupt-parent = <500>; };
mscan@900 { device_type = "mscan"; compatible = "mpc5200-mscan"; cell-index = <0>; interrupts = <2 11 0>; interrupt-parent = <500>; reg = <900 80>; };
mscan@980 { device_type = "mscan"; compatible = "mpc5200-mscan"; cell-index = <1>; interrupts = <1 12 0>; interrupt-parent = <500>; reg = <980 80>; };
gpio@b00 { compatible = "mpc5200-gpio"; reg = <b00 40>; interrupts = <1 7 0>; interrupt-parent = <500>; };
gpio-wkup@b00 { compatible = "mpc5200-gpio-wkup"; reg = <c00 40>; interrupts = <1 8 0 0 3 0>; interrupt-parent = <500>; };
pci@0d00 { #interrupt-cells = <1>; #size-cells = <2>; #address-cells = <3>; device_type = "pci"; compatible = "mpc5200-pci"; reg = <d00 100>; interrupt-map-mask = <f800 0 0 7>; interrupt-map = <c000 0 0 1 500 0 0 3 c000 0 0 2 500 0 0 3 c000 0 0 3 500 0 0 3 c000 0 0 4 500 0 0 3>; clock-frequency = <0>; // From boot loader interrupts = <2 8 0 2 9 0 2 a 0>; interrupt-parent = <500>; bus-range = <0 0>; ranges = <42000000 0 80000000 80000000 0 20000000 02000000 0 a0000000 a0000000 0 10000000 01000000 0 00000000 b0000000 0 01000000>; };
spi@f00 { device_type = "spi"; compatible = "mpc5200-spi"; reg = <f00 20>; interrupts = <2 d 0 2 e 0>; interrupt-parent = <500>; };
usb@1000 { device_type = "usb-ohci-be"; compatible = "mpc5200-ohci\0ohci-be"; reg = <1000 ff>; interrupts = <2 6 0>; interrupt-parent = <500>; };
bestcomm@1200 { device_type = "dma-controller"; compatible = "mpc5200-bestcomm"; reg = <1200 80>; interrupts = <3 0 0 3 1 0 3 2 0 3 3 0 3 4 0 3 5 0 3 6 0 3 7 0 3 8 0 3 9 0 3 a 0 3 b 0 3 c 0 3 d 0 3 e 0 3 f 0>; interrupt-parent = <500>; };
xlb@1f00 { compatible = "mpc5200-xlb"; reg = <1f00 100>; };
serial@2000 { // PSC1 device_type = "serial"; compatible = "mpc5200-psc-uart"; port-number = <0>; // Logical port assignment cell-index = <0>; reg = <2000 100>; interrupts = <2 1 0>; interrupt-parent = <500>; };
// PSC2 in ac97 mode example //ac97@2200 { // PSC2 // device_type = "sound"; // compatible = "mpc5200-psc-ac97"; // cell-index = <1>; // reg = <2200 100>; // interrupts = <2 2 0>; // interrupt-parent = <500>; //};
// PSC3 in CODEC mode example //i2s@2400 { // PSC3 // device_type = "sound"; // compatible = "mpc5200-psc-i2s"; // cell-index = <2>; // reg = <2400 100>; // interrupts = <2 3 0>; // interrupt-parent = <500>; //};
// PSC4 in uart mode example //serial@2600 { // PSC4 // device_type = "serial"; // compatible = "mpc5200-psc-uart"; // cell-index = <3>; // reg = <2600 100>; // interrupts = <2 b 0>; // interrupt-parent = <500>; //};
// PSC5 in uart mode example //serial@2800 { // PSC5 // device_type = "serial"; // compatible = "mpc5200-psc-uart"; // cell-index = <4>; // reg = <2800 100>; // interrupts = <2 c 0>; // interrupt-parent = <500>; //};
// PSC6 in spi mode example //spi@2c00 { // PSC6 // device_type = "spi"; // compatible = "mpc5200-psc-spi"; // cell-index = <5>; // reg = <2c00 100>; // interrupts = <2 4 0>; // interrupt-parent = <500>; //};
ethernet@3000 { device_type = "network"; compatible = "mpc5200-fec"; reg = <3000 800>; mac-address = [ 02 03 04 05 06 07 ]; // Bad! interrupts = <2 5 0>; interrupt-parent = <500>; };
ata@3a00 { device_type = "ata"; compatible = "mpc5200-ata"; reg = <3a00 100>; interrupts = <2 7 0>; interrupt-parent = <500>; };
i2c@3d00 { device_type = "i2c"; compatible = "mpc5200-i2c\0fsl-i2c"; cell-index = <0>; reg = <3d00 40>; interrupts = <2 f 0>; interrupt-parent = <500>; fsl5200-clocking; };
i2c@3d40 { device_type = "i2c"; compatible = "mpc5200-i2c\0fsl-i2c"; cell-index = <1>; reg = <3d40 40>; interrupts = <2 10 0>; interrupt-parent = <500>; fsl5200-clocking; }; sram@8000 { device_type = "sram"; compatible = "mpc5200-sram\0sram"; reg = <8000 4000>; }; }; };
Wolfgang Grandegger wrote:
Hi Jerry,
Wolfgang Grandegger wrote:
Jerry Van Baren wrote:
[snip]
Yes, blob parsing will be done from the start of the blob until an answer is found every time a question is asked of it. Not a paradigm of efficiency. :-/
WRT the cached version, I have doubts about how much time it will save since I expect the "find compatible" will only be used during initialization. Is it worth optimizing? Really slow memory - yes. Fast memory - I doubt it. a) I don't picture blobs being stored in really slow memory (no i2c memories). b) If the memory really is slow, it seems like it would be as good or better to copy the blob to RAM and use it out of RAM (but there may be chicken & egg problems with that - I don't know how deeply you are looking to embed this).
I don't know what board/processor/memory you are ultimately targeting with this, so my criticisms may not be valid. I know denx.de support(s|ed) some very slow to boot boards that lots of tricks were done WRT optimization of env variables because they were stored in i2c memory.
I'm doing that for a MPC823 at 50 MHz, a very low-end system, and almost to slow for 2.6. I will do some real measurements when time permits to get a better feeling.
Here are the results of some quick measurements on my MPC855 at 80/40 MHz with the attached code example and my DTS test file:
from FLASH from MemoryNon-cached: 11116 us 1703 us Cached : 2800 us 6226 us
Well, I think we can drop the cached version even if its 4 times faster, as it make life more difficult, especially in case the FDT gets updated.
Wolfgang.
Risk & reward. The reward is pretty substantial if you read directly from slow flash, but iffy for faster RAM.
In the "from Memory" column, do you have the numbers switched? The non-cached is shown as being 4x faster than the cached.
To be a fair comparison, the "from Memory" column also needs the time it would take to copy the blob from flash to RAM added, yes? That penalty can be bypassed by loading the blob directly into RAM via tftp or the copy to RAM time may already be part of the boot process, but it should be identified.
gvb
Jerry Van Baren wrote:
Wolfgang Grandegger wrote:
Hi Jerry,
Wolfgang Grandegger wrote:
Jerry Van Baren wrote:
[snip]
Yes, blob parsing will be done from the start of the blob until an answer is found every time a question is asked of it. Not a paradigm of efficiency. :-/
WRT the cached version, I have doubts about how much time it will save since I expect the "find compatible" will only be used during initialization. Is it worth optimizing? Really slow memory - yes. Fast memory - I doubt it. a) I don't picture blobs being stored in really slow memory (no i2c memories). b) If the memory really is slow, it seems like it would be as good or better to copy the blob to RAM and use it out of RAM (but there may be chicken & egg problems with that - I don't know how deeply you are looking to embed this).
I don't know what board/processor/memory you are ultimately targeting with this, so my criticisms may not be valid. I know denx.de support(s|ed) some very slow to boot boards that lots of tricks were done WRT optimization of env variables because they were stored in i2c memory.
I'm doing that for a MPC823 at 50 MHz, a very low-end system, and almost to slow for 2.6. I will do some real measurements when time permits to get a better feeling.
Here are the results of some quick measurements on my MPC855 at 80/40 MHz with the attached code example and my DTS test file:
from FLASH from MemoryNon-cached: 11116 us 1703 us Cached : 2800 us 6226 us
Well, I think we can drop the cached version even if its 4 times faster, as it make life more difficult, especially in case the FDT gets updated.
Wolfgang.
Risk & reward. The reward is pretty substantial if you read directly from slow flash, but iffy for faster RAM.
In the "from Memory" column, do you have the numbers switched? The non-cached is shown as being 4x faster than the cached.
Yes, a cut & paste error, sorry.
To be a fair comparison, the "from Memory" column also needs the time it would take to copy the blob from flash to RAM added, yes? That penalty can be bypassed by loading the blob directly into RAM via tftp or the copy to RAM time may already be part of the boot process, but it should be identified.
Here is the revised list:
from FLASH from Memory Non-cached: 11116 us 6226 us Cached : 2800 us 1703 us (2096 us)
The number in brackets is _with_ memcpy. The FDT should normally be access from memory, I fully agree.
Wolfgang.
participants (3)
-
Jerry Van Baren -
Jerry Van Baren -
Wolfgang Grandegger