[PATCH v2 1/4] of: base: factor out of_merge_nodes from of_copy_node

[PATCH v2 1/4] of: base: factor out of_merge_nodes from of_copy_node

[Ahmad Fatoum]

Later commit will need to merge two DTs from the root up. Refactor
that part out of of_copy_node to make it usable on its own.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - no changes
---
 drivers/of/base.c | 17 ++++++++++++-----
 include/of.h      |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 937847f44ab7..1221cd316cdf 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2616,19 +2616,26 @@ out:
 	return dn;
 }
 
-struct device_node *of_copy_node(struct device_node *parent, const struct device_node *other)
+void of_merge_nodes(struct device_node *np, const struct device_node *other)
 {
-	struct device_node *np, *child;
+	struct device_node *child;
 	struct property *pp;
 
-	np = of_new_node(parent, other->name);
-	np->phandle = other->phandle;
-
 	list_for_each_entry(pp, &other->properties, list)
 		of_new_property(np, pp->name, pp->value, pp->length);
 
 	for_each_child_of_node(other, child)
 		of_copy_node(np, child);
+}
+
+struct device_node *of_copy_node(struct device_node *parent, const struct device_node *other)
+{
+	struct device_node *np;
+
+	np = of_new_node(parent, other->name);
+	np->phandle = other->phandle;
+
+	of_merge_nodes(np, other);
 
 	return np;
 }
diff --git a/include/of.h b/include/of.h
index 7ee1304b932b..1a38774615a4 100644
--- a/include/of.h
+++ b/include/of.h
@@ -180,6 +180,7 @@ extern struct device_node *of_new_node(struct device_node *parent,
 				const char *name);
 extern struct device_node *of_create_node(struct device_node *root,
 					const char *path);
+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);
 extern struct device_node *of_dup(const struct device_node *root);
-- 
2.30.2

[PATCH v2 2/4] of: support of_ensure_probed for top-level machine device

[Ahmad Fatoum]

Creation of a machine device for the top-level node has special casing
in of_probe(). Export of_platform_device_create_root(), so it's possible
to ensure probe of the machine device. This is required when doing
of_devices_ensure_probed_by_dev_id with the machine compatible.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch to facilitate patch 3/4
---
 drivers/of/base.c     | 10 +++++++---
 drivers/of/platform.c |  2 +-
 include/of.h          |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1221cd316cdf..7ba0655401cc 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2512,13 +2512,13 @@ static int of_probe_memory(void)
 }
 mem_initcall(of_probe_memory);
 
