On Sat, Feb 03, 2018 at 10:14:31AM +0100, Maxime Ripard wrote:
> From: Heiko Stuebner <heiko.stuebner@xxxxxx>
Sorry I've taken so long to look at this.
>
> On i.MX, which carries a lot of pin-groups of which most are unused on
> individual boards, they noticed that this plethora of nodes also results
> in the runtime-lookup-performance also degrading [0].
I do kind of wonder if that indicates that using the tree without
doing some kind of unflattening and indexing on it is the right
option.
> A i.MX-specific solution defining the pingroups in the board files but
> using macros to reference the pingroup-data was not well received.
Ok. I'm not seeing much sign of poor reception in that thread; did
that happen elsewhere?
> This patch is trying to solve this issue in a more general way, by
> adding the ability to mark nodes as needing to be referenced somewhere
> in the tree.
>
> To mark a node a needing to be referenced it must be prefixed with
> /delete-if-unreferenced/. This makes dtc check the nodes reference-status
> when creating the flattened tree, dropping it if unreferenced.
>
> For example, the i.MX6SL pingroup
>
> /delete-if-unreferenced/ pinctrl_ecspi1_1: ecspi1grp-1 {
> fsl,pins = <
> MX6SL_PAD_ECSPI1_MISO__ECSPI1_MISO 0x100b1
> MX6SL_PAD_ECSPI1_MOSI__ECSPI1_MOSI 0x100b1
> MX6SL_PAD_ECSPI1_SCLK__ECSPI1_SCLK 0x100b1
> >;
> };
>
> would only be included in the dtb if it got referenced somewhere
> as pingroup via
>
> node {
> pinctrl-0 <&pinctrl_ecscpi1_1>;
> };
>
> [0] http://thread.gmane.org/gmane.linux.ports.arm.kernel/275912/
Ok. As you might guess from above, I have some misgivings about how
significant this use case is. That said, though, this is pretty small
and well-contained, so I'm happy enough to merge something like this
even if the use case is a bit marginal.
There are some fairly minor things to fix in the implementation, see
below:
> Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxx>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> ---
> Hi,
>
> This is a respin of the patch previously sent by Heiko here:
> https://patchwork.kernel.org/patch/3840371/
>
> With hopefully all your comments addressed.
>
> Let me know what you think,
> Maxime
>
> checks.c | 2 ++
> dtc-lexer.l | 7 +++++++
> dtc-parser.y | 5 +++++
> dtc.h | 4 ++++
> flattree.c | 3 +++
> livetree.c | 14 ++++++++++++++
> 6 files changed, 35 insertions(+)
>
> diff --git a/checks.c b/checks.c
> index 1cded3658491..a1e0ad91b083 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -571,6 +571,8 @@ static void fixup_phandle_references(struct check *c, struct dt_info *dti,
>
> phandle = get_node_phandle(dt, refnode);
> *((fdt32_t *)(prop->val.val + m->offset)) = cpu_to_fdt32(phandle);
> +
> + reference_node(refnode);
You only reference nodes for phandle references, not for path
references. That's probably what not what you want. If that really
was your intention, then I don't think you actually need the
referenced flag - you could infer it from whether the node has a
phandle assigned.
> }
> }
> }
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index fd825ebba69c..2afde3acad8a 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -153,6 +153,13 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
> return DT_DEL_NODE;
> }
>
> +<*>"/delete-if-unreferenced/" {
Maybe "/omit-if-no-ref/". It's a bit shorter, and I prefer "omit" to
"delete", because this doesn't work quite the same way that say
/delete-node/ and /delete-property/ do.
> + DPRINT("Keyword: /delete-if-unreferenced/\n");
> + DPRINT("<PROPNODENAME>\n");
> + BEGIN(PROPNODENAME);
> + return DT_DEL_UNREFERENCED;
> + }
> +
> <*>{LABEL}: {
> DPRINT("Label: %s\n", yytext);
> yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 44af170abfea..55c951c5327f 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -63,6 +63,7 @@ extern bool treesource_error;
> %token DT_BITS
> %token DT_DEL_PROP
> %token DT_DEL_NODE
> +%token DT_DEL_UNREFERENCED
> %token <propnodename> DT_PROPNODENAME
> %token <integer> DT_LITERAL
> %token <integer> DT_CHAR_LITERAL
> @@ -523,6 +524,10 @@ subnode:
> {
> $$ = name_node(build_node_delete(), $2);
> }
> + | DT_DEL_UNREFERENCED subnode
> + {
> + $$ = mark_node_needs_reference($2);
> + }
> | DT_LABEL subnode
> {
> add_label(&$2->labels, $1);
> diff --git a/dtc.h b/dtc.h
> index 3b18a42b866e..cb7e3752af5a 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -168,6 +168,8 @@ struct node {
>
> struct label *labels;
> const struct bus_type *bus;
> +
> + int needs_reference, is_referenced;
I'd prefer to rename "needs_reference" to something closer matching
the actual tag. It really is an exact correspondance, so matching the
names makes it easier to grep for.
Also, use 'bool' for these.
> };
>
> #define for_each_label_withdel(l0, l) \
> @@ -202,6 +204,8 @@ struct property *reverse_properties(struct property *first);
> struct node *build_node(struct property *proplist, struct node *children);
> struct node *build_node_delete(void);
> struct node *name_node(struct node *node, char *name);
> +struct node *mark_node_needs_reference(struct node *node);
> +struct node *reference_node(struct node *node);
> struct node *chain_node(struct node *first, struct node *list);
> struct node *merge_nodes(struct node *old_node, struct node *new_node);
> struct node *add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
> diff --git a/flattree.c b/flattree.c
> index 8d268fb785db..39eb9934457b 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -258,6 +258,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
> if (tree->deleted)
> return;
>
> + if (tree->needs_reference && !tree->is_referenced)
> + return;
> +
So this only applies the change when using -Odtb or -Oasm, which might
violate least surprise. In particular -Idts -Odts will leave the node
in, while stripping the /delete-if-unreferenced/ tag. That means that
chaining -I dts -O dts then -I dts -O dtb won't result in the same
output as using -I dts -O dtb directly, which isn't really expected.
I think it would probably be preferable to do the stripping of
unreferenced nodes before running the output pass (you can probably
make it another "fixup" pass) would be preferable.
> emit->beginnode(etarget, tree->labels);
>
> if (vi->flags & FTF_FULLPATH)
> diff --git a/livetree.c b/livetree.c
> index 57b7db2ed153..4b595c4b5e0a 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -134,6 +134,20 @@ struct node *name_node(struct node *node, char *name)
> return node;
> }
>
> +struct node *mark_node_needs_reference(struct node *node)
> +{
> + node->needs_reference = 1;
> +
> + return node;
> +}
> +
> +struct node *reference_node(struct node *node)
> +{
> + node->is_referenced = 1;
> +
> + return node;
> +}
> +
> struct node *merge_nodes(struct node *old_node, struct node *new_node)
> {
> struct property *new_prop, *old_prop;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
