[RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
patch moved this configuration to entire riscv.

Although BUILTIN_DTB is not a good choice for most platforms, it is likely
to be a debug feature when some bootloader will always override something
like the memory node in the device tree to adjust the memory size from SPD
or configuration resistor, which makes it hard to do some debugging. As an
example, some platforms with numa like sg2042 only support sv39 will fail
to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we want
a kernel without this patch to boot, we need to write the memory nodes 
in the DT manually.

Also, changing DT on some platforms is not easy. For Milk-V Pioneer, the
boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
changed, OpenSBI or LinuxBoot may refuse to boot. And there is some bug on
LinuxBoot now which does not consume --dtb argument on kexec and always
uses DT from memory. So I would like to do debugging on DT using
BUILTIN_DTB, which makes it very simple, I can even install the kernel in
the distro's way and provide a kernel package for other users to test.

Yangyu Chen (1):
  riscv: dts: Allow BUILTIN_DTB for all socs

 arch/riscv/Kconfig                  | 16 ++++++++++++++-
 arch/riscv/Kconfig.socs             | 32 -----------------------------
 arch/riscv/boot/dts/Makefile        |  2 +-
 arch/riscv/boot/dts/canaan/Makefile |  2 --
 4 files changed, 16 insertions(+), 36 deletions(-)

-- 
2.43.0

[RFC PATCH 1/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
patch moved this configuration to entire riscv.

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 arch/riscv/Kconfig                  | 16 ++++++++++++++-
 arch/riscv/Kconfig.socs             | 32 -----------------------------
 arch/riscv/boot/dts/Makefile        |  2 +-
 arch/riscv/boot/dts/canaan/Makefile |  2 --
 4 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index b49016bb5077..23d501561e64 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -996,7 +996,21 @@ config RISCV_ISA_FALLBACK
 config BUILTIN_DTB
 	bool "Built-in device tree"
 	depends on OF && NONPORTABLE
-	default y if XIP_KERNEL
+	default y if XIP_KERNEL || SOC_CANAAN
+	help
+	  Build a device tree into the Linux image.
+	  This option should be selected if no bootloader is being used.
+	  If unsure, say Y.
+
+
+config BUILTIN_DTB_SOURCE
+	string "Built-in device tree source"
+	depends on BUILTIN_DTB
+	default "canaan/k210_generic" if SOC_CANAAN
+	help
+	  DTS file path (without suffix, relative to arch/riscv/boot/dts)
+	  for the DTS file that will be used to produce the DTB linked into the
+	  kernel.
 
 endmenu # "Boot options"
 
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index e08e91c49abe..623de5f8a208 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -84,36 +84,4 @@ config SOC_CANAAN
 	help
 	  This enables support for Canaan Kendryte K210 SoC platform hardware.
 
-if ARCH_CANAAN
-
-config ARCH_CANAAN_K210_DTB_BUILTIN
-	def_bool SOC_CANAAN_K210_DTB_BUILTIN
-
-config SOC_CANAAN_K210_DTB_BUILTIN
-	bool "Builtin device tree for the Canaan Kendryte K210"
-	depends on ARCH_CANAAN
-	default y
-	select OF
-	select BUILTIN_DTB
-	help
-	  Build a device tree for the Kendryte K210 into the Linux image.
-	  This option should be selected if no bootloader is being used.
-	  If unsure, say Y.
-
-config ARCH_CANAAN_K210_DTB_SOURCE
-	string
-	default SOC_CANAAN_K210_DTB_SOURCE
-
-config SOC_CANAAN_K210_DTB_SOURCE
-	string "Source file for the Canaan Kendryte K210 builtin DTB"
-	depends on ARCH_CANAAN
-	depends on ARCH_CANAAN_K210_DTB_BUILTIN
-	default "k210_generic"
-	help
-	  Base name (without suffix, relative to arch/riscv/boot/dts/canaan)
-	  for the DTS file that will be used to produce the DTB linked into the
-	  kernel.
-
-endif # ARCH_CANAAN
-
 endmenu # "SoC selection"
diff --git a/arch/riscv/boot/dts/Makefile b/arch/riscv/boot/dts/Makefile
index 72030fd727af..318239d9423b 100644
--- a/arch/riscv/boot/dts/Makefile
+++ b/arch/riscv/boot/dts/Makefile
@@ -8,4 +8,4 @@ subdir-y += sophgo
 subdir-y += starfive
 subdir-y += thead
 
-obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix /, $(subdir-y))
+obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix .dtb.o, $(CONFIG_BUILTIN_DTB_SOURCE))
\ No newline at end of file
diff --git a/arch/riscv/boot/dts/canaan/Makefile b/arch/riscv/boot/dts/canaan/Makefile
index 520623264c87..987d1f0c41f0 100644
--- a/arch/riscv/boot/dts/canaan/Makefile
+++ b/arch/riscv/boot/dts/canaan/Makefile
@@ -5,5 +5,3 @@ dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_bit.dtb
 dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_dock.dtb
 dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_go.dtb
 dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maixduino.dtb
-
-obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o, $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
-- 
2.43.0

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Alexandre Ghiti]

Hi Yangyu,

On 20/02/2024 20:01, Yangyu Chen wrote:
> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
> patch moved this configuration to entire riscv.
>
> Although BUILTIN_DTB is not a good choice for most platforms, it is likely
> to be a debug feature when some bootloader will always override something
> like the memory node in the device tree to adjust the memory size from SPD
> or configuration resistor, which makes it hard to do some debugging. As an
> example, some platforms with numa like sg2042 only support sv39 will fail
> to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we want


Orthogonal to this patch, but if needs be, we can fix this issue with 
128G on sv39 by changing the sv39 address space layout, we still have 
room to gain the 4G you miss, at that time I was pretty sure that amount 
of memory would come with sv48 :)

Thanks,

Alex


> a kernel without this patch to boot, we need to write the memory nodes
> in the DT manually.
>
> Also, changing DT on some platforms is not easy. For Milk-V Pioneer, the
> boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
> changed, OpenSBI or LinuxBoot may refuse to boot. And there is some bug on
> LinuxBoot now which does not consume --dtb argument on kexec and always
> uses DT from memory. So I would like to do debugging on DT using
> BUILTIN_DTB, which makes it very simple, I can even install the kernel in
> the distro's way and provide a kernel package for other users to test.
>
> Yangyu Chen (1):
>    riscv: dts: Allow BUILTIN_DTB for all socs
>
>   arch/riscv/Kconfig                  | 16 ++++++++++++++-
>   arch/riscv/Kconfig.socs             | 32 -----------------------------
>   arch/riscv/boot/dts/Makefile        |  2 +-
>   arch/riscv/boot/dts/canaan/Makefile |  2 --
>   4 files changed, 16 insertions(+), 36 deletions(-)
>

