Message ID | 1318666015-4535-1-git-send-email-sbabic@denx.de |
---|---|
State | Rejected |
Headers | show |
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,
On 10/15/2011 12:15 PM, Albert ARIBAUD wrote: > Hi Stefano, > Hi Albert, > > 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 Right, it is. > > I conclude like Wolfgang that despite this code having gone through > review unnoticed, it should not be promoted for wider use. Understood. > > 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). I understand your points - and I agree with you, thanks for clarification. Stefano
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))
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/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