Skip to contentSkip to conversation

ARM: fix the Kconfig misconversion of CONFIG_USE_ARCH_MEMCPY/MEMSET

PATCH New Masahiro Yamada 3 participants 15 messages Dec 19, 2016 – Dec 29, 2016 Arm (32-bit)

lore
Share & tools
lore

Masahiro Yamada

author
#0 lore raw

cover ARM: fix the Kconfig misconversion of CONFIG_USE_ARCH_MEMCPY/MEMSET

Masahiro Yamada (3):
ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra common/init: remove meaningless defined(CONFIG_USE_ARCH_MEMSET)
README: remove description about CONFIG_USE_ARCH_MEMCPY/SET

 README                        | 6 ------
 arch/arm/Kconfig              | 4 ++--
 arch/arm/include/asm/string.h | 4 ++--
 common/init/board_init.c      | 4 +---
 4 files changed, 5 insertions(+), 13 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>

---

I am restoring the original behavior for now.
But, I have been wondering if we could remove these options entirely.

As you see scripts/config_whitelist.txt, we still have 8000 options. As I stated before, it is a crazy idea to move all of them as they are.

We should refactor the code, and we should delete options if they turned out unneeded.

I think CONFIG_USE_ARCH_MEMSET/MEMCPY are questionable ones.

U-Boot supports arch-optimized memcpy/memset for ARC, ARM (only 32bit), Blackfin, PowerPC, and x86.

Strange enough, only ARM 32bit uses CONFIG_USE_ARCH_MEMSET/MEMCPY to make it user-configurable.
(Other architectures always turn on the optimized variants if supported)

Even more strangely, those two options enable/disable ARM-specific memcpy/memset for the U-Boot full image. (Please note they are never compiled for SPL)

The U-Boot full image generally does not have strong memory footprint constraint. So, if we talk about the image size, it shouldn't hurt to enable ARM-optimized memcpy/set all the time.

 arch/arm/Kconfig              | 4 ++--
 arch/arm/include/asm/string.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/Kconfig +2 -2
index 587f288..6820479 100644
@@ -129,7 +129,7 @@ config ENABLE_ARM_SOC_BOOT0_HOOK
 config USE_ARCH_MEMCPY
 	bool "Use an assembly optimized implementation of memcpy"
 	default y if CPU_V7
-	depends on !ARM64 && !SPL
+	depends on !ARM64
 	help
 	  Enable the generation of an optimized version of memcpy.
 	  Such implementation may be faster under some conditions

@@ -138,7 +138,7 @@ config USE_ARCH_MEMCPY
 config USE_ARCH_MEMSET
 	bool "Use an assembly optimized implementation of memset"
 	default y if CPU_V7
-	depends on !ARM64 && !SPL
+	depends on !ARM64
 	help
 	  Enable the generation of an optimized version of memset.
 	  Such implementation may be faster under some conditions
arch/arm/include/asm/string.h +2 -2
index c6dfb25..11eaa34 100644
@@ -14,7 +14,7 @@ extern char * strrchr(const char * s, int c);
 #undef __HAVE_ARCH_STRCHR
 extern char * strchr(const char * s, int c);
 
-#ifdef CONFIG_USE_ARCH_MEMCPY
+#if CONFIG_IS_ENABLED(USE_ARCH_MEMCPY)
 #define __HAVE_ARCH_MEMCPY
 #endif
 extern void * memcpy(void *, const void *, __kernel_size_t);

@@ -26,7 +26,7 @@ extern void * memmove(void *, const void *, __kernel_size_t);
 extern void * memchr(const void *, int, __kernel_size_t);
 
 #undef __HAVE_ARCH_MEMZERO
-#ifdef CONFIG_USE_ARCH_MEMSET
+#if CONFIG_IS_ENABLED(USE_ARCH_MEMSET)
 #define __HAVE_ARCH_MEMSET
 #endif
 extern void * memset(void *, int, __kernel_size_t);
Signature · 1 line
2.7.4
#2 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

Quoted 17 lines · Masahiro Yamada

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Quoted 7 lines

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Fabio Estevam <[email protected]>
#3 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

In reply to Masahiro Yamada
Quoted 17 lines · Masahiro Yamada

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Quoted 7 lines

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>

Ah, oops, thanks for spotting that one.

Quoted 4 lines

---

I am restoring the original behavior for now.
But, I have been wondering if we could remove these options entirely.

We cannot. That was my first attempt and we have a handful of active (I checked) boards with tiny enough SPL constraints that switching to the optimized memcpy/memset push them over size limit and they do not have a "something" to disable to gain the space back. So I went with asking for asking for a conversion to enable by default these options as widely as possible as it's a good thing by and (no pun intended) large.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161219/ade17d95/attachment.sig>
author
#4 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

