Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Geert Uytterhoeven]

Hi Frank,

On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> I was hoping to be able to convert the .dts files to use sugar syntax
> instead of hand coding the fragment nodes, but for this specific set
> of files I failed, since the labels that would have been required do
> not already exist in the base .dts files that that overlays would be
> applied against.

Indeed, hence the fixup overlays use "target-path".

BTW, is there any specific reason there is no sugar syntax support in dtc
for absolute target paths? I guess to prevent adding stuff to a random
existing node, and to encourage people to use a "connector" API defined in
term of labels?

I'm also in the process of converting my collection of DT overlays to sugar
syntax, and lack of support for "target-path" is the sole thing that holds
me back from completing this. So for now I use a mix of sugar and
traditional overlay syntax.

In particular, I need "target-path" for two things:
  1. To refer to the root node, for adding devices that should live at
     (a board subnode of) the root node, like:
       - devices connected to GPIO controllers provided by other base or
         overlay devices (e.g. LEDs, displays, buttons, ...),
       - clock providers for other overlays devices (e.g. fixed-clock).
  2. To refer to the aliases node, for adding mandatory serialX aliases.

The former is the real blocker for me.

The latter doesn't work with plain upstream (hacky patches available), so
I'm working on getting rid of the serialX requirement in the serial driver.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 8669 bytes --]

On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> > I was hoping to be able to convert the .dts files to use sugar syntax
> > instead of hand coding the fragment nodes, but for this specific set
> > of files I failed, since the labels that would have been required do
> > not already exist in the base .dts files that that overlays would be
> > applied against.
> 
> Indeed, hence the fixup overlays use "target-path".
> 
> BTW, is there any specific reason there is no sugar syntax support in dtc
> for absolute target paths? I guess to prevent adding stuff to a random
> existing node, and to encourage people to use a "connector" API defined in
> term of labels?

Only because it hasn't been implemented.  Using &{/whatever} should
IMO generate a target-path and the fact it doesn't is a bug.

> I'm also in the process of converting my collection of DT overlays to sugar
> syntax, and lack of support for "target-path" is the sole thing that holds
> me back from completing this. So for now I use a mix of sugar and
> traditional overlay syntax.
> 
> In particular, I need "target-path" for two things:
>   1. To refer to the root node, for adding devices that should live at
>      (a board subnode of) the root node, like:
>        - devices connected to GPIO controllers provided by other base or
>          overlay devices (e.g. LEDs, displays, buttons, ...),
>        - clock providers for other overlays devices (e.g. fixed-clock).
>   2. To refer to the aliases node, for adding mandatory serialX aliases.
> 
> The former is the real blocker for me.
> 
> The latter doesn't work with plain upstream (hacky patches available), so
> I'm working on getting rid of the serialX requirement in the serial driver.

Below is draft patch adding target-path support.  The pretty minimal
test examples do include a case using &{/}

From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Tue, 6 Mar 2018 13:27:53 +1100
Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
 fragments

We've recently added "syntactic sugar" support to generate runtime dtb
overlays using similar syntax to the compile time overlays we've had for
a while.  This worked with the &label { ... } syntax, adjusting an existing
labelled node, but would fail with the &{/path} { ... } syntax attempting
to adjust an existing node referenced by its path.

The previous code would always try to use the "target" property in the
output overlay, which needs to be fixed up, and __fixups__ can only encode
symbols, not paths, so the result could never work properly.

This adds support for the &{/path} syntax for overlays, translating it into
the "target-path" encoding in the output.  It also changes existing
behaviour a little because we now unconditionally one fragment for each
overlay section in the source.  Previously we would only create a fragment
if we couldn't locally resolve the node referenced.  We need this for
path references, because the path is supposed to be referencing something
in the (not yet known) base tree, rather than the overlay tree we are
working with now.  In particular one useful case for path based overlays
is using &{/} - but the constructed overlay tree will always have a root
node, meaning that without the change that would attempt to resolve the
fragment locally, which is not what we want.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 dtc-parser.y                     | 22 +++++++++---------
 livetree.c                       | 12 +++++++---
 tests/overlay_overlay_bypath.dts | 48 ++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh               | 12 ++++++++++
 4 files changed, 80 insertions(+), 14 deletions(-)
 create mode 100644 tests/overlay_overlay_bypath.dts