Re: [RFC PATCH 1/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

On Wed, Feb 21, 2024 at 03:03:57AM +0800, Yangyu Chen wrote:
> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
> patch moved this configuration to entire riscv.
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>

A passing remark here only:
The detail in the cover letter belongs in the commit message. There's
very little value in a cover letter for a one patch "series" - it
either will duplicate information from the commit message (and is
therefore useless) or, as in this case, splits information that should
be in the commit message away from it.

Thanks,
Conor.

> ---
>  arch/riscv/Kconfig                  | 16 ++++++++++++++-
>  arch/riscv/Kconfig.socs             | 32 -----------------------------
>  arch/riscv/boot/dts/Makefile        |  2 +-
>  arch/riscv/boot/dts/canaan/Makefile |  2 --
>  4 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b49016bb5077..23d501561e64 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -996,7 +996,21 @@ config RISCV_ISA_FALLBACK
>  config BUILTIN_DTB
>  	bool "Built-in device tree"
>  	depends on OF && NONPORTABLE
> -	default y if XIP_KERNEL
> +	default y if XIP_KERNEL || SOC_CANAAN
> +	help
> +	  Build a device tree into the Linux image.
> +	  This option should be selected if no bootloader is being used.
> +	  If unsure, say Y.
> +
> +
> +config BUILTIN_DTB_SOURCE
> +	string "Built-in device tree source"
> +	depends on BUILTIN_DTB
> +	default "canaan/k210_generic" if SOC_CANAAN
> +	help
> +	  DTS file path (without suffix, relative to arch/riscv/boot/dts)
> +	  for the DTS file that will be used to produce the DTB linked into the
> +	  kernel.
>  
>  endmenu # "Boot options"
>  
> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index e08e91c49abe..623de5f8a208 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -84,36 +84,4 @@ config SOC_CANAAN
>  	help
>  	  This enables support for Canaan Kendryte K210 SoC platform hardware.
>  
> -if ARCH_CANAAN
> -
> -config ARCH_CANAAN_K210_DTB_BUILTIN
> -	def_bool SOC_CANAAN_K210_DTB_BUILTIN
> -
> -config SOC_CANAAN_K210_DTB_BUILTIN
> -	bool "Builtin device tree for the Canaan Kendryte K210"
> -	depends on ARCH_CANAAN
> -	default y
> -	select OF
> -	select BUILTIN_DTB
> -	help
> -	  Build a device tree for the Kendryte K210 into the Linux image.
> -	  This option should be selected if no bootloader is being used.
> -	  If unsure, say Y.
> -
> -config ARCH_CANAAN_K210_DTB_SOURCE
> -	string
> -	default SOC_CANAAN_K210_DTB_SOURCE
> -
> -config SOC_CANAAN_K210_DTB_SOURCE
> -	string "Source file for the Canaan Kendryte K210 builtin DTB"
> -	depends on ARCH_CANAAN
> -	depends on ARCH_CANAAN_K210_DTB_BUILTIN
> -	default "k210_generic"
> -	help
> -	  Base name (without suffix, relative to arch/riscv/boot/dts/canaan)
> -	  for the DTS file that will be used to produce the DTB linked into the
> -	  kernel.
> -
> -endif # ARCH_CANAAN
> -
>  endmenu # "SoC selection"
> diff --git a/arch/riscv/boot/dts/Makefile b/arch/riscv/boot/dts/Makefile
> index 72030fd727af..318239d9423b 100644
> --- a/arch/riscv/boot/dts/Makefile
> +++ b/arch/riscv/boot/dts/Makefile
> @@ -8,4 +8,4 @@ subdir-y += sophgo
>  subdir-y += starfive
>  subdir-y += thead
>  
> -obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix /, $(subdir-y))
> +obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix .dtb.o, $(CONFIG_BUILTIN_DTB_SOURCE))
> \ No newline at end of file
> diff --git a/arch/riscv/boot/dts/canaan/Makefile b/arch/riscv/boot/dts/canaan/Makefile
> index 520623264c87..987d1f0c41f0 100644
> --- a/arch/riscv/boot/dts/canaan/Makefile
> +++ b/arch/riscv/boot/dts/canaan/Makefile
> @@ -5,5 +5,3 @@ dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_bit.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_dock.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_go.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maixduino.dtb
> -
> -obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o, $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 1/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

On Wed, 2024-02-21 at 01:11 +0000, Conor Dooley wrote:
> On Wed, Feb 21, 2024 at 03:03:57AM +0800, Yangyu Chen wrote:
> > The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC
> > only. This
> > patch moved this configuration to entire riscv.
> > 
> > Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> 
> A passing remark here only:
> The detail in the cover letter belongs in the commit message. There's
> very little value in a cover letter for a one patch "series" - it
> either will duplicate information from the commit message (and is
> therefore useless) or, as in this case, splits information that
> should
> be in the commit message away from it.
> 
> Thanks,
> Conor.
> 

Thanks. I will move these detailed reasons to commit message. I am
waiting for other review comments before submitting patch v2.

> > ---
> >  arch/riscv/Kconfig                  | 16 ++++++++++++++-
> >  arch/riscv/Kconfig.socs             | 32 -------------------------
> > ----
> >  arch/riscv/boot/dts/Makefile        |  2 +-
> >  arch/riscv/boot/dts/canaan/Makefile |  2 --
> >  4 files changed, 16 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index b49016bb5077..23d501561e64 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -996,7 +996,21 @@ config RISCV_ISA_FALLBACK
> >  config BUILTIN_DTB
> >  	bool "Built-in device tree"
> >  	depends on OF && NONPORTABLE
> > -	default y if XIP_KERNEL
> > +	default y if XIP_KERNEL || SOC_CANAAN
> > +	help
> > +	  Build a device tree into the Linux image.
> > +	  This option should be selected if no bootloader is being
> > used.
> > +	  If unsure, say Y.
> > +
> > +
> > +config BUILTIN_DTB_SOURCE
> > +	string "Built-in device tree source"
> > +	depends on BUILTIN_DTB
> > +	default "canaan/k210_generic" if SOC_CANAAN
> > +	help
> > +	  DTS file path (without suffix, relative to
> > arch/riscv/boot/dts)
> > +	  for the DTS file that will be used to produce the DTB
> > linked into the
> > +	  kernel.
> >  
> >  endmenu # "Boot options"
> >  
> > diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> > index e08e91c49abe..623de5f8a208 100644
> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> > @@ -84,36 +84,4 @@ config SOC_CANAAN
> >  	help
> >  	  This enables support for Canaan Kendryte K210 SoC
> > platform hardware.
> >  
> > -if ARCH_CANAAN
> > -
> > -config ARCH_CANAAN_K210_DTB_BUILTIN
> > -	def_bool SOC_CANAAN_K210_DTB_BUILTIN
> > -
> > -config SOC_CANAAN_K210_DTB_BUILTIN
> > -	bool "Builtin device tree for the Canaan Kendryte K210"
> > -	depends on ARCH_CANAAN
> > -	default y
> > -	select OF
> > -	select BUILTIN_DTB
> > -	help
> > -	  Build a device tree for the Kendryte K210 into the Linux
> > image.
> > -	  This option should be selected if no bootloader is being
> > used.
> > -	  If unsure, say Y.
> > -
> > -config ARCH_CANAAN_K210_DTB_SOURCE
> > -	string
> > -	default SOC_CANAAN_K210_DTB_SOURCE
> > -
> > -config SOC_CANAAN_K210_DTB_SOURCE
> > -	string "Source file for the Canaan Kendryte K210 builtin
> > DTB"
> > -	depends on ARCH_CANAAN
> > -	depends on ARCH_CANAAN_K210_DTB_BUILTIN
> > -	default "k210_generic"
> > -	help
> > -	  Base name (without suffix, relative to
> > arch/riscv/boot/dts/canaan)
> > -	  for the DTS file that will be used to produce the DTB
> > linked into the
> > -	  kernel.
> > -
> > -endif # ARCH_CANAAN
> > -
> >  endmenu # "SoC selection"
> > diff --git a/arch/riscv/boot/dts/Makefile
> > b/arch/riscv/boot/dts/Makefile
> > index 72030fd727af..318239d9423b 100644
> > --- a/arch/riscv/boot/dts/Makefile
> > +++ b/arch/riscv/boot/dts/Makefile
> > @@ -8,4 +8,4 @@ subdir-y += sophgo
> >  subdir-y += starfive
> >  subdir-y += thead
> >  
> > -obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix /, $(subdir-y))
> > +obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix .dtb.o,
> > $(CONFIG_BUILTIN_DTB_SOURCE))
> > \ No newline at end of file

I should keep the newline at EOF, I will fix this for patch v2.

> > diff --git a/arch/riscv/boot/dts/canaan/Makefile
> > b/arch/riscv/boot/dts/canaan/Makefile
> > index 520623264c87..987d1f0c41f0 100644
> > --- a/arch/riscv/boot/dts/canaan/Makefile
> > +++ b/arch/riscv/boot/dts/canaan/Makefile
> > @@ -5,5 +5,3 @@ dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_bit.dtb
> >  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_dock.dtb
> >  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_go.dtb
> >  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maixduino.dtb
> > -
> > -obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> > $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> > -- 
> > 2.43.0
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2422 bytes --]

Hey,

On Wed, Feb 21, 2024 at 03:01:53AM +0800, Yangyu Chen wrote:
> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
> patch moved this configuration to entire riscv.

To be honest, I would rather delete BUILTIN_DTB (and the configurations
that depend on it) than expand its usefulness.

> Although BUILTIN_DTB is not a good choice for most platforms, it is likely
> to be a debug feature when some bootloader will always override something
> like the memory node in the device tree to adjust the memory size from SPD
> or configuration resistor, which makes it hard to do some debugging.

My inclination here is to say "fix your bootloader" and if that's not
possible, chainload a bootloader that allows you control over
modifications to your devicetree.

> As an
> example, some platforms with numa like sg2042 only support sv39 will fail
> to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we want
> a kernel without this patch to boot, we need to write the memory nodes 
> in the DT manually.

If, as Alex suggests, there's a way to gain support some more memory in
sv39, we should do so - but it is worth mentioning that highmem is on the
removal list for the kernel, so mainline support for that is highly
unlikely.

> Also, changing DT on some platforms is not easy. For Milk-V Pioneer, the
> boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
> changed, OpenSBI or LinuxBoot may refuse to boot. And there is some bug on
> LinuxBoot now which does not consume --dtb argument on kexec and always
> uses DT from memory.

I don't use Linuxboot, but let me try to understand. Linuxboot uses kexec
to boot the main Linux kernel, but the dtb you want to use is not used, and
instead the one that Linuxboot itself was booted with is used?

It sounds like Linuxboot has a --dtb argumet that is meant to be used to
set the dtb for the next stage, but that argument is being ignored?

That sounds like a pretty serious issue with Linuxboot which should be
fixed - what am I missing?

> So I would like to do debugging on DT using
> BUILTIN_DTB, which makes it very simple,

> I can even install the kernel in
> the distro's way and provide a kernel package for other users to test.

I'm not sure what you mean by this, other distros manage to create
kernel packages without using builtin dtbs.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

On Wed, 2024-02-21 at 11:30 +0000, Conor Dooley wrote:
> Hey,
> 
> On Wed, Feb 21, 2024 at 03:01:53AM +0800, Yangyu Chen wrote:
> > The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC
> > only. This
> > patch moved this configuration to entire riscv.
> 
> To be honest, I would rather delete BUILTIN_DTB (and the
> configurations
> that depend on it) than expand its usefulness.
> 

I agree it’s useless for most platforms because we need to start SBI
before kernel on RISC-V except NOMMU M-Mode Linux and SBI also need a
DT to work. However, it has been there for M-Mode K210 and it is set by
default for XIP kernel. So there might eventually be another patch to
support some new soc that will do this like this patch.

> > Although BUILTIN_DTB is not a good choice for most platforms, it is
> > likely
> > to be a debug feature when some bootloader will always override
> > something
> > like the memory node in the device tree to adjust the memory size
> > from SPD
> > or configuration resistor, which makes it hard to do some
> > debugging.
> 
> My inclination here is to say "fix your bootloader" and if that's not
> possible, chainload a bootloader that allows you control over
> modifications to your devicetree.
> 

Chainload a bootloader like S-Mode U-Boot on some platforms is hard due
to some drivers like pcie controller does not come to the mainline repo
of the bootloader, and some bootloader source repos provided by the
vendor may require specific versions of the compiler to work, which
makes users not easy to do some kernel debugging if change DT is
needed. The simplest way to do this I can imagine is to write a simple
bootloader by myself link the kernel binary and the dtb I want to it
and replace the a1 register point to the dtb address before jumping to
the kernel. However, kernel has this feature, why should I do it
manually rather than provide a more generic patch for everyone with
this need to use?

> > As an
> > example, some platforms with numa like sg2042 only support sv39
> > will fail
> > to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we
> > want
> > a kernel without this patch to boot, we need to write the memory
> > nodes 
> > in the DT manually.
> 
> If, as Alex suggests, there's a way to gain support some more memory
> in
> sv39, we should do so - but it is worth mentioning that highmem is on
> the
> removal list for the kernel, so mainline support for that is highly
> unlikely.
> 

Yes. But I’m debugging some mm performance issues on the sg2042 kernel.
Specifically, it’s about the IPI latency when doing rfence on
sfence.vma or fence.i. I would like to reduce the memory size and allow
the mainline kernel to boot and test without taking some out-of-tree
kernel patches. If I remove some DIMM modules from the board to reduce
the memory size, it will also lose some memory channels and even leave
some numa nodes with zero memory, and the compatible DIMM module is
hard to find.

> > Also, changing DT on some platforms is not easy. For Milk-V
> > Pioneer, the
> > boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
> > changed, OpenSBI or LinuxBoot may refuse to boot. And there is some
> > bug on
> > LinuxBoot now which does not consume --dtb argument on kexec and
> > always
> > uses DT from memory.
> 
> I don't use Linuxboot, but let me try to understand. Linuxboot uses
> kexec
> to boot the main Linux kernel, but the dtb you want to use is not
> used, and
> instead the one that Linuxboot itself was booted with is used?
> 
> It sounds like Linuxboot has a --dtb argumet that is meant to be used
> to
> set the dtb for the next stage, but that argument is being ignored?
> 

Yes. That’s correct.

> That sounds like a pretty serious issue with Linuxboot which should
> be
> fixed - what am I missing?
> 


Sure, that should be fixed in the LinuxBoot. However, I think not every
kernel developer should fix some complex bootloader like LinuxBoot
which is built upon the linux kernel with a huge initrd rootfs and runs
some userspace tools to support the boot process. If something is hard
to control, skip it, and doing some override for debugging will be a
better choice.

> > So I would like to do debugging on DT using
> > BUILTIN_DTB, which makes it very simple,
> 
> > I can even install the kernel in
> > the distro's way and provide a kernel package for other users to
> > test.
> 
> I'm not sure what you mean by this, other distros manage to create
> kernel packages without using builtin dtbs.
> 

I mean I can provide a distro package like Debian .deb and distribute
it to other users to test without changing their dtb from the entire
boot process. Because changing the DT from the entire boot process
might prevent their vendor-provided OpenSBI or LinuxBoot from working.
Some vendor kernels may be developed out-of-tree and do not use the dt-
binding from mainline. Even for very basic CLINT and PLIC dt bindings.
It is only for testing, not for the production environment.

> Thanks,
> Conor.

I want this feature to allow more people to participate in debugging
some kernel issues without taking a huge amount of time to deal with
bootloader issues about changing the DT. I think it will be good for
our under-development RISC-V community. Imagine we hardly change the
ACPI table for x86 machines but we sometimes change the DT for
ARM/RISC-V board, right? Also, some SoCs that run M-Mode NOMMU Linux
may need it in the future like K210 for XIP without a prior bootloader.

Thanks,
Yangyu Chen.

Re: [RFC PATCH 1/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

On Wed, 2024-02-21 at 03:03 +0800, Yangyu Chen wrote:
> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only.
> This
> patch moved this configuration to entire riscv.
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  arch/riscv/Kconfig                  | 16 ++++++++++++++-
>  arch/riscv/Kconfig.socs             | 32 ---------------------------
> --
>  arch/riscv/boot/dts/Makefile        |  2 +-
>  arch/riscv/boot/dts/canaan/Makefile |  2 --
>  4 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b49016bb5077..23d501561e64 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -996,7 +996,21 @@ config RISCV_ISA_FALLBACK
>  config BUILTIN_DTB
>  	bool "Built-in device tree"
>  	depends on OF && NONPORTABLE
> -	default y if XIP_KERNEL
> +	default y if XIP_KERNEL || SOC_CANAAN
> +	help
> +	  Build a device tree into the Linux image.
> +	  This option should be selected if no bootloader is being
> used.
> +	  If unsure, say Y.
> +
> +
> +config BUILTIN_DTB_SOURCE
> +	string "Built-in device tree source"
> +	depends on BUILTIN_DTB
> +	default "canaan/k210_generic" if SOC_CANAAN
> +	help
> +	  DTS file path (without suffix, relative to
> arch/riscv/boot/dts)
> +	  for the DTS file that will be used to produce the DTB
> linked into the
> +	  kernel.
>  
>  endmenu # "Boot options"
>  
> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index e08e91c49abe..623de5f8a208 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -84,36 +84,4 @@ config SOC_CANAAN
>  	help
>  	  This enables support for Canaan Kendryte K210 SoC platform
> hardware.
>  
> -if ARCH_CANAAN
> -
> -config ARCH_CANAAN_K210_DTB_BUILTIN
> -	def_bool SOC_CANAAN_K210_DTB_BUILTIN
> -
> -config SOC_CANAAN_K210_DTB_BUILTIN
> -	bool "Builtin device tree for the Canaan Kendryte K210"
> -	depends on ARCH_CANAAN
> -	default y
> -	select OF
> -	select BUILTIN_DTB
> -	help
> -	  Build a device tree for the Kendryte K210 into the Linux
> image.
> -	  This option should be selected if no bootloader is being
> used.
> -	  If unsure, say Y.
> -
> -config ARCH_CANAAN_K210_DTB_SOURCE
> -	string
> -	default SOC_CANAAN_K210_DTB_SOURCE
> -
> -config SOC_CANAAN_K210_DTB_SOURCE
> -	string "Source file for the Canaan Kendryte K210 builtin
> DTB"
> -	depends on ARCH_CANAAN
> -	depends on ARCH_CANAAN_K210_DTB_BUILTIN
> -	default "k210_generic"
> -	help
> -	  Base name (without suffix, relative to
> arch/riscv/boot/dts/canaan)
> -	  for the DTS file that will be used to produce the DTB
> linked into the
> -	  kernel.
> -
> -endif # ARCH_CANAAN
> -
>  endmenu # "SoC selection"
> diff --git a/arch/riscv/boot/dts/Makefile
> b/arch/riscv/boot/dts/Makefile
> index 72030fd727af..318239d9423b 100644
> --- a/arch/riscv/boot/dts/Makefile
> +++ b/arch/riscv/boot/dts/Makefile
> @@ -8,4 +8,4 @@ subdir-y += sophgo
>  subdir-y += starfive
>  subdir-y += thead
>  
> -obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix /, $(subdir-y))
> +obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix .dtb.o,
> $(CONFIG_BUILTIN_DTB_SOURCE))
> \ No newline at end of file
> diff --git a/arch/riscv/boot/dts/canaan/Makefile
> b/arch/riscv/boot/dts/canaan/Makefile
> index 520623264c87..987d1f0c41f0 100644
> --- a/arch/riscv/boot/dts/canaan/Makefile
> +++ b/arch/riscv/boot/dts/canaan/Makefile
> @@ -5,5 +5,3 @@ dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_bit.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_dock.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_go.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maixduino.dtb
> -
> -obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))

After carefully reviewing the code from the kernel I found that
`arch/riscv/boot/dts/sifive(microchip)/Makefile` will also build the
dtb object file and finally link to the kernel. This results in
multiple dtb inside the kernel but the kernel always takes the first to
use as someone mentioned before [1]. So only applying this patch to the
mainline kernel does not work for choosing one dtb. I should also
revert the modification on commit 0ddd7eaffa64 ("riscv: Fix BUILTIN_DTB
for sifive and microchip soc") to prevent building these files and let
them be managed by `arch/riscv/boot/dts/Makefile`.

I'm also waiting for other review comments on this idea before
submitting patch v2.

[1]
https://lore.kernel.org/linux-riscv/CAK7LNATt_56mO2Le4v4EnPnAfd3gC8S_Sm5-GCsfa=qXy=8Lrg@mail.gmail.com/

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Palmer Dabbelt]

On Tue, 20 Feb 2024 12:46:57 PST (-0800), alex@ghiti.fr wrote:
> Hi Yangyu,
>
> On 20/02/2024 20:01, Yangyu Chen wrote:
>> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
>> patch moved this configuration to entire riscv.
>>
>> Although BUILTIN_DTB is not a good choice for most platforms, it is likely
>> to be a debug feature when some bootloader will always override something
>> like the memory node in the device tree to adjust the memory size from SPD
>> or configuration resistor, which makes it hard to do some debugging. As an
>> example, some platforms with numa like sg2042 only support sv39 will fail
>> to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we want

I guess that's a surprising one, but there's always some Kconfig entries 
that are necessary to set in order to get platforms to boot -- at a bare 
minimum errata and drivers, for example.

> Orthogonal to this patch, but if needs be, we can fix this issue with
> 128G on sv39 by changing the sv39 address space layout, we still have
> room to gain the 4G you miss, at that time I was pretty sure that amount
> of memory would come with sv48 :)

That seems like a reasonable way to go, as long as it's not overly 
complex.

>
> Thanks,
>
> Alex
>
>
>> a kernel without this patch to boot, we need to write the memory nodes
>> in the DT manually.
>>
>> Also, changing DT on some platforms is not easy. For Milk-V Pioneer, the
>> boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
>> changed, OpenSBI or LinuxBoot may refuse to boot. And there is some bug on
>> LinuxBoot now which does not consume --dtb argument on kexec and always
>> uses DT from memory. So I would like to do debugging on DT using

I'd argue those are bootloader/firmware bugs.

>> BUILTIN_DTB, which makes it very simple, I can even install the kernel in
>> the distro's way and provide a kernel package for other users to test.
>>
>> Yangyu Chen (1):
>>    riscv: dts: Allow BUILTIN_DTB for all socs
>>
>>   arch/riscv/Kconfig                  | 16 ++++++++++++++-
>>   arch/riscv/Kconfig.socs             | 32 -----------------------------
>>   arch/riscv/boot/dts/Makefile        |  2 +-
>>   arch/riscv/boot/dts/canaan/Makefile |  2 --
>>   4 files changed, 16 insertions(+), 36 deletions(-)
>>

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8439 bytes --]

On Wed, Feb 21, 2024 at 10:28:08PM +0800, Yangyu Chen wrote:
> On Wed, 2024-02-21 at 11:30 +0000, Conor Dooley wrote:
> > Hey,
> > 
> > On Wed, Feb 21, 2024 at 03:01:53AM +0800, Yangyu Chen wrote:
> > > The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC
> > > only. This
> > > patch moved this configuration to entire riscv.
> > 
> > To be honest, I would rather delete BUILTIN_DTB (and the
> > configurations
> > that depend on it) than expand its usefulness.
> > 
> 
> I agree it’s useless for most platforms because we need to start SBI
> before kernel on RISC-V except NOMMU M-Mode Linux and SBI also need a
> DT to work. However, it has been there for M-Mode K210 and it is set by
> default for XIP kernel. So there might eventually be another patch to
> support some new soc that will do this like this patch.

To be clear, I was not suggesting that it was useless. I was saying that
I would rather reduce the number of configurations that use builtin dtbs
than increase the level of support for it.

> 
> > > Although BUILTIN_DTB is not a good choice for most platforms, it is
> > > likely
> > > to be a debug feature when some bootloader will always override
> > > something
> > > like the memory node in the device tree to adjust the memory size
> > > from SPD
> > > or configuration resistor, which makes it hard to do some
> > > debugging.
> > 
> > My inclination here is to say "fix your bootloader" and if that's not
> > possible, chainload a bootloader that allows you control over
> > modifications to your devicetree.
> > 
> 
> Chainload a bootloader like S-Mode U-Boot on some platforms is hard due
> to some drivers like pcie controller does not come to the mainline repo
> of the bootloader, and some bootloader source repos provided by the
> vendor may require specific versions of the compiler to work, which
> makes users not easy to do some kernel debugging if change DT is
> needed. The simplest way to do this I can imagine is to write a simple
> bootloader by myself link the kernel binary and the dtb I want to it
> and replace the a1 register point to the dtb address before jumping to
> the kernel. However, kernel has this feature, why should I do it
> manually rather than provide a more generic patch for everyone with
> this need to use?
> 
> > > As an
> > > example, some platforms with numa like sg2042 only support sv39
> > > will fail
> > > to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we
> > > want
> > > a kernel without this patch to boot, we need to write the memory
> > > nodes 
> > > in the DT manually.
> > 
> > If, as Alex suggests, there's a way to gain support some more memory
> > in
> > sv39, we should do so - but it is worth mentioning that highmem is on
> > the
> > removal list for the kernel, so mainline support for that is highly
> > unlikely.
> > 
> 
> Yes. But I’m debugging some mm performance issues on the sg2042 kernel.
> Specifically, it’s about the IPI latency when doing rfence on
> sfence.vma or fence.i. I would like to reduce the memory size and allow
> the mainline kernel to boot and test without taking some out-of-tree
> kernel patches. If I remove some DIMM modules from the board to reduce
> the memory size, it will also lose some memory channels and even leave
> some numa nodes with zero memory, and the compatible DIMM module is
> hard to find.

I'm not really sure how this relates to my comment about HIGHMEM. If
Alex is able to give you the extra 4 GiB of memory that he says there is
space for in the memory map, will the device boot properly?

> > > Also, changing DT on some platforms is not easy. For Milk-V
> > > Pioneer, the
> > > boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
> > > changed, OpenSBI or LinuxBoot may refuse to boot. And there is some
> > > bug on
> > > LinuxBoot now which does not consume --dtb argument on kexec and
> > > always
> > > uses DT from memory.
> > 
> > I don't use Linuxboot, but let me try to understand. Linuxboot uses
> > kexec
> > to boot the main Linux kernel, but the dtb you want to use is not
> > used, and
> > instead the one that Linuxboot itself was booted with is used?
> > 
> > It sounds like Linuxboot has a --dtb argumet that is meant to be used
> > to
> > set the dtb for the next stage, but that argument is being ignored?
> > 
> 
> Yes. That’s correct.
> 
> > That sounds like a pretty serious issue with Linuxboot which should
> > be
> > fixed - what am I missing?
> > 
> 
> Sure, that should be fixed in the LinuxBoot. However, I think not every
> kernel developer should fix some complex bootloader like LinuxBoot
> which is built upon the linux kernel with a huge initrd rootfs and runs
> some userspace tools to support the boot process. If something is hard
> to control, skip it, and doing some override for debugging will be a
> better choice.

Has anyone even /reported/ the issues with LinuxBoot to the LinuxBoot
developers? Without that being fixed, there's unlikely to ever be
mainstream distro support for it, since they're going to have to build
kernels for it alone.

> > > So I would like to do debugging on DT using
> > > BUILTIN_DTB, which makes it very simple,
> > 
> > > I can even install the kernel in
> > > the distro's way and provide a kernel package for other users to
> > > test.
> > 
> > I'm not sure what you mean by this, other distros manage to create
> > kernel packages without using builtin dtbs.
> > 
> 
> I mean I can provide a distro package like Debian .deb and distribute
> it to other users to test without changing their dtb from the entire
> boot process.

Other distros, like Ubuntu, manage to do this without relying on builtin
dtbs. I suppose this comes down to having bootloaders that

> Because changing the DT from the entire boot process
> might prevent their vendor-provided OpenSBI or LinuxBoot from working.
> Some vendor kernels may be developed out-of-tree and do not use the dt-
> binding from mainline. Even for very basic CLINT and PLIC dt bindings.

Which is verging on ridiculous at this point. Does the sg2042 also have
a version of OpenSBI that is not capable of booting a mainline kernel?

> It is only for testing, not for the production environment.

If things are just for testing, I'm not particularly keen on merging on
that basis alone. We all have various bits of testing code that doesn't
end up being merged to mainline. That said, it is broken at present and
its hard to argue against fixing it and any patch fixing it would
ultimately look very similar to your patch here.

> I want this feature to allow more people to participate in debugging
> some kernel issues without taking a huge amount of time to deal with
> bootloader issues about changing the DT. I think it will be good for
> our under-development RISC-V community.

And on the other hand, it provides no incentive for vendors to fix
broken bootloaders or firmware, which is some we suffer from on RISC-V,
in particular vendors that ship T-Head's vendor copy of OpenSBI.

> Imagine we hardly change the
> ACPI table for x86 machines but we sometimes change the DT for
> ARM/RISC-V board, right?

Usually we change them because nobody gets things "right" and we end up
having different stuff in mainline to what the vendor did. Usually also
a vendor has a relatively complete description in their vendor tree, but
things only trickle into mainline, so mainline ends up requiring regular
dtb updates until a platform stabilises. More infrequently, changes are
needed for bugfixes.

The other thing you do is compare to the ACPI table. I don't think it is
quite apples to apples there - those machines mostly have devices on
discoverable buses etc. If they had the same number of non discoverable
devices, I think you'd end up having to do more BIOS updates etc.

> Also, some SoCs that run M-Mode NOMMU Linux
> may need it in the future like K210 for XIP without a prior bootloader.

And the k210 is one of the things that is on the chopping block at the
moment. It's removal was discussed at LPC this year, with Damien
surprisingly agreeing to its removal. FWIW, builtin dtb is not required
for XIP.

BTW, I noticed that your patch only removes one of the $(addsuffix)
calls in a platform makefile.
Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 1/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

Hey,

On Wed, Feb 21, 2024 at 03:03:57AM +0800, Yangyu Chen wrote:
> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC only. This
> patch moved this configuration to entire riscv.
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  arch/riscv/Kconfig                  | 16 ++++++++++++++-
>  arch/riscv/Kconfig.socs             | 32 -----------------------------
>  arch/riscv/boot/dts/Makefile        |  2 +-
>  arch/riscv/boot/dts/canaan/Makefile |  2 --
>  4 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b49016bb5077..23d501561e64 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -996,7 +996,21 @@ config RISCV_ISA_FALLBACK
>  config BUILTIN_DTB
>  	bool "Built-in device tree"
>  	depends on OF && NONPORTABLE
> -	default y if XIP_KERNEL
> +	default y if XIP_KERNEL || SOC_CANAAN

s/SOC/ARCH/

> +	help
> +	  Build a device tree into the Linux image.
> +	  This option should be selected if no bootloader is being used.
> +	  If unsure, say Y.
> +
> +
> +config BUILTIN_DTB_SOURCE
> +	string "Built-in device tree source"
> +	depends on BUILTIN_DTB

> +	default "canaan/k210_generic" if SOC_CANAAN

I think we should drop this default, I don't think that "k210_generic"
should really exist in the first place. And suggest that there are
platform-wide default devicetrees isn't something I think should be
encouraged.

> +	help
> +	  DTS file path (without suffix, relative to arch/riscv/boot/dts)
> +	  for the DTS file that will be used to produce the DTB linked into the
> +	  kernel.
>  
>  endmenu # "Boot options"

> -config SOC_CANAAN_K210_DTB_BUILTIN
> -config SOC_CANAAN_K210_DTB_SOURCE

> diff --git a/arch/riscv/boot/dts/canaan/Makefile b/arch/riscv/boot/dts/canaan/Makefile
> index 520623264c87..987d1f0c41f0 100644
> --- a/arch/riscv/boot/dts/canaan/Makefile
> +++ b/arch/riscv/boot/dts/canaan/Makefile
> @@ -5,5 +5,3 @@ dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_bit.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_dock.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maix_go.dtb
>  dtb-$(CONFIG_ARCH_CANAAN) += sipeed_maixduino.dtb
> -
> -obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o, $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))

There are two more instances of this that need to be removed.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Yangyu Chen]

On 2024/2/23 04:59, Conor Dooley wrote:
> On Wed, Feb 21, 2024 at 10:28:08PM +0800, Yangyu Chen wrote:
>> On Wed, 2024-02-21 at 11:30 +0000, Conor Dooley wrote:
>>> Hey,
>>>
>>> On Wed, Feb 21, 2024 at 03:01:53AM +0800, Yangyu Chen wrote:
>>>> The BUILTIN_DTB kernel feature on RISC-V only works on K210 SoC
>>>> only. This
>>>> patch moved this configuration to entire riscv.
>>>
>>> To be honest, I would rather delete BUILTIN_DTB (and the
>>> configurations
>>> that depend on it) than expand its usefulness.
>>>
>>
>> I agree it’s useless for most platforms because we need to start SBI
>> before kernel on RISC-V except NOMMU M-Mode Linux and SBI also need a
>> DT to work. However, it has been there for M-Mode K210 and it is set by
>> default for XIP kernel. So there might eventually be another patch to
>> support some new soc that will do this like this patch.
> 
> To be clear, I was not suggesting that it was useless. I was saying that
> I would rather reduce the number of configurations that use builtin dtbs
> than increase the level of support for it.
> 

I see.

>>
>>>> Although BUILTIN_DTB is not a good choice for most platforms, it is
>>>> likely
>>>> to be a debug feature when some bootloader will always override
>>>> something
>>>> like the memory node in the device tree to adjust the memory size
>>>> from SPD
>>>> or configuration resistor, which makes it hard to do some
>>>> debugging.
>>>
>>> My inclination here is to say "fix your bootloader" and if that's not
>>> possible, chainload a bootloader that allows you control over
>>> modifications to your devicetree.
>>>
>>
>> Chainload a bootloader like S-Mode U-Boot on some platforms is hard due
>> to some drivers like pcie controller does not come to the mainline repo
>> of the bootloader, and some bootloader source repos provided by the
>> vendor may require specific versions of the compiler to work, which
>> makes users not easy to do some kernel debugging if change DT is
>> needed. The simplest way to do this I can imagine is to write a simple
>> bootloader by myself link the kernel binary and the dtb I want to it
>> and replace the a1 register point to the dtb address before jumping to
>> the kernel. However, kernel has this feature, why should I do it
>> manually rather than provide a more generic patch for everyone with
>> this need to use?
>>
>>>> As an
>>>> example, some platforms with numa like sg2042 only support sv39
>>>> will fail
>>>> to boot when there is no ZONE_HIGHMEM patch with 128G memory. If we
>>>> want
>>>> a kernel without this patch to boot, we need to write the memory
>>>> nodes
>>>> in the DT manually.
>>>
>>> If, as Alex suggests, there's a way to gain support some more memory
>>> in
>>> sv39, we should do so - but it is worth mentioning that highmem is on
>>> the
>>> removal list for the kernel, so mainline support for that is highly
>>> unlikely.
>>>
>>
>> Yes. But I’m debugging some mm performance issues on the sg2042 kernel.
>> Specifically, it’s about the IPI latency when doing rfence on
>> sfence.vma or fence.i. I would like to reduce the memory size and allow
>> the mainline kernel to boot and test without taking some out-of-tree
>> kernel patches. If I remove some DIMM modules from the board to reduce
>> the memory size, it will also lose some memory channels and even leave
>> some numa nodes with zero memory, and the compatible DIMM module is
>> hard to find.
> 
> I'm not really sure how this relates to my comment about HIGHMEM. If
> Alex is able to give you the extra 4 GiB of memory that he says there is
> space for in the memory map, will the device boot properly?
> 

That is I said I want "mainline kernel to boot and test without taking 
some out-of-tree kernel patches" as it doesn't come to mainline now. And 
I don't see any performance issues on sifive socs with the mainline 
kernel, but it doesn't have many cores like sg2042 either. Whatever, it 
is a reason for simplifying the debug process on performance, not for 
getting 128G memory on sg2042 boot properly.

>>>> Also, changing DT on some platforms is not easy. For Milk-V
>>>> Pioneer, the
>>>> boot procedure is ZSBL -> OpenSBI -> LinuxBoot -> Linux. If DT gets
>>>> changed, OpenSBI or LinuxBoot may refuse to boot. And there is some
>>>> bug on
>>>> LinuxBoot now which does not consume --dtb argument on kexec and
>>>> always
>>>> uses DT from memory.
>>>
>>> I don't use Linuxboot, but let me try to understand. Linuxboot uses
>>> kexec
>>> to boot the main Linux kernel, but the dtb you want to use is not
>>> used, and
>>> instead the one that Linuxboot itself was booted with is used?
>>>
>>> It sounds like Linuxboot has a --dtb argumet that is meant to be used
>>> to
>>> set the dtb for the next stage, but that argument is being ignored?
>>>
>>
>> Yes. That’s correct.
>>
>>> That sounds like a pretty serious issue with Linuxboot which should
>>> be
>>> fixed - what am I missing?
>>>
>>
>> Sure, that should be fixed in the LinuxBoot. However, I think not every
>> kernel developer should fix some complex bootloader like LinuxBoot
>> which is built upon the linux kernel with a huge initrd rootfs and runs
>> some userspace tools to support the boot process. If something is hard
>> to control, skip it, and doing some override for debugging will be a
>> better choice.
> 
> Has anyone even /reported/ the issues with LinuxBoot to the LinuxBoot
> developers? Without that being fixed, there's unlikely to ever be
> mainstream distro support for it, since they're going to have to build
> kernels for it alone.
> 

