diff mbox

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

Message ID 1482143465-14584-2-git-send-email-yamada.masahiro@socionext.com
State Accepted
Commit 085be482f6c62614ddbe51000a09db9dec360e90
Headers show

Commit Message

Masahiro Yamada Dec. 19, 2016, 10:31 a.m. UTC 
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 <yamada.masahiro@socionext.com>

---

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(-)

-- 
2.7.4

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Comments

Fabio Estevam Dec. 19, 2016, 11:31 a.m. UTC | #1 
On Mon, Dec 19, 2016 at 8:31 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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 <yamada.masahiro@socionext.com>


Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Tom Rini Dec. 19, 2016, 9:59 p.m. UTC | #2 
On Mon, Dec 19, 2016 at 07:31:02PM +0900, Masahiro Yamada wrote:
> 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 <yamada.masahiro@socionext.com>


Ah, oops, thanks for spotting that one.

> ---

> 

> 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.

-- 
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Masahiro Yamada Dec. 20, 2016, 4:39 a.m. UTC | #3 
2016-12-20 6:59 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Mon, Dec 19, 2016 at 07:31:02PM +0900, Masahiro Yamada wrote:

>> 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 <yamada.masahiro@socionext.com>

>

> Ah, oops, thanks for spotting that one.

>

>> ---

>>

>> 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.



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






-- 
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Tom Rini Dec. 20, 2016, 1:39 p.m. UTC | #4 
On Tue, Dec 20, 2016 at 01:39:40PM +0900, Masahiro Yamada wrote:
> 2016-12-20 6:59 GMT+09:00 Tom Rini <trini@konsulko.com>:

> > On Mon, Dec 19, 2016 at 07:31:02PM +0900, Masahiro Yamada wrote:

> >> 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 <yamada.masahiro@socionext.com>

> >

> > Ah, oops, thanks for spotting that one.

> >

> >> ---

> >>

> >> 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.

> 

> 

> 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.


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.

-- 
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Tom Rini Dec. 27, 2016, 11:03 p.m. UTC | #5 
On Mon, Dec 19, 2016 at 07:31:02PM +0900, Masahiro Yamada wrote:

> 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 <yamada.masahiro@socionext.com>

> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>


Applied to u-boot/master, thanks!

-- 
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
diff mbox

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 587f288..6820479 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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
diff --git a/arch/arm/include/asm/string.h b/arch/arm/include/asm/string.h
index c6dfb25..11eaa34 100644
--- a/arch/arm/include/asm/string.h
+++ b/arch/arm/include/asm/string.h
@@ -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);