On Fri, Feb 16, 2024 at 05:05:51PM -0800, Frank Rowand 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.
>
> If firmware provides a flattened device tree (FDT) then the FDT is
> unflattened via setup_arch(). Otherwise, the call to
> unflatten(_and_copy)?_device_tree() will create an empty root node.
>
> We make of_have_populated_dt() return true only if the DTB was loaded by
> firmware so that existing callers don't change behavior after this
> patch. The call in the of platform code is removed because it prevents
> overlays from creating platform devices when the empty root node is
> used.
>
> [[email protected]: Update of_have_populated_dt() to treat this empty dtb
> as not populated. Drop setup_of() initcall]
>
> Signed-off-by: Frank Rowand <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Cc: Rob Herring <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/of/Kconfig | 5 ++---
> drivers/of/Makefile | 2 +-
> drivers/of/empty_root.dts | 6 ++++++
> drivers/of/fdt.c | 32 +++++++++++++++++++++++++++++++-
> drivers/of/platform.c | 3 ---
> include/linux/of.h | 25 +++++++++++++++----------
> 6 files changed, 55 insertions(+), 18 deletions(-)
> create mode 100644 drivers/of/empty_root.dts
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index da9826accb1b..d738fbad9c36 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -14,9 +14,8 @@ if OF
>
> config OF_UNITTEST
> bool "Device Tree runtime unit tests"
> - depends on !SPARC
> + depends on OF_EARLY_FLATTREE
> select IRQ_DOMAIN
> - select OF_EARLY_FLATTREE
> select OF_RESOLVE
> help
> This option builds in test cases for the device tree infrastructure
> @@ -54,7 +53,7 @@ config OF_FLATTREE
> select CRC32
>
> config OF_EARLY_FLATTREE
> - bool
> + def_bool OF && !(SPARC || ALPHA || HEXAGON || M68K || PARISC || S390)
> select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> select OF_FLATTREE
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index eff624854575..df305348d1cb 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -2,7 +2,7 @@
> obj-y = base.o cpu.o device.o module.o platform.o property.o
> obj-$(CONFIG_OF_KOBJ) += kobj.o
> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> -obj-$(CONFIG_OF_FLATTREE) += fdt.o
> +obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
> obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
> obj-$(CONFIG_OF_PROMTREE) += pdt.o
> obj-$(CONFIG_OF_ADDRESS) += address.o
> 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/;
> +
> +/ {
> +
> +};
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index dfeba8b8ce94..e5a385285149 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -8,6 +8,7 @@
>
> #define pr_fmt(fmt) "OF: fdt: " fmt
>
> +#include <linux/acpi.h>
> #include <linux/crash_dump.h>
> #include <linux/crc32.h>
> #include <linux/kernel.h>
> @@ -32,6 +33,13 @@
>
> #include "of_private.h"
>
> +/*
> + * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
> + * cmd_dt_S_dtb in scripts/Makefile.lib
> + */
> +extern uint8_t __dtb_empty_root_begin[];
> +extern uint8_t __dtb_empty_root_end[];
> +
> /*
> * of_fdt_limit_memory - limit the number of regions in the /memory node
> * @limit: maximum entries
> @@ -1343,7 +1351,29 @@ static void *__init copy_device_tree(void *fdt)
> */
> void __init unflatten_device_tree(void)
> {
> - __unflatten_device_tree(initial_boot_params, NULL, &of_root,
> + void *fdt = initial_boot_params;
> +
> + /* Don't use the bootloader provided DTB if ACPI is enabled */
> + if (!acpi_disabled)
> + fdt = NULL;
> +
> + /*
> + * Populate an empty root node when ACPI is enabled or bootloader
> + * doesn't provide one.
> + */
> + if (!fdt) {
> + fdt = (void *) __dtb_empty_root_begin;
> + /* fdt_totalsize() will be used for copy size */
> + if (fdt_totalsize(fdt) >
> + __dtb_empty_root_end - __dtb_empty_root_begin) {
> + pr_err("invalid size in dtb_empty_root\n");
> + return;
> + }
> + of_fdt_crc32 = crc32_be(~0, fdt, fdt_totalsize(fdt));
> + fdt = copy_device_tree(fdt);
> + }
> +
> + __unflatten_device_tree(fdt, NULL, &of_root,
> early_init_dt_alloc_memory_arch, false);
>
> /* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b7708a06dc78..39c0ceee3e95 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -510,9 +510,6 @@ static int __init of_platform_default_populate_init(void)
>
> device_links_supplier_sync_state_pause();
>
> - if (!of_have_populated_dt())
> - return -ENODEV;
> -
> if (IS_ENABLED(CONFIG_PPC)) {
> struct device_node *boot_display = NULL;
> struct platform_device *dev;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 331e05918f11..168357486260 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -180,11 +180,6 @@ static inline bool is_of_node(const struct fwnode_handle *fwnode)
> &__of_fwnode_handle_node->fwnode : NULL; \
> })
>
> -static inline bool of_have_populated_dt(void)
> -{
> - return of_root != NULL;
> -}
> -
> static inline bool of_node_is_root(const struct device_node *node)
> {
> return node && (node->parent == NULL);
> @@ -557,11 +552,6 @@ static inline struct device_node *of_find_node_with_property(
>
> #define of_fwnode_handle(node) NULL
>
> -static inline bool of_have_populated_dt(void)
> -{
> - return false;
> -}
> -
> static inline struct device_node *of_get_compatible_child(const struct device_node *parent,
> const char *compatible)
> {
> @@ -1645,6 +1635,21 @@ static inline bool of_device_is_system_power_controller(const struct device_node
> return of_property_read_bool(np, "system-power-controller");
> }
>
> +/**
> + * of_have_populated_dt() - Has DT been populated by bootloader
> + *
> + * Return: True if a DTB has been populated by the bootloader and it isn't the
> + * empty builtin one. False otherwise.
> + */
> +static inline bool of_have_populated_dt(void)
> +{
> +#ifdef CONFIG_OF
> + return of_property_present(of_root, "compatible");
This adds the strict check for compatible which makes compatible
to be mandatory for root nodes. So far, DeviceTree without compatible
property in root nodes can work. Do we want to make this documented
somewhere ?
- Saurabh
> +#else
> + return false;
> +#endif
> +}
> +
> /*
> * Overlay support
> */
> --
> 2.34.1
On Fri, Feb 16, 2024 at 11:08 PM Saurabh Singh Sengar
<[email protected]> wrote:
>
> On Fri, Feb 16, 2024 at 05:05:51PM -0800, Frank Rowand 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.
> >
> > If firmware provides a flattened device tree (FDT) then the FDT is
> > unflattened via setup_arch(). Otherwise, the call to
> > unflatten(_and_copy)?_device_tree() will create an empty root node.
> >
> > We make of_have_populated_dt() return true only if the DTB was loaded by
> > firmware so that existing callers don't change behavior after this
> > patch. The call in the of platform code is removed because it prevents
> > overlays from creating platform devices when the empty root node is
> > used.
> >
> > [[email protected]: Update of_have_populated_dt() to treat this empty dtb
> > as not populated. Drop setup_of() initcall]
> >
> > Signed-off-by: Frank Rowand <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Cc: Rob Herring <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > @@ -1645,6 +1635,21 @@ static inline bool of_device_is_system_power_controller(const struct device_node
> > return of_property_read_bool(np, "system-power-controller");
> > }
> >
> > +/**
> > + * of_have_populated_dt() - Has DT been populated by bootloader
> > + *
> > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > + * empty builtin one. False otherwise.
> > + */
> > +static inline bool of_have_populated_dt(void)
> > +{
> > +#ifdef CONFIG_OF
> > + return of_property_present(of_root, "compatible");
>
> This adds the strict check for compatible which makes compatible
> to be mandatory for root nodes. So far, DeviceTree without compatible
> property in root nodes can work. Do we want to make this documented
> somewhere ?
It already is in the DT spec and schemas.
Rob
Hi Rob,
CC broonie
CC somlo
On Wed, Feb 21, 2024 at 3:06 PM Rob Herring <[email protected]> wrote:
> On Fri, Feb 16, 2024 at 11:08 PM Saurabh Singh Sengar
> <[email protected]> wrote:
> > On Fri, Feb 16, 2024 at 05:05:51PM -0800, Frank Rowand 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.
> > >
> > > If firmware provides a flattened device tree (FDT) then the FDT is
> > > unflattened via setup_arch(). Otherwise, the call to
> > > unflatten(_and_copy)?_device_tree() will create an empty root node.
> > >
> > > We make of_have_populated_dt() return true only if the DTB was loaded by
> > > firmware so that existing callers don't change behavior after this
> > > patch. The call in the of platform code is removed because it prevents
> > > overlays from creating platform devices when the empty root node is
> > > used.
> > >
> > > [[email protected]: Update of_have_populated_dt() to treat this empty dtb
> > > as not populated. Drop setup_of() initcall]
> > >
> > > Signed-off-by: Frank Rowand <[email protected]>
> > > Link: https://lore.kernel.org/r/[email protected]
> > > Cc: Rob Herring <[email protected]>
> > > Signed-off-by: Stephen Boyd <[email protected]>
Thanks for your patch, which is now commit 7b937cc243e5b1df ("of:
Create of_root if no dtb provided by firmware") in dt-rh/for-next
(next-20240312 and later).
On my OrangeCrab FPGA running Linux on LiteX/VexRiscv-V, the attached
OLED display now fails to probe:
-ssd130x-i2c 0-003c: supply vcc not found, using dummy regulator
-[drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
+ssd130x-i2c 0-003c: incomplete constraints, dummy supplies not allowed
+ssd130x-i2c 0-003c: error -ENODEV: Failed to get VCC regulator
> > > @@ -1645,6 +1635,21 @@ static inline bool of_device_is_system_power_controller(const struct device_node
> > > return of_property_read_bool(np, "system-power-controller");
> > > }
> > >
> > > +/**
> > > + * of_have_populated_dt() - Has DT been populated by bootloader
> > > + *
> > > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > > + * empty builtin one. False otherwise.
> > > + */
> > > +static inline bool of_have_populated_dt(void)
> > > +{
> > > +#ifdef CONFIG_OF
> > > + return of_property_present(of_root, "compatible");
> >
> > This adds the strict check for compatible which makes compatible
> > to be mandatory for root nodes. So far, DeviceTree without compatible
> > property in root nodes can work. Do we want to make this documented
> > somewhere ?
>
> It already is in the DT spec and schemas.
How many systems in the wild violate this?
Apparently the DTS generated by LiteX does not have a root compatible
(and model) property, hence of_have_populated_dt() returns false.
While my gateware and DTS is quite old, a quick look at recent
litex_json2dts_linux.py history shows this is still true for current LiteX.
Now, how is this related to the failure I am seeing?
drivers/regulator/core.c has:
static bool have_full_constraints(void)
{
return has_full_constraints || of_have_populated_dt();
}
and
static int __init regulator_init_complete(void)
{
/*
* Since DT doesn't provide an idiomatic mechanism for
* enabling full constraints and since it's much more natural
* with DT to provide them just assume that a DT enabled
* system has full constraints.
*/
if (of_have_populated_dt())
has_full_constraints = true;
...
}
late_initcall_sync(regulator_init_complete);
(The latter looks a bit futile, as have_full_constraints() already
contains a check for of_have_populated_dt()?)
When have_full_constraints() returns false, any dummy regulator will
fail to probe.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On Tue, Mar 12, 2024 at 03:41:32PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 21, 2024 at 3:06 PM Rob Herring <[email protected]> wrote:
> static int __init regulator_init_complete(void)
> {
> /*
> * Since DT doesn't provide an idiomatic mechanism for
> * enabling full constraints and since it's much more natural
> * with DT to provide them just assume that a DT enabled
> * system has full constraints.
> */
> if (of_have_populated_dt())
> has_full_constraints = true;
>
> ...
> }
> late_initcall_sync(regulator_init_complete);
> (The latter looks a bit futile, as have_full_constraints() already
> contains a check for of_have_populated_dt()?)
Yes, it's duplicated and we should remove it.
On Tue, Mar 12, 2024 at 3:41 PM Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Feb 21, 2024 at 3:06 PM Rob Herring <[email protected]> wrote:
> > On Fri, Feb 16, 2024 at 11:08 PM Saurabh Singh Sengar
> > <[email protected]> wrote:
> > > This adds the strict check for compatible which makes compatible
> > > to be mandatory for root nodes. So far, DeviceTree without compatible
> > > property in root nodes can work. Do we want to make this documented
> > > somewhere ?
> >
> > It already is in the DT spec and schemas.
>
> How many systems in the wild violate this?
>
> Apparently the DTS generated by LiteX does not have a root compatible
> (and model) property, hence of_have_populated_dt() returns false.
> While my gateware and DTS is quite old, a quick look at recent
> litex_json2dts_linux.py history shows this is still true for current LiteX.
https://github.com/enjoy-digital/litex/issues/1905
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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