diff --git a/dtc-parser.y b/dtc-parser.y
index 44af170..25e92d6 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -190,18 +190,18 @@ devicetree:
 		}
 	| devicetree DT_REF nodedef
 		{
-			struct node *target = get_node_by_ref($1, $2);
-
-			if (target) {
-				merge_nodes(target, $3);
+			/*
+			 * We rely on the rule being always:
+			 *   versioninfo plugindecl memreserves devicetree
+			 * so $-1 is what we want (plugindecl)
+			 */
+			if ($<flags>-1 & DTSF_PLUGIN) {
+				add_orphan_node($1, $3, $2);
 			} else {
-				/*
-				 * We rely on the rule being always:
-				 *   versioninfo plugindecl memreserves devicetree
-				 * so $-1 is what we want (plugindecl)
-				 */
-				if ($<flags>-1 & DTSF_PLUGIN)
-					add_orphan_node($1, $3, $2);
+				struct node *target = get_node_by_ref($1, $2);
+
+				if (target)
+					merge_nodes(target, $3);
 				else
 					ERROR(&@2, "Label or path %s not found", $2);
 			}
diff --git a/livetree.c b/livetree.c
index 57b7db2..f691c9b 100644
--- a/livetree.c
+++ b/livetree.c
@@ -224,10 +224,16 @@ struct node * add_orphan_node(struct node *dt, struct node *new_node, char *ref)
 	struct data d = empty_data;
 	char *name;
 
-	d = data_add_marker(d, REF_PHANDLE, ref);
-	d = data_append_integer(d, 0xffffffff, 32);
+	if (ref[0] == '/') {
+		d = data_append_data(d, ref, strlen(ref) + 1);
 
-	p = build_property("target", d);
+		p = build_property("target-path", d);
+	} else {
+		d = data_add_marker(d, REF_PHANDLE, ref);
+		d = data_append_integer(d, 0xffffffff, 32);
+
+		p = build_property("target", d);
+	}
 
 	xasprintf(&name, "fragment@%u",
 			next_orphan_fragment++);
diff --git a/tests/overlay_overlay_bypath.dts b/tests/overlay_overlay_bypath.dts
new file mode 100644
index 0000000..f23e7b6
--- /dev/null
+++ b/tests/overlay_overlay_bypath.dts
@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+/* Test that we can change an int by another */
+&{/test-node} {
+	test-int-property = <43>;
+};
+
+/* Test that we can replace a string by a longer one */
+&{/test-node} {
+	test-str-property = "foobar";
+};
+
+/* Test that we add a new property */
+&{/test-node} {
+	test-str-property-2 = "foobar2";
+};
+
+/* Test that we add a new node (by phandle) */
+&{/test-node} {
+	new-node {
+		new-property;
+	};
+};
+
+&{/} {
+	local: new-local-node {
+		new-property;
+	};
+};
+
+&{/} {
+	test-several-phandle = <&local>, <&local>;
+};
+
+&{/test-node} {
+	sub-test-node {
+		new-sub-test-property;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 4d944fa..c2ce1e6 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -251,13 +251,25 @@ dtc_overlay_tests () {
     run_test check_path overlay_overlay_nosugar.test.dtb exists "/__fixups__"
     run_test check_path overlay_overlay_nosugar.test.dtb exists "/__local_fixups__"
 
+    # Using target-path
+    run_dtc_test -I dts -O dtb -o overlay_overlay_bypath.test.dtb overlay_overlay_bypath.dts
+    run_test check_path overlay_overlay_bypath.test.dtb not-exists "/__symbols__"
+    run_test check_path overlay_overlay_bypath.test.dtb not-exists "/__fixups__"
+    run_test check_path overlay_overlay_bypath.test.dtb exists "/__local_fixups__"
+
     # Check building works the same as manual constructions
     run_test dtbs_equal_ordered overlay_overlay.test.dtb overlay_overlay_nosugar.test.dtb
 
     run_dtc_test -I dts -O dtb -o overlay_overlay_manual_fixups.test.dtb overlay_overlay_manual_fixups.dts
     run_test dtbs_equal_ordered overlay_overlay.test.dtb overlay_overlay_manual_fixups.test.dtb
 
+    run_dtc_test -I dts -O dtb -o overlay_overlay_no_fixups.test.dtb overlay_overlay_no_fixups.dts
+    run_test dtbs_equal_ordered overlay_overlay_bypath.test.dtb overlay_overlay_no_fixups.test.dtb
+
+    # Check we can actually apply the result
+    run_dtc_test -I dts -O dtb -o overlay_base_no_symbols.test.dtb overlay_base.dts
     run_test overlay overlay_base.test.dtb overlay_overlay.test.dtb
+    run_test overlay overlay_base_no_symbols.test.dtb overlay_overlay_bypath.test.dtb
 
     # test plugin source to dtb and back
     run_dtc_test -I dtb -O dts -o overlay_overlay_decompile.test.dts overlay_overlay.test.dtb
-- 
2.14.3




-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Geert Uytterhoeven]

Hi David,

On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> > I was hoping to be able to convert the .dts files to use sugar syntax
>> > instead of hand coding the fragment nodes, but for this specific set
>> > of files I failed, since the labels that would have been required do
>> > not already exist in the base .dts files that that overlays would be
>> > applied against.
>>
>> Indeed, hence the fixup overlays use "target-path".
>>
>> BTW, is there any specific reason there is no sugar syntax support in dtc
>> for absolute target paths? I guess to prevent adding stuff to a random
>> existing node, and to encourage people to use a "connector" API defined in
>> term of labels?
>
> Only because it hasn't been implemented.  Using &{/whatever} should
> IMO generate a target-path and the fact it doesn't is a bug.
>
>> I'm also in the process of converting my collection of DT overlays to sugar
>> syntax, and lack of support for "target-path" is the sole thing that holds
>> me back from completing this. So for now I use a mix of sugar and
>> traditional overlay syntax.
>>
>> In particular, I need "target-path" for two things:
>>   1. To refer to the root node, for adding devices that should live at
>>      (a board subnode of) the root node, like:
>>        - devices connected to GPIO controllers provided by other base or
>>          overlay devices (e.g. LEDs, displays, buttons, ...),
>>        - clock providers for other overlays devices (e.g. fixed-clock).

>> The former is the real blocker for me.

> Below is draft patch adding target-path support.  The pretty minimal
> test examples do include a case using &{/}
>
> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
> From: David Gibson <david@gibson.dropbear.id.au>
> Date: Tue, 6 Mar 2018 13:27:53 +1100
> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>  fragments
>
> We've recently added "syntactic sugar" support to generate runtime dtb
> overlays using similar syntax to the compile time overlays we've had for
> a while.  This worked with the &label { ... } syntax, adjusting an existing
> labelled node, but would fail with the &{/path} { ... } syntax attempting
> to adjust an existing node referenced by its path.
>
> The previous code would always try to use the "target" property in the
> output overlay, which needs to be fixed up, and __fixups__ can only encode
> symbols, not paths, so the result could never work properly.
>
> This adds support for the &{/path} syntax for overlays, translating it into
> the "target-path" encoding in the output.  It also changes existing
> behaviour a little because we now unconditionally one fragment for each
> overlay section in the source.  Previously we would only create a fragment
> if we couldn't locally resolve the node referenced.  We need this for
> path references, because the path is supposed to be referencing something
> in the (not yet known) base tree, rather than the overlay tree we are
> working with now.  In particular one useful case for path based overlays
> is using &{/} - but the constructed overlay tree will always have a root
> node, meaning that without the change that would attempt to resolve the
> fragment locally, which is not what we want.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Thank you, seems to work fine on dtc.git.

Note that while the dtc part applies on the in-kernel copy of dtc, it
doesn't work there: "&{/}" behaves the same as "/" (i.e. no overlay
fragment is generated), but "&{/foo}" does create the overlay fragment.
Merging in Rob's for-next branch to upgrade Linux' copy of dtc fixes that.

Thanks a lot!
Converting my remaining overlay fragments to sugar syntax...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 4579 bytes --]

On Tue, Mar 06, 2018 at 01:30:05PM +0100, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
> >> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> > I was hoping to be able to convert the .dts files to use sugar syntax
> >> > instead of hand coding the fragment nodes, but for this specific set
> >> > of files I failed, since the labels that would have been required do
> >> > not already exist in the base .dts files that that overlays would be
> >> > applied against.
> >>
> >> Indeed, hence the fixup overlays use "target-path".
> >>
> >> BTW, is there any specific reason there is no sugar syntax support in dtc
> >> for absolute target paths? I guess to prevent adding stuff to a random
> >> existing node, and to encourage people to use a "connector" API defined in
> >> term of labels?
> >
> > Only because it hasn't been implemented.  Using &{/whatever} should
> > IMO generate a target-path and the fact it doesn't is a bug.
> >
> >> I'm also in the process of converting my collection of DT overlays to sugar
> >> syntax, and lack of support for "target-path" is the sole thing that holds
> >> me back from completing this. So for now I use a mix of sugar and
> >> traditional overlay syntax.
> >>
> >> In particular, I need "target-path" for two things:
> >>   1. To refer to the root node, for adding devices that should live at
> >>      (a board subnode of) the root node, like:
> >>        - devices connected to GPIO controllers provided by other base or
> >>          overlay devices (e.g. LEDs, displays, buttons, ...),
> >>        - clock providers for other overlays devices (e.g. fixed-clock).
> 
> >> The former is the real blocker for me.
> 
> > Below is draft patch adding target-path support.  The pretty minimal
> > test examples do include a case using &{/}
> >
> > From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
> > From: David Gibson <david@gibson.dropbear.id.au>
> > Date: Tue, 6 Mar 2018 13:27:53 +1100
> > Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
> >  fragments
> >
> > We've recently added "syntactic sugar" support to generate runtime dtb
> > overlays using similar syntax to the compile time overlays we've had for
> > a while.  This worked with the &label { ... } syntax, adjusting an existing
> > labelled node, but would fail with the &{/path} { ... } syntax attempting
> > to adjust an existing node referenced by its path.
> >
> > The previous code would always try to use the "target" property in the
> > output overlay, which needs to be fixed up, and __fixups__ can only encode
> > symbols, not paths, so the result could never work properly.
> >
> > This adds support for the &{/path} syntax for overlays, translating it into
> > the "target-path" encoding in the output.  It also changes existing
> > behaviour a little because we now unconditionally one fragment for each
> > overlay section in the source.  Previously we would only create a fragment
> > if we couldn't locally resolve the node referenced.  We need this for
> > path references, because the path is supposed to be referencing something
> > in the (not yet known) base tree, rather than the overlay tree we are
> > working with now.  In particular one useful case for path based overlays
> > is using &{/} - but the constructed overlay tree will always have a root
> > node, meaning that without the change that would attempt to resolve the
> > fragment locally, which is not what we want.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Thank you, seems to work fine on dtc.git.
> 
> Note that while the dtc part applies on the in-kernel copy of dtc, it
> doesn't work there: "&{/}" behaves the same as "/" (i.e. no overlay
> fragment is generated), but "&{/foo}" does create the overlay fragment.
> Merging in Rob's for-next branch to upgrade Linux' copy of dtc fixes
> that.

I think that'll be because the kernel makefiles (at least by default)
use a pre-generated version of the parser, rather than running bison.
Since there were changes in the .y file, those will be missing which
would cause the error you describe.

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Rob Herring]

