Patchwork [U-Boot] ARM: Convert {in,out}s[bwl] to inline functions

login
register
mail settings
Submitter Marek Vasut
Date Sept. 26, 2011, 6:48 p.m.
Message ID <1317062895-3847-1-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/116465/
State Rejected
Headers show

Comments

Marek Vasut - Sept. 26, 2011, 6:48 p.m.
The size of uboot binary grows by a few bytes, but the gain (better type
checking) is worth it.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Nick Thompson <nick.thompson@ge.com>
Cc: Simon Glass <sjg@chromium.org>
---
 arch/arm/include/asm/io.h |   34 ++++++++++++++++++++++++++++------
 1 files changed, 28 insertions(+), 6 deletions(-)
Nick Thompson - Sept. 27, 2011, 8:59 a.m.
On 26/09/11 19:48, Marek Vasut wrote:
> The size of uboot binary grows by a few bytes, but the gain (better type
> checking) is worth it.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Nick Thompson <nick.thompson@ge.com>
> Cc: Simon Glass <sjg@chromium.org>

Do you want to cc: Albert ARIBAUD <albert.u.boot@aribaud.net> as well?

> ---
>  arch/arm/include/asm/io.h |   34 ++++++++++++++++++++++++++++------
>  1 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index 61f4987..d22325d 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -255,13 +255,35 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)
>  #define inw(p)	({ unsigned int __v = le16_to_cpu(__raw_readw(__io(p))); __v; })
>  #define inl(p)	({ unsigned int __v = le32_to_cpu(__raw_readl(__io(p))); __v; })
>  
> -#define outsb(p,d,l)			__raw_writesb(__io(p),d,l)
> -#define outsw(p,d,l)			__raw_writesw(__io(p),d,l)
> -#define outsl(p,d,l)			__raw_writesl(__io(p),d,l)

These changes are clearly related, but we started out talking about '__arch_putb',
which is in the same file of course. Did I miss something?

This specific patch looks reasonable to me though.

Reviewed-by: Nick Thompson <nick.thompson@ge.com>

> +extern inline void outsb(unsigned int addr, const void *data, int bytelen)
> +{
> +	__raw_writesb(addr, data, bytelen);
> +}
> +
> +extern inline void outsw(unsigned int addr, const void *data, int wordlen)
> +{
> +	__raw_writesw(addr, data, wordlen);
> +}
> +
> +extern inline void outsl(unsigned int addr, const void *data, int longlen)
> +{
> +	__raw_writesl(addr, data, longlen);
> +}
>  
> -#define insb(p,d,l)			__raw_readsb(__io(p),d,l)
> -#define insw(p,d,l)			__raw_readsw(__io(p),d,l)
> -#define insl(p,d,l)			__raw_readsl(__io(p),d,l)
> +extern inline void insb(unsigned int addr, void *data, int bytelen)
> +{
> +	__raw_readsb(addr, data, bytelen);
> +}
> +
> +extern inline void insw(unsigned int addr, void *data, int wordlen)
> +{
> +	__raw_readsw(addr, data, wordlen);
> +}
> +
> +extern inline void insl(unsigned int addr, void *data, int longlen)
> +{
> +	__raw_readsl(addr, data, longlen);
> +}
>  #endif
>  
>  #define outb_p(val,port)		outb((val),(port))
Wolfgang Denk - Sept. 27, 2011, 9:31 a.m.
Dear Marek Vasut,

In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you wrote:
> The size of uboot binary grows by a few bytes, but the gain (better type
> checking) is worth it.

And what _exactly_ are "a few bytes" ?

Best regards,

Wolfgang Denk
Wolfgang Denk - Sept. 27, 2011, 9:32 a.m.
Dear Marek Vasut,

In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you wrote:
> The size of uboot binary grows by a few bytes, but the gain (better type
> checking) is worth it.
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Nick Thompson <nick.thompson@ge.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  arch/arm/include/asm/io.h |   34 ++++++++++++++++++++++++++++------
>  1 files changed, 28 insertions(+), 6 deletions(-)

And will you PLEASE get used to sticking with the rules?

There is no patch version in the Subject line.

There is no change log in the comment section either.


NAK.



Wolfgang Denk
Marek Vasut - Sept. 27, 2011, 10:08 a.m.
On Tuesday, September 27, 2011 11:32:38 AM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you wrote:
> > The size of uboot binary grows by a few bytes, but the gain (better type
> > checking) is worth it.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Nick Thompson <nick.thompson@ge.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> > 
> >  arch/arm/include/asm/io.h |   34 ++++++++++++++++++++++++++++------
> >  1 files changed, 28 insertions(+), 6 deletions(-)
> 
> And will you PLEASE get used to sticking with the rules?
> 
> There is no patch version in the Subject line.
> 
> There is no change log in the comment section either.

This is a new patch ... that's why there's no changelog and no V1.

Cheers

