| Message ID | 20240217010557.2381548-6-sboyd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/7] of: Always unflatten in unflatten_and_copy_device_tree() | expand |
On Fri, Feb 16, 2024 at 05:05:54PM -0800, Stephen Boyd wrote: > Call this function unconditionally so that we can populate an empty DTB > on platforms that don't boot with a firmware provided or builtin DTB. > When ACPI is in use, unflatten_device_tree() ignores the > 'initial_boot_params' pointer so the live DT on those systems won't be > whatever that's pointing to. Similarly, when kexec copies the DT data > the previous kernel to the new one on ACPI systems, > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > one) and copy the 'initial_boot_params' data. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: <linux-arm-kernel@lists.infradead.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > arch/arm64/kernel/setup.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Catalin, Will, Can I get an ack on this so I can take the series via the DT tree. Rob
On Thu, Feb 22, 2024 at 05:03:17PM -0700, Rob Herring wrote: > On Fri, Feb 16, 2024 at 05:05:54PM -0800, Stephen Boyd wrote: > > Call this function unconditionally so that we can populate an empty DTB > > on platforms that don't boot with a firmware provided or builtin DTB. > > When ACPI is in use, unflatten_device_tree() ignores the > > 'initial_boot_params' pointer so the live DT on those systems won't be > > whatever that's pointing to. Similarly, when kexec copies the DT data > > the previous kernel to the new one on ACPI systems, > > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > > one) and copy the 'initial_boot_params' data. > > > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Frank Rowand <frowand.list@gmail.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: <linux-arm-kernel@lists.infradead.org> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > --- > > arch/arm64/kernel/setup.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > Catalin, Will, Can I get an ack on this so I can take the series via the > DT tree. Mark had strong pretty strong objections to this in version one: https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/ and this patch looks the same now as it did then. Did something else change? Will
On Fri, Feb 23, 2024 at 3:23 AM Will Deacon <will@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 05:03:17PM -0700, Rob Herring wrote: > > On Fri, Feb 16, 2024 at 05:05:54PM -0800, Stephen Boyd wrote: > > > Call this function unconditionally so that we can populate an empty DTB > > > on platforms that don't boot with a firmware provided or builtin DTB. > > > When ACPI is in use, unflatten_device_tree() ignores the > > > 'initial_boot_params' pointer so the live DT on those systems won't be > > > whatever that's pointing to. Similarly, when kexec copies the DT data > > > the previous kernel to the new one on ACPI systems, > > > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > > > one) and copy the 'initial_boot_params' data. > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: <linux-arm-kernel@lists.infradead.org> > > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > > --- > > > arch/arm64/kernel/setup.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > Catalin, Will, Can I get an ack on this so I can take the series via the > > DT tree. > > Mark had strong pretty strong objections to this in version one: Yes, I had concerns with it as well. > https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/ > > and this patch looks the same now as it did then. Did something else > change? Yes, that version unflattened the bootloader passed DT. Now within unflatten_devicetree(), the bootloader DT is ignored if ACPI is enabled and we unflatten an empty tree. That will prevent the kernel getting 2 h/w descriptions if/when a platform does such a thing. Also, kexec still uses the bootloader provided DT as before. Rob
On 2/16/2024 5:05 PM, Stephen Boyd wrote: > Call this function unconditionally so that we can populate an empty DTB > on platforms that don't boot with a firmware provided or builtin DTB. > When ACPI is in use, unflatten_device_tree() ignores the > 'initial_boot_params' pointer so the live DT on those systems won't be > whatever that's pointing to. Similarly, when kexec copies the DT data > the previous kernel to the new one on ACPI systems, > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > one) and copy the 'initial_boot_params' data. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: <linux-arm-kernel@lists.infradead.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> This change looks good to me. I am working on a patch set that will benefit from this. Reviewed-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com> Regards, Oreoluwa > --- > arch/arm64/kernel/setup.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 42c690bb2d60..0d210720d47d 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -351,8 +351,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) > /* Parse the ACPI tables for possible boot-time configuration */ > acpi_boot_table_init(); > > - if (acpi_disabled) > - unflatten_device_tree(); > + unflatten_device_tree(); > > bootmem_init();
On Fri, Feb 23, 2024 at 11:17:02AM -0700, Rob Herring wrote: > On Fri, Feb 23, 2024 at 3:23 AM Will Deacon <will@kernel.org> wrote: > > > > On Thu, Feb 22, 2024 at 05:03:17PM -0700, Rob Herring wrote: > > > On Fri, Feb 16, 2024 at 05:05:54PM -0800, Stephen Boyd wrote: > > > > Call this function unconditionally so that we can populate an empty DTB > > > > on platforms that don't boot with a firmware provided or builtin DTB. > > > > When ACPI is in use, unflatten_device_tree() ignores the > > > > 'initial_boot_params' pointer so the live DT on those systems won't be > > > > whatever that's pointing to. Similarly, when kexec copies the DT data > > > > the previous kernel to the new one on ACPI systems, > > > > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > > > > one) and copy the 'initial_boot_params' data. > > > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > > Cc: Will Deacon <will@kernel.org> > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > Cc: <linux-arm-kernel@lists.infradead.org> > > > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > > > --- > > > > arch/arm64/kernel/setup.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > Catalin, Will, Can I get an ack on this so I can take the series via the > > > DT tree. > > > > Mark had strong pretty strong objections to this in version one: > > Yes, I had concerns with it as well. > > > https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/ > > > > and this patch looks the same now as it did then. Did something else > > change? > > Yes, that version unflattened the bootloader passed DT. Now within > unflatten_devicetree(), the bootloader DT is ignored if ACPI is > enabled and we unflatten an empty tree. That will prevent the kernel > getting 2 h/w descriptions if/when a platform does such a thing. Also, > kexec still uses the bootloader provided DT as before. That avoids the main instance of my concern, and means that this'll boot without issue, but IIUC this opens the door to dynamically instantiating DT devices atop an ACPI base system, which I think in general is something that's liable to cause more problems than it solves. I understand that's desireable for the selftests, though I still don't believe it's strictly necessary -- there are plenty of other things that only work if the kernel is booted in a specific configuration. Putting the selftests aside, why do we need to do this? Is there any other reason to enable this? Mark.
On Tue, Feb 27, 2024 at 05:34:58PM +0000, Mark Rutland wrote: > On Fri, Feb 23, 2024 at 11:17:02AM -0700, Rob Herring wrote: > > On Fri, Feb 23, 2024 at 3:23 AM Will Deacon <will@kernel.org> wrote: > > > > > > On Thu, Feb 22, 2024 at 05:03:17PM -0700, Rob Herring wrote: > > > > On Fri, Feb 16, 2024 at 05:05:54PM -0800, Stephen Boyd wrote: > > > > > Call this function unconditionally so that we can populate an empty DTB > > > > > on platforms that don't boot with a firmware provided or builtin DTB. > > > > > When ACPI is in use, unflatten_device_tree() ignores the > > > > > 'initial_boot_params' pointer so the live DT on those systems won't be > > > > > whatever that's pointing to. Similarly, when kexec copies the DT data > > > > > the previous kernel to the new one on ACPI systems, > > > > > of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root > > > > > one) and copy the 'initial_boot_params' data. > > > > > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > > > Cc: Will Deacon <will@kernel.org> > > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > > Cc: <linux-arm-kernel@lists.infradead.org> > > > > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > > > > --- > > > > > arch/arm64/kernel/setup.c | 3 +-- > > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > Catalin, Will, Can I get an ack on this so I can take the series via the > > > > DT tree. > > > > > > Mark had strong pretty strong objections to this in version one: > > > > Yes, I had concerns with it as well. > > > > > https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/ > > > > > > and this patch looks the same now as it did then. Did something else > > > change? > > > > Yes, that version unflattened the bootloader passed DT. Now within > > unflatten_devicetree(), the bootloader DT is ignored if ACPI is > > enabled and we unflatten an empty tree. That will prevent the kernel > > getting 2 h/w descriptions if/when a platform does such a thing. Also, > > kexec still uses the bootloader provided DT as before. > > That avoids the main instance of my concern, and means that this'll boot > without issue, but IIUC this opens the door to dynamically instantiating DT > devices atop an ACPI base system, which I think in general is something that's > liable to cause more problems than it solves. > > I understand that's desireable for the selftests, though I still don't believe > it's strictly necessary -- there are plenty of other things that only work if > the kernel is booted in a specific configuration. Why add to the test matrix if we don't have to? > Putting the selftests aside, why do we need to do this? Is there any other > reason to enable this? See my Plumbers talk... Or in short, there's 3 main usecases: - PCI FPGA card with devices instantiated in it - SoCs which expose their peripherals via a PCI endpoint. - Injecting test devices with QEMU (testing, but not what this series does. Jonathan Cameron's usecase) In all cases, drivers already exist for the devices, and they often only support DT. DT overlays is the natural solution for this, and there's now kernel support for it (dynamically generating PCI DT nodes when they don't exist). The intent is to do the same thing on ACPI systems. I don't see another solution other than 'go away, you're crazy'. There's ACPI overlays, but that's only a debug feature. Also, that would encourage more of the DT bindings in ACPI which I find worse than this mixture. There's swnodes, but that's just board files and platform_data 2.0. I share the concerns with mixing, but I don't see a better solution. The scope of what's possible is contained enough to avoid issues. Rob
Hi, On Wed, 28 Feb 2024 10:26:47 -0600 Rob Herring <robh@kernel.org> wrote: ... > > > > > > Yes, that version unflattened the bootloader passed DT. Now within > > > unflatten_devicetree(), the bootloader DT is ignored if ACPI is > > > enabled and we unflatten an empty tree. That will prevent the kernel > > > getting 2 h/w descriptions if/when a platform does such a thing. Also, > > > kexec still uses the bootloader provided DT as before. > > > > That avoids the main instance of my concern, and means that this'll boot > > without issue, but IIUC this opens the door to dynamically instantiating DT > > devices atop an ACPI base system, which I think in general is something that's > > liable to cause more problems than it solves. > > > > I understand that's desireable for the selftests, though I still don't believe > > it's strictly necessary -- there are plenty of other things that only work if > > the kernel is booted in a specific configuration. > > Why add to the test matrix if we don't have to? > > > Putting the selftests aside, why do we need to do this? Is there any other > > reason to enable this? > > See my Plumbers talk... > > Or in short, there's 3 main usecases: > > - PCI FPGA card with devices instantiated in it > - SoCs which expose their peripherals via a PCI endpoint. > - Injecting test devices with QEMU (testing, but not what this series > does. Jonathan Cameron's usecase) > > In all cases, drivers already exist for the devices, and they often only > support DT. DT overlays is the natural solution for this, and there's > now kernel support for it (dynamically generating PCI DT nodes when they > don't exist). The intent is to do the same thing on ACPI systems. > > I don't see another solution other than 'go away, you're crazy'. There's > ACPI overlays, but that's only a debug feature. Also, that would > encourage more of the DT bindings in ACPI which I find worse than this > mixture. There's swnodes, but that's just board files and platform_data > 2.0. > > I share the concerns with mixing, but I don't see a better solution. The > scope of what's possible is contained enough to avoid issues. > I tested on a x86 system. My use case is 'SoCs which expose their peripherals via a PCI endpoint' described by Rob. Indeed, I have a Microchip Lan9662 board (the one mentioned by Rob in his Plumbers talk) and the root DT node creation is obviously needed. I have previously used Frank Rowan's patches [1] that did this DT root node creation. This series perfectly replace them and the root DT node is successfully created. Tested-by: Herve Codina <herve.codina@bootlin.com> [1]: https://lore.kernel.org/all/20230317053415.2254616-1-frowand.list@gmail.com/ Best regards, Hervé Codina
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 42c690bb2d60..0d210720d47d 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -351,8 +351,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) /* Parse the ACPI tables for possible boot-time configuration */ acpi_boot_table_init(); - if (acpi_disabled) - unflatten_device_tree(); + unflatten_device_tree(); bootmem_init();
Call this function unconditionally so that we can populate an empty DTB on platforms that don't boot with a firmware provided or builtin DTB. When ACPI is in use, unflatten_device_tree() ignores the 'initial_boot_params' pointer so the live DT on those systems won't be whatever that's pointing to. Similarly, when kexec copies the DT data the previous kernel to the new one on ACPI systems, of_kexec_alloc_and_setup_fdt() will ignore the live DT (the empty root one) and copy the 'initial_boot_params' data. Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: <linux-arm-kernel@lists.infradead.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- arch/arm64/kernel/setup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)