On Tue, Mar 6, 2018 at 8:07 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Tue, Mar 06, 2018 at 01:30:05PM +0100, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
>> <david@gibson.dropbear.id.au> wrote:
>> > On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>> >> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> >> > I was hoping to be able to convert the .dts files to use sugar syntax
>> >> > instead of hand coding the fragment nodes, but for this specific set
>> >> > of files I failed, since the labels that would have been required do
>> >> > not already exist in the base .dts files that that overlays would be
>> >> > applied against.
>> >>
>> >> Indeed, hence the fixup overlays use "target-path".
>> >>
>> >> BTW, is there any specific reason there is no sugar syntax support in dtc
>> >> for absolute target paths? I guess to prevent adding stuff to a random
>> >> existing node, and to encourage people to use a "connector" API defined in
>> >> term of labels?
>> >
>> > Only because it hasn't been implemented.  Using &{/whatever} should
>> > IMO generate a target-path and the fact it doesn't is a bug.
>> >
>> >> I'm also in the process of converting my collection of DT overlays to sugar
>> >> syntax, and lack of support for "target-path" is the sole thing that holds
>> >> me back from completing this. So for now I use a mix of sugar and
>> >> traditional overlay syntax.
>> >>
>> >> In particular, I need "target-path" for two things:
>> >>   1. To refer to the root node, for adding devices that should live at
>> >>      (a board subnode of) the root node, like:
>> >>        - devices connected to GPIO controllers provided by other base or
>> >>          overlay devices (e.g. LEDs, displays, buttons, ...),
>> >>        - clock providers for other overlays devices (e.g. fixed-clock).
>>
>> >> The former is the real blocker for me.
>>
>> > Below is draft patch adding target-path support.  The pretty minimal
>> > test examples do include a case using &{/}
>> >
>> > From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
>> > From: David Gibson <david@gibson.dropbear.id.au>
>> > Date: Tue, 6 Mar 2018 13:27:53 +1100
>> > Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>> >  fragments
>> >
>> > We've recently added "syntactic sugar" support to generate runtime dtb
>> > overlays using similar syntax to the compile time overlays we've had for
>> > a while.  This worked with the &label { ... } syntax, adjusting an existing
>> > labelled node, but would fail with the &{/path} { ... } syntax attempting
>> > to adjust an existing node referenced by its path.
>> >
>> > The previous code would always try to use the "target" property in the
>> > output overlay, which needs to be fixed up, and __fixups__ can only encode
>> > symbols, not paths, so the result could never work properly.
>> >
>> > This adds support for the &{/path} syntax for overlays, translating it into
>> > the "target-path" encoding in the output.  It also changes existing
>> > behaviour a little because we now unconditionally one fragment for each
>> > overlay section in the source.  Previously we would only create a fragment
>> > if we couldn't locally resolve the node referenced.  We need this for
>> > path references, because the path is supposed to be referencing something
>> > in the (not yet known) base tree, rather than the overlay tree we are
>> > working with now.  In particular one useful case for path based overlays
>> > is using &{/} - but the constructed overlay tree will always have a root
>> > node, meaning that without the change that would attempt to resolve the
>> > fragment locally, which is not what we want.
>> >
>> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Thank you, seems to work fine on dtc.git.
>>
>> Note that while the dtc part applies on the in-kernel copy of dtc, it
>> doesn't work there: "&{/}" behaves the same as "/" (i.e. no overlay
>> fragment is generated), but "&{/foo}" does create the overlay fragment.
>> Merging in Rob's for-next branch to upgrade Linux' copy of dtc fixes
>> that.
>
> I think that'll be because the kernel makefiles (at least by default)
> use a pre-generated version of the parser, rather than running bison.

