[PATCH 0/2] Add capability to append to property

[PATCH 0/2] Add capability to append to property

[Ayush Singh]

Allow appending values to a property instead of overriding the previous
values of property.

Currently, we have /delete-node/ and /delete-property/, but lack
/append-property/. Hence we end up having to repeat all existing values
when appending to a property (e.g. see [1] appending to clocks from
[2]).

This functionality is also important for creating a device tree based
implementation to support different types of addon-boards such as
mikroBUS, Grove [3], etc.

Open items
- Better tests: I only started the work today, so well, I still don't
  have a good picture of how to properly write tests.

- Appending to non-existent property: Currently am just adding a new
  property if not found. Not sure if this is the desired behaviour.

- Incompatible values: Not sure how to handle when trying to append
  incompatible values.

[3] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
[2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L39
[1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L3334

Signed-off-by: Ayush Singh <ayush@beagleboard.org>

---
Ayush Singh (2):
      dtc: Add /append-property/
      tests: Add test for append-property

 dtc-lexer.l                 |  7 +++++++
 dtc-parser.y                |  6 ++++++
 dtc.h                       |  3 +++
 livetree.c                  | 30 +++++++++++++++++++++++++-----
 tests/run_tests.sh          |  3 +++
 tests/test_tree1_append.dts | 21 +++++++++++++++++++++
 6 files changed, 65 insertions(+), 5 deletions(-)
---
base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
change-id: 20240827-append-f614fe3261f9

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

[PATCH 1/2] dtc: Add /append-property/

[Ayush Singh]

Allow appending values to a property instead of overriding the previous
values of property.

Currently, we have /delete-node/ and /delete-property/, but lack
/append-property/. Hence we end up having to repeat all existing values
when appending to a property (e.g. see [1] appending to clocks from
[2]).

This functionality is also important for creating a device tree based
implementation to support different types of addon-boards such as
mikroBUS, Grove [3], etc.

[3] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
[2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L39
[1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L3334

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 dtc-lexer.l  |  7 +++++++
 dtc-parser.y |  6 ++++++
 dtc.h        |  3 +++
 livetree.c   | 30 +++++++++++++++++++++++++-----
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index de60a70..5da4ca5 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -137,6 +137,13 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
 			return DT_DEL_NODE;
 		}
 
+<*>"/append-property/"	{
+			DPRINT("Keyword: /append-property/\n");
+			DPRINT("<PROPNODENAME>\n");
+			BEGIN(PROPNODENAME);
+			return DT_APP_PROP;
+		}
+
 <*>"/omit-if-no-ref/"	{
 			DPRINT("Keyword: /omit-if-no-ref/\n");
 			DPRINT("<PROPNODENAME>\n");
diff --git a/dtc-parser.y b/dtc-parser.y
index 4d5eece..94ce405 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -58,6 +58,7 @@ static bool is_ref_relative(const char *ref)
 %token DT_BITS
 %token DT_DEL_PROP
 %token DT_DEL_NODE
+%token DT_APP_PROP
 %token DT_OMIT_NO_REF
 %token <propnodename> DT_PROPNODENAME
 %token <integer> DT_LITERAL
@@ -296,6 +297,11 @@ propdef:
 			$$ = build_property_delete($2);
 			free($2);
 		}
+	| DT_APP_PROP DT_PROPNODENAME '=' propdata ';'
+		{
+			$$ = build_property_append($2, $4, &@$);
+			free($2);
+		}
 	| DT_LABEL propdef
 		{
 			add_label(&$2->labels, $1);
diff --git a/dtc.h b/dtc.h
index 4c4aaca..06771c5 100644
--- a/dtc.h
+++ b/dtc.h
@@ -205,6 +205,7 @@ struct bus_type {
 
 struct property {
 	bool deleted;
+	bool append;
 	char *name;
 	struct data val;
 
@@ -263,6 +264,8 @@ void delete_labels(struct label **labels);
 struct property *build_property(const char *name, struct data val,
 				struct srcpos *srcpos);
 struct property *build_property_delete(const char *name);
+struct property *build_property_append(const char *name, struct data val,
+				       struct srcpos *srcpos);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
 
diff --git a/livetree.c b/livetree.c
index 49f7230..74eca1b 100644
--- a/livetree.c
+++ b/livetree.c
@@ -62,6 +62,20 @@ struct property *build_property_delete(const char *name)
 	return new;
 }
 
+struct property *build_property_append(const char *name, struct data val,
+				       struct srcpos *srcpos)
+{
+	struct property *new = xmalloc(sizeof(*new));
+
+	memset(new, 0, sizeof(*new));
+	new->name = xstrdup(name);
+	new->append = true;
+	new->val = val;
+	new->srcpos = srcpos_copy(srcpos);
+
+	return new;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);
@@ -151,8 +165,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	for_each_label_withdel(new_node->labels, l)
 		add_label(&old_node->labels, l->label);
 
-	/* Move properties from the new node to the old node.  If there
-	 * is a collision, replace the old value with the new */
+	/* Move properties from the new node to the old node. If there
+	 * is a collision, replace/append the old value with the new */
 	while (new_node->proplist) {
 		/* Pop the property off the list */
 		new_prop = new_node->proplist;
@@ -165,15 +179,21 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 			continue;
 		}
 
-		/* Look for a collision, set new value if there is */
+		/* Look for a collision, set new value/append if there is */
 		for_each_property_withdel(old_node, old_prop) {
 			if (streq(old_prop->name, new_prop->name)) {
 				/* Add new labels to old property */
 				for_each_label_withdel(new_prop->labels, l)
 					add_label(&old_prop->labels, l->label);
 
-				old_prop->val = new_prop->val;
-				old_prop->deleted = 0;
+				if (new_prop->append) {
+					old_prop->val = data_merge(old_prop->val, new_prop->val);
+				} else {
+					old_prop->val = new_prop->val;
+				}
+
+				old_prop->deleted = false;
+				old_prop->append = false;
 				free(old_prop->srcpos);
 				old_prop->srcpos = new_prop->srcpos;
 				free(new_prop);

-- 
2.46.0

[PATCH 2/2] tests: Add test for append-property

[Ayush Singh]

Simple test to build a device tree with /append-property/.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 tests/run_tests.sh          |  3 +++
 tests/test_tree1_append.dts | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 937b128..ac0dc05 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -708,6 +708,9 @@ dtc_tests () {
     run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
     tree1_tests dtc_tree1_delete.test.dtb
 
+    # Check prop append functionality
+    run_dtc_test -I dts -O dtb -o dtc_tree1_append.test.dtb "$SRCDIR/test_tree1_append.dts"
+
     # Check omit-if-no-ref functionality
     run_dtc_test -I dts -O dtb -o omit-no-ref.test.dtb "$SRCDIR/omit-no-ref.dts"
     run_test check_path omit-no-ref.test.dtb not-exists "/node1"
diff --git a/tests/test_tree1_append.dts b/tests/test_tree1_append.dts
new file mode 100644
index 0000000..953095d
--- /dev/null
+++ b/tests/test_tree1_append.dts
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/include/ "test_tree1.dts"
+
+/ {
+	nonexistant-property = <0x00000000>;
+
+	subnode@1 {
+		sub-prop = <0>;
+		str-prop = "0";
+	};
+};
+
+/ {
+	/append-property/ nonexistant-property = <0x00000001 0x00000002>;
+
+	subnode@1 {
+		/append-property/ sub-prop = <1 2>;
+		/append-property/ str-prop = "1", "2";
+  };
+};

-- 
2.46.0

Re: [PATCH 2/2] tests: Add test for append-property

[Simon Glass]

Hi Ayush,

On Tue, 27 Aug 2024 at 11:55, Ayush Singh <ayush@beagleboard.org> wrote:
>
> Simple test to build a device tree with /append-property/.
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  tests/run_tests.sh          |  3 +++
>  tests/test_tree1_append.dts | 21 +++++++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 937b128..ac0dc05 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -708,6 +708,9 @@ dtc_tests () {
>      run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
>      tree1_tests dtc_tree1_delete.test.dtb
>
> +    # Check prop append functionality
> +    run_dtc_test -I dts -O dtb -o dtc_tree1_append.test.dtb "$SRCDIR/test_tree1_append.dts"
> +

How does this check that something actually happened? From your cover
letter, it seems that you might send a v2, but I'm just checking?

>      # Check omit-if-no-ref functionality
>      run_dtc_test -I dts -O dtb -o omit-no-ref.test.dtb "$SRCDIR/omit-no-ref.dts"
>      run_test check_path omit-no-ref.test.dtb not-exists "/node1"
> diff --git a/tests/test_tree1_append.dts b/tests/test_tree1_append.dts
> new file mode 100644
> index 0000000..953095d
> --- /dev/null
> +++ b/tests/test_tree1_append.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +
> +/include/ "test_tree1.dts"
> +
> +/ {
> +       nonexistant-property = <0x00000000>;
> +
> +       subnode@1 {
> +               sub-prop = <0>;
> +               str-prop = "0";
> +       };
> +};
> +
> +/ {
> +       /append-property/ nonexistant-property = <0x00000001 0x00000002>;
> +
> +       subnode@1 {
> +               /append-property/ sub-prop = <1 2>;
> +               /append-property/ str-prop = "1", "2";
> +  };
> +};
>
> --
> 2.46.0
>
>

Regards,
Simon

Re: [PATCH 1/2] dtc: Add /append-property/

[Simon Glass]

Hi Ayush,

On Tue, 27 Aug 2024 at 11:55, Ayush Singh <ayush@beagleboard.org> wrote:
>
> Allow appending values to a property instead of overriding the previous
> values of property.
>
> Currently, we have /delete-node/ and /delete-property/, but lack
> /append-property/. Hence we end up having to repeat all existing values
> when appending to a property (e.g. see [1] appending to clocks from
> [2]).
>
> This functionality is also important for creating a device tree based
> implementation to support different types of addon-boards such as
> mikroBUS, Grove [3], etc.
>
> [3] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
> [2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L39
> [1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L3334
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  dtc-lexer.l  |  7 +++++++
>  dtc-parser.y |  6 ++++++
>  dtc.h        |  3 +++
>  livetree.c   | 30 +++++++++++++++++++++++++-----
>  4 files changed, 41 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

nits below

>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index de60a70..5da4ca5 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -137,6 +137,13 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
>                         return DT_DEL_NODE;
>                 }
>
> +<*>"/append-property/" {
> +                       DPRINT("Keyword: /append-property/\n");
> +                       DPRINT("<PROPNODENAME>\n");
> +                       BEGIN(PROPNODENAME);
> +                       return DT_APP_PROP;
> +               }
> +
>  <*>"/omit-if-no-ref/"  {
>                         DPRINT("Keyword: /omit-if-no-ref/\n");
>                         DPRINT("<PROPNODENAME>\n");
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4d5eece..94ce405 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -58,6 +58,7 @@ static bool is_ref_relative(const char *ref)
>  %token DT_BITS
>  %token DT_DEL_PROP
>  %token DT_DEL_NODE
> +%token DT_APP_PROP
>  %token DT_OMIT_NO_REF
>  %token <propnodename> DT_PROPNODENAME
>  %token <integer> DT_LITERAL
> @@ -296,6 +297,11 @@ propdef:
>                         $$ = build_property_delete($2);
>                         free($2);
>                 }
> +       | DT_APP_PROP DT_PROPNODENAME '=' propdata ';'
> +               {
> +                       $$ = build_property_append($2, $4, &@$);
> +                       free($2);
> +               }
>         | DT_LABEL propdef
>                 {
>                         add_label(&$2->labels, $1);
> diff --git a/dtc.h b/dtc.h
> index 4c4aaca..06771c5 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -205,6 +205,7 @@ struct bus_type {
>
>  struct property {
>         bool deleted;
> +       bool append;

Can you add a comment as to what this means?

>         char *name;
>         struct data val;
>
> @@ -263,6 +264,8 @@ void delete_labels(struct label **labels);
>  struct property *build_property(const char *name, struct data val,
>                                 struct srcpos *srcpos);
>  struct property *build_property_delete(const char *name);
> +struct property *build_property_append(const char *name, struct data val,
> +                                      struct srcpos *srcpos);
>  struct property *chain_property(struct property *first, struct property *list);
>  struct property *reverse_properties(struct property *first);
>
> diff --git a/livetree.c b/livetree.c
> index 49f7230..74eca1b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -62,6 +62,20 @@ struct property *build_property_delete(const char *name)
>         return new;
>  }
>
> +struct property *build_property_append(const char *name, struct data val,
> +                                      struct srcpos *srcpos)
> +{
> +       struct property *new = xmalloc(sizeof(*new));
> +
> +       memset(new, 0, sizeof(*new));
> +       new->name = xstrdup(name);
> +       new->append = true;
> +       new->val = val;
> +       new->srcpos = srcpos_copy(srcpos);
> +
> +       return new;
> +}
> +
>  struct property *chain_property(struct property *first, struct property *list)
>  {
>         assert(first->next == NULL);
> @@ -151,8 +165,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>         for_each_label_withdel(new_node->labels, l)
>                 add_label(&old_node->labels, l->label);
>
> -       /* Move properties from the new node to the old node.  If there
> -        * is a collision, replace the old value with the new */
> +       /* Move properties from the new node to the old node. If there
> +        * is a collision, replace/append the old value with the new */
>         while (new_node->proplist) {
>                 /* Pop the property off the list */
>                 new_prop = new_node->proplist;
> @@ -165,15 +179,21 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>                         continue;
>                 }
>
> -               /* Look for a collision, set new value if there is */
> +               /* Look for a collision, set new value/append if there is */
>                 for_each_property_withdel(old_node, old_prop) {
>                         if (streq(old_prop->name, new_prop->name)) {
>                                 /* Add new labels to old property */
>                                 for_each_label_withdel(new_prop->labels, l)
>                                         add_label(&old_prop->labels, l->label);
>
> -                               old_prop->val = new_prop->val;
> -                               old_prop->deleted = 0;
> +                               if (new_prop->append) {
> +                                       old_prop->val = data_merge(old_prop->val, new_prop->val);
> +                               } else {
> +                                       old_prop->val = new_prop->val;
> +                               }

Braces are not needed here.

> +
> +                               old_prop->deleted = false;
> +                               old_prop->append = false;
>                                 free(old_prop->srcpos);
>                                 old_prop->srcpos = new_prop->srcpos;
>                                 free(new_prop);
>
> --
> 2.46.0
>
>

Regards,
Simon

Re: [PATCH 2/2] tests: Add test for append-property

[Ayush Singh]

On 8/29/24 19:34, Simon Glass wrote:

> Hi Ayush,
>
> On Tue, 27 Aug 2024 at 11:55, Ayush Singh <ayush@beagleboard.org> wrote:
>> Simple test to build a device tree with /append-property/.
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   tests/run_tests.sh          |  3 +++
>>   tests/test_tree1_append.dts | 21 +++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
>> index 937b128..ac0dc05 100755
>> --- a/tests/run_tests.sh
>> +++ b/tests/run_tests.sh
>> @@ -708,6 +708,9 @@ dtc_tests () {
>>       run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
>>       tree1_tests dtc_tree1_delete.test.dtb
>>
>> +    # Check prop append functionality
>> +    run_dtc_test -I dts -O dtb -o dtc_tree1_append.test.dtb "$SRCDIR/test_tree1_append.dts"
>> +
> How does this check that something actually happened? From your cover
> letter, it seems that you might send a v2, but I'm just checking?

Yeah, it tries to build it and that's all. I am not sure if 
`run_dtc_test` fails if the command fails.


I can see there are helpers like `check_path` but is there any helper to 
check the value of a property? If not, I can try to write one.


Ayush Singh

Re: [PATCH 2/2] tests: Add test for append-property

[Simon Glass]

Hi Ayush,

On Thu, 29 Aug 2024 at 08:17, Ayush Singh <ayush@beagleboard.org> wrote:
>
> On 8/29/24 19:34, Simon Glass wrote:
>
> > Hi Ayush,
> >
> > On Tue, 27 Aug 2024 at 11:55, Ayush Singh <ayush@beagleboard.org> wrote:
> >> Simple test to build a device tree with /append-property/.
> >>
> >> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> >> ---
> >>   tests/run_tests.sh          |  3 +++
> >>   tests/test_tree1_append.dts | 21 +++++++++++++++++++++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> >> index 937b128..ac0dc05 100755
> >> --- a/tests/run_tests.sh
> >> +++ b/tests/run_tests.sh
> >> @@ -708,6 +708,9 @@ dtc_tests () {
> >>       run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
> >>       tree1_tests dtc_tree1_delete.test.dtb
> >>
> >> +    # Check prop append functionality
> >> +    run_dtc_test -I dts -O dtb -o dtc_tree1_append.test.dtb "$SRCDIR/test_tree1_append.dts"
> >> +
> > How does this check that something actually happened? From your cover
> > letter, it seems that you might send a v2, but I'm just checking?
>
> Yeah, it tries to build it and that's all. I am not sure if
> `run_dtc_test` fails if the command fails.
>
>
> I can see there are helpers like `check_path` but is there any helper to
> check the value of a property? If not, I can try to write one.

The fdtget tests do some of that.

Regards,
Simon