[PATCH master v2 1/2] of: implement of_reparent_node helper

[PATCH master v2 1/2] of: implement of_reparent_node helper

[Ahmad Fatoum]

Reparenting nodes can be a useful thing to do in fixups. Add a helper
for that.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - rename from of_move_node to of_reparent_node
  - ensure that there's a parent to avoid null pointer deref
    (Sascha)
  - set node->parent to new_parent (Sascha)
  - Remove broken condition on old parent (Sascha)
---
 drivers/of/base.c           | 26 ++++++++++++++++++++++++++
 include/of.h                |  1 +
 test/self/of_manipulation.c |  8 ++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4dc1c76b136d..e1134e9c694b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2697,6 +2697,32 @@ void of_delete_node(struct device_node *node)
 	free(node);
 }
 
+/*
+ * of_reparent_node - Move node from beneath one parent to another
+ * @new_parent: The new parent node
+ * @node        The node to be moved
+ */
+int of_reparent_node(struct device_node *new_parent, struct device_node *node)
+{
+	if (!node)
+		return 0;
+
+	if (!new_parent || !node->parent)
+		return -EINVAL;
+
+	list_del(&node->parent_list);
+	list_del(&node->list);
+
+	free(node->full_name);
+	node->full_name = basprintf("%s/%s", new_parent->full_name, node->name);
+
+	list_add(&node->list, &new_parent->list);
+	list_add_tail(&node->parent_list, &new_parent->children);
+	node->parent = new_parent;
+
+	return 0;
+}
+
 /*
  * of_find_node_by_chosen - Find a node given a chosen property pointing at it
  * @propname:   the name of the property containing a path or alias
diff --git a/include/of.h b/include/of.h
index 92a15f5c4a13..4cc26164a703 100644
--- a/include/of.h
+++ b/include/of.h
@@ -188,6 +188,7 @@ extern struct device_node *of_create_node(struct device_node *root,
 extern void of_merge_nodes(struct device_node *np, const struct device_node *other);
 extern struct device_node *of_copy_node(struct device_node *parent,
 				const struct device_node *other);
+int of_reparent_node(struct device_node *parent, struct device_node *node);
 extern struct device_node *of_dup(const struct device_node *root);
 extern void of_delete_node(struct device_node *node);
 
diff --git a/test/self/of_manipulation.c b/test/self/of_manipulation.c
index 64913ac1eab8..8843cfbe607e 100644
--- a/test/self/of_manipulation.c
+++ b/test/self/of_manipulation.c
@@ -37,7 +37,9 @@ static void test_of_basics(struct device_node *root)
 	struct device_node *node1, *node2, *node21;
 
 	node1 = of_new_node(root, "node1");
-	node2 = of_new_node(root, "node2");
+	node2 = of_new_node(node1, "node2");
+
+	of_reparent_node(root, node2);
 
 	assert_equal(node1, node2);
 
@@ -77,7 +79,9 @@ static void test_of_property_strings(struct device_node *root)
 	np1 = of_new_node(root, "np1");
 	np2 = of_new_node(root, "np2");
 	np3 = of_new_node(root, "np3");
-	np4 = of_new_node(root, "np4");
+	np4 = of_new_node(np1, "np4");
+
+	of_reparent_node(root, np4);
 
 	of_property_sprintf(np1, "property-single", "%c%c%c", 'a', 'y', 'y');
 
-- 
2.39.2

[PATCH master v2 2/2] boards: qemu-virt: support older QEMU with /soc/flash

[Ahmad Fatoum]

Qemu versions differ in whether the flash is top-level or under the /soc
node. As we apply a state fixup to the Qemu-passed DT, we need to know
where the flash at. Easiest way is just to move it for older Qemus to
the new location.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - no change (besides renaming of_move_node -> of_reparent_node)
---
 common/boards/qemu-virt/board.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/common/boards/qemu-virt/board.c b/common/boards/qemu-virt/board.c
index 4c6df5e30252..91d852ed72d2 100644
--- a/common/boards/qemu-virt/board.c
+++ b/common/boards/qemu-virt/board.c
@@ -52,7 +52,7 @@ BAREBOX_DEEP_PROBE_ENABLE(virt_of_match);
  */
 static int virt_board_driver_init(void)
 {
-	struct device_node *root = of_get_root_node();
+	struct device_node *soc, *root = of_get_root_node();
 	struct device_node *overlay, *pubkey;
 	const struct of_device_id *id;
 	void (*init)(void);
@@ -66,6 +66,11 @@ static int virt_board_driver_init(void)
 		init();
 	}
 