Just FYI, as of that branch, that is no longer true. We now run bison.

> Since there were changes in the .y file, those will be missing which
> would cause the error you describe.
>
> --
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 04:30, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
>> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>> I was hoping to be able to convert the .dts files to use sugar syntax
>>>> instead of hand coding the fragment nodes, but for this specific set
>>>> of files I failed, since the labels that would have been required do
>>>> not already exist in the base .dts files that that overlays would be
>>>> applied against.
>>>
>>> Indeed, hence the fixup overlays use "target-path".
>>>
>>> BTW, is there any specific reason there is no sugar syntax support in dtc
>>> for absolute target paths? I guess to prevent adding stuff to a random
>>> existing node, and to encourage people to use a "connector" API defined in
>>> term of labels?
>>
>> Only because it hasn't been implemented.  Using &{/whatever} should
>> IMO generate a target-path and the fact it doesn't is a bug.
>>
>>> I'm also in the process of converting my collection of DT overlays to sugar
>>> syntax, and lack of support for "target-path" is the sole thing that holds
>>> me back from completing this. So for now I use a mix of sugar and
>>> traditional overlay syntax.
>>>
>>> In particular, I need "target-path" for two things:
>>>   1. To refer to the root node, for adding devices that should live at
>>>      (a board subnode of) the root node, like:
>>>        - devices connected to GPIO controllers provided by other base or
>>>          overlay devices (e.g. LEDs, displays, buttons, ...),
>>>        - clock providers for other overlays devices (e.g. fixed-clock).
> 
>>> The former is the real blocker for me.
> 
>> Below is draft patch adding target-path support.  The pretty minimal
>> test examples do include a case using &{/}
>>
>> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
>> From: David Gibson <david@gibson.dropbear.id.au>
>> Date: Tue, 6 Mar 2018 13:27:53 +1100
>> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>>  fragments
>>
>> We've recently added "syntactic sugar" support to generate runtime dtb
>> overlays using similar syntax to the compile time overlays we've had for
>> a while.  This worked with the &label { ... } syntax, adjusting an existing
>> labelled node, but would fail with the &{/path} { ... } syntax attempting
>> to adjust an existing node referenced by its path.
>>
>> The previous code would always try to use the "target" property in the
>> output overlay, which needs to be fixed up, and __fixups__ can only encode
>> symbols, not paths, so the result could never work properly.
>>
>> This adds support for the &{/path} syntax for overlays, translating it into
>> the "target-path" encoding in the output.  It also changes existing
>> behaviour a little because we now unconditionally one fragment for each
>> overlay section in the source.  Previously we would only create a fragment
>> if we couldn't locally resolve the node referenced.  We need this for
>> path references, because the path is supposed to be referencing something
>> in the (not yet known) base tree, rather than the overlay tree we are
>> working with now.  In particular one useful case for path based overlays
>> is using &{/} - but the constructed overlay tree will always have a root
>> node, meaning that without the change that would attempt to resolve the
>> fragment locally, which is not what we want.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Thank you, seems to work fine on dtc.git.

