From patchwork Sat Jan 1 12:04:17 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dirk Behme X-Patchwork-Id: 77139 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 5CA9DB70D4 for ; Sat, 1 Jan 2011 23:04:59 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 7C60928224; Sat, 1 Jan 2011 13:04:56 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id O9fo3uZFcPt4; Sat, 1 Jan 2011 13:04:56 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id C6A7428225; Sat, 1 Jan 2011 13:04:54 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 2D35B28225 for ; Sat, 1 Jan 2011 13:04:52 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wO6YZFbjy23L for ; Sat, 1 Jan 2011 13:04:50 +0100 (CET) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-bw0-f44.google.com (mail-bw0-f44.google.com [209.85.214.44]) by theia.denx.de (Postfix) with ESMTP id 0853428224 for ; Sat, 1 Jan 2011 13:04:48 +0100 (CET) Received: by bwz12 with SMTP id 12so12223147bwz.3 for ; Sat, 01 Jan 2011 04:04:48 -0800 (PST) Received: by 10.204.4.211 with SMTP id 19mr959029bks.213.1293883488228; Sat, 01 Jan 2011 04:04:48 -0800 (PST) Received: from [192.168.178.23] (p5B047B0F.dip.t-dialin.net [91.4.123.15]) by mx.google.com with ESMTPS id v1sm10399145bkt.17.2011.01.01.04.04.44 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sat, 01 Jan 2011 04:04:45 -0800 (PST) Message-ID: <4D1F1841.5060508@googlemail.com> Date: Sat, 01 Jan 2011 13:04:17 +0100 From: Dirk Behme User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7 MIME-Version: 1.0 To: Alexander Holler References: <1293015862-3678-1-git-send-email-holler@ahsoftware.de> In-Reply-To: <1293015862-3678-1-git-send-email-holler@ahsoftware.de> Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v3] ARM: Avoid compiler optimization for usages of readb, writeb and friends. X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.9 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de On 22.12.2010 12:04, Alexander Holler wrote: > gcc 4.5.1 seems to ignore (at least some) volatile definitions, > avoid that as done in the kernel. > > Reading C99 6.7.3 8 and the comment 114) there, I think it is a bug of that > gcc version to ignore the volatile type qualifier used e.g. in __arch_getl(). > Anyway, using a definition as in the kernel headers avoids such optimizations when > gcc 4.5.1 is used. > > Maybe the headers as used in the current linux-kernel should be used, > but to avoid large changes, I've just added a small change to the current headers. > > I haven't add the definitions which are using a memory barrier because I haven't found > a place in the kernel where they were actually enabled (CONFIG_ARM_DMA_MEM_BUFFERABLE). > > Signed-off-by: Alexander Holler > --- > arch/arm/include/asm/io.h | 20 ++++++++++++++------ > 1 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index ff1518e..068ed17 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -125,13 +125,21 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) > #define __raw_readw(a) __arch_getw(a) > #define __raw_readl(a) __arch_getl(a) > > -#define writeb(v,a) __arch_putb(v,a) > -#define writew(v,a) __arch_putw(v,a) > -#define writel(v,a) __arch_putl(v,a) > +/* > + * TODO: The kernel offers some more advanced versions of barriers, it might > + * have some advantages to use them instead of the simple one here. > + */ > +#define dmb() __asm__ __volatile__ ("" : : : "memory") > +#define __iormb() dmb() > +#define __iowmb() dmb() > + > +#define writeb(v,c) do { __iowmb(); __arch_putb(v,c); } while (0) > +#define writew(v,c) do { __iowmb(); __arch_putw(v,c); } while (0) > +#define writel(v,c) do { __iowmb(); __arch_putl(v,c); } while (0) > > -#define readb(a) __arch_getb(a) > -#define readw(a) __arch_getw(a) > -#define readl(a) __arch_getl(a) > +#define readb(c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) > +#define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > +#define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) Do you like to test the patch in the attachment? I named it 'v4'. After some thinking and testing, it seems to me that the volatile optimization issue this patch shall fix is only with the readx() macros. So the idea is to drop all writex() changes done in the v3 version of this patch. With dropping the writex() changes, we would drop all issues we discussed with e.g. the GCC statement-expression and the do while workaround, too. Thanks Dirk Subject: [PATCH v4] ARM: Avoid compiler optimization for usages of readb and friends. gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel. Reading C99 6.7.3 8 and the comment 114) there, I think it is a bug of that gcc version to ignore the volatile type qualifier used e.g. in __arch_getl(). Anyway, using a definition as in the kernel headers avoids such optimizations when gcc 4.5.1 is used. Maybe the headers as used in the current linux-kernel should be used, but to avoid large changes, I've just added a small change to the current headers. I haven't add the definitions which are using a memory barrier because I haven't found a place in the kernel where they were actually enabled (CONFIG_ARM_DMA_MEM_BUFFERABLE). Signed-off-by: Alexander Holler Signed-off-by: Wolfgang Denk Signed-off-by: Dirk Behme --- Changes since v3: Drop all changes to writex(). It seems that the compiler issue is only with readx(), so we don't have to touch the writex() macros. With not touching the writex() macros, we don't have to care about issues introduced by touching them, too. Note: Tested by compilation only, not tested on real HW. arch/arm/include/asm/io.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) Index: u-boot.git/arch/arm/include/asm/io.h =================================================================== --- u-boot.git.orig/arch/arm/include/asm/io.h +++ u-boot.git/arch/arm/include/asm/io.h @@ -128,10 +128,16 @@ extern inline void __raw_readsl(unsigned #define writeb(v,a) __arch_putb(v,a) #define writew(v,a) __arch_putw(v,a) #define writel(v,a) __arch_putl(v,a) +/* + * TODO: The kernel offers some more advanced versions of barriers, it might + * have some advantages to use them instead of the simple one here. + */ +#define dmb() __asm__ __volatile__ ("" : : : "memory") +#define __iormb() dmb() -#define readb(a) __arch_getb(a) -#define readw(a) __arch_getw(a) -#define readl(a) __arch_getl(a) +#define readb(c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) +#define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) +#define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) /* * The compiler seems to be incapable of optimising constants