[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[Masahiro Yamada]

Commit e2f88dfd2d96 ("libfdt: Introduce new ARCH_FIXUP_FDT option")
allows us to skip memory setup of DTB, but a problem for ARM is that
spin_table_update_dt() and psci_update_dt() are skipped as well if
CONFIG_ARCH_FIXUP_FDT is disabled.

This commit allows us to skip only fdt_fixup_memory_banks() instead
of the whole of arch_fixup_fdt().  It will be useful when we want to
use a memory node from a kernel DTB as is, but need some fixups for
Spin-Table/PSCI.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Kconfig                  | 5 ++---
 arch/arm/lib/bootm-fdt.c | 2 --
 arch/arm/lib/bootm.c     | 2 --
 arch/mips/lib/bootm.c    | 2 --
 common/fdt_support.c     | 2 ++
 common/image-fdt.c       | 7 +++++--
 include/fdt_support.h    | 8 ++++++++
 7 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/Kconfig b/Kconfig
index 1263d0b..b7cb142 100644
--- a/Kconfig
+++ b/Kconfig
@@ -324,9 +324,8 @@ config SYS_CLK_FREQ
 	help
 	  TODO: Move CONFIG_SYS_CLK_FREQ for all the architecture
 
-config ARCH_FIXUP_FDT
-	bool "Enable arch_fixup_fdt() call"
-	depends on ARM || MIPS
+config ARCH_FIXUP_FDT_MEMORY
+	bool "Enable arch_fixup_memory_banks() call"
 	default y
 	help
 	  Enable FDT memory map syncup before OS boot. This feature can be
diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
index a517550..4481f9e 100644
--- a/arch/arm/lib/bootm-fdt.c
+++ b/arch/arm/lib/bootm-fdt.c
@@ -25,7 +25,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#ifdef CONFIG_ARCH_FIXUP_FDT
 int arch_fixup_fdt(void *blob)
 {
 	bd_t *bd = gd->bd;
@@ -61,4 +60,3 @@ int arch_fixup_fdt(void *blob)
 
 	return 0;
 }
-#endif
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 53c3141..0e890ce 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -372,10 +372,8 @@ void boot_prep_vxworks(bootm_headers_t *images)
 	if (images->ft_addr) {
 		off = fdt_path_offset(images->ft_addr, "/memory");
 		if (off < 0) {
-#ifdef CONFIG_ARCH_FIXUP_FDT
 			if (arch_fixup_fdt(images->ft_addr))
 				puts("## WARNING: fixup memory failed!\n");
-#endif
 		}
 	}
 #endif
diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 0c6a4ab..aa0475a 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -253,7 +253,6 @@ static int boot_reloc_fdt(bootm_headers_t *images)
 #endif
 }
 
-#ifdef CONFIG_ARCH_FIXUP_FDT
 int arch_fixup_fdt(void *blob)
 {
 #if CONFIG_IS_ENABLED(MIPS_BOOT_FDT) && CONFIG_IS_ENABLED(OF_LIBFDT)
@@ -265,7 +264,6 @@ int arch_fixup_fdt(void *blob)
 	return 0;
 #endif
 }
-#endif
 
 static int boot_setup_fdt(bootm_headers_t *images)
 {
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 2020586..c87031f 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -381,6 +381,7 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
 	do_fixup_by_compat(fdt, compat, prop, &tmp, 4, create);
 }
 
+#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
 /*
  * fdt_pack_reg - pack address and size array into the "reg"-suitable stream
  */
@@ -459,6 +460,7 @@ int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks)
 	}
 	return 0;
 }
+#endif
 
 int fdt_fixup_memory(void *blob, u64 start, u64 size)
 {
diff --git a/common/image-fdt.c b/common/image-fdt.c
index 3d23608..91970d4 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -458,6 +458,11 @@ __weak int ft_verify_fdt(void *fdt)
 	return 1;
 }
 
+__weak int arch_fixup_fdt(void *blob)
+{
+	return 0;
+}
+
 int image_setup_libfdt(bootm_headers_t *images, void *blob,
 		       int of_size, struct lmb *lmb)
 {
@@ -474,12 +479,10 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
 		printf("ERROR: /chosen node create failed\n");
 		goto err;
 	}
-#ifdef CONFIG_ARCH_FIXUP_FDT
 	if (arch_fixup_fdt(blob) < 0) {
 		printf("ERROR: arch-specific fdt fixup failed\n");
 		goto err;
 	}