And the patched dtc works for a dts file that I was trying to convert
to sugar dts syntax in the thread that Geert was responding to when he
created this thread.

(Laurent -- no need to worry about this for your current series.
Converting your dts files will be an easy task to do in a future
kernel version -- remind me if I forget to send a patch.)

-Frank

> 
> Note that while the dtc part applies on the in-kernel copy of dtc, it
> doesn't work there: "&{/}" behaves the same as "/" (i.e. no overlay
> fragment is generated), but "&{/foo}" does create the overlay fragment.
> Merging in Rob's for-next branch to upgrade Linux' copy of dtc fixes that.
> 
> Thanks a lot!
> Converting my remaining overlay fragments to sugar syntax...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> .
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 11:51, Frank Rowand wrote:
> On 03/06/18 04:30, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
>> <david@gibson.dropbear.id.au> wrote:
>>> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>>> I was hoping to be able to convert the .dts files to use sugar syntax
>>>>> instead of hand coding the fragment nodes, but for this specific set
>>>>> of files I failed, since the labels that would have been required do
>>>>> not already exist in the base .dts files that that overlays would be
>>>>> applied against.
>>>>
>>>> Indeed, hence the fixup overlays use "target-path".
>>>>
>>>> BTW, is there any specific reason there is no sugar syntax support in dtc
>>>> for absolute target paths? I guess to prevent adding stuff to a random
>>>> existing node, and to encourage people to use a "connector" API defined in
>>>> term of labels?
>>>
>>> Only because it hasn't been implemented.  Using &{/whatever} should
>>> IMO generate a target-path and the fact it doesn't is a bug.
>>>
>>>> I'm also in the process of converting my collection of DT overlays to sugar
>>>> syntax, and lack of support for "target-path" is the sole thing that holds
>>>> me back from completing this. So for now I use a mix of sugar and
>>>> traditional overlay syntax.
>>>>
>>>> In particular, I need "target-path" for two things:
>>>>   1. To refer to the root node, for adding devices that should live at
>>>>      (a board subnode of) the root node, like:
>>>>        - devices connected to GPIO controllers provided by other base or
>>>>          overlay devices (e.g. LEDs, displays, buttons, ...),
>>>>        - clock providers for other overlays devices (e.g. fixed-clock).
>>
>>>> The former is the real blocker for me.
>>
>>> Below is draft patch adding target-path support.  The pretty minimal
>>> test examples do include a case using &{/}
>>>
>>> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
>>> From: David Gibson <david@gibson.dropbear.id.au>
>>> Date: Tue, 6 Mar 2018 13:27:53 +1100
>>> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>>>  fragments
>>>
>>> We've recently added "syntactic sugar" support to generate runtime dtb
>>> overlays using similar syntax to the compile time overlays we've had for
>>> a while.  This worked with the &label { ... } syntax, adjusting an existing
>>> labelled node, but would fail with the &{/path} { ... } syntax attempting
>>> to adjust an existing node referenced by its path.
>>>
>>> The previous code would always try to use the "target" property in the
>>> output overlay, which needs to be fixed up, and __fixups__ can only encode
>>> symbols, not paths, so the result could never work properly.
>>>
>>> This adds support for the &{/path} syntax for overlays, translating it into
>>> the "target-path" encoding in the output.  It also changes existing
>>> behaviour a little because we now unconditionally one fragment for each
>>> overlay section in the source.  Previously we would only create a fragment
>>> if we couldn't locally resolve the node referenced.  We need this for
>>> path references, because the path is supposed to be referencing something
>>> in the (not yet known) base tree, rather than the overlay tree we are
>>> working with now.  In particular one useful case for path based overlays
>>> is using &{/} - but the constructed overlay tree will always have a root
>>> node, meaning that without the change that would attempt to resolve the
>>> fragment locally, which is not what we want.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Thank you, seems to work fine on dtc.git.
> 
> And the patched dtc works for a dts file that I was trying to convert
> to sugar dts syntax