-static void of_platform_device_create_root(struct device_node *np)
+struct device *of_platform_device_create_root(struct device_node *np)
 {
 	static struct device *dev;
 	int ret;
 
 	if (dev)
-		return;
+		return dev;
 
 	dev = xzalloc(sizeof(*dev));
 	dev->id = DEVICE_ID_SINGLE;
@@ -2526,8 +2526,12 @@ static void of_platform_device_create_root(struct device_node *np)
 	dev_set_name(dev, "machine");
 
 	ret = platform_device_register(dev);
-	if (ret)
+	if (ret) {
 		free_device(dev);
+		return ERR_PTR(ret);
+	}
+
+	return dev;
 }
 
 static const struct of_device_id reserved_mem_matches[] = {
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 69f9ddec50ca..11cde8924f94 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -421,7 +421,7 @@ static struct device *of_device_create_on_demand(struct device_node *np)
 
 	parent = of_get_parent(np);
 	if (!parent)
-		return NULL;
+		return of_platform_device_create_root(np);
 
 	if (!np->dev && parent->dev) {
 		ret = device_detect(parent->dev);
diff --git a/include/of.h b/include/of.h
index 1a38774615a4..63c90255cd64 100644
--- a/include/of.h
+++ b/include/of.h
@@ -298,6 +298,7 @@ extern void of_platform_device_dummy_drv(struct device *dev);
 extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				struct device *parent);
+extern struct device *of_platform_device_create_root(struct device_node *np);
 extern struct device *of_find_device_by_node(struct device_node *np);
 extern struct device *of_device_enable_and_register(struct device_node *np);
 extern struct device *of_device_enable_and_register_by_name(const char *name);
-- 
2.30.2

Re: [PATCH v2 2/4] of: support of_ensure_probed for top-level machine device

[Michael Riesch]

Hi Ahmad,

On 2/10/23 17:53, Ahmad Fatoum wrote:
> Creation of a machine device for the top-level node has special casing
> in of_probe(). Export of_platform_device_create_root(), so it's possible
> to ensure probe of the machine device. This is required when doing
> of_devices_ensure_probed_by_dev_id with the machine compatible.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

This patch breaks my board code that calls
of_device_ensure_probed_by_alias() in the probe() of the board driver.
The function always returns -ENODEV.

Do I have to adjust my (downstream) board code somehow?

Thanks and best regards,
Michael

> ---
> v1 -> v2:
>   - new patch to facilitate patch 3/4
> ---
>  drivers/of/base.c     | 10 +++++++---
>  drivers/of/platform.c |  2 +-
>  include/of.h          |  1 +
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 1221cd316cdf..7ba0655401cc 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2512,13 +2512,13 @@ static int of_probe_memory(void)
>  }
>  mem_initcall(of_probe_memory);
>  
> -static void of_platform_device_create_root(struct device_node *np)
> +struct device *of_platform_device_create_root(struct device_node *np)
>  {
>  	static struct device *dev;
>  	int ret;
>  
>  	if (dev)
> -		return;
> +		return dev;
>  
>  	dev = xzalloc(sizeof(*dev));
>  	dev->id = DEVICE_ID_SINGLE;
> @@ -2526,8 +2526,12 @@ static void of_platform_device_create_root(struct device_node *np)
>  	dev_set_name(dev, "machine");
>  
>  	ret = platform_device_register(dev);
> -	if (ret)
> +	if (ret) {
>  		free_device(dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return dev;
>  }
>  
>  static const struct of_device_id reserved_mem_matches[] = {
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 69f9ddec50ca..11cde8924f94 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -421,7 +421,7 @@ static struct device *of_device_create_on_demand(struct device_node *np)
>  
>  	parent = of_get_parent(np);
>  	if (!parent)
> -		return NULL;
> +		return of_platform_device_create_root(np);
>  
>  	if (!np->dev && parent->dev) {
>  		ret = device_detect(parent->dev);
> diff --git a/include/of.h b/include/of.h
> index 1a38774615a4..63c90255cd64 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -298,6 +298,7 @@ extern void of_platform_device_dummy_drv(struct device *dev);
>  extern int of_platform_populate(struct device_node *root,
>  				const struct of_device_id *matches,
>  				struct device *parent);
> +extern struct device *of_platform_device_create_root(struct device_node *np);
>  extern struct device *of_find_device_by_node(struct device_node *np);
>  extern struct device *of_device_enable_and_register(struct device_node *np);
>  extern struct device *of_device_enable_and_register_by_name(const char *name);

Re: [PATCH v2 2/4] of: support of_ensure_probed for top-level machine device

[Ahmad Fatoum]

On 10.03.23 15:46, Michael Riesch wrote:
> Hi Ahmad,
> 
> On 2/10/23 17:53, Ahmad Fatoum wrote:
>> Creation of a machine device for the top-level node has special casing
>> in of_probe(). Export of_platform_device_create_root(), so it's possible
>> to ensure probe of the machine device. This is required when doing
>> of_devices_ensure_probed_by_dev_id with the machine compatible.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> This patch breaks my board code that calls
> of_device_ensure_probed_by_alias() in the probe() of the board driver.
> The function always returns -ENODEV.
> 
> Do I have to adjust my (downstream) board code somehow?

Not sure. I'd need to reproduce. Is there an easy way to adjust a mainline
board to get the same behavior? Is your downstream board code inside a
board driver probe function or is it raw in an initcall?

Cheers,
Ahmad

> 
> Thanks and best regards,
> Michael
> 
>> ---
>> v1 -> v2:
>>   - new patch to facilitate patch 3/4
>> ---
>>  drivers/of/base.c     | 10 +++++++---
>>  drivers/of/platform.c |  2 +-
>>  include/of.h          |  1 +
>>  3 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 1221cd316cdf..7ba0655401cc 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2512,13 +2512,13 @@ static int of_probe_memory(void)
>>  }
>>  mem_initcall(of_probe_memory);
>>  
>> -static void of_platform_device_create_root(struct device_node *np)
>> +struct device *of_platform_device_create_root(struct device_node *np)
>>  {
>>  	static struct device *dev;
>>  	int ret;
>>  
>>  	if (dev)
>> -		return;
>> +		return dev;
>>  
>>  	dev = xzalloc(sizeof(*dev));
>>  	dev->id = DEVICE_ID_SINGLE;
>> @@ -2526,8 +2526,12 @@ static void of_platform_device_create_root(struct device_node *np)
>>  	dev_set_name(dev, "machine");
>>  
>>  	ret = platform_device_register(dev);
>> -	if (ret)
>> +	if (ret) {
>>  		free_device(dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return dev;
>>  }
>>  
>>  static const struct of_device_id reserved_mem_matches[] = {
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 69f9ddec50ca..11cde8924f94 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -421,7 +421,7 @@ static struct device *of_device_create_on_demand(struct device_node *np)
>>  
>>  	parent = of_get_parent(np);
>>  	if (!parent)
>> -		return NULL;
>> +		return of_platform_device_create_root(np);
>>  
>>  	if (!np->dev && parent->dev) {
>>  		ret = device_detect(parent->dev);
>> diff --git a/include/of.h b/include/of.h
>> index 1a38774615a4..63c90255cd64 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -298,6 +298,7 @@ extern void of_platform_device_dummy_drv(struct device *dev);
>>  extern int of_platform_populate(struct device_node *root,
>>  				const struct of_device_id *matches,
>>  				struct device *parent);
>> +extern struct device *of_platform_device_create_root(struct device_node *np);
>>  extern struct device *of_find_device_by_node(struct device_node *np);
>>  extern struct device *of_device_enable_and_register(struct device_node *np);
>>  extern struct device *of_device_enable_and_register_by_name(const char *name);
> 

-- 
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 v2 2/4] of: support of_ensure_probed for top-level machine device

[Michael Riesch]

Hi Ahmad,

On 3/10/23 17:19, Ahmad Fatoum wrote:
> On 10.03.23 15:46, Michael Riesch wrote:
>> Hi Ahmad,
>>
>> On 2/10/23 17:53, Ahmad Fatoum wrote:
>>> Creation of a machine device for the top-level node has special casing
>>> in of_probe(). Export of_platform_device_create_root(), so it's possible
>>> to ensure probe of the machine device. This is required when doing
>>> of_devices_ensure_probed_by_dev_id with the machine compatible.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>
>> This patch breaks my board code that calls
>> of_device_ensure_probed_by_alias() in the probe() of the board driver.
>> The function always returns -ENODEV.
>>
>> Do I have to adjust my (downstream) board code somehow?
> 
> Not sure. I'd need to reproduce. Is there an easy way to adjust a mainline
> board to get the same behavior? Is your downstream board code inside a
> board driver probe function or is it raw in an initcall?

I can sketch a patch that triggers this behavior on a Radxa ROCK3.
Similar to my board code, this happens in the scope of a board probe
function. Please find the sketch below.

Basing on the current next:

Board: Radxa ROCK3 Model A
deep-probe: supported due to radxa,rock3a
board-rock3 machine: of_device_ensure_probed_by_alias returned -19

With 2b46c9df976c ("of: support of_ensure_probed for top-level machine
device") reverted:

Board: Radxa ROCK3 Model A
deep-probe: supported due to radxa,rock3a
rk808 rk8090: chip id: 0x8090
rockchip_saradc fe720000.saradc@fe720000.of: registered as saradc
board-rock3 machine: of_device_ensure_probed_by_alias returned 0

Thanks and best regards,
Michael

> [...]
---- %< snip ----------------------------------------------------

diff --git a/arch/arm/boards/radxa-rock3/board.c
b/arch/arm/boards/radxa-rock3/board.c
index 4b4e0613d3..771777d4ea 100644
--- a/arch/arm/boards/radxa-rock3/board.c
+++ b/arch/arm/boards/radxa-rock3/board.c
@@ -15,6 +15,7 @@ static int rock3_probe(struct device *dev)
        enum bootsource bootsource = bootsource_get();
        int instance = bootsource_get_instance();
        const struct rock3_model *model;
+       int ret;

        model = device_get_match_data(dev);

@@ -30,6 +31,9 @@ static int rock3_probe(struct device *dev)
                                "/dev/mmc1");
        rk3568_bbu_mmc_register("sd", 0, "/dev/mmc0");

+       ret = of_device_ensure_probed_by_alias("saradc");
+       dev_info(dev, "of_device_ensure_probed_by_alias returned %d\n",
ret);
+
        return 0;
 }

diff --git a/arch/arm/dts/rk3568-rock-3a.dts
b/arch/arm/dts/rk3568-rock-3a.dts
index 25a0c05737..b8dc46c706 100644
--- a/arch/arm/dts/rk3568-rock-3a.dts
+++ b/arch/arm/dts/rk3568-rock-3a.dts
@@ -6,6 +6,10 @@
 #include "rk356x.dtsi"

 / {
+       aliases {
+               saradc = &saradc;
+       };
+
        chosen: chosen {
                environment-sd {
                        compatible = "barebox,environment";

Re: [PATCH v2 2/4] of: support of_ensure_probed for top-level machine device

[Ahmad Fatoum]

Hello Michael,

On 13.03.23 08:05, Michael Riesch wrote:
> I can sketch a patch that triggers this behavior on a Radxa ROCK3.
> Similar to my board code, this happens in the scope of a board probe
> function. Please find the sketch below.
> 
> Basing on the current next:

[snip]

I see now how this could not work. I dropped the commit introducing
the breakage and redid what I wanted to achieve differently.

Thanks for reporting and sorry for the hassle,
Ahmad

> 
> Board: Radxa ROCK3 Model A
> deep-probe: supported due to radxa,rock3a
> board-rock3 machine: of_device_ensure_probed_by_alias returned -19
> 
> With 2b46c9df976c ("of: support of_ensure_probed for top-level machine
> device") reverted:
> 
> Board: Radxa ROCK3 Model A
> deep-probe: supported due to radxa,rock3a
> rk808 rk8090: chip id: 0x8090
> rockchip_saradc fe720000.saradc@fe720000.of: registered as saradc
> board-rock3 machine: of_device_ensure_probed_by_alias returned 0
> 
> Thanks and best regards,
> Michael
> 
>> [...]
> ---- %< snip ----------------------------------------------------
> 
> diff --git a/arch/arm/boards/radxa-rock3/board.c
> b/arch/arm/boards/radxa-rock3/board.c
> index 4b4e0613d3..771777d4ea 100644
> --- a/arch/arm/boards/radxa-rock3/board.c
> +++ b/arch/arm/boards/radxa-rock3/board.c
> @@ -15,6 +15,7 @@ static int rock3_probe(struct device *dev)
>         enum bootsource bootsource = bootsource_get();
>         int instance = bootsource_get_instance();
>         const struct rock3_model *model;
> +       int ret;
> 
>         model = device_get_match_data(dev);
> 
> @@ -30,6 +31,9 @@ static int rock3_probe(struct device *dev)
>                                 "/dev/mmc1");
>         rk3568_bbu_mmc_register("sd", 0, "/dev/mmc0");
> 
> +       ret = of_device_ensure_probed_by_alias("saradc");
> +       dev_info(dev, "of_device_ensure_probed_by_alias returned %d\n",
> ret);
> +
>         return 0;
>  }
> 
> diff --git a/arch/arm/dts/rk3568-rock-3a.dts
> b/arch/arm/dts/rk3568-rock-3a.dts
> index 25a0c05737..b8dc46c706 100644
> --- a/arch/arm/dts/rk3568-rock-3a.dts
> +++ b/arch/arm/dts/rk3568-rock-3a.dts
> @@ -6,6 +6,10 @@
>  #include "rk356x.dtsi"
> 
>  / {
> +       aliases {
> +               saradc = &saradc;
> +       };
> +
>         chosen: chosen {
>                 environment-sd {
>                         compatible = "barebox,environment";
> 

-- 
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 |

[PATCH v2 3/4] boards: qemu-virt: ensure board driver probe at postcore_initcall level

[Ahmad Fatoum]

Qemu board driver fixups should be applied very early to be able to
influence core components, even if they are controlled directly by
initcalls. For this reason, enforce probe of the driver at postcore_initcall
level.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch -- otherwise, probe happens after initcall adding rsa keys
---
 common/boards/qemu-virt/board.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/boards/qemu-virt/board.c b/common/boards/qemu-virt/board.c
index d0f4412cdea5..ec92ae94aec9 100644
--- a/common/boards/qemu-virt/board.c
+++ b/common/boards/qemu-virt/board.c
@@ -65,4 +65,14 @@ static struct driver virt_board_driver = {
 	.of_compatible = virt_of_match,
 };
 
-postcore_platform_driver(virt_board_driver);
+static int virt_board_driver_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&virt_board_driver);
+	if (ret)
+		return ret;
+
+	return of_devices_ensure_probed_by_dev_id(virt_of_match);
+}
+postcore_initcall(virt_board_driver_init);
-- 
2.30.2

[PATCH v2 4/4] boards: qemu-virt: support passing in FIT public key

[Ahmad Fatoum]

FIT public key is usually passed in via board DT. Usual way to use
barebox with QEMU Virt however is to use DT supplied by Qemu and apply
overlay to it. mkimage doesn't generate overlay DTB though. To make
barbebox Qemu Virt behave like other boards, let's define a dummy DT
that includes CONFIG_BOOTM_FITIMAGE_PUBKEY, which is merged with the
barebox live device tree.

Suggested-by: Jan Lübbe <jlu@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - no changes
---
 common/boards/qemu-virt/Makefile            | 2 +-
 common/boards/qemu-virt/board.c             | 7 ++++++-
 common/boards/qemu-virt/fitimage-pubkey.dts | 7 +++++++
 3 files changed, 14 insertions(+), 2 deletions(-)
 create mode 100644 common/boards/qemu-virt/fitimage-pubkey.dts

diff --git a/common/boards/qemu-virt/Makefile b/common/boards/qemu-virt/Makefile
index 88184e9a7969..00bfdfbda696 100644
--- a/common/boards/qemu-virt/Makefile
+++ b/common/boards/qemu-virt/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 obj-y += board.o
-obj-y += overlay-of-flash.dtb.o
+obj-y += overlay-of-flash.dtb.o fitimage-pubkey.dtb.o
 ifeq ($(CONFIG_RISCV),y)
 DTC_CPP_FLAGS_overlay-of-flash.dtb := -DRISCV_VIRT=1
 endif
diff --git a/common/boards/qemu-virt/board.c b/common/boards/qemu-virt/board.c
index ec92ae94aec9..2669e9de5a2a 100644
--- a/common/boards/qemu-virt/board.c
+++ b/common/boards/qemu-virt/board.c
@@ -35,10 +35,11 @@ static inline void arm_virt_init(void) {}
 #endif
 
 extern char __dtb_overlay_of_flash_start[];
+extern char __dtb_fitimage_pubkey_start[];
 
 static int virt_probe(struct device *dev)
 {
-	struct device_node *overlay;
+	struct device_node *overlay, *pubkey;
 	void (*init)(void);
 
 	init = device_get_match_data(dev);
@@ -47,6 +48,10 @@ static int virt_probe(struct device *dev)
 
 	overlay = of_unflatten_dtb(__dtb_overlay_of_flash_start, INT_MAX);
 	of_overlay_apply_tree(dev->of_node, overlay);
+
+	pubkey = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
+	of_merge_nodes(dev->of_node, pubkey);
+
 	/* of_probe() will happen later at of_populate_initcall */
 
 	return 0;
diff --git a/common/boards/qemu-virt/fitimage-pubkey.dts b/common/boards/qemu-virt/fitimage-pubkey.dts
new file mode 100644
index 000000000000..497799fa4b60
--- /dev/null
+++ b/common/boards/qemu-virt/fitimage-pubkey.dts
@@ -0,0 +1,7 @@
+/dts-v1/;
+
+#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
+#include CONFIG_BOOTM_FITIMAGE_PUBKEY
+#endif
+
+/{ };
-- 
2.30.2

Re: [PATCH v2 4/4] boards: qemu-virt: support passing in FIT public key

[Jan Lübbe]

On Fri, 2023-02-10 at 17:53 +0100, Ahmad Fatoum wrote:
> FIT public key is usually passed in via board DT. Usual way to use
> barebox with QEMU Virt however is to use DT supplied by Qemu and apply
> overlay to it. mkimage doesn't generate overlay DTB though. To make
> barbebox Qemu Virt behave like other boards, let's define a dummy DT
> that includes CONFIG_BOOTM_FITIMAGE_PUBKEY, which is merged with the
> barebox live device tree.
> 
> Suggested-by: Jan Lübbe <jlu@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

For the whole series:

Tested-by: Jan Lübbe <jlu@pengutronix.de>

> ---
> v1 -> v2:
>   - no changes
> ---
>  common/boards/qemu-virt/Makefile            | 2 +-
>  common/boards/qemu-virt/board.c             | 7 ++++++-
>  common/boards/qemu-virt/fitimage-pubkey.dts | 7 +++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>  create mode 100644 common/boards/qemu-virt/fitimage-pubkey.dts
> 
> diff --git a/common/boards/qemu-virt/Makefile b/common/boards/qemu-virt/Makefile
> index 88184e9a7969..00bfdfbda696 100644
> --- a/common/boards/qemu-virt/Makefile
> +++ b/common/boards/qemu-virt/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  obj-y += board.o
> -obj-y += overlay-of-flash.dtb.o
> +obj-y += overlay-of-flash.dtb.o fitimage-pubkey.dtb.o
>  ifeq ($(CONFIG_RISCV),y)
>  DTC_CPP_FLAGS_overlay-of-flash.dtb := -DRISCV_VIRT=1
>  endif
> diff --git a/common/boards/qemu-virt/board.c b/common/boards/qemu-virt/board.c
> index ec92ae94aec9..2669e9de5a2a 100644
> --- a/common/boards/qemu-virt/board.c
> +++ b/common/boards/qemu-virt/board.c
> @@ -35,10 +35,11 @@ static inline void arm_virt_init(void) {}
>  #endif
>  
>  extern char __dtb_overlay_of_flash_start[];
> +extern char __dtb_fitimage_pubkey_start[];
>  
>  static int virt_probe(struct device *dev)
>  {
> -	struct device_node *overlay;
> +	struct device_node *overlay, *pubkey;
>  	void (*init)(void);
>  
>  	init = device_get_match_data(dev);
> @@ -47,6 +48,10 @@ static int virt_probe(struct device *dev)
>  
>  	overlay = of_unflatten_dtb(__dtb_overlay_of_flash_start, INT_MAX);
>  	of_overlay_apply_tree(dev->of_node, overlay);
> +
> +	pubkey = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
> +	of_merge_nodes(dev->of_node, pubkey);
> +
>  	/* of_probe() will happen later at of_populate_initcall */
>  
>  	return 0;
> diff --git a/common/boards/qemu-virt/fitimage-pubkey.dts b/common/boards/qemu-virt/fitimage-pubkey.dts
> new file mode 100644
> index 000000000000..497799fa4b60
> --- /dev/null
> +++ b/common/boards/qemu-virt/fitimage-pubkey.dts
> @@ -0,0 +1,7 @@
> +/dts-v1/;
> +
> +#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
> +#include CONFIG_BOOTM_FITIMAGE_PUBKEY
> +#endif
> +
> +/{ };

-- 
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 v2 4/4] boards: qemu-virt: support passing in FIT public key

[Sascha Hauer]

On Fri, Feb 10, 2023 at 05:53:53PM +0100, Ahmad Fatoum wrote:
> FIT public key is usually passed in via board DT. Usual way to use
> barebox with QEMU Virt however is to use DT supplied by Qemu and apply
> overlay to it. mkimage doesn't generate overlay DTB though. To make
> barbebox Qemu Virt behave like other boards, let's define a dummy DT
> that includes CONFIG_BOOTM_FITIMAGE_PUBKEY, which is merged with the
> barebox live device tree.
> 
> Suggested-by: Jan Lübbe <jlu@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>   - no changes
> ---
>  common/boards/qemu-virt/Makefile            | 2 +-
>  common/boards/qemu-virt/board.c             | 7 ++++++-
>  common/boards/qemu-virt/fitimage-pubkey.dts | 7 +++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>  create mode 100644 common/boards/qemu-virt/fitimage-pubkey.dts
> 
> diff --git a/common/boards/qemu-virt/Makefile b/common/boards/qemu-virt/Makefile
> index 88184e9a7969..00bfdfbda696 100644
> --- a/common/boards/qemu-virt/Makefile
> +++ b/common/boards/qemu-virt/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  obj-y += board.o
> -obj-y += overlay-of-flash.dtb.o
> +obj-y += overlay-of-flash.dtb.o fitimage-pubkey.dtb.o
>  ifeq ($(CONFIG_RISCV),y)
>  DTC_CPP_FLAGS_overlay-of-flash.dtb := -DRISCV_VIRT=1
>  endif
> diff --git a/common/boards/qemu-virt/board.c b/common/boards/qemu-virt/board.c
> index ec92ae94aec9..2669e9de5a2a 100644
> --- a/common/boards/qemu-virt/board.c
> +++ b/common/boards/qemu-virt/board.c
> @@ -35,10 +35,11 @@ static inline void arm_virt_init(void) {}
>  #endif
>  
>  extern char __dtb_overlay_of_flash_start[];
> +extern char __dtb_fitimage_pubkey_start[];
>  
>  static int virt_probe(struct device *dev)
>  {
> -	struct device_node *overlay;
> +	struct device_node *overlay, *pubkey;
>  	void (*init)(void);
>  
>  	init = device_get_match_data(dev);
> @@ -47,6 +48,10 @@ static int virt_probe(struct device *dev)
>  
>  	overlay = of_unflatten_dtb(__dtb_overlay_of_flash_start, INT_MAX);
>  	of_overlay_apply_tree(dev->of_node, overlay);
> +
> +	pubkey = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
> +	of_merge_nodes(dev->of_node, pubkey);
> +
>  	/* of_probe() will happen later at of_populate_initcall */
>  
>  	return 0;
> diff --git a/common/boards/qemu-virt/fitimage-pubkey.dts b/common/boards/qemu-virt/fitimage-pubkey.dts
> new file mode 100644
> index 000000000000..497799fa4b60
> --- /dev/null
> +++ b/common/boards/qemu-virt/fitimage-pubkey.dts
> @@ -0,0 +1,7 @@
> +/dts-v1/;
> +
> +#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
> +#include CONFIG_BOOTM_FITIMAGE_PUBKEY
> +#endif

I wonder if we've gone the wrong path here. Every board that wants to
put a key into the device tree needs this snippet.

Instead of compiling the dtsi containing the key into the barebox main
device tree wouldn't it be better to always create an extra dtb from
the dtsi provdided in CONFIG_BOOTM_FITIMAGE_PUBKEY and apply something
along the following?

What's missing is some Makefile magic to compile an extra dtb named
fitimage_pubkey from whatever name is provided in
CONFIG_BOOTM_FITIMAGE_PUBKEY, but that should be doable as well.


diff --git a/crypto/rsa.c b/crypto/rsa.c
index fc21efdb6d..6939513db9 100644
--- a/crypto/rsa.c
+++ b/crypto/rsa.c
@@ -491,16 +491,13 @@ static struct rsa_public_key *rsa_key_dup(const struct rsa_public_key *key)
 extern const struct rsa_public_key * const __rsa_keys_start;
 extern const struct rsa_public_key * const __rsa_keys_end;
 
-static void rsa_init_keys_of(void)
+static void rsa_init_keys_of(struct device_node *root)
 {
 	struct device_node *sigs, *sig;
 	struct rsa_public_key *key;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_OFTREE))
-		return;
-
-	sigs = of_find_node_by_path("/signature");
+	sigs = of_find_node_by_path_from(root, "/signature");
 	if (!sigs)
 		return;
 
@@ -519,6 +516,26 @@ static void rsa_init_keys_of(void)
 	}
 }
 
+extern char __dtb_fitimage_pubkey_start[];
+
+static void rsa_of_init_keys(void)
+{
+	struct device_node *root;
+
+	if (!IS_ENABLED(CONFIG_OFTREE))
+		return;
+
+	root = of_get_root_node();
+	if (root)
+		rsa_init_keys_of(root);
+
+#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
+	root = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
+	if (root)
+		rsa_init_keys_of(root);
+#endif
+}
+
 static int rsa_init_keys(void)
 {
 	const struct rsa_public_key * const *iter;
@@ -533,7 +550,7 @@ static int rsa_init_keys(void)
 			       key->key_name_hint, strerror(-ret));
 	}
 
-	rsa_init_keys_of();
+	rsa_of_init_keys();
 
 	return 0;
 }
-- 
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 v2 4/4] boards: qemu-virt: support passing in FIT public key

[Ahmad Fatoum]

Hello Sascha,

On 13.02.23 09:45, Sascha Hauer wrote:
>> diff --git a/common/boards/qemu-virt/fitimage-pubkey.dts b/common/boards/qemu-virt/fitimage-pubkey.dts
>> new file mode 100644
>> index 000000000000..497799fa4b60
>> --- /dev/null
>> +++ b/common/boards/qemu-virt/fitimage-pubkey.dts
>> @@ -0,0 +1,7 @@
>> +/dts-v1/;
>> +
>> +#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
>> +#include CONFIG_BOOTM_FITIMAGE_PUBKEY
>> +#endif
> 
> I wonder if we've gone the wrong path here. Every board that wants to
> put a key into the device tree needs this snippet.

We have an alternative nowadays: put the DTS snippet into
CONFIG_EXTERNAL_DTS_FRAGMENTS. This doesn't play nicely with overlays
unfortunately, but that's something that should be fixed anyway.

> Instead of compiling the dtsi containing the key into the barebox main
> device tree wouldn't it be better to always create an extra dtb from
> the dtsi provdided in CONFIG_BOOTM_FITIMAGE_PUBKEY and apply something
> along the following?
> 
> What's missing is some Makefile magic to compile an extra dtb named
> fitimage_pubkey from whatever name is provided in
> CONFIG_BOOTM_FITIMAGE_PUBKEY, but that should be doable as well.

The intention of this patch series is to provide the exact same mechanism,
we already use in non-emulated platforms somehow for QEMU as well.

I agree that in the future, we may want to generally restructure how we
do this:

Instead of decompiling mkimage output and including it into a device tree,
let's have /env/signatures/, where the user can place any number of DTBs.

All DTB within the directory would have their keys then "installed".

I'll keep this in mind for when I do some secure-boot related thing the
next time.

Cheers,
Ahmad

> 
> 
> diff --git a/crypto/rsa.c b/crypto/rsa.c
> index fc21efdb6d..6939513db9 100644
> --- a/crypto/rsa.c
> +++ b/crypto/rsa.c
> @@ -491,16 +491,13 @@ static struct rsa_public_key *rsa_key_dup(const struct rsa_public_key *key)
>  extern const struct rsa_public_key * const __rsa_keys_start;
>  extern const struct rsa_public_key * const __rsa_keys_end;
>  
> -static void rsa_init_keys_of(void)
> +static void rsa_init_keys_of(struct device_node *root)
>  {
>  	struct device_node *sigs, *sig;
>  	struct rsa_public_key *key;
>  	int ret;
>  
> -	if (!IS_ENABLED(CONFIG_OFTREE))
> -		return;
> -
> -	sigs = of_find_node_by_path("/signature");
> +	sigs = of_find_node_by_path_from(root, "/signature");
>  	if (!sigs)
>  		return;
>  
> @@ -519,6 +516,26 @@ static void rsa_init_keys_of(void)
>  	}
>  }
>  
> +extern char __dtb_fitimage_pubkey_start[];
> +
> +static void rsa_of_init_keys(void)
> +{
> +	struct device_node *root;
> +
> +	if (!IS_ENABLED(CONFIG_OFTREE))
> +		return;
> +
> +	root = of_get_root_node();
> +	if (root)
> +		rsa_init_keys_of(root);
> +
> +#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
> +	root = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
> +	if (root)
> +		rsa_init_keys_of(root);
> +#endif
> +}
> +
>  static int rsa_init_keys(void)
>  {
>  	const struct rsa_public_key * const *iter;
> @@ -533,7 +550,7 @@ static int rsa_init_keys(void)
>  			       key->key_name_hint, strerror(-ret));
>  	}
>  
> -	rsa_init_keys_of();
> +	rsa_of_init_keys();
>  
>  	return 0;
>  }

-- 
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 v2 4/4] boards: qemu-virt: support passing in FIT public key

