[PATCH 00/10] Make bootph tags transistive to parents
The bootph binding[1] is defined such that if a tag is present in a subnode then it is supposed to apply to the parent node as well.
This series aligned U-Boot with that binding.
It also includes a few clean-ups.
[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/ schemas/bootph.yaml
Simon Glass (10): x86: coral: Align bootph SPI-flash subnodes with parent fdtgrep: Tidy up a few type warnings and comments fdtgrep: Correct ordering of flags fdtgrep: Correct references to fdt_find_regions() fdtgrep: Tidy up comment for h_include() fdtgrep: Simplify code to inverting the match fdtgrep: Move property checking into a function sandbox: Correct SPL condition for building devicetree fdtgrep: Allow propagating properties up to supernodes Makefile: Use the fdtgrep -u flag
arch/x86/dts/chromebook_coral.dts | 6 +- scripts/Makefile.lib | 2 +- scripts/Makefile.spl | 2 +- tools/fdtgrep.c | 123 ++++++++++++++++++++---------- 4 files changed, 87 insertions(+), 46 deletions(-)
The subnode has different tags from the parents, which is not correct. Fix the subnode.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/dts/chromebook_coral.dts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index 8bfb2c0d19dc..2412801302ea 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -369,12 +369,14 @@ rw-mrc-cache { label = "rw-mrc-cache"; reg = <0x008e0000 0x00010000>; - bootph-all; + bootph-some-ram; + bootph-pre-ram; }; rw-var-mrc-cache { label = "rw-mrc-cache"; reg = <0x008f0000 0x0001000>; - bootph-all; + bootph-some-ram; + bootph-pre-ram; }; }; };
The subnode has different tags from the parents, which is not correct. Fix the subnode.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/dts/chromebook_coral.dts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Applied to u-boot-dm/next, thanks!
Align the code with the upstream version at fdt-tools which had a few tweaks before being applied.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index 7eabcab4399d..f5493aaed3d8 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -375,8 +375,9 @@ static int display_fdt_by_regions(struct display_info *disp, const void *blob, const char *str; int str_base = fdt_off_dt_strings(blob);
- for (offset = 0; offset < fdt_size_dt_strings(blob); - offset += strlen(str) + 1) { + for (offset = 0; + offset < (int)fdt_size_dt_strings(blob); + offset += strlen(str) + 1) { str = fdt_string(blob, offset); int len = strlen(str) + 1; int show; @@ -431,7 +432,7 @@ static int dump_fdt_regions(struct display_info *disp, const void *blob, { struct fdt_header *fdt; int size, struct_start; - int ptr; + unsigned int ptr; int i;
/* Set up a basic header (even if we don't actually write it) */ @@ -683,10 +684,10 @@ static int fdtgrep_find_regions(const void *fdt, return new_count; } else if (new_count <= max_regions) { /* - * The alias regions will now be at the end of the list. - * Sort the regions by offset to get things into the - * right order - */ + * The alias regions will now be at the end of the list. + * Sort the regions by offset to get things into the + * right order + */ count = new_count; qsort(region, count, sizeof(struct fdt_region), h_cmp_region); @@ -880,7 +881,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) size = fdt_totalsize(fdt); }
- if (size != fwrite(fdt, 1, size, disp->fout)) { + if ((size_t)size != fwrite(fdt, 1, size, disp->fout)) { fprintf(stderr, "Write failure, %d bytes\n", size); free(fdt); ret = 1; @@ -934,7 +935,7 @@ static const char usage_synopsis[] = static const char usage_short_opts[] = "haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv" USAGE_COMMON_SHORT_OPTS; -static struct option const usage_long_opts[] = { +static const struct option usage_long_opts[] = { {"show-address", no_argument, NULL, 'a'}, {"colour", no_argument, NULL, 'A'}, {"include-node-with-prop", a_argument, NULL, 'b'},
Align the code with the upstream version at fdt-tools which had a few tweaks before being applied.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Applied to u-boot-dm/next, thanks!
Two of the flags are out of order, so fix this.
Also adjust the ordering of one flag in the main switch()
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index f5493aaed3d8..41d8b41252dd 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -953,6 +953,8 @@ static const struct option usage_long_opts[] = { {"include-mem", no_argument, NULL, 'm'}, {"include-node", a_argument, NULL, 'n'}, {"exclude-node", a_argument, NULL, 'N'}, + {"out", a_argument, NULL, 'o'}, + {"out-format", a_argument, NULL, 'O'}, {"include-prop", a_argument, NULL, 'p'}, {"exclude-prop", a_argument, NULL, 'P'}, {"remove-strings", no_argument, NULL, 'r'}, @@ -961,8 +963,6 @@ static const struct option usage_long_opts[] = { {"skip-supernodes", no_argument, NULL, 'S'}, {"show-stringtab", no_argument, NULL, 't'}, {"show-aliases", no_argument, NULL, 'T'}, - {"out", a_argument, NULL, 'o'}, - {"out-format", a_argument, NULL, 'O'}, {"invert-match", no_argument, NULL, 'v'}, USAGE_COMMON_LONG_OPTS, }; @@ -984,6 +984,8 @@ static const char * const usage_opts_help[] = { "Include mem_rsvmap section in binary output", "Node to include in grep", "Node to exclude in grep", + "-o <output file>", + "-O <output format>", "Property to include in grep", "Property to exclude in grep", "Remove unused strings from string table", @@ -992,8 +994,6 @@ static const char * const usage_opts_help[] = { "Don't include supernodes of matching nodes", "Include string table in binary output", "Include matching aliases in output", - "-o <output file>", - "-O <output format>", "Invert the sense of matching (select non-matching lines)", USAGE_COMMON_OPTS_HELP }; @@ -1125,6 +1125,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'H': disp->header = 1; break; + case 'I': + disp->show_dts_version = 1; + break; case 'l': disp->region_list = 1; break; @@ -1180,9 +1183,6 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'v': disp->invert = 1; break; - case 'I': - disp->show_dts_version = 1; - break; }
if (type && value_add(disp, &disp->value_head, type, inc,
Two of the flags are out of order, so fix this.
Also adjust the ordering of one flag in the main switch()
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Applied to u-boot-dm/next, thanks!
The function name is actually fdtgrep_find_regions() so update the name in comments accordinging.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index 41d8b41252dd..b56d2fe21c9a 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -576,10 +576,10 @@ static int check_type_include(void *priv, int type, const char *data, int size) }
/** - * h_include() - Include handler function for fdt_find_regions() + * h_include() - Include handler function for fdtgrep_find_regions() * * This function decides whether to include or exclude a node, property or - * compatible string. The function is defined by fdt_find_regions(). + * compatible string. The function is defined by fdtgrep_find_regions(). * * The algorithm is documented in the code - disp->invert is 0 for normal * operation, and 1 to invert the sense of all matches. @@ -822,7 +822,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) region, max_regions, path, sizeof(path), disp->flags); if (count < 0) { - report_error("fdt_find_regions", count); + report_error("fdtgrep_find_regions", count); free(region); return -1; }
The function name is actually fdtgrep_find_regions() so update the name in comments accordinging.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Applied to u-boot-dm/next, thanks!
Copy the comment from fdt_first_region() so that it is clear what value this function returns.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index b56d2fe21c9a..a6cdc326709d 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -576,15 +576,21 @@ static int check_type_include(void *priv, int type, const char *data, int size) }
/** - * h_include() - Include handler function for fdtgrep_find_regions() + * h_include() - Include handler function for fdt_first_region() * * This function decides whether to include or exclude a node, property or - * compatible string. The function is defined by fdtgrep_find_regions(). + * compatible string. The function is defined by fdt_first_region(). * * The algorithm is documented in the code - disp->invert is 0 for normal * operation, and 1 to invert the sense of all matches. * - * See + * @priv: Private pointer as passed to fdtgrep_find_regions() + * @fdt: Pointer to FDT blob + * @offset: Offset of this node / property + * @type: Type of this part, FDT_IS_... + * @data: Pointer to data (node name, property name, compatible string) + * @size: Size of data, or 0 if none + * Return: 0 to exclude, 1 to include, -1 if no information is available */ static int h_include(void *priv, const void *fdt, int offset, int type, const char *data, int size)
Copy the comment from fdt_first_region() so that it is clear what value this function returns.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Applied to u-boot-dm/next, thanks!
The code to invert the match in h_include() is a bit convoluted. Simplify it by using disp->invert only once.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index a6cdc326709d..b06a1a7a8384 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -634,14 +634,8 @@ static int h_include(void *priv, const void *fdt, int offset, int type, inc = 0; }
- switch (inc) { - case 1: - inc = !disp->invert; - break; - case 0: - inc = disp->invert; - break; - } + if (inc != -1 && disp->invert) + inc = !inc; debug(" - returning %d\n", inc);
return inc;
The code to invert the match in h_include() is a bit convoluted. Simplify it by using disp->invert only once.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
Applied to u-boot-dm/next, thanks!
The h_include() function includes a piece which checks if a node contains a property being searched for. Move this into its own function to reduce the size of the h_include() function.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 48 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index b06a1a7a8384..ca639a2d9f4f 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -575,6 +575,40 @@ static int check_type_include(void *priv, int type, const char *data, int size) return 0; }
+/** + * check_props() - Check if a node has properties that we want to include + * + * Calls check_type_include() for each property in the nodn, returning 1 if + * that function returns 1 for any of them + * + * @disp: Display structure, holding info about our options + * @fdt: Devicetree blob to check + * @node: Node offset to check + * @inc: Current value of the 'include' variable (see h_include()) + * Return: 0 to exclude, 1 to include, -1 if no information is available + */ +static int check_props(struct display_info *disp, const void *fdt, int node, + int inc) +{ + int offset; + + for (offset = fdt_first_property_offset(fdt, node); + offset > 0 && inc != 1; + offset = fdt_next_property_offset(fdt, offset)) { + const struct fdt_property *prop; + const char *str; + + prop = fdt_get_property_by_offset(fdt, offset, NULL); + if (!prop) + continue; + str = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); + inc = check_type_include(disp, FDT_NODE_HAS_PROP, str, + strlen(str)); + } + + return inc; +} + /** * h_include() - Include handler function for fdt_first_region() * @@ -617,19 +651,7 @@ static int h_include(void *priv, const void *fdt, int offset, int type, (disp->types_inc & FDT_NODE_HAS_PROP)) { debug(" - checking node '%s'\n", fdt_get_name(fdt, offset, NULL)); - for (offset = fdt_first_property_offset(fdt, offset); - offset > 0 && inc != 1; - offset = fdt_next_property_offset(fdt, offset)) { - const struct fdt_property *prop; - const char *str; - - prop = fdt_get_property_by_offset(fdt, offset, NULL); - if (!prop) - continue; - str = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); - inc = check_type_include(priv, FDT_NODE_HAS_PROP, str, - strlen(str)); - } + inc = check_props(disp, fdt, offset, inc); if (inc == -1) inc = 0; }
The h_include() function includes a piece which checks if a node contains a property being searched for. Move this into its own function to reduce the size of the h_include() function.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 48 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 13 deletions(-)
Applied to u-boot-dm/next, thanks!
With sandbox, CONFIG_SANDBOX is y so the current rule ends up building the devicetree for only those SPL builds where it is unwanted.
Correct the condition. This allows sandbox_vpl to produce a u-boot-vpl.dtb file.
Fixes: e7fb789612e ("sandbox: Remove OF_HOSTFILE")
Signed-off-by: Simon Glass sjg@chromium.org ---
scripts/Makefile.spl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index e450ffd5d5ef..407fc52376a5 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -314,7 +314,7 @@ endif # - we have either OF_SEPARATE or OF_HOSTFILE build_dtb := ifneq ($(CONFIG_$(SPL_TPL_)OF_REAL),) -ifeq ($(CONFIG_OF_SEPARATE)$(CONFIG_SANDBOX),y) +ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_SANDBOX),) build_dtb := y endif endif
With sandbox, CONFIG_SANDBOX is y so the current rule ends up building the devicetree for only those SPL builds where it is unwanted.
Correct the condition. This allows sandbox_vpl to produce a u-boot-vpl.dtb file.
Fixes: e7fb789612e ("sandbox: Remove OF_HOSTFILE")
Signed-off-by: Simon Glass sjg@chromium.org ---
scripts/Makefile.spl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to u-boot-dm/next, thanks!
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };
Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index ca639a2d9f4f..f1ff1946bd4a 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -63,6 +63,7 @@ struct display_info { int types_inc; /* Mask of types that we include (FDT_IS...) */ int types_exc; /* Mask of types that we exclude (FDT_IS...) */ int invert; /* Invert polarity of match */ + int props_up; /* Imply properties up to supernodes */ struct value_node *value_head; /* List of values to match */ const char *output_fname; /* Output filename */ FILE *fout; /* File to write dts/dtb output */ @@ -606,6 +607,16 @@ static int check_props(struct display_info *disp, const void *fdt, int node, strlen(str)); }
+ /* if requested, check all subnodes for this property too */ + if (inc != 1 && disp->props_up) { + int subnode; + + for (subnode = fdt_first_subnode(fdt, node); + subnode > 0 && inc != 1; + subnode = fdt_next_subnode(fdt, subnode)) + inc = check_props(disp, fdt, subnode, inc); + } + return inc; }
@@ -955,7 +966,7 @@ static const char usage_synopsis[] = case '?': usage("unknown option");
static const char usage_short_opts[] = - "haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv" + "haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTuv" USAGE_COMMON_SHORT_OPTS; static const struct option usage_long_opts[] = { {"show-address", no_argument, NULL, 'a'}, @@ -985,6 +996,7 @@ static const struct option usage_long_opts[] = { {"skip-supernodes", no_argument, NULL, 'S'}, {"show-stringtab", no_argument, NULL, 't'}, {"show-aliases", no_argument, NULL, 'T'}, + {"props-up-to-supernode", no_argument, NULL, 'u'}, {"invert-match", no_argument, NULL, 'v'}, USAGE_COMMON_LONG_OPTS, }; @@ -1016,6 +1028,7 @@ static const char * const usage_opts_help[] = { "Don't include supernodes of matching nodes", "Include string table in binary output", "Include matching aliases in output", + "Add -p properties to supernodes too", "Invert the sense of matching (select non-matching lines)", USAGE_COMMON_OPTS_HELP }; @@ -1202,6 +1215,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'T': disp->add_aliases = 1; break; + case 'u': + disp->props_up = 1; + break; case 'v': disp->invert = 1; break;
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };
Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/fdtgrep.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Applied to u-boot-dm/next, thanks!
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
[0]: https://lore.kernel.org/linux-arm-kernel/20240705-b4-upstream-bootph-all-v2-...
Regards, Manorit
tools/fdtgrep.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index ca639a2d9f4f..f1ff1946bd4a 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -63,6 +63,7 @@ struct display_info { int types_inc; /* Mask of types that we include (FDT_IS...) */ int types_exc; /* Mask of types that we exclude (FDT_IS...) */ int invert; /* Invert polarity of match */
- int props_up; /* Imply properties up to supernodes */ struct value_node *value_head; /* List of values to match */ const char *output_fname; /* Output filename */ FILE *fout; /* File to write dts/dtb output */
@@ -606,6 +607,16 @@ static int check_props(struct display_info *disp, const void *fdt, int node, strlen(str)); }
- /* if requested, check all subnodes for this property too */
- if (inc != 1 && disp->props_up) {
int subnode;for (subnode = fdt_first_subnode(fdt, node);subnode > 0 && inc != 1;subnode = fdt_next_subnode(fdt, subnode))inc = check_props(disp, fdt, subnode, inc);- }
- return inc;
}
@@ -955,7 +966,7 @@ static const char usage_synopsis[] = case '?': usage("unknown option");
static const char usage_short_opts[] =
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv"
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTuv"static const struct option usage_long_opts[] = { {"show-address", no_argument, NULL, 'a'}, @@ -985,6 +996,7 @@ static const struct option usage_long_opts[] = { {"skip-supernodes", no_argument, NULL, 'S'}, {"show-stringtab", no_argument, NULL, 't'}, {"show-aliases", no_argument, NULL, 'T'},
- {"props-up-to-supernode", no_argument, NULL, 'u'}, {"invert-match", no_argument, NULL, 'v'}, USAGE_COMMON_LONG_OPTS,
}; @@ -1016,6 +1028,7 @@ static const char * const usage_opts_help[] = { "Don't include supernodes of matching nodes", "Include string table in binary output", "Include matching aliases in output",
- "Add -p properties to supernodes too", "Invert the sense of matching (select non-matching lines)", USAGE_COMMON_OPTS_HELP
}; @@ -1202,6 +1215,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'T': disp->add_aliases = 1; break;
case 'u':disp->props_up = 1;break;-- 2.43.0.472.g3155946c3a-goog
Hi Manorit,
On 7/10/24 8:15 AM, Manorit Chawdhry wrote:
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys"; btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
I haven't followed the whole thread but it seems to match something I looked into recently.
The properties aren't actually propagated. What happens is that this option is used to NOT delete parents (or rather, their properties) of a node with the passed bootph- property. This is only useful for TPL and SPL device trees which are stripped down versions of the full U-Boot DTB. The parent nodes also incidentally do not have this property added, they just are merely allowed to exist.
fdtgrep needs to be reworked to allow passing the bootph- properties to the parent nodes. I quickly looked into it a few weeks ago and it didn't seem like a 5min job to me. This would be also beneficial for U-Boot proper pre-relocation stage, which suffers from missing bootph- properties in parent nodes (been there recently) as well.
Cheers, Quentin
Hi Manorit,
On Wed, 10 Jul 2024 at 07:15, Manorit Chawdhry m-chawdhry@ti.com wrote:
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
Yes, that is because fdtgrep is only used with the SPL DT. The U-Boot one is used unmodified by U-Boot and it does not check parents for such properties (neither does SPL, but fdtgrep has logic to imply these properties internally).
The easiest way to fix this would probably be to process the DT in Binman for insertion into the final image, since Binman already does lots of other DT processing.
If you like, you could create an issue for it [1]. If you want to have a crack at fixing it, I could provide some pointers.
Regards, Simon
[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues
Regards, Manorit
tools/fdtgrep.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index ca639a2d9f4f..f1ff1946bd4a 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -63,6 +63,7 @@ struct display_info { int types_inc; /* Mask of types that we include (FDT_IS...) */ int types_exc; /* Mask of types that we exclude (FDT_IS...) */ int invert; /* Invert polarity of match */
int props_up; /* Imply properties up to supernodes */ struct value_node *value_head; /* List of values to match */ const char *output_fname; /* Output filename */ FILE *fout; /* File to write dts/dtb output */@@ -606,6 +607,16 @@ static int check_props(struct display_info *disp, const void *fdt, int node, strlen(str)); }
/* if requested, check all subnodes for this property too */if (inc != 1 && disp->props_up) {int subnode;for (subnode = fdt_first_subnode(fdt, node);subnode > 0 && inc != 1;subnode = fdt_next_subnode(fdt, subnode))inc = check_props(disp, fdt, subnode, inc);}return inc;}
@@ -955,7 +966,7 @@ static const char usage_synopsis[] = case '?': usage("unknown option");
static const char usage_short_opts[] =
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv"
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTuv" USAGE_COMMON_SHORT_OPTS;static const struct option usage_long_opts[] = { {"show-address", no_argument, NULL, 'a'}, @@ -985,6 +996,7 @@ static const struct option usage_long_opts[] = { {"skip-supernodes", no_argument, NULL, 'S'}, {"show-stringtab", no_argument, NULL, 't'}, {"show-aliases", no_argument, NULL, 'T'},
{"props-up-to-supernode", no_argument, NULL, 'u'}, {"invert-match", no_argument, NULL, 'v'}, USAGE_COMMON_LONG_OPTS,}; @@ -1016,6 +1028,7 @@ static const char * const usage_opts_help[] = { "Don't include supernodes of matching nodes", "Include string table in binary output", "Include matching aliases in output",
"Add -p properties to supernodes too", "Invert the sense of matching (select non-matching lines)", USAGE_COMMON_OPTS_HELP}; @@ -1202,6 +1215,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'T': disp->add_aliases = 1; break;
case 'u':disp->props_up = 1;break; case 'v': disp->invert = 1; break;-- 2.43.0.472.g3155946c3a-goog
Hi Simon,
On 10:39-20240711, Simon Glass wrote:
Hi Manorit,
On Wed, 10 Jul 2024 at 07:15, Manorit Chawdhry m-chawdhry@ti.com wrote:
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
Yes, that is because fdtgrep is only used with the SPL DT. The U-Boot one is used unmodified by U-Boot and it does not check parents for such properties (neither does SPL, but fdtgrep has logic to imply these properties internally).
The easiest way to fix this would probably be to process the DT in Binman for insertion into the final image, since Binman already does lots of other DT processing.
If you like, you could create an issue for it [1]. If you want to have a crack at fixing it, I could provide some pointers.
I am not sure if I'll be able to focus on this now but feel free to give your pointers, might help anyone else as well if they want to take a look at it.
Meanwhile I will open a issue as you mentioned for tracking this, trying to register on the URL that you provided. Thanks!
Regards, Manorit
Regards, Simon
[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues
Regards, Manorit
tools/fdtgrep.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index ca639a2d9f4f..f1ff1946bd4a 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -63,6 +63,7 @@ struct display_info { int types_inc; /* Mask of types that we include (FDT_IS...) */ int types_exc; /* Mask of types that we exclude (FDT_IS...) */ int invert; /* Invert polarity of match */
int props_up; /* Imply properties up to supernodes */ struct value_node *value_head; /* List of values to match */ const char *output_fname; /* Output filename */ FILE *fout; /* File to write dts/dtb output */@@ -606,6 +607,16 @@ static int check_props(struct display_info *disp, const void *fdt, int node, strlen(str)); }
/* if requested, check all subnodes for this property too */if (inc != 1 && disp->props_up) {int subnode;for (subnode = fdt_first_subnode(fdt, node);subnode > 0 && inc != 1;subnode = fdt_next_subnode(fdt, subnode))inc = check_props(disp, fdt, subnode, inc);}return inc;}
@@ -955,7 +966,7 @@ static const char usage_synopsis[] = case '?': usage("unknown option");
static const char usage_short_opts[] =
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv"
"haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTuv" USAGE_COMMON_SHORT_OPTS;static const struct option usage_long_opts[] = { {"show-address", no_argument, NULL, 'a'}, @@ -985,6 +996,7 @@ static const struct option usage_long_opts[] = { {"skip-supernodes", no_argument, NULL, 'S'}, {"show-stringtab", no_argument, NULL, 't'}, {"show-aliases", no_argument, NULL, 'T'},
{"props-up-to-supernode", no_argument, NULL, 'u'}, {"invert-match", no_argument, NULL, 'v'}, USAGE_COMMON_LONG_OPTS,}; @@ -1016,6 +1028,7 @@ static const char * const usage_opts_help[] = { "Don't include supernodes of matching nodes", "Include string table in binary output", "Include matching aliases in output",
"Add -p properties to supernodes too", "Invert the sense of matching (select non-matching lines)", USAGE_COMMON_OPTS_HELP}; @@ -1202,6 +1215,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) case 'T': disp->add_aliases = 1; break;
case 'u':disp->props_up = 1;break; case 'v': disp->invert = 1; break;-- 2.43.0.472.g3155946c3a-goog
Hi Simon,
On 10:12-20240716, Manorit Chawdhry wrote:
Hi Simon,
On 10:39-20240711, Simon Glass wrote:
Hi Manorit,
On Wed, 10 Jul 2024 at 07:15, Manorit Chawdhry m-chawdhry@ti.com wrote:
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
Yes, that is because fdtgrep is only used with the SPL DT. The U-Boot one is used unmodified by U-Boot and it does not check parents for such properties (neither does SPL, but fdtgrep has logic to imply these properties internally).
The easiest way to fix this would probably be to process the DT in Binman for insertion into the final image, since Binman already does lots of other DT processing.
If you like, you could create an issue for it [1]. If you want to have a crack at fixing it, I could provide some pointers.
I am not sure if I'll be able to focus on this now but feel free to give your pointers, might help anyone else as well if they want to take a look at it.
Would you be able to give some pointers? Also I was wondering why do we even need to process the DT in binman, why isn't the parent node probed by default in this flow, is there some use case like that where the child needs to be probed but not the parent before it or is it some other bug?
Meanwhile I will open a issue as you mentioned for tracking this, trying to register on the URL that you provided. Thanks!
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21
Regards, Manorit
Regards, Manorit
Regards, Simon
[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues
Regards, Manorit
Hi Manorit,
On Thu, 19 Dec 2024 at 02:16, Manorit Chawdhry m-chawdhry@ti.com wrote:
Hi Simon,
On 10:12-20240716, Manorit Chawdhry wrote:
Hi Simon,
On 10:39-20240711, Simon Glass wrote:
Hi Manorit,
On Wed, 10 Jul 2024 at 07:15, Manorit Chawdhry m-chawdhry@ti.com wrote:
Hi Simon,
On 09:36-20231217, Simon Glass wrote:
The existing bootph binding is defined such that properties in a subnode are also implied in the supernode also, as in this example:
buttons { /* bootph,pre-ram is implied by btn1 */ compatible = "gpio-keys";
btn1 { bootph,pre-ram; gpios = <&gpio_a 3 0>; label = "button1"; linux,code = <BTN_1>; };Provide an option to implement this in fdtgrep.
Signed-off-by: Simon Glass sjg@chromium.org
Coming back to this patch as am facing some issues with the bootph-all propagation to the parent nodes [0]. We have a dmsc/sms node in our devices and it is a parent node of k3_pds, k3_clks, k3_reset.
During U-boot proper we are initializing the serial console and during that time we have to make some calls to our Device manager for clocks and power domains. With the DT modelling, the clocks and power domains come under the device manager node ( dmsc/sms ) but interestingly that driver is not getting probed while putting bootph-all in the child nodes. Do you know anything about this? I thought putting it in child node will propagate it to the parent nodes as well so it'll be probed as well. We are having to put bootph-all explicitely in the dmsc/sms node for it to work.
Yes, that is because fdtgrep is only used with the SPL DT. The U-Boot one is used unmodified by U-Boot and it does not check parents for such properties (neither does SPL, but fdtgrep has logic to imply these properties internally).
The easiest way to fix this would probably be to process the DT in Binman for insertion into the final image, since Binman already does lots of other DT processing.
If you like, you could create an issue for it [1]. If you want to have a crack at fixing it, I could provide some pointers.
I am not sure if I'll be able to focus on this now but feel free to give your pointers, might help anyone else as well if they want to take a look at it.
Would you be able to give some pointers?
Can we first just be clear on the problem? Is it that before relocation, devices are not bound because the bootph tag is not there? Can you update your issue with a bit more info?
If so, then there are two options I can think of:
1. Update dm_scan_fdt_node() / lists_bind_fdt() so that it scans children before relocation, even when a parent does not have the required bootph tag. But it only binds the parent if any of its children have the tag. This should be possible and I suspect the performance would be OK.
2. Update binman so that it adds tags into the devicetree when it sees that they are implied in a parent. This would avoid code changes in U-Boot, and perhaps be a bit faster.
Also I was wondering why do we even need to process the DT in binman, why isn't the parent node probed by default in this flow, is there some use case like that where the child needs to be probed but not the parent before it or is it some other bug?
Basically, Rob Herring wanted to have child tags implied in the parent. This was the 'deal' under which the bootph stuff went into the schema. U-Boot has never supported this previously, of course and we have had to add the tags manually in the parent, when any child needs it. But anyway Rob's idea seemed reasonable to me since it is actually a pain to add these tags in parents, particular when there are several levels of hierarchy, e.g. an soc node, etc.
So I then implemented this in fdtgrep, thinking that this was the end of the story, but as you have found (if I have this right), it is not.
Meanwhile I will open a issue as you mentioned for tracking this, trying to register on the URL that you provided. Thanks!
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21
[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues
Regards, Manorit
Regards, Simon
Use this flag so that the bootph binding is obeyed correctly.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/12 ---
scripts/Makefile.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8dc6ec82cd56..21b1f7daa72c 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -631,7 +631,7 @@ fdtgrep_props := -b bootph-all -b bootph-pre-ram $(migrate_spl) endif endif quiet_cmd_fdtgrep = FDTGREP $@ - cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \ + cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u -RT $< \ -n /chosen -n /config -O dtb | \ $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ -P bootph-all -P bootph-pre-ram -P bootph-pre-sram \
On Sun, Dec 17, 2023 at 09:36:23AM -0700, Simon Glass wrote:
Use this flag so that the bootph binding is obeyed correctly.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/12
First of all, yay, this will I believe unblock upstreaming of these properties.
scripts/Makefile.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8dc6ec82cd56..21b1f7daa72c 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -631,7 +631,7 @@ fdtgrep_props := -b bootph-all -b bootph-pre-ram $(migrate_spl) endif endif quiet_cmd_fdtgrep = FDTGREP $@
cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \
cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u -RT $< \
Maybe we need to expand / add some comment here? In practice right now I don't think we have an issue in that say "keep-power-in-suspend" is already not being included in these special subset trees, but we want to I think at least make it clear for future users what unexpected things might happen.
participants (4)
-
Manorit Chawdhry -
Quentin Schulz -
Simon Glass -
Tom Rini