< snip >

I noticed that a space in "&{/}" is an error.  I wanted to check whether
that was deliberate, or that the patch wasn't fully complete yet.
cat path_sugar_v1.dts 

$ nl -ba path_sugar_v1.dts
     1	
     2	/dts-v1/;
     3	/plugin/;
     4	&{/} {
     5			#address-cells = <2>;
     6			#size-cells = <2>;
     7	
     8			my_node@feb90000 {
     9				compatible = "vendor,device";
    10				reg = <0 0xfeb90000 0 0x1c>;
    11	
    12			};
    13	
    14	};

$ dtc -O dts path_sugar_v1.dts 
/dts-v1/;

/ {

	fragment@0 {
		target-path = [2f 00];

		__overlay__ {
			#address-cells = <0x2>;
			#size-cells = <0x2>;

			my_node@feb90000 {
				compatible = "vendor,device";
				reg = <0x0 0xfeb90000 0x0 0x1c>;
			};
		};
	};
};

$ nl -ba path_sugar_v2.dts
     1	
     2	/dts-v1/;
     3	/plugin/;
     4	&{ / } {
     5			#address-cells = <2>;
     6			#size-cells = <2>;
     7	
     8			my_node@feb90000 {
     9				compatible = "vendor,device";
    10				reg = <0 0xfeb90000 0 0x1c>;
    11	
    12			};
    13	
    14	};

