2022-06-23 11:11:40

by Clément Léger

[permalink] [raw]
Subject: [PATCH v1 1/2] of: base: populate of_root node if not set

When enabling CONFIG_OF on a platform where of_root is not populated by
firmware, we end up without a root node. In order to apply overlays and
create subnodes of the root node, we need one. Create this root node
by unflattening an empty builtin dtb with of_fdt_unflatten().

Co-developed-by: Rob Herring <[email protected]>
Signed-off-by: Rob Herring <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
---
drivers/of/Makefile | 2 +-
drivers/of/base.c | 18 ++++++++++++++++--
drivers/of/empty_root.dts | 6 ++++++
3 files changed, 23 insertions(+), 3 deletions(-)
create mode 100644 drivers/of/empty_root.dts

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e0360a44306e..ce56c8b95c83 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-obj-y = base.o device.o platform.o property.o
+obj-y = base.o empty_root.dtb.o device.o platform.o property.o
obj-$(CONFIG_OF_KOBJ) += kobj.o
obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
obj-$(CONFIG_OF_FLATTREE) += fdt.o
diff --git a/drivers/of/base.c b/drivers/of/base.c
index d4f98c8469ed..43e0f027a49c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -22,6 +22,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_fdt.h>
#include <linux/of_graph.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
@@ -163,6 +164,8 @@ void __of_phandle_cache_inv_entry(phandle handle)
phandle_cache[handle_hash] = NULL;
}

+extern const char __dtb_empty_root_begin[];
+
void __init of_core_init(void)
{
struct device_node *np;
@@ -176,6 +179,18 @@ void __init of_core_init(void)
pr_err("failed to register existing nodes\n");
return;
}
+
+ if (!of_root) {
+ void *dt;
+ const unsigned long *fdt = (const unsigned long *)
+ __dtb_empty_root_begin;
+ dt = of_fdt_unflatten_tree(fdt, NULL, &of_root);
+ if (!dt) {
+ pr_err("Failed to setup empty root dt\n");
+ return;
+ }
+ }
+
for_each_of_allnodes(np) {
__of_attach_node_sysfs(np);
if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
@@ -184,8 +199,7 @@ void __init of_core_init(void)
mutex_unlock(&of_mutex);

/* Symlink in /proc as required by userspace ABI */
- if (of_root)
- proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+ proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
}

static struct property *__of_find_property(const struct device_node *np,
diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
new file mode 100644
index 000000000000..cf9e97a60f48
--- /dev/null
+++ b/drivers/of/empty_root.dts
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/dts-v1/;
+
+/ {
+
+};
--
2.36.1


2022-06-24 00:06:58

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] of: base: populate of_root node if not set

Hi Clement,

I said that I would send a patch to do this, but have failed to follow
through. Sorry about that.


On 6/23/22 06:50, Clément Léger wrote:
> When enabling CONFIG_OF on a platform where of_root is not populated by
> firmware, we end up without a root node. In order to apply overlays and
> create subnodes of the root node, we need one. Create this root node
> by unflattening an empty builtin dtb with of_fdt_unflatten().
>
> Co-developed-by: Rob Herring <[email protected]>
> Signed-off-by: Rob Herring <[email protected]>
> Signed-off-by: Clément Léger <[email protected]>
> ---
> drivers/of/Makefile | 2 +-
> drivers/of/base.c | 18 ++++++++++++++++--
> drivers/of/empty_root.dts | 6 ++++++
> 3 files changed, 23 insertions(+), 3 deletions(-)
> create mode 100644 drivers/of/empty_root.dts
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e0360a44306e..ce56c8b95c83 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-y = base.o device.o platform.o property.o
> +obj-y = base.o empty_root.dtb.o device.o platform.o property.o
> obj-$(CONFIG_OF_KOBJ) += kobj.o
> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> obj-$(CONFIG_OF_FLATTREE) += fdt.o
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4f98c8469ed..43e0f027a49c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -22,6 +22,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/of_fdt.h>
> #include <linux/of_graph.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> @@ -163,6 +164,8 @@ void __of_phandle_cache_inv_entry(phandle handle)
> phandle_cache[handle_hash] = NULL;
> }
>
> +extern const char __dtb_empty_root_begin[];
> +
> void __init of_core_init(void)
> {
> struct device_node *np;
> @@ -176,6 +179,18 @@ void __init of_core_init(void)
> pr_err("failed to register existing nodes\n");
> return;
> }
> +
> + if (!of_root) {
> + void *dt;
> + const unsigned long *fdt = (const unsigned long *)
> + __dtb_empty_root_begin;
> + dt = of_fdt_unflatten_tree(fdt, NULL, &of_root);
> + if (!dt) {
> + pr_err("Failed to setup empty root dt\n");
> + return;
> + }
> + }
> +
> for_each_of_allnodes(np) {
> __of_attach_node_sysfs(np);
> if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> @@ -184,8 +199,7 @@ void __init of_core_init(void)
> mutex_unlock(&of_mutex);
>
> /* Symlink in /proc as required by userspace ABI */
> - if (of_root)
> - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> }