[Ahmad Fatoum]

On 17.02.23 14:03, Ahmad Fatoum wrote:
> The intention of this patch series is to provide the exact same mechanism,
> we already use in non-emulated platforms somehow for QEMU as well.

Ping.

> 
> I agree that in the future, we may want to generally restructure how we
> do this:
> 
> Instead of decompiling mkimage output and including it into a device tree,
> let's have /env/signatures/, where the user can place any number of DTBs.
> 
> All DTB within the directory would have their keys then "installed".
> 
> I'll keep this in mind for when I do some secure-boot related thing the
> next time.
> 
> Cheers,
> Ahmad
> 
>>
>>
>> diff --git a/crypto/rsa.c b/crypto/rsa.c
>> index fc21efdb6d..6939513db9 100644
>> --- a/crypto/rsa.c
>> +++ b/crypto/rsa.c
>> @@ -491,16 +491,13 @@ static struct rsa_public_key *rsa_key_dup(const struct rsa_public_key *key)
>>  extern const struct rsa_public_key * const __rsa_keys_start;
>>  extern const struct rsa_public_key * const __rsa_keys_end;
>>  
>> -static void rsa_init_keys_of(void)
>> +static void rsa_init_keys_of(struct device_node *root)
>>  {
>>  	struct device_node *sigs, *sig;
>>  	struct rsa_public_key *key;
>>  	int ret;
>>  
>> -	if (!IS_ENABLED(CONFIG_OFTREE))
>> -		return;
>> -
>> -	sigs = of_find_node_by_path("/signature");
>> +	sigs = of_find_node_by_path_from(root, "/signature");
>>  	if (!sigs)
>>  		return;
>>  
>> @@ -519,6 +516,26 @@ static void rsa_init_keys_of(void)
>>  	}
>>  }
>>  
>> +extern char __dtb_fitimage_pubkey_start[];
>> +
>> +static void rsa_of_init_keys(void)
>> +{
>> +	struct device_node *root;
>> +
>> +	if (!IS_ENABLED(CONFIG_OFTREE))
>> +		return;
>> +
>> +	root = of_get_root_node();
>> +	if (root)
>> +		rsa_init_keys_of(root);
>> +
>> +#ifdef CONFIG_BOOTM_FITIMAGE_PUBKEY
>> +	root = of_unflatten_dtb(__dtb_fitimage_pubkey_start, INT_MAX);
>> +	if (root)
>> +		rsa_init_keys_of(root);
>> +#endif
>> +}
>> +
>>  static int rsa_init_keys(void)
>>  {
>>  	const struct rsa_public_key * const *iter;
>> @@ -533,7 +550,7 @@ static int rsa_init_keys(void)
>>  			       key->key_name_hint, strerror(-ret));
>>  	}
>>  
>> -	rsa_init_keys_of();
>> +	rsa_of_init_keys();
>>  
>>  	return 0;
>>  }
> 