$ dtc -O dts path_sugar_v2.dts 
Error: path_sugar_v2.dts:4.1-2 syntax error
FATAL ERROR: Unable to parse input tree

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 6130 bytes --]

On Tue, Mar 06, 2018 at 01:40:20PM -0800, Frank Rowand wrote:
> On 03/06/18 11:51, Frank Rowand wrote:
> > On 03/06/18 04:30, Geert Uytterhoeven wrote:
> >> Hi David,
> >>
> >> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
> >> <david@gibson.dropbear.id.au> wrote:
> >>> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
> >>>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>> I was hoping to be able to convert the .dts files to use sugar syntax
> >>>>> instead of hand coding the fragment nodes, but for this specific set
> >>>>> of files I failed, since the labels that would have been required do
> >>>>> not already exist in the base .dts files that that overlays would be
> >>>>> applied against.
> >>>>
> >>>> Indeed, hence the fixup overlays use "target-path".
> >>>>
> >>>> BTW, is there any specific reason there is no sugar syntax support in dtc
> >>>> for absolute target paths? I guess to prevent adding stuff to a random
> >>>> existing node, and to encourage people to use a "connector" API defined in
> >>>> term of labels?
> >>>
> >>> Only because it hasn't been implemented.  Using &{/whatever} should
> >>> IMO generate a target-path and the fact it doesn't is a bug.
> >>>
> >>>> I'm also in the process of converting my collection of DT overlays to sugar
> >>>> syntax, and lack of support for "target-path" is the sole thing that holds
> >>>> me back from completing this. So for now I use a mix of sugar and
> >>>> traditional overlay syntax.
> >>>>
> >>>> In particular, I need "target-path" for two things:
> >>>>   1. To refer to the root node, for adding devices that should live at
> >>>>      (a board subnode of) the root node, like:
> >>>>        - devices connected to GPIO controllers provided by other base or
> >>>>          overlay devices (e.g. LEDs, displays, buttons, ...),
> >>>>        - clock providers for other overlays devices (e.g. fixed-clock).
> >>
> >>>> The former is the real blocker for me.
> >>
> >>> Below is draft patch adding target-path support.  The pretty minimal
> >>> test examples do include a case using &{/}
> >>>
> >>> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
> >>> From: David Gibson <david@gibson.dropbear.id.au>
> >>> Date: Tue, 6 Mar 2018 13:27:53 +1100
> >>> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
> >>>  fragments
> >>>
> >>> We've recently added "syntactic sugar" support to generate runtime dtb
> >>> overlays using similar syntax to the compile time overlays we've had for
> >>> a while.  This worked with the &label { ... } syntax, adjusting an existing
> >>> labelled node, but would fail with the &{/path} { ... } syntax attempting
> >>> to adjust an existing node referenced by its path.
> >>>
> >>> The previous code would always try to use the "target" property in the
> >>> output overlay, which needs to be fixed up, and __fixups__ can only encode
> >>> symbols, not paths, so the result could never work properly.
> >>>
> >>> This adds support for the &{/path} syntax for overlays, translating it into
> >>> the "target-path" encoding in the output.  It also changes existing
> >>> behaviour a little because we now unconditionally one fragment for each
> >>> overlay section in the source.  Previously we would only create a fragment
> >>> if we couldn't locally resolve the node referenced.  We need this for
> >>> path references, because the path is supposed to be referencing something
> >>> in the (not yet known) base tree, rather than the overlay tree we are
> >>> working with now.  In particular one useful case for path based overlays
> >>> is using &{/} - but the constructed overlay tree will always have a root
> >>> node, meaning that without the change that would attempt to resolve the
> >>> fragment locally, which is not what we want.
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>
> >> Thank you, seems to work fine on dtc.git.
> > 
> > And the patched dtc works for a dts file that I was trying to convert
> > to sugar dts syntax
> 
> < snip >
> 
> I noticed that a space in "&{/}" is an error.  I wanted to check whether
> that was deliberate, or that the patch wasn't fully complete yet.