> 
> 
> NAK.
> 
> 
> 
> Wolfgang Denk
Marek Vasut - Sept. 27, 2011, 10:21 a.m.
On Tuesday, September 27, 2011 11:31:15 AM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you wrote:
> > The size of uboot binary grows by a few bytes, but the gain (better type
> > checking) is worth it.
> 
> And what _exactly_ are "a few bytes" ?

Nevermind, it must have been some kind of a fluctuation yesterday. Right now, I 
made a new measurement and the size didn't change with/without the patch (this 
is more what I'd expect to happen).

Cheers

> 
> Best regards,
> 
> Wolfgang Denk
Nick Thompson - Sept. 27, 2011, 11:57 a.m.
On 27/09/11 11:21, Marek Vasut wrote:
> On Tuesday, September 27, 2011 11:31:15 AM Wolfgang Denk wrote:
>> Dear Marek Vasut,
>>
>> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you wrote:
>>> The size of uboot binary grows by a few bytes, but the gain (better type
>>> checking) is worth it.
>> And what _exactly_ are "a few bytes" ?
> Nevermind, it must have been some kind of a fluctuation yesterday. Right now, I 
> made a new measurement and the size didn't change with/without the patch (this 
> is more what I'd expect to happen).
>
> Cheers

Pure speculation on my part, but /could/ this be because ARM drivers don't tend
to use these macros/functions. write[bwl] and the like are much more common. I
don't know this to be a fact though.

Nick.
Marek Vasut - Sept. 27, 2011, 12:02 p.m.
On Tuesday, September 27, 2011 01:57:52 PM Nick Thompson wrote:
> On 27/09/11 11:21, Marek Vasut wrote:
> > On Tuesday, September 27, 2011 11:31:15 AM Wolfgang Denk wrote:
> >> Dear Marek Vasut,
> >> 
> >> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you 
wrote:
> >>> The size of uboot binary grows by a few bytes, but the gain (better
> >>> type checking) is worth it.
> >> 
> >> And what _exactly_ are "a few bytes" ?
> > 
> > Nevermind, it must have been some kind of a fluctuation yesterday. Right
> > now, I made a new measurement and the size didn't change with/without
> > the patch (this is more what I'd expect to happen).
> > 
> > Cheers
> 
> Pure speculation on my part, but /could/ this be because ARM drivers don't
> tend to use these macros/functions. write[bwl] and the like are much more
> common. I don't know this to be a fact though.

No, I'm dead sure I use this macro in the test.

> 
> Nick.
Simon Glass - Sept. 27, 2011, 10:40 p.m.
On Tue, Sep 27, 2011 at 5:02 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Tuesday, September 27, 2011 01:57:52 PM Nick Thompson wrote:
>> On 27/09/11 11:21, Marek Vasut wrote:
>> > On Tuesday, September 27, 2011 11:31:15 AM Wolfgang Denk wrote:
>> >> Dear Marek Vasut,
>> >>
>> >> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com> you
> wrote:
>> >>> The size of uboot binary grows by a few bytes, but the gain (better
>> >>> type checking) is worth it.
>> >>
>> >> And what _exactly_ are "a few bytes" ?
>> >
>> > Nevermind, it must have been some kind of a fluctuation yesterday. Right
>> > now, I made a new measurement and the size didn't change with/without
>> > the patch (this is more what I'd expect to happen).
>> >
>> > Cheers
>>
>> Pure speculation on my part, but /could/ this be because ARM drivers don't
>> tend to use these macros/functions. write[bwl] and the like are much more
>> common. I don't know this to be a fact though.
>
> No, I'm dead sure I use this macro in the test.
>
>>
>> Nick.
>

Hi,

Can't comment on the patch format, etc.

I tested this on my Seaboard, with no code size increase, and all
worked as expected. I can't see why it would increase code size
either.

But I have a few questions: what devices actually uses this macro?
Otherwise I'm not sure if I am testing anything. Also, why not convert
all the macros in this file? Seems like a good idea to me. Or is this
patch just to test the waters? :-)

Regards,
Simon
Marek Vasut - Sept. 28, 2011, 10:56 a.m.
On Wednesday, September 28, 2011 12:40:01 AM Simon Glass wrote:
> On Tue, Sep 27, 2011 at 5:02 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Tuesday, September 27, 2011 01:57:52 PM Nick Thompson wrote:
> >> On 27/09/11 11:21, Marek Vasut wrote:
> >> > On Tuesday, September 27, 2011 11:31:15 AM Wolfgang Denk wrote:
> >> >> Dear Marek Vasut,
> >> >> 
> >> >> In message <1317062895-3847-1-git-send-email-marek.vasut@gmail.com>
> >> >> you
> > 
> > wrote:
> >> >>> The size of uboot binary grows by a few bytes, but the gain (better
> >> >>> type checking) is worth it.
> >> >> 
> >> >> And what _exactly_ are "a few bytes" ?
> >> > 
> >> > Nevermind, it must have been some kind of a fluctuation yesterday.
> >> > Right now, I made a new measurement and the size didn't change
> >> > with/without the patch (this is more what I'd expect to happen).
> >> > 
> >> > Cheers
> >> 
> >> Pure speculation on my part, but /could/ this be because ARM drivers
> >> don't tend to use these macros/functions. write[bwl] and the like are
> >> much more common. I don't know this to be a fact though.
> > 
> > No, I'm dead sure I use this macro in the test.
> > 
> >> Nick.
> 
> Hi,
> 
> Can't comment on the patch format, etc.
> 
> I tested this on my Seaboard, with no code size increase, and all
> worked as expected. I can't see why it would increase code size
> either.
> 
> But I have a few questions: what devices actually uses this macro?

common/cmd_ide.c for example.

> Otherwise I'm not sure if I am testing anything. Also, why not convert
> all the macros in this file? Seems like a good idea to me. Or is this
> patch just to test the waters? :-)

We should eventually get rid of all that crap altogether and unify the hardware 
access. But that seems like a long-term plan :-(

Cheers
> 
> Regards,
> Simon
Wolfgang Denk - Sept. 28, 2011, 8:46 p.m.
Dear Marek Vasut,

In message <201109271208.09363.marek.vasut@gmail.com> you wrote:
>
> > And will you PLEASE get used to sticking with the rules?
> > 
> > There is no patch version in the Subject line.
> > 
> > There is no change log in the comment section either.
> 
> This is a new patch ... that's why there's no changelog and no V1.

Oh, is it?

And what is this, then:

09/26 Marek Vasut        [PATCH] ARM: Convert {in,out}s[bwl] to inline functions

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/109509


Looks to be exactly the same, judging by the Subject: line...

Best regards,

Wolfgang Denk
Marek Vasut - Sept. 28, 2011, 8:58 p.m.
On Wednesday, September 28, 2011 10:46:57 PM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109271208.09363.marek.vasut@gmail.com> you wrote:
> > > And will you PLEASE get used to sticking with the rules?
> > > 
> > > There is no patch version in the Subject line.
> > > 
> > > There is no change log in the comment section either.
> > 
> > This is a new patch ... that's why there's no changelog and no V1.
> 
> Oh, is it?
> 
> And what is this, then:
> 
> 09/26 Marek Vasut        [PATCH] ARM: Convert {in,out}s[bwl] to inline
> functions
> 
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/109509
> 
> 
> Looks to be exactly the same, judging by the Subject: line...

Well ... that's this patch, right ?

Cheers
Wolfgang Denk - Sept. 28, 2011, 9:15 p.m.
Dear Marek Vasut,

In message <201109282258.10007.marek.vasut@gmail.com> you wrote:
>
> > > This is a new patch ... that's why there's no changelog and no V1.
> > 
> > Oh, is it?
> > 
> > And what is this, then:
> > 
> > 09/26 Marek Vasut        [PATCH] ARM: Convert {in,out}s[bwl] to inline
> > functions
> > 
> > http://article.gmane.org/gmane.comp.boot-loaders.u-boot/109509
> > 
> > 
> > Looks to be exactly the same, judging by the Subject: line...
> 
> Well ... that's this patch, right ?

Oops.  You are right.   Sorry.



Best regards,

Wolfgang Denk

Patch

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 61f4987..d22325d 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -255,13 +255,35 @@  extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)
 #define inw(p)	({ unsigned int __v = le16_to_cpu(__raw_readw(__io(p))); __v; })
 #define inl(p)	({ unsigned int __v = le32_to_cpu(__raw_readl(__io(p))); __v; })
 
-#define outsb(p,d,l)			__raw_writesb(__io(p),d,l)
-#define outsw(p,d,l)			__raw_writesw(__io(p),d,l)
-#define outsl(p,d,l)			__raw_writesl(__io(p),d,l)
+extern inline void outsb(unsigned int addr, const void *data, int bytelen)
+{
+	__raw_writesb(addr, data, bytelen);
+}
+
+extern inline void outsw(unsigned int addr, const void *data, int wordlen)
+{
+	__raw_writesw(addr, data, wordlen);
+}
+
+extern inline void outsl(unsigned int addr, const void *data, int longlen)
+{
+	__raw_writesl(addr, data, longlen);
+}
 
-#define insb(p,d,l)			__raw_readsb(__io(p),d,l)
-#define insw(p,d,l)			__raw_readsw(__io(p),d,l)
-#define insl(p,d,l)			__raw_readsl(__io(p),d,l)
+extern inline void insb(unsigned int addr, void *data, int bytelen)
+{
+	__raw_readsb(addr, data, bytelen);
+}
+
+extern inline void insw(unsigned int addr, void *data, int wordlen)
+{
+	__raw_readsw(addr, data, wordlen);
+}
+
+extern inline void insl(unsigned int addr, void *data, int longlen)
+{
+	__raw_readsl(addr, data, longlen);
+}
 #endif
 
 #define outb_p(val,port)		outb((val),(port))