2016-12-20 6:59 GMT+09:00 Tom Rini <[email protected]>:

Quoted 33 lines · Masahiro Yamada
Quoted 17 lines

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Quoted 7 lines

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>

Ah, oops, thanks for spotting that one.

Quoted 4 lines

---

I am restoring the original behavior for now.
But, I have been wondering if we could remove these options entirely.

Quoted 6 lines

We cannot. That was my first attempt and we have a handful of active (I checked) boards with tiny enough SPL constraints that switching to the optimized memcpy/memset push them over size limit and they do not have a "something" to disable to gain the space back. So I went with asking for asking for a conversion to enable by default these options as widely as possible as it's a good thing by and (no pun intended) large.

Perhaps, I may be missing something, but I could not understand why you were talking about SPL size constraints.

As far as I understood arch/arm/lib/Makefile,
arch/arm/lib/memset.o is never compiled for SPL
in the first place.

I believe CONFIG_USE_ARCH_MEMSET has no impact to SPL.

ifndef CONFIG_SPL_BUILD
ifdef CONFIG_ARM64
obj-y += relocate_64.o
else
obj-y += relocate.o
endif

obj-$(CONFIG_CPU_V7M) += cmd_boot.o
obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
obj-$(CONFIG_CMD_BOOTI) += bootm.o
obj-$(CONFIG_CMD_BOOTM) += bootm.o
obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
obj-$(CONFIG_SYS_L2_PL310) += cache-pl310.o
obj-$(CONFIG_USE_ARCH_MEMSET) += memset.o
obj-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
else
obj-$(CONFIG_SPL_FRAMEWORK) += spl.o
obj-$(CONFIG_SPL_FRAMEWORK) += zimage.o
endif

Signature · 2 lines
Best Regards
Masahiro Yamada
#5 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

Quoted 46 lines · Masahiro Yamada

2016-12-20 6:59 GMT+09:00 Tom Rini <[email protected]>:

Quoted 33 lines · Masahiro Yamada
Quoted 17 lines

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Quoted 7 lines

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>

Ah, oops, thanks for spotting that one.

Quoted 4 lines

---

I am restoring the original behavior for now.
But, I have been wondering if we could remove these options entirely.

Quoted 6 lines

We cannot. That was my first attempt and we have a handful of active (I checked) boards with tiny enough SPL constraints that switching to the optimized memcpy/memset push them over size limit and they do not have a "something" to disable to gain the space back. So I went with asking for asking for a conversion to enable by default these options as widely as possible as it's a good thing by and (no pun intended) large.

Perhaps, I may be missing something, but I could not understand why you were talking about SPL size constraints.

Quoted 5 lines

As far as I understood arch/arm/lib/Makefile,
arch/arm/lib/memset.o is never compiled for SPL
in the first place.

I believe CONFIG_USE_ARCH_MEMSET has no impact to SPL.

Because, blarg, oversight. We want these available to SPL because it fixes problems (due to non-optimized memset being so slow) on some platforms and otherwise is a speed win. I had been testing changes to move this all globally over, and found the size problems there.

But... at this point, no, I shouldn't also pull in making these functions be in SPL as I had intended, I should be good and correct that part in v2017.03. And take your Kconfig fix as-is, as it's correct.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161220/d4b47053/attachment.sig>
#6 lore raw

PATCH 1/3 ARM: revive CONFIG_USE_ARCH_MEMCPY/MEMSET for UniPhier and Tegra

In reply to Masahiro Yamada
Quoted 17 lines · Masahiro Yamada

Commit be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig") is misconversion.

The original logic in include/configs/uniphier.h was as follows:

  #if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_ARM64)
  #define CONFIG_USE_ARCH_MEMSET
  #define CONFIG_USE_ARCH_MEMCPY
  #endif

This means those configs were enabled when building U-Boot proper, but disabled when building SPL. Likewise for Tegra.

Now "depends on !SPL" prevents any boards with SPL support from reaching these options. This changed the behavior for UniPhier and Tegra SoC family.

Quoted 8 lines

Please notice these two options only control the U-Boot proper build. As you see arch/arm/Makefile, ARM-specific memset/memcpy are never compiled for SPL. So, __HAVE_ARCH_MEMCPY/MEMSET should not set for SPL.

Fixes be72591bcd64 ("Kconfig: Move USE_ARCH_MEMCPY/MEMSET to Kconfig")
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Fabio Estevam <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161227/8ded5198/attachment.sig>
author
#7 lore raw

PATCH 2/3 common/init: remove meaningless defined(CONFIG_USE_ARCH_MEMSET)

In reply to Masahiro Yamada