This approach is adding an additional method of unflattening the tree.
I would prefer to consolidate in a single location.

I have leveraged this patch series into a different patch series to
accomplish that. I have boot tested with one configuration, but want
to test two more configurations before sending the new series. It
should only take "a few minutes".

-Frank

>
> static struct property *__of_find_property(const struct device_node *np,
> diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
> new file mode 100644
> index 000000000000..cf9e97a60f48
> --- /dev/null
> +++ b/drivers/of/empty_root.dts
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/dts-v1/;
> +
> +/ {
> +
> +};

2022-06-24 01:49:08

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] of: base: populate of_root node if not set

On 6/23/22 19:15, Frank Rowand wrote:
> Hi Clement,
>
> I said that I would send a patch to do this, but have failed to follow
> through. Sorry about that.
>
>
> On 6/23/22 06:50, Clément Léger wrote:
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. Create this root node
>> by unflattening an empty builtin dtb with of_fdt_unflatten().
>>
>> Co-developed-by: Rob Herring <[email protected]>
>> Signed-off-by: Rob Herring <[email protected]>
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> drivers/of/Makefile | 2 +-
>> drivers/of/base.c | 18 ++++++++++++++++--
>> drivers/of/empty_root.dts | 6 ++++++
>> 3 files changed, 23 insertions(+), 3 deletions(-)
>> create mode 100644 drivers/of/empty_root.dts
>>
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index e0360a44306e..ce56c8b95c83 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,5 +1,5 @@
>> # SPDX-License-Identifier: GPL-2.0
>> -obj-y = base.o device.o platform.o property.o
>> +obj-y = base.o empty_root.dtb.o device.o platform.o property.o
>> obj-$(CONFIG_OF_KOBJ) += kobj.o
>> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index d4f98c8469ed..43e0f027a49c 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -22,6 +22,7 @@
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_fdt.h>
>> #include <linux/of_graph.h>
>> #include <linux/spinlock.h>
>> #include <linux/slab.h>
>> @@ -163,6 +164,8 @@ void __of_phandle_cache_inv_entry(phandle handle)
>> phandle_cache[handle_hash] = NULL;
>> }
>>
>> +extern const char __dtb_empty_root_begin[];
>> +
>> void __init of_core_init(void)
>> {
>> struct device_node *np;
>> @@ -176,6 +179,18 @@ void __init of_core_init(void)
>> pr_err("failed to register existing nodes\n");
>> return;
>> }
>> +
>> + if (!of_root) {
>> + void *dt;
>> + const unsigned long *fdt = (const unsigned long *)
>> + __dtb_empty_root_begin;
>> + dt = of_fdt_unflatten_tree(fdt, NULL, &of_root);
>> + if (!dt) {
>> + pr_err("Failed to setup empty root dt\n");
>> + return;
>> + }
>> + }
>> +
>> for_each_of_allnodes(np) {
>> __of_attach_node_sysfs(np);
>> if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
>> @@ -184,8 +199,7 @@ void __init of_core_init(void)
>> mutex_unlock(&of_mutex);
>>
>> /* Symlink in /proc as required by userspace ABI */
>> - if (of_root)
>> - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>> + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>> }
>
> This approach is adding an additional method of unflattening the tree.
> I would prefer to consolidate in a single location.
>
> I have leveraged this patch series into a different patch series to
> accomplish that. I have boot tested with one configuration, but want
> to test two more configurations before sending the new series. It
> should only take "a few minutes".

"a few minutes" is taking longer than expected. One of the devicetree
interrupt unittests is not giving the expected result for one of the
configurations I am checking.

-Frank

>
> -Frank
>
>>
>> static struct property *__of_find_property(const struct device_node *np,
>> diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
>> new file mode 100644
>> index 000000000000..cf9e97a60f48
>> --- /dev/null
>> +++ b/drivers/of/empty_root.dts
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/dts-v1/;
>> +
>> +/ {
>> +
>> +};
>

2022-06-24 04:22:14

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] of: base: populate of_root node if not set

On 6/23/22 21:01, Frank Rowand wrote:
> On 6/23/22 19:15, Frank Rowand wrote:
>> Hi Clement,
>>
>> I said that I would send a patch to do this, but have failed to follow
>> through. Sorry about that.
>>
>>
>> On 6/23/22 06:50, Clément Léger wrote:
>>> When enabling CONFIG_OF on a platform where of_root is not populated by
>>> firmware, we end up without a root node. In order to apply overlays and
>>> create subnodes of the root node, we need one. Create this root node
>>> by unflattening an empty builtin dtb with of_fdt_unflatten().
>>>
>>> Co-developed-by: Rob Herring <[email protected]>
>>> Signed-off-by: Rob Herring <[email protected]>
>>> Signed-off-by: Clément Léger <[email protected]>
>>> ---
>>> drivers/of/Makefile | 2 +-
>>> drivers/of/base.c | 18 ++++++++++++++++--
>>> drivers/of/empty_root.dts | 6 ++++++
>>> 3 files changed, 23 insertions(+), 3 deletions(-)
>>> create mode 100644 drivers/of/empty_root.dts
>>>
>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>>> index e0360a44306e..ce56c8b95c83 100644
>>> --- a/drivers/of/Makefile
>>> +++ b/drivers/of/Makefile
>>> @@ -1,5 +1,5 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> -obj-y = base.o device.o platform.o property.o
>>> +obj-y = base.o empty_root.dtb.o device.o platform.o property.o
>>> obj-$(CONFIG_OF_KOBJ) += kobj.o
>>> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index d4f98c8469ed..43e0f027a49c 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> #include <linux/of_device.h>
>>> +#include <linux/of_fdt.h>
>>> #include <linux/of_graph.h>
>>> #include <linux/spinlock.h>
>>> #include <linux/slab.h>
>>> @@ -163,6 +164,8 @@ void __of_phandle_cache_inv_entry(phandle handle)
>>> phandle_cache[handle_hash] = NULL;
>>> }
>>>
>>> +extern const char __dtb_empty_root_begin[];
>>> +
>>> void __init of_core_init(void)
>>> {
>>> struct device_node *np;
>>> @@ -176,6 +179,18 @@ void __init of_core_init(void)
>>> pr_err("failed to register existing nodes\n");
>>> return;
>>> }
>>> +
>>> + if (!of_root) {
>>> + void *dt;
>>> + const unsigned long *fdt = (const unsigned long *)
>>> + __dtb_empty_root_begin;
>>> + dt = of_fdt_unflatten_tree(fdt, NULL, &of_root);
>>> + if (!dt) {
>>> + pr_err("Failed to setup empty root dt\n");
>>> + return;
>>> + }
>>> + }
>>> +
>>> for_each_of_allnodes(np) {
>>> __of_attach_node_sysfs(np);
>>> if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
>>> @@ -184,8 +199,7 @@ void __init of_core_init(void)
>>> mutex_unlock(&of_mutex);
>>>
>>> /* Symlink in /proc as required by userspace ABI */
>>> - if (of_root)
>>> - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>>> + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>>> }
>>
>> This approach is adding an additional method of unflattening the tree.
>> I would prefer to consolidate in a single location.
>>
>> I have leveraged this patch series into a different patch series to
>> accomplish that. I have boot tested with one configuration, but want
>> to test two more configurations before sending the new series. It
>> should only take "a few minutes".
>
> "a few minutes" is taking longer than expected. One of the devicetree
> interrupt unittests is not giving the expected result for one of the
> configurations I am checking.

The test is ok. The problem was unrelated asynchronous output to the
console intermingled with test output.

My suggested alternative patch series is at:

https://lore.kernel.org/r/[email protected]

-Frank

>
> -Frank
>
>>
>> -Frank
>>
>>>
>>> static struct property *__of_find_property(const struct device_node *np,
>>> diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
>>> new file mode 100644
>>> index 000000000000..cf9e97a60f48
>>> --- /dev/null
>>> +++ b/drivers/of/empty_root.dts
>>> @@ -0,0 +1,6 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/dts-v1/;
>>> +
>>> +/ {
>>> +
>>> +};
>>
>

2022-06-24 12:14:34

by Clément Léger

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] of: base: populate of_root node if not set

Le Thu, 23 Jun 2022 18:15:31 -0500,
Frank Rowand <[email protected]> a écrit :

> Hi Clement,
>
> I said that I would send a patch to do this, but have failed to follow
> through. Sorry about that.

Hi Frank, no worries, I just needed this topic to move forward, that's
why I decided to sent this .

Thanks !

Clément

>
>
> On 6/23/22 06:50, Clément Léger wrote:
> > When enabling CONFIG_OF on a platform where of_root is not populated by
> > firmware, we end up without a root node. In order to apply overlays and
> > create subnodes of the root node, we need one. Create this root node
> > by unflattening an empty builtin dtb with of_fdt_unflatten().
> >
> > Co-developed-by: Rob Herring <[email protected]>
> > Signed-off-by: Rob Herring <[email protected]>
> > Signed-off-by: Clément Léger <[email protected]>
> > ---
> > drivers/of/Makefile | 2 +-
> > drivers/of/base.c | 18 ++++++++++++++++--
> > drivers/of/empty_root.dts | 6 ++++++
> > 3 files changed, 23 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/of/empty_root.dts
> >
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index e0360a44306e..ce56c8b95c83 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-y = base.o device.o platform.o property.o
> > +obj-y = base.o empty_root.dtb.o device.o platform.o property.o
> > obj-$(CONFIG_OF_KOBJ) += kobj.o
> > obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> > obj-$(CONFIG_OF_FLATTREE) += fdt.o
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d4f98c8469ed..43e0f027a49c 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -22,6 +22,7 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > +#include <linux/of_fdt.h>
> > #include <linux/of_graph.h>
> > #include <linux/spinlock.h>
> > #include <linux/slab.h>
> > @@ -163,6 +164,8 @@ void __of_phandle_cache_inv_entry(phandle handle)
> > phandle_cache[handle_hash] = NULL;
> > }
> >
> > +extern const char __dtb_empty_root_begin[];
> > +
> > void __init of_core_init(void)
> > {
> > struct device_node *np;
> > @@ -176,6 +179,18 @@ void __init of_core_init(void)
> > pr_err("failed to register existing nodes\n");
> > return;
> > }
> > +
> > + if (!of_root) {
> > + void *dt;
> > + const unsigned long *fdt = (const unsigned long *)
> > + __dtb_empty_root_begin;
> > + dt = of_fdt_unflatten_tree(fdt, NULL, &of_root);
> > + if (!dt) {
> > + pr_err("Failed to setup empty root dt\n");
> > + return;
> > + }
> > + }
> > +
> > for_each_of_allnodes(np) {
> > __of_attach_node_sysfs(np);
> > if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> > @@ -184,8 +199,7 @@ void __init of_core_init(void)
> > mutex_unlock(&of_mutex);
> >
> > /* Symlink in /proc as required by userspace ABI */
> > - if (of_root)
> > - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> > + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> > }
>
> This approach is adding an additional method of unflattening the tree.
> I would prefer to consolidate in a single location.
>
> I have leveraged this patch series into a different patch series to
> accomplish that. I have boot tested with one configuration, but want
> to test two more configurations before sending the new series. It
> should only take "a few minutes".
>
> -Frank
>
> >
> > static struct property *__of_find_property(const struct device_node *np,
> > diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
> > new file mode 100644
> > index 000000000000..cf9e97a60f48
> > --- /dev/null
> > +++ b/drivers/of/empty_root.dts
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/dts-v1/;
> > +
> > +/ {
> > +
> > +};
>



--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com