From mboxrd@z Thu Jan  1 00:00:00 1970
From: Albert ARIBAUD <albert.u.boot@aribaud.net>
Date: Sat, 15 Oct 2011 12:15:24 +0200
Subject: [U-Boot] [PATCH V2 5/8] ARM: moved general function to arm/lib
In-Reply-To: <1318666015-4535-1-git-send-email-sbabic@denx.de>
References: <1316166383-11314-1-git-send-email-sbabic@denx.de>
	<1318666015-4535-1-git-send-email-sbabic@denx.de>
Message-ID: <4E995D3C.7020806@aribaud.net>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Stefano,

Le 15/10/2011 10:06, Stefano Babic a ?crit :
> Functions inside armv7/syslib can be used by other ARM
> architectures, too. The file is added as part of
> ARM library.
>
> Signed-off-by: Stefano Babic<sbabic@denx.de>
> CC: Albert ARIBAUD<albert.u.boot@aribaud.net>
> CC: Wolfgang Denk<wd@denx.de>
> ---
>
> Changes:
>
> - use -C in git format-patch to detect renames
>
>   arch/arm/cpu/armv7/Makefile          |    2 --
>   arch/arm/lib/Makefile                |    2 ++
>   arch/arm/{cpu/armv7 =>  lib}/syslib.c |    0
>   3 files changed, 2 insertions(+), 2 deletions(-)
>   rename arch/arm/{cpu/armv7 =>  lib}/syslib.c (100%)
>
> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> index 92a5a96..5b7f1c7 100644
> --- a/arch/arm/cpu/armv7/Makefile
> +++ b/arch/arm/cpu/armv7/Makefile
> @@ -32,8 +32,6 @@ COBJS	+= cache_v7.o
>   COBJS	+= cpu.o
>   endif
>
> -COBJS  += syslib.o
> -
>   SRCS	:= $(START:.o=.S) $(COBJS:.o=.c)
>   OBJS	:= $(addprefix $(obj),$(COBJS))
>   START	:= $(addprefix $(obj),$(START))
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 300c8fa..c966aa7 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -48,6 +48,8 @@ SOBJS-$(CONFIG_USE_ARCH_MEMSET) += memset.o
>   SOBJS-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
>   endif
>
> +COBJS-y	+= syslib.o
> +
>   SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
>   	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
>   OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
> diff --git a/arch/arm/cpu/armv7/syslib.c b/arch/arm/lib/syslib.c
> similarity index 100%
> rename from arch/arm/cpu/armv7/syslib.c
> rename to arch/arm/lib/syslib.c

I agree with Wofgang that sdelay() is redundant wrt udelay() and has 
weaker semantics.

I'll add that sr32() is kind of not ARM specific, so I fail to see why 
it should move to generic ARM, and besides, it is a half-baked solution 
to the general problem of setting a bitfield in a register -- half-baked 
in that it only applies to bits. Besides, There's been a long discussion 
on this list about similar-minded helper macros, and the conclusion was 
that keeping the operation explicitly as logical combinations of 
bitmasks was the best way to let the reader know what is going on 
without forcing them to look up a macro or function definition.

As for wait-on_value(), it is based on the same weak semantics as 
sdelay(), i.e. a number of loops, the duration of which depends on many 
factors and is thus undependable. Besides, if the field changes value 
but is not the one expected, it wastes cycles waiting for the expected 
one. This is inefficient especially of the field is of the form "one 
value for "ok", many others for various failure cases", where even once 
a failure is indicated, the code still loops waiting for an "ok" that 
will never come.

I conclude like Wolfgang that despite this code having gone through 
review unnoticed, it should not be promoted for wider use.

If some functionality of these three functions is desirable and cannot 
be met with other means, please submit new code for general ARM 
inclusion (and maybe consider adapting armv7 to this new code).

Amicalement,
-- 
Albert.