I created a github issue on sophgo/bootloader-riscv [1] . Seems no body 
reported it before. Yeah it will be better to fix LinuxBoot to solve my 
own need for debugging.

>>>> So I would like to do debugging on DT using
>>>> BUILTIN_DTB, which makes it very simple,
>>>
>>>> I can even install the kernel in
>>>> the distro's way and provide a kernel package for other users to
>>>> test.
>>>
>>> I'm not sure what you mean by this, other distros manage to create
>>> kernel packages without using builtin dtbs.
>>>
>>
>> I mean I can provide a distro package like Debian .deb and distribute
>> it to other users to test without changing their dtb from the entire
>> boot process.
> 
> Other distros, like Ubuntu, manage to do this without relying on builtin
> dtbs. I suppose this comes down to having bootloaders that
> 
>> Because changing the DT from the entire boot process
>> might prevent their vendor-provided OpenSBI or LinuxBoot from working.
>> Some vendor kernels may be developed out-of-tree and do not use the dt-
>> binding from mainline. Even for very basic CLINT and PLIC dt bindings.
> 
> Which is verging on ridiculous at this point. Does the sg2042 also have
> a version of OpenSBI that is not capable of booting a mainline kernel?
> 

Yes, their vendor provided old OpenSBI can not parse aclint dt binding 
from the mainline dts, so there will be no timer for OpenSBI and refused 
to boot. That can be solved if I change the dt and use mainline opensbi 
and cherry-pick some vendor patches to get this work.

>> It is only for testing, not for the production environment.
> 
> If things are just for testing, I'm not particularly keen on merging on
> that basis alone. We all have various bits of testing code that doesn't
> end up being merged to mainline. That said, it is broken at present and
> its hard to argue against fixing it and any patch fixing it would
> ultimately look very similar to your patch here.
> 

OK. You convinced me for this reason.

>> I want this feature to allow more people to participate in debugging
>> some kernel issues without taking a huge amount of time to deal with
>> bootloader issues about changing the DT. I think it will be good for
>> our under-development RISC-V community.
> 
> And on the other hand, it provides no incentive for vendors to fix
> broken bootloaders or firmware, which is some we suffer from on RISC-V,
> in particular vendors that ship T-Head's vendor copy of OpenSBI.
> 

That's true.

>> Imagine we hardly change the
>> ACPI table for x86 machines but we sometimes change the DT for
>> ARM/RISC-V board, right?
> 
> Usually we change them because nobody gets things "right" and we end up
> having different stuff in mainline to what the vendor did. Usually also
> a vendor has a relatively complete description in their vendor tree, but
> things only trickle into mainline, so mainline ends up requiring regular
> dtb updates until a platform stabilises. More infrequently, changes are
> needed for bugfixes.
> 
> The other thing you do is compare to the ACPI table. I don't think it is
> quite apples to apples there - those machines mostly have devices on
> discoverable buses etc. If they had the same number of non discoverable
> devices, I think you'd end up having to do more BIOS updates etc.
> 

OK.

>> Also, some SoCs that run M-Mode NOMMU Linux
>> may need it in the future like K210 for XIP without a prior bootloader.
> 
> And the k210 is one of the things that is on the chopping block at the
> moment. It's removal was discussed at LPC this year, with Damien
> surprisingly agreeing to its removal. FWIW, builtin dtb is not required
> for XIP.
> 
> BTW, I noticed that your patch only removes one of the $(addsuffix)
> calls in a platform makefile.
> Thanks,
> Conor.

To sum up, I agree with the reasons to refuse it for debugging purposes. 
I am wondering what to do next. After reviewing the code carefully, I 
found this feature not only for K210 but also for other SoCs. But for 
other SoCs, it is broken as it will link multiple dtbs to the kernel, 
but the kernel will always pick up the first dtb to use as discussed on 
this thread [2]. That is because SOC_BUILTIN_DTB_DECLARE is removed from 
this patch [3] then no code to select for multiple dtbs now. And 
makefiles on other soc which is on the mainline kernel currently will 
build every dtb object file from this patch [4]. So this feature for 
other SoCs is broken now.

Choices might be one of the following:

1. Remove BUILTIN_DTB feature if K210 support get removed
2. Continue to add this feature to get this work for other socs

I prefer to continue to get this feature to work. Not only for my 
debugging purposes but also for fixes.

[1] https://github.com/sophgo/bootloader-riscv/issues/73
[2] 
https://lore.kernel.org/linux-riscv/CAK7LNATt_56mO2Le4v4EnPnAfd3gC8S_Sm5-GCsfa=qXy=8Lrg@mail.gmail.com/
[3] 
https://lore.kernel.org/linux-riscv/20201208073355.40828-5-damien.lemoal@wdc.com/
[4] 
https://lore.kernel.org/linux-riscv/20210604120639.1447869-1-alex@ghiti.fr/

Re: [RFC PATCH 0/1] riscv: dts: Allow BUILTIN_DTB for all socs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

On Fri, Feb 23, 2024 at 03:42:01PM +0800, Yangyu Chen wrote:
> To sum up, I agree with the reasons to refuse it for debugging purposes. I
> am wondering what to do next. After reviewing the code carefully, I found
> this feature not only for K210 but also for other SoCs. But for other SoCs,
> it is broken as it will link multiple dtbs to the kernel, but the kernel
> will always pick up the first dtb to use as discussed on this thread [2].
> That is because SOC_BUILTIN_DTB_DECLARE is removed from this patch [3] then
> no code to select for multiple dtbs now. And makefiles on other soc which is
> on the mainline kernel currently will build every dtb object file from this
> patch [4]. So this feature for other SoCs is broken now.
> 
> Choices might be one of the following:
> 
> 1. Remove BUILTIN_DTB feature if K210 support get removed
> 2. Continue to add this feature to get this work for other socs
> 
> I prefer to continue to get this feature to work. Not only for my debugging
> purposes but also for fixes.

My email might have been a bit unclear because I responded to things out
of order. My final thoughts were that we should apply a v2 (or 3 etc) of
this patch if for no other reason than fixing BUILTIN_DTB. I could not
really justify refusing your changes when a patch fixing BUILTIN_DTB for
the non-k210 platforms that support it would look almost identical to
this patch. Apologies if you misunderstood.

I left some comments on the patch itself with a v2 in mind.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]