That's essentially deliberate - it's not really related to this patch
at all.  The patch just re-uses the existing syntax for a "path
reference".  The whole thing is treated as a single token, hence, no
spaces.

It might be possible to change that, but it could introduce some
complications when the path reference syntax is used in other places.
So I'm disinclined to change it, unless there's a very strong reason
to.

> cat path_sugar_v1.dts 
> 
> $ nl -ba path_sugar_v1.dts
>      1	
>      2	/dts-v1/;
>      3	/plugin/;
>      4	&{/} {
>      5			#address-cells = <2>;
>      6			#size-cells = <2>;
>      7	
>      8			my_node@feb90000 {
>      9				compatible = "vendor,device";
>     10				reg = <0 0xfeb90000 0 0x1c>;
>     11	
>     12			};
>     13	
>     14	};
> 
> $ dtc -O dts path_sugar_v1.dts 
> /dts-v1/;
> 
> / {
> 
> 	fragment@0 {
> 		target-path = [2f 00];
> 
> 		__overlay__ {
> 			#address-cells = <0x2>;
> 			#size-cells = <0x2>;
> 
> 			my_node@feb90000 {
> 				compatible = "vendor,device";
> 				reg = <0x0 0xfeb90000 0x0 0x1c>;
> 			};
> 		};
> 	};
> };
> 
> $ nl -ba path_sugar_v2.dts
>      1	
>      2	/dts-v1/;
>      3	/plugin/;
>      4	&{ / } {
>      5			#address-cells = <2>;
>      6			#size-cells = <2>;
>      7	
>      8			my_node@feb90000 {
>      9				compatible = "vendor,device";
>     10				reg = <0 0xfeb90000 0 0x1c>;
>     11	
>     12			};
>     13	
>     14	};
> 
> $ dtc -O dts path_sugar_v2.dts 
> Error: path_sugar_v2.dts:4.1-2 syntax error
> FATAL ERROR: Unable to parse input tree
> 

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 18:30, David Gibson wrote:
> On Tue, Mar 06, 2018 at 01:40:20PM -0800, Frank Rowand wrote:
>> On 03/06/18 11:51, Frank Rowand wrote:
>>> On 03/06/18 04:30, Geert Uytterhoeven wrote:

< snip >

>>> And the patched dtc works for a dts file that I was trying to convert
>>> to sugar dts syntax
>>
>> < snip >
>>
>> I noticed that a space in "&{/}" is an error.  I wanted to check whether
>> that was deliberate, or that the patch wasn't fully complete yet.
> 
> That's essentially deliberate - it's not really related to this patch
> at all.  The patch just re-uses the existing syntax for a "path
> reference".  The whole thing is treated as a single token, hence, no
> spaces.
> 
> It might be possible to change that, but it could introduce some
> complications when the path reference syntax is used in other places.
> So I'm disinclined to change it, unless there's a very strong reason
> to.
> 

< snip >

No, please do not change.  I just wanted to make sure I understood the
intended syntax.

BTW, thanks for all the time you've been putting into this recent stuff.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel