Hi Rob,
On Wed, Feb 21, 2018 at 09:20:45AM -0600, Rob Herring wrote:
> On Sat, Feb 3, 2018 at 3:14 AM, Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> > From: Heiko Stuebner <heiko.stuebner@xxxxxx>
> >
> > 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].
> >
> > A i.MX-specific solution defining the pingroups in the board files but
> > using macros to reference the pingroup-data was not well received.
> >
> > 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
> > >;
> > };
>
> This could also be out of band somewhere else in the tree or have a
> newline after the tag, right? Like this:
>
> /delete-if-unreferenced/ &pinctrl_ecspi1_1;
>
> Otherwise, the line could get quite long if other tags are added.
Indeed, I'll make sure this is the case (if not already).
> > 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/
> >
> > 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);
> > }
> > }
> > }
> > 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/" {
> > + 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;
>
> bool and/or bitfields.
Ack.
> This could be done as a single refcount instead. Everything starts
> with ref of 1, /delete-if-unreferenced/ decrements the refcount,
> phandles increment the refcount. Not really much benefit now, but
> perhaps there could be some other reason to know how many references
> there are.
I'm not so convinced about this one. Refcounting also introduces bugs,
while this is still pretty simple. And if we ever really need
refcouting, moving to it will also be quite simple by removing the
is_referenced assignations to increments.
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