-#endif
 	if (IMAGE_OF_BOARD_SETUP) {
 		fdt_ret = ft_board_setup(blob, gd->bd);
 		if (fdt_ret) {
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 8f40231..7110061 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -93,7 +93,15 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size);
  *			property will be left untouched.
  * @return 0 if ok, or -1 or -FDT_ERR_... on error
  */
+#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks);
+#else
+static inline int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[],
+					 int banks)
+{
+	return 0;
+}
+#endif
 
 void fdt_fixup_ethernet(void *fdt);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
-- 
1.9.1

[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[Simon Glass]

Hi Masahiro,

On 4 October 2016 at 06:03, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Commit e2f88dfd2d96 ("libfdt: Introduce new ARCH_FIXUP_FDT option")
> allows us to skip memory setup of DTB, but a problem for ARM is that
> spin_table_update_dt() and psci_update_dt() are skipped as well if
> CONFIG_ARCH_FIXUP_FDT is disabled.
>
> This commit allows us to skip only fdt_fixup_memory_banks() instead
> of the whole of arch_fixup_fdt().  It will be useful when we want to
> use a memory node from a kernel DTB as is, but need some fixups for
> Spin-Table/PSCI.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  Kconfig                  | 5 ++---
>  arch/arm/lib/bootm-fdt.c | 2 --
>  arch/arm/lib/bootm.c     | 2 --
>  arch/mips/lib/bootm.c    | 2 --
>  common/fdt_support.c     | 2 ++
>  common/image-fdt.c       | 7 +++++--
>  include/fdt_support.h    | 8 ++++++++
>  7 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/Kconfig b/Kconfig
> index 1263d0b..b7cb142 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -324,9 +324,8 @@ config SYS_CLK_FREQ
>         help
>           TODO: Move CONFIG_SYS_CLK_FREQ for all the architecture
>
> -config ARCH_FIXUP_FDT
> -       bool "Enable arch_fixup_fdt() call"
> -       depends on ARM || MIPS
> +config ARCH_FIXUP_FDT_MEMORY
> +       bool "Enable arch_fixup_memory_banks() call"
>         default y
>         help
>           Enable FDT memory map syncup before OS boot. This feature can be
> diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
> index a517550..4481f9e 100644
> --- a/arch/arm/lib/bootm-fdt.c
> +++ b/arch/arm/lib/bootm-fdt.c
> @@ -25,7 +25,6 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> -#ifdef CONFIG_ARCH_FIXUP_FDT
>  int arch_fixup_fdt(void *blob)
>  {
>         bd_t *bd = gd->bd;
> @@ -61,4 +60,3 @@ int arch_fixup_fdt(void *blob)
>
>         return 0;
>  }
> -#endif
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 53c3141..0e890ce 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -372,10 +372,8 @@ void boot_prep_vxworks(bootm_headers_t *images)
>         if (images->ft_addr) {
>                 off = fdt_path_offset(images->ft_addr, "/memory");
>                 if (off < 0) {
> -#ifdef CONFIG_ARCH_FIXUP_FDT
>                         if (arch_fixup_fdt(images->ft_addr))
>                                 puts("## WARNING: fixup memory failed!\n");
> -#endif
>                 }
>         }
>  #endif
> diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
> index 0c6a4ab..aa0475a 100644
> --- a/arch/mips/lib/bootm.c
> +++ b/arch/mips/lib/bootm.c
> @@ -253,7 +253,6 @@ static int boot_reloc_fdt(bootm_headers_t *images)
>  #endif
>  }
>
> -#ifdef CONFIG_ARCH_FIXUP_FDT
>  int arch_fixup_fdt(void *blob)
>  {
>  #if CONFIG_IS_ENABLED(MIPS_BOOT_FDT) && CONFIG_IS_ENABLED(OF_LIBFDT)
> @@ -265,7 +264,6 @@ int arch_fixup_fdt(void *blob)
>         return 0;
>  #endif
>  }
> -#endif
>
>  static int boot_setup_fdt(bootm_headers_t *images)
>  {
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 2020586..c87031f 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -381,6 +381,7 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
>         do_fixup_by_compat(fdt, compat, prop, &tmp, 4, create);
>  }
>
> +#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
>  /*
>   * fdt_pack_reg - pack address and size array into the "reg"-suitable stream
>   */
> @@ -459,6 +460,7 @@ int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks)
>         }
>         return 0;
>  }
> +#endif
>
>  int fdt_fixup_memory(void *blob, u64 start, u64 size)
>  {
> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index 3d23608..91970d4 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -458,6 +458,11 @@ __weak int ft_verify_fdt(void *fdt)
>         return 1;
>  }
>
> +__weak int arch_fixup_fdt(void *blob)
> +{
> +       return 0;
> +}

