[PATCH] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[PATCH] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Pali Rohár]

When U-Boot is running from flash memory (execute in place) then
gd->fdt_blob before relocation points to read-only flash memory.

So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
cause immediate CPU crash when callback is trying to modify gd->fdt_blob.

Fix this issue by introducing a new config option OF_INITIAL_DTB_READONLY
which moves fix_fdt callback after the reloc_fdt callback. This makes
CONFIG_OF_BOARD_FIXUP working also if U-Boot before relocation is not
running from read/write (S)RAM memory.

This is required for mpc85xx boards when booting from flash NOR.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/board_f.c | 8 +++++++-
 dts/Kconfig      | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index 18e2246733b0..56b4eea24885 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -911,7 +911,7 @@ static const init_fnc_t init_sequence_f[] = {
 	 *  - board info struct
 	 */
 	setup_dest_addr,
-#ifdef CONFIG_OF_BOARD_FIXUP
+#if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY)
 	fix_fdt,
 #endif
 #ifdef CONFIG_PRAM
@@ -926,6 +926,10 @@ static const init_fnc_t init_sequence_f[] = {
 	reserve_board,
 	reserve_global_data,
 	reserve_fdt,
+#if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY)
+	reloc_fdt,
+	fix_fdt,
+#endif
 	reserve_bootstage,
 	reserve_bloblist,
 	reserve_arch,
@@ -936,7 +940,9 @@ static const init_fnc_t init_sequence_f[] = {
 	setup_bdinfo,
 	display_new_sp,
 	INIT_FUNC_WATCHDOG_RESET
+#if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY)
 	reloc_fdt,
+#endif
 	reloc_bootstage,
 	reloc_bloblist,
 	setup_reloc,
diff --git a/dts/Kconfig b/dts/Kconfig
index bc5f22029ff9..0bc3d741f8bc 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -105,6 +105,12 @@ config OF_EMBED
 
 endchoice
 
+config OF_INITIAL_DTB_READONLY
+	bool "Initial DTB for DT control is read-only"
+	help
+	  If initial DTB for DT control is read-only (e.g. points to
+	  memory-mapped flash memory), then set this option.
+
 config OF_BOARD
 	bool "Provided by the board (e.g a previous loader) at runtime"
 	default y if SANDBOX || OF_HAS_PRIOR_STAGE
-- 
2.20.1

Re: [PATCH] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Marek Behún]

Can you rename the option to
  OF_FDT_READONLY_BEFORE_RELOC
?

The INITIAL in the name may confuse people.

The functions are called *_fdt (reloc_fdt, fix_fdt), so maybe put FDT
instead of DTB in the config option name.

Marek

Re: [PATCH] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Pali Rohár]

On Sunday 28 August 2022 05:47:51 Marek Behún wrote:
> Can you rename the option to
>   OF_FDT_READONLY_BEFORE_RELOC
> ?

I do not care what is the name of this option. All other options for
this configuration have OF and DTB in its name, not FDT, and that is why
I chose the current name.

> The INITIAL in the name may confuse people.
> 
> The functions are called *_fdt (reloc_fdt, fix_fdt), so maybe put FDT
> instead of DTB in the config option name.
> 
> Marek

[PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

When U-Boot is running from flash memory (execute in place) then
gd->fdt_blob before relocation points to read-only flash memory.

So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
cause immediate CPU crash when callback is trying to modify gd->fdt_blob.

Fix this issue by introducing a new config option
OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
U-Boot before relocation is not running from read/write (S)RAM memory.

This is required for mpc85xx boards when booting from flash NOR.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since v1:
- just changed the new Kconfig option name and associated help string
---
 common/board_f.c | 8 +++++++-
 dts/Kconfig      | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index 18e2246733..35fb4a0753 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -911,7 +911,7 @@ static const init_fnc_t init_sequence_f[] = {
 	 *  - board info struct
 	 */
 	setup_dest_addr,
-#ifdef CONFIG_OF_BOARD_FIXUP
+#if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
 	fix_fdt,
 #endif
 #ifdef CONFIG_PRAM
@@ -926,6 +926,10 @@ static const init_fnc_t init_sequence_f[] = {
 	reserve_board,
 	reserve_global_data,
 	reserve_fdt,
+#if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
+	reloc_fdt,
+	fix_fdt,
+#endif
 	reserve_bootstage,
 	reserve_bloblist,
 	reserve_arch,
@@ -936,7 +940,9 @@ static const init_fnc_t init_sequence_f[] = {
 	setup_bdinfo,
 	display_new_sp,
 	INIT_FUNC_WATCHDOG_RESET
+#if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
 	reloc_fdt,
+#endif
 	reloc_bootstage,
 	reloc_bloblist,
 	setup_reloc,
diff --git a/dts/Kconfig b/dts/Kconfig
index bc5f22029f..29e5248b15 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -105,6 +105,12 @@ config OF_EMBED
 
 endchoice
 
+config OF_DTB_READONLY_BEFORE_RELOC
+	bool "Initial DTB (before relocation) for DT control is read-only"
+	help
+	  If initial (before relocation) DTB for DT control is read-only,
+	  (e.g. points to memory-mapped flash memory), set this option.
+
 config OF_BOARD
 	bool "Provided by the board (e.g a previous loader) at runtime"
 	default y if SANDBOX || OF_HAS_PRIOR_STAGE
-- 
2.35.1

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Pali Rohár]

Simon, could you review this patch? If not, who can do it?

On Sunday 28 August 2022 17:19:29 Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> When U-Boot is running from flash memory (execute in place) then
> gd->fdt_blob before relocation points to read-only flash memory.
> 
> So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
> 
> Fix this issue by introducing a new config option
> OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> U-Boot before relocation is not running from read/write (S)RAM memory.
> 
> This is required for mpc85xx boards when booting from flash NOR.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> Changes since v1:
> - just changed the new Kconfig option name and associated help string
> ---
>  common/board_f.c | 8 +++++++-
>  dts/Kconfig      | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index 18e2246733..35fb4a0753 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -911,7 +911,7 @@ static const init_fnc_t init_sequence_f[] = {
>  	 *  - board info struct
>  	 */
>  	setup_dest_addr,
> -#ifdef CONFIG_OF_BOARD_FIXUP
> +#if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
>  	fix_fdt,
>  #endif
>  #ifdef CONFIG_PRAM
> @@ -926,6 +926,10 @@ static const init_fnc_t init_sequence_f[] = {
>  	reserve_board,
>  	reserve_global_data,
>  	reserve_fdt,
> +#if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
> +	reloc_fdt,
> +	fix_fdt,
> +#endif
>  	reserve_bootstage,
>  	reserve_bloblist,
>  	reserve_arch,
> @@ -936,7 +940,9 @@ static const init_fnc_t init_sequence_f[] = {
>  	setup_bdinfo,
>  	display_new_sp,
>  	INIT_FUNC_WATCHDOG_RESET
> +#if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_DTB_READONLY_BEFORE_RELOC)
>  	reloc_fdt,
> +#endif
>  	reloc_bootstage,
>  	reloc_bloblist,
>  	setup_reloc,
> diff --git a/dts/Kconfig b/dts/Kconfig
> index bc5f22029f..29e5248b15 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -105,6 +105,12 @@ config OF_EMBED
>  
>  endchoice
>  
> +config OF_DTB_READONLY_BEFORE_RELOC
> +	bool "Initial DTB (before relocation) for DT control is read-only"
> +	help
> +	  If initial (before relocation) DTB for DT control is read-only,
> +	  (e.g. points to memory-mapped flash memory), set this option.
> +
>  config OF_BOARD
>  	bool "Provided by the board (e.g a previous loader) at runtime"
>  	default y if SANDBOX || OF_HAS_PRIOR_STAGE
> -- 
> 2.35.1
> 

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Simon Glass]

Hi,

On Sun, 28 Aug 2022 at 09:19, Marek Behún <kabel@kernel.org> wrote:
>
> From: Pali Rohár <pali@kernel.org>
>
> When U-Boot is running from flash memory (execute in place) then
> gd->fdt_blob before relocation points to read-only flash memory.
>
> So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
>
> Fix this issue by introducing a new config option
> OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> U-Boot before relocation is not running from read/write (S)RAM memory.
>
> This is required for mpc85xx boards when booting from flash NOR.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> Changes since v1:
> - just changed the new Kconfig option name and associated help string
> ---
>  common/board_f.c | 8 +++++++-
>  dts/Kconfig      | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)

Can we just make this the normal behaviour? I think it makes more
sense to change the FDT after we have relocated it.

Regards,
Simon

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Pali Rohár]

On Monday 10 October 2022 17:48:58 Simon Glass wrote:
> Hi,
> 
> On Sun, 28 Aug 2022 at 09:19, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Pali Rohár <pali@kernel.org>
> >
> > When U-Boot is running from flash memory (execute in place) then
> > gd->fdt_blob before relocation points to read-only flash memory.
> >
> > So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> > cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
> >
> > Fix this issue by introducing a new config option
> > OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> > reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> > U-Boot before relocation is not running from read/write (S)RAM memory.
> >
> > This is required for mpc85xx boards when booting from flash NOR.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> > Changes since v1:
> > - just changed the new Kconfig option name and associated help string
> > ---
> >  common/board_f.c | 8 +++++++-
> >  dts/Kconfig      | 6 ++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> Can we just make this the normal behaviour? I think it makes more
> sense to change the FDT after we have relocated it.
> 
> Regards,
> Simon

Marek? Any comments?

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Pali Rohár]

+ Tom

On Wednesday 02 November 2022 00:23:03 Pali Rohár wrote:
> On Monday 10 October 2022 17:48:58 Simon Glass wrote:
> > Hi,
> > 
> > On Sun, 28 Aug 2022 at 09:19, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > When U-Boot is running from flash memory (execute in place) then
> > > gd->fdt_blob before relocation points to read-only flash memory.
> > >
> > > So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> > > cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
> > >
> > > Fix this issue by introducing a new config option
> > > OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> > > reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> > > U-Boot before relocation is not running from read/write (S)RAM memory.
> > >
> > > This is required for mpc85xx boards when booting from flash NOR.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---
> > > Changes since v1:
> > > - just changed the new Kconfig option name and associated help string
> > > ---
> > >  common/board_f.c | 8 +++++++-
> > >  dts/Kconfig      | 6 ++++++
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > Can we just make this the normal behaviour? I think it makes more
> > sense to change the FDT after we have relocated it.
> > 
> > Regards,
> > Simon
> 
> Marek? Any comments?

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Tom Rini]

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

On Mon, Nov 21, 2022 at 06:42:01PM +0100, Pali Rohár wrote:
> + Tom
> 
> On Wednesday 02 November 2022 00:23:03 Pali Rohár wrote:
> > On Monday 10 October 2022 17:48:58 Simon Glass wrote:
> > > Hi,
> > > 
> > > On Sun, 28 Aug 2022 at 09:19, Marek Behún <kabel@kernel.org> wrote:
> > > >
> > > > From: Pali Rohár <pali@kernel.org>
> > > >
> > > > When U-Boot is running from flash memory (execute in place) then
> > > > gd->fdt_blob before relocation points to read-only flash memory.
> > > >
> > > > So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> > > > cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
> > > >
> > > > Fix this issue by introducing a new config option
> > > > OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> > > > reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> > > > U-Boot before relocation is not running from read/write (S)RAM memory.
> > > >
> > > > This is required for mpc85xx boards when booting from flash NOR.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > ---
> > > > Changes since v1:
> > > > - just changed the new Kconfig option name and associated help string
> > > > ---
> > > >  common/board_f.c | 8 +++++++-
> > > >  dts/Kconfig      | 6 ++++++
> > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > Can we just make this the normal behaviour? I think it makes more
> > > sense to change the FDT after we have relocated it.

Can we?

-- 
Tom

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

Re: [PATCH v2] board_f: Add support for CONFIG_OF_BOARD_FIXUP for XIP images

[Marek Behún]

On Mon, 21 Nov 2022 12:45:32 -0500
Tom Rini <trini@konsulko.com> wrote:

> On Mon, Nov 21, 2022 at 06:42:01PM +0100, Pali Roh_r wrote:
> > + Tom
> > 
> > On Wednesday 02 November 2022 00:23:03 Pali Roh_r wrote:  
> > > On Monday 10 October 2022 17:48:58 Simon Glass wrote:  
> > > > Hi,
> > > > 
> > > > On Sun, 28 Aug 2022 at 09:19, Marek Beh_n <kabel@kernel.org> wrote:  
> > > > >
> > > > > From: Pali Roh_r <pali@kernel.org>
> > > > >
> > > > > When U-Boot is running from flash memory (execute in place) then
> > > > > gd->fdt_blob before relocation points to read-only flash memory.
> > > > >
> > > > > So U-Boot calls board_fix_fdt() with read-only gd->fdt_blob pointer which
> > > > > cause immediate CPU crash when callback is trying to modify gd->fdt_blob.
> > > > >
> > > > > Fix this issue by introducing a new config option
> > > > > OF_DTB_READONLY_BEFORE_RELOC which moves fix_fdt callback after the
> > > > > reloc_fdt callback. This makes CONFIG_OF_BOARD_FIXUP working also if
> > > > > U-Boot before relocation is not running from read/write (S)RAM memory.
> > > > >
> > > > > This is required for mpc85xx boards when booting from flash NOR.
> > > > >
> > > > > Signed-off-by: Pali Roh_r <pali@kernel.org>
> > > > > Signed-off-by: Marek Beh_n <kabel@kernel.org>
> > > > > ---
> > > > > Changes since v1:
> > > > > - just changed the new Kconfig option name and associated help string
> > > > > ---
> > > > >  common/board_f.c | 8 +++++++-
> > > > >  dts/Kconfig      | 6 ++++++
> > > > >  2 files changed, 13 insertions(+), 1 deletion(-)  
> > > > 
> > > > Can we just make this the normal behaviour? I think it makes more
> > > > sense to change the FDT after we have relocated it.  
> 
> Can we?
> 

Sorry, I didn't notice this thread. Do we know if there aren't boards
that change fdt before reloc?

Marek