+	/* Rename older QEMU's /soc/flash@X to /flash@X */
+	soc = of_get_child_by_name(root, "soc");
+	if (soc)
+		of_reparent_node(root, of_find_node_by_name(soc, "flash"));
+
 	overlay = of_unflatten_dtb(__dtbo_qemu_virt_flash_start, INT_MAX);
 	of_overlay_apply_tree(root, overlay);
 
-- 
2.39.2

Re: [PATCH master v2 1/2] of: implement of_reparent_node helper

[Sascha Hauer]

On Thu, Jun 29, 2023 at 08:56:12AM +0200, Ahmad Fatoum wrote:
> Reparenting nodes can be a useful thing to do in fixups. Add a helper
> for that.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>   - rename from of_move_node to of_reparent_node
>   - ensure that there's a parent to avoid null pointer deref
>     (Sascha)
>   - set node->parent to new_parent (Sascha)
>   - Remove broken condition on old parent (Sascha)
> ---
>  drivers/of/base.c           | 26 ++++++++++++++++++++++++++
>  include/of.h                |  1 +
>  test/self/of_manipulation.c |  8 ++++++--
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4dc1c76b136d..e1134e9c694b 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2697,6 +2697,32 @@ void of_delete_node(struct device_node *node)
>  	free(node);
>  }
>  
> +/*
> + * of_reparent_node - Move node from beneath one parent to another
> + * @new_parent: The new parent node
> + * @node        The node to be moved
> + */
> +int of_reparent_node(struct device_node *new_parent, struct device_node *node)
> +{
> +	if (!node)
> +		return 0;
> +
> +	if (!new_parent || !node->parent)
> +		return -EINVAL;
> +
> +	list_del(&node->parent_list);
> +	list_del(&node->list);

You could put these list operations into a if (node->parent), then @node
wouldn't necessarily need a parent.

> +
> +	free(node->full_name);
> +	node->full_name = basprintf("%s/%s", new_parent->full_name, node->name);

That fixes full_name of @node, but not its children.

One obvious solution would be to iterate over the children and fix the
name recursively. Another one would be to git rid of the full_name
member and generate the name dynamically when needed, but that wouldn't be
master material anymore.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master v2 1/2] of: implement of_reparent_node helper

[Ahmad Fatoum]

On 03.07.23 10:54, Sascha Hauer wrote:
> On Thu, Jun 29, 2023 at 08:56:12AM +0200, Ahmad Fatoum wrote:
>> Reparenting nodes can be a useful thing to do in fixups. Add a helper
>> for that.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> v1 -> v2:
>>   - rename from of_move_node to of_reparent_node
>>   - ensure that there's a parent to avoid null pointer deref
>>     (Sascha)
>>   - set node->parent to new_parent (Sascha)
>>   - Remove broken condition on old parent (Sascha)
>> ---
>>  drivers/of/base.c           | 26 ++++++++++++++++++++++++++
>>  include/of.h                |  1 +
>>  test/self/of_manipulation.c |  8 ++++++--
>>  3 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 4dc1c76b136d..e1134e9c694b 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2697,6 +2697,32 @@ void of_delete_node(struct device_node *node)
>>  	free(node);
>>  }
>>  
>> +/*
>> + * of_reparent_node - Move node from beneath one parent to another
>> + * @new_parent: The new parent node
>> + * @node        The node to be moved
>> + */
>> +int of_reparent_node(struct device_node *new_parent, struct device_node *node)
>> +{
>> +	if (!node)
>> +		return 0;
>> +
>> +	if (!new_parent || !node->parent)
>> +		return -EINVAL;
>> +
>> +	list_del(&node->parent_list);
>> +	list_del(&node->list);
> 
> You could put these list operations into a if (node->parent), then @node
> wouldn't necessarily need a parent.

A node without a parent doesn't have a name, that's why I limited
it to nodes that have a parent.

> 
>> +
>> +	free(node->full_name);
>> +	node->full_name = basprintf("%s/%s", new_parent->full_name, node->name);
> 
> That fixes full_name of @node, but not its children.
> 
> One obvious solution would be to iterate over the children and fix the
> name recursively. Another one would be to git rid of the full_name
> member and generate the name dynamically when needed, but that wouldn't be
> master material anymore.

Argh.. Thanks for noticing.


> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |