| Message ID | 1301291335-13734-2-git-send-email-leiwen@marvell.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Lei Wen, In message <1301291335-13734-2-git-send-email-leiwen@marvell.com> you wrote: > Those api take use of read*/write* to align the current dmb usage. > Also this could short the code length in one line. > > Signed-off-by: Lei Wen <leiwen@marvell.com> > --- > arch/arm/include/asm/io.h | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index 1fbc531..71e85e8 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -141,6 +141,14 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > > +#define orb(v,c) writeb(readb(c) | v, c) > +#define orw(v,c) writew(readw(c) | v, c) > +#define orl(v,c) writel(readl(c) | v, c) > + > +#define andb(v,c) writeb(readb(c) & v, c) > +#define andw(v,c) writew(readw(c) & v, c) > +#define andl(v,c) writel(readl(c) & v, c) checkpatch gixes errors for all of these lines: ERROR: space required after that ',' (ctx:VxV) #72: FILE: arch/arm/include/asm/io.h:144: +#define orb(v,c) writeb(readb(c) | v, c) ^ etc. Please fix. Best regards, Wolfgang Denk
Hi Wolfgang, On Mon, Mar 28, 2011 at 1:57 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <1301291335-13734-2-git-send-email-leiwen@marvell.com> you wrote: >> Those api take use of read*/write* to align the current dmb usage. >> Also this could short the code length in one line. >> >> Signed-off-by: Lei Wen <leiwen@marvell.com> >> --- >> arch/arm/include/asm/io.h | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h >> index 1fbc531..71e85e8 100644 >> --- a/arch/arm/include/asm/io.h >> +++ b/arch/arm/include/asm/io.h >> @@ -141,6 +141,14 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) >> #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) >> #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) >> >> +#define orb(v,c) writeb(readb(c) | v, c) >> +#define orw(v,c) writew(readw(c) | v, c) >> +#define orl(v,c) writel(readl(c) | v, c) >> + >> +#define andb(v,c) writeb(readb(c) & v, c) >> +#define andw(v,c) writew(readw(c) & v, c) >> +#define andl(v,c) writel(readl(c) & v, c) > > checkpatch gixes errors for all of these lines: > > ERROR: space required after that ',' (ctx:VxV) > #72: FILE: arch/arm/include/asm/io.h:144: > +#define orb(v,c) writeb(readb(c) | v, c) > ^ > > etc. > > Please fix. Sorry for this... patch to come... Best regards, Lei
On Sun, 27 Mar 2011 22:48:50 -0700 Lei Wen <leiwen@marvell.com> wrote: > Those api take use of read*/write* to align the current dmb usage. > Also this could short the code length in one line. > > Signed-off-by: Lei Wen <leiwen@marvell.com> > --- > arch/arm/include/asm/io.h | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index 1fbc531..71e85e8 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -141,6 +141,14 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > > +#define orb(v,c) writeb(readb(c) | v, c) > +#define orw(v,c) writew(readw(c) | v, c) > +#define orl(v,c) writel(readl(c) | v, c) > + > +#define andb(v,c) writeb(readb(c) & v, c) > +#define andw(v,c) writew(readw(c) & v, c) > +#define andl(v,c) writel(readl(c) & v, c) > + > /* > * The compiler seems to be incapable of optimising constants > * properly. Spell it out to the compiler in some cases. What does this do that setbits*/clrbits* don't? Other than be missing parentheses around "v", causing problems if a complex expression is passed in. -scott
Hi Scott, On Tue, Mar 29, 2011 at 12:05 AM, Scott Wood <scottwood@freescale.com> wrote: > On Sun, 27 Mar 2011 22:48:50 -0700 > Lei Wen <leiwen@marvell.com> wrote: > >> Those api take use of read*/write* to align the current dmb usage. >> Also this could short the code length in one line. >> >> Signed-off-by: Lei Wen <leiwen@marvell.com> >> --- >> arch/arm/include/asm/io.h | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h >> index 1fbc531..71e85e8 100644 >> --- a/arch/arm/include/asm/io.h >> +++ b/arch/arm/include/asm/io.h >> @@ -141,6 +141,14 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) >> #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) >> #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) >> >> +#define orb(v,c) writeb(readb(c) | v, c) >> +#define orw(v,c) writew(readw(c) | v, c) >> +#define orl(v,c) writel(readl(c) | v, c) >> + >> +#define andb(v,c) writeb(readb(c) & v, c) >> +#define andw(v,c) writew(readw(c) & v, c) >> +#define andl(v,c) writel(readl(c) & v, c) >> + >> /* >> * The compiler seems to be incapable of optimising constants >> * properly. Spell it out to the compiler in some cases. > > What does this do that setbits*/clrbits* don't? Those and*/or* include the dmb() operation included in the read*/write*, which is not included in the __raw_read*/__raw_write* that setbits*/clrbits* refer to. I think it's better to keep another instance to set the bit, since there is read* and __raw_read* exist and have difference. > > Other than be missing parentheses aro und "v", causing problems if a > complex expression is passed in. Yep, that make sense. Thanks for pionting it out, I would make a update for this patch. Best regards, Lei
On Tue, 29 Mar 2011 10:47:04 +0800 Lei Wen <adrian.wenl@gmail.com> wrote: > Hi Scott, > > On Tue, Mar 29, 2011 at 12:05 AM, Scott Wood <scottwood@freescale.com> wrote: > > What does this do that setbits*/clrbits* don't? > > Those and*/or* include the dmb() operation included in the read*/write*, which > is not included in the __raw_read*/__raw_write* that setbits*/clrbits* refer to. > I think it's better to keep another instance to set the bit, since > there is read* and __raw_read* > exist and have difference. But why are setbits/clrbits using raw accesses? That's not how they're defined on powerpc, which is where these accessors originated. I suspect they were defined that way out of laziness from before ARM made a distinction between raw and non-raw accessors, and it is now a bug in setbits/clrbits (and out_be*, in_le*, etc) which should be fixed rather than introducing an alternative. And then if you want a raw version of these functions, introduce raw_setbits_le32, raw_in_le16, etc. -Scott
Hi Scott, On Wed, Mar 30, 2011 at 12:03 AM, Scott Wood <scottwood@freescale.com> wrote: > On Tue, 29 Mar 2011 10:47:04 +0800 > Lei Wen <adrian.wenl@gmail.com> wrote: > >> Hi Scott, >> >> On Tue, Mar 29, 2011 at 12:05 AM, Scott Wood <scottwood@freescale.com> wrote: >> > What does this do that setbits*/clrbits* don't? >> >> Those and*/or* include the dmb() operation included in the read*/write*, which >> is not included in the __raw_read*/__raw_write* that setbits*/clrbits* refer to. >> I think it's better to keep another instance to set the bit, since >> there is read* and __raw_read* >> exist and have difference. > > But why are setbits/clrbits using raw accesses? That's not how they're > defined on powerpc, which is where these accessors originated. I suspect > they were defined that way out of laziness from before ARM made a > distinction between raw and non-raw accessors, and it is now a bug in > setbits/clrbits (and out_be*, in_le*, etc) which should be fixed rather > than introducing an alternative. > > And then if you want a raw version of these functions, introduce > raw_setbits_le32, raw_in_le16, etc. > Yep, that make sense to me. But since this patch is tend to be more discussion. I'm going to remove this change out of this patch set, and post seperately. Meanwhile, just use the writel(readl* style in this patch set for current stage. Best regards, Lei
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 1fbc531..71e85e8 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -141,6 +141,14 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) +#define orb(v,c) writeb(readb(c) | v, c) +#define orw(v,c) writew(readw(c) | v, c) +#define orl(v,c) writel(readl(c) | v, c) + +#define andb(v,c) writeb(readb(c) & v, c) +#define andw(v,c) writew(readw(c) & v, c) +#define andl(v,c) writel(readl(c) & v, c) + /* * The compiler seems to be incapable of optimising constants * properly. Spell it out to the compiler in some cases.
Those api take use of read*/write* to align the current dmb usage. Also this could short the code length in one line. Signed-off-by: Lei Wen <leiwen@marvell.com> --- arch/arm/include/asm/io.h | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)