-- 
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 v2 1/4] of: base: factor out of_merge_nodes from of_copy_node

[Sascha Hauer]

On Fri, Feb 10, 2023 at 05:53:50PM +0100, Ahmad Fatoum wrote:
> Later commit will need to merge two DTs from the root up. Refactor
> that part out of of_copy_node to make it usable on its own.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---

Applied, thanks

Sascha

> v1 -> v2:
>   - no changes
> ---
>  drivers/of/base.c | 17 ++++++++++++-----
>  include/of.h      |  1 +
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 937847f44ab7..1221cd316cdf 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2616,19 +2616,26 @@ out:
>  	return dn;
>  }
>  
> -struct device_node *of_copy_node(struct device_node *parent, const struct device_node *other)
> +void of_merge_nodes(struct device_node *np, const struct device_node *other)
>  {
> -	struct device_node *np, *child;
> +	struct device_node *child;
>  	struct property *pp;
>  
> -	np = of_new_node(parent, other->name);
> -	np->phandle = other->phandle;
> -
>  	list_for_each_entry(pp, &other->properties, list)
>  		of_new_property(np, pp->name, pp->value, pp->length);
>  
>  	for_each_child_of_node(other, child)
>  		of_copy_node(np, child);
> +}
> +
> +struct device_node *of_copy_node(struct device_node *parent, const struct device_node *other)
> +{
> +	struct device_node *np;
> +
> +	np = of_new_node(parent, other->name);
> +	np->phandle = other->phandle;
> +
> +	of_merge_nodes(np, other);
>  
>  	return np;
>  }
> diff --git a/include/of.h b/include/of.h
> index 7ee1304b932b..1a38774615a4 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -180,6 +180,7 @@ extern struct device_node *of_new_node(struct device_node *parent,
>  				const char *name);
>  extern struct device_node *of_create_node(struct device_node *root,
>  					const char *path);
> +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);
>  extern struct device_node *of_dup(const struct device_node *root);
> -- 
> 2.30.2
> 
> 
> 

-- 
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 |