Do we have to have a weak function? I was hoping we could avoid these
since they make it hard to figure out at build time what code is
executed.

> +
>  int image_setup_libfdt(bootm_headers_t *images, void *blob,
>                        int of_size, struct lmb *lmb)
>  {
> @@ -474,12 +479,10 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
>                 printf("ERROR: /chosen node create failed\n");
>                 goto err;
>         }
> -#ifdef CONFIG_ARCH_FIXUP_FDT
>         if (arch_fixup_fdt(blob) < 0) {
>                 printf("ERROR: arch-specific fdt fixup failed\n");
>                 goto err;
>         }
> -#endif
>         if (IMAGE_OF_BOARD_SETUP) {
>                 fdt_ret = ft_board_setup(blob, gd->bd);
>                 if (fdt_ret) {
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 8f40231..7110061 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -93,7 +93,15 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size);
>   *                     property will be left untouched.
>   * @return 0 if ok, or -1 or -FDT_ERR_... on error
>   */
> +#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
>  int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks);
> +#else
> +static inline int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[],
> +                                        int banks)
> +{
> +       return 0;
> +}
> +#endif
>
>  void fdt_fixup_ethernet(void *fdt);
>  int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[Masahiro Yamada]

Hi Simon,

2016-10-05 0:37 GMT+09:00 Simon Glass <sjg@chromium.org>:

>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>> index 3d23608..91970d4 100644
>> --- a/common/image-fdt.c
>> +++ b/common/image-fdt.c
>> @@ -458,6 +458,11 @@ __weak int ft_verify_fdt(void *fdt)
>>         return 1;
>>  }
>>
>> +__weak int arch_fixup_fdt(void *blob)
>> +{
>> +       return 0;
>> +}
>
> Do we have to have a weak function? I was hoping we could avoid these
> since they make it hard to figure out at build time what code is
> executed.
>


This hunk is just reverting Michal's commit e2f88dfd2d9671.

Is it better to add an empty stub to every architecture that may call it?




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[Simon Glass]

Hi Masahiro,

On 4 October 2016 at 21:27, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Simon,
>
> 2016-10-05 0:37 GMT+09:00 Simon Glass <sjg@chromium.org>:
>
>>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>>> index 3d23608..91970d4 100644
>>> --- a/common/image-fdt.c
>>> +++ b/common/image-fdt.c
>>> @@ -458,6 +458,11 @@ __weak int ft_verify_fdt(void *fdt)
>>>         return 1;
>>>  }
>>>
>>> +__weak int arch_fixup_fdt(void *blob)
>>> +{
>>> +       return 0;
>>> +}
>>
>> Do we have to have a weak function? I was hoping we could avoid these
>> since they make it hard to figure out at build time what code is
>> executed.
>>
>
>
> This hunk is just reverting Michal's commit e2f88dfd2d9671.
>
> Is it better to add an empty stub to every architecture that may call it?

IMO all the FDT fixups need work. Perhaps we need a linker list
approach so we can declare these fixups more easily? Or perhaps that
will just make things harder to figure out?

But in this case I'm keen to avoid going back to using a weak
function. Can we do that?

Regards,
Simon

[U-Boot] [PATCH] libfdt: replace ARCH_FIXUP_FDT with ARCH_FIXUP_FDT_MEMORY

[Masahiro Yamada]

Hi Simon,


2016-10-06 1:09 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 4 October 2016 at 21:27, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> Hi Simon,
>>
>> 2016-10-05 0:37 GMT+09:00 Simon Glass <sjg@chromium.org>:
>>
>>>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>>>> index 3d23608..91970d4 100644
>>>> --- a/common/image-fdt.c
>>>> +++ b/common/image-fdt.c
>>>> @@ -458,6 +458,11 @@ __weak int ft_verify_fdt(void *fdt)
>>>>         return 1;
>>>>  }
>>>>
>>>> +__weak int arch_fixup_fdt(void *blob)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>
>>> Do we have to have a weak function? I was hoping we could avoid these
>>> since they make it hard to figure out at build time what code is
>>> executed.
>>>
>>
>>
>> This hunk is just reverting Michal's commit e2f88dfd2d9671.
>>
>> Is it better to add an empty stub to every architecture that may call it?
>
> IMO all the FDT fixups need work. Perhaps we need a linker list
> approach so we can declare these fixups more easily? Or perhaps that
> will just make things harder to figure out?

This is up to you.


My interest is to not touch memory node,
but need other DT fixups.

I sent v2 with no-op stubs instead of the weak function.



-- 
Best Regards
Masahiro Yamada