CONFIG_USE_ARCH_MEMSET controls nothing about SPL. (it is effective only on U-Boot proper building of ARM).

Signed-off-by Masahiro Yamada <[email protected]>

---

 common/init/board_init.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
common/init/board_init.c +1 -3
index ef01a9a..a2edb4e 100644
@@ -15,9 +15,7 @@ DECLARE_GLOBAL_DATA_PTR;
  * It isn't trivial to figure out whether memcpy() exists. The arch-specific
  * memcpy() is not normally available in SPL due to code size.
  */
-#if !defined(CONFIG_SPL_BUILD) || \
-		(defined(CONFIG_SPL_LIBGENERIC_SUPPORT) && \
-		!defined(CONFIG_USE_ARCH_MEMSET))
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBGENERIC_SUPPORT)
 #define _USE_MEMCPY
 #endif
Signature · 1 line
2.7.4
#9 lore raw

PATCH 2/3 common/init: remove meaningless defined(CONFIG_USE_ARCH_MEMSET)

In reply to Masahiro Yamada
Quoted 2 lines · Masahiro Yamada

CONFIG_USE_ARCH_MEMSET controls nothing about SPL. (it is effective only on U-Boot proper building of ARM).

That's not true. We have these functions available to SPL and use them there by default now (as it's a speed win and we want out of SPL ASAP).

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161219/b674cdcc/attachment.sig>
author
#10 lore raw

PATCH 2/3 common/init: remove meaningless defined(CONFIG_USE_ARCH_MEMSET)

Hi Tom.

2016-12-20 7:02 GMT+09:00 Tom Rini <[email protected]>:

Quoted 8 lines · Masahiro Yamada
Quoted 3 lines

CONFIG_USE_ARCH_MEMSET controls nothing about SPL. (it is effective only on U-Boot proper building of ARM).

That's not true. We have these functions available to SPL and use them there by default now (as it's a speed win and we want out of SPL ASAP).

This is not clear to me.

How can you make the optimized memset available to SPL?

As far as see arch/arm/lib/Makefile,
memset.o is only compiled (if CONFIG_USE_ARCH_MEMSET is defined) for the U-Boot full image.

ifndef CONFIG_SPL_BUILD
ifdef CONFIG_ARM64
obj-y += relocate_64.o
else
obj-y += relocate.o
endif

obj-$(CONFIG_CPU_V7M) += cmd_boot.o
obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
obj-$(CONFIG_CMD_BOOTI) += bootm.o
obj-$(CONFIG_CMD_BOOTM) += bootm.o
obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
obj-$(CONFIG_SYS_L2_PL310) += cache-pl310.o
obj-$(CONFIG_USE_ARCH_MEMSET) += memset.o
obj-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
else
obj-$(CONFIG_SPL_FRAMEWORK) += spl.o
obj-$(CONFIG_SPL_FRAMEWORK) += zimage.o
endif

Signature · 2 lines
Best Regards
Masahiro Yamada
author
#11 lore raw

PATCH 3/3 README: remove description about CONFIG_USE_ARCH_MEMCPY/SET

In reply to Masahiro Yamada

These options are now described in the Kconfig help. We do not want to maintain duplicated documentation.

Signed-off-by Masahiro Yamada <[email protected]>

---

 README | 6 ------
 1 file changed, 6 deletions(-)
README +0 -6
index 25cad2f..0bd6e6c 100644
@@ -4593,12 +4593,6 @@ Low Level (hardware related) configuration options:
 		addressable memory. This option causes some memory accesses
 		to be mapped through map_sysmem() / unmap_sysmem().
 
-- CONFIG_USE_ARCH_MEMCPY
-  CONFIG_USE_ARCH_MEMSET
-		If these options are used a optimized version of memcpy/memset will
-		be used if available. These functions may be faster under some
-		conditions but may increase the binary size.
-
 - CONFIG_X86_RESET_VECTOR
 		If defined, the x86 reset vector code is included. This is not
 		needed when U-Boot is running from Coreboot.
Signature · 1 line
2.7.4
#13 lore raw

PATCH 3/3 README: remove description about CONFIG_USE_ARCH_MEMCPY/SET

In reply to Masahiro Yamada
Quoted 4 lines · Masahiro Yamada

These options are now described in the Kconfig help. We do not want to maintain duplicated documentation.

Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161219/4e57e5a0/attachment.sig>
#14 lore raw

PATCH 3/3 README: remove description about CONFIG_USE_ARCH_MEMCPY/SET

In reply to Masahiro Yamada
Quoted 6 lines · Masahiro Yamada

These options are now described in the Kconfig help. We do not want to maintain duplicated documentation.

Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Fabio Estevam <[email protected]>
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161229/a3b280e5/attachment.sig>