On Thu, Jan 18, 2024 at 2:46 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Rob, > > On Wed, Jan 17, 2024 at 6:41 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > On Tue, Jan 16, 2024 at 05:18:15PM -0800, Stephen Boyd wrote: > > > Quoting Rob Herring (2024-01-15 12:32:30) > > > > On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote: > > > > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > > > > index da9826accb1b..9628e48baa15 100644 > > > > > --- a/drivers/of/Kconfig > > > > > +++ b/drivers/of/Kconfig > > > > > @@ -54,9 +54,14 @@ config OF_FLATTREE > > > > > select CRC32 > > > > > > > > > > config OF_EARLY_FLATTREE > > > > > - bool > > > > > + bool "Functions for accessing Flat Devicetree (FDT) early in boot" > > > > > > > > I think we could instead just get rid of this kconfig option. Or > > > > always enable with CONFIG_OF (except on Sparc). The only cost of > > > > enabling it is init section functions which get freed anyways. > > > > > > Getting rid of it is a more massive change. It can be the default and > > > kept hidden instead? If it can't be selected on Sparc then it should be > > > hidden there anyway. > > > > The easier option is certainly fine for this series. I just don't want > > it visible. > > > > > > > select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM > > > > > select OF_FLATTREE > > > > > + help > > > > > + Normally selected by platforms that process an FDT that has been > > > > > + passed to the kernel by the bootloader. If the bootloader does not > > > > > + pass an FDT to the kernel and you need an empty devicetree that > > > > > + contains only a root node to exist, then say Y here. > > > > > > > > > > config OF_PROMTREE > > > > > bool > > > [...] > > > > > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long > > > > > return test_bit(flag, &n->_flags); > > > > > } > > > > > > > > > > +/** > > > > > + * 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) > > > > > +{ > > > > > + return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT); > > > > > > > > Just a side comment, but I think many/all callers of this function could > > > > just be removed. > > > > > > > > I don't love new flags. Another possible way to handle this would be > > > > checking for "compatible" being present in the root node. I guess this > > > > is fine as-is for now at least. > > > > > > Ok. I can add a check for a compatible property. That's probably better > > > anyway. Should there be a compatible property there to signal that this > > > DT isn't compatible with anything? I worry about DT overlays injecting a > > > compatible string into the root node, but maybe that is already > > > prevented. > > > > I worry about DT overlays injecting anything... > > > > I don't think it is explicitly forbidden, but I have asked that any > > general purpose interface to apply overlays be restricted to nodes > > explicitly allowed (e.g. downstream of a connector node). For now, the > > places (i.e. drivers) overlays are applied are limited. > > > > We could probably restrict the root node to new nodes only and no new > > or changed properties. > > Changing (<wild dream>or appending to</wild dream>) the root > "compatible" and/or "model" properties is useful in case of large > extension boards, though. This is also the case for DTBs created from > a base DTB and one or more overlays using fdtoverlay. I think appending by adding another compatible value could be okay. Removing or appending to an existing entry is not. We don't want the following sequence to be possible: of_machine_is_compatible("foo") --> true apply overlay of_machine_is_compatible("foo") --> false For Stephen's case, it's going from no root compatible at all to something. I don't think your case would apply here. To put it another way, if we've booted with ACPI, compatible in the root node is not valid. > For the latter, see also the following threads, where you weren't > (but probably should have been) CCed: > > [1] "[PATCH v9 2/2] arm64: boot: Support Flat Image Tree" > https://lore.kernel.org/all/20231202035511.487946-3-sjg@xxxxxxxxxxxx > [2] "Proposal: FIT support for extension boards / overlays" > https://lore.kernel.org/all/CAPnjgZ06s64C2ux1rABNAnMv3q4W++sjhNGCO_uPMH_9sTF7Mw@xxxxxxxxxxxxxx That all seems pretty orthogonal to the issues here. Rob