| Message ID | 1317002798-27112-1-git-send-email-marek.vasut@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 26/09/11 03:06, Marek Vasut wrote: > Signed-off-by: Marek Vasut <marek.vasut@gmail.com> > --- > arch/arm/include/asm/io.h | 30 ++++++++++++++++++------------ > 1 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index 1fbc531..61f4987 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr) > extern inline void __raw_writesb(unsigned int addr, const void *data, int bytelen) > { > uint8_t *buf = (uint8_t *)data; > - while(bytelen--) > - __arch_putb(*buf++, addr); > + int i; > + for (i = 0; i < bytelen; i++) > + __arch_putb(buf[i], addr); > } > This fixes the problem in these use cases, but leaves the door open. Would it be better to change the __arch_putb macro into an extern inline function instead which would catch these and future cases? Nick.
On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote: > On 26/09/11 03:06, Marek Vasut wrote: > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com> > > --- > > > > arch/arm/include/asm/io.h | 30 ++++++++++++++++++------------ > > 1 files changed, 18 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > > index 1fbc531..61f4987 100644 > > --- a/arch/arm/include/asm/io.h > > +++ b/arch/arm/include/asm/io.h > > @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr) > > > > extern inline void __raw_writesb(unsigned int addr, const void *data, > > int bytelen) { > > > > uint8_t *buf = (uint8_t *)data; > > > > - while(bytelen--) > > - __arch_putb(*buf++, addr); > > + int i; > > + for (i = 0; i < bytelen; i++) > > + __arch_putb(buf[i], addr); > > > > } > > This fixes the problem in these use cases, but leaves the door open. > > Would it be better to change the __arch_putb macro into an extern inline > function instead which would catch these and future cases? Yes, but you'll need to do that on a much larger scale. Is anyone up for doing it ? > > Nick.
On 26/09/11 10:32, Marek Vasut wrote: > On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote: >> On 26/09/11 03:06, Marek Vasut wrote: >>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com> >>> --- >>> >>> arch/arm/include/asm/io.h | 30 ++++++++++++++++++------------ >>> 1 files changed, 18 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h >>> index 1fbc531..61f4987 100644 >>> --- a/arch/arm/include/asm/io.h >>> +++ b/arch/arm/include/asm/io.h >>> @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr) >>> >>> extern inline void __raw_writesb(unsigned int addr, const void *data, >>> int bytelen) { >>> >>> uint8_t *buf = (uint8_t *)data; >>> >>> - while(bytelen--) >>> - __arch_putb(*buf++, addr); >>> + int i; >>> + for (i = 0; i < bytelen; i++) >>> + __arch_putb(buf[i], addr); >>> >>> } >> This fixes the problem in these use cases, but leaves the door open. >> >> Would it be better to change the __arch_putb macro into an extern inline >> function instead which would catch these and future cases? > Yes, but you'll need to do that on a much larger scale. Is anyone up for doing > it ? > I don't follow that. I found only three (identical) definitions in arm, sparc and sh. In those three cases __raw_writeb were also (identical) macro 'aliases' for __arch_putb. I guess you are referring to the testing required for all the boards in those three arches, or even just arm, with changes to all the (get|set)(b|w|l) cases? Maybe I see your point now... Nick.
On Monday, September 26, 2011 11:44:22 AM Nick Thompson wrote: > On 26/09/11 10:32, Marek Vasut wrote: > > On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote: > >> On 26/09/11 03:06, Marek Vasut wrote: > >>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com> > >>> --- > >>> > >>> arch/arm/include/asm/io.h | 30 ++++++++++++++++++------------ > >>> 1 files changed, 18 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > >>> index 1fbc531..61f4987 100644 > >>> --- a/arch/arm/include/asm/io.h > >>> +++ b/arch/arm/include/asm/io.h > >>> @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * > >>> vaddr) > >>> > >>> extern inline void __raw_writesb(unsigned int addr, const void *data, > >>> int bytelen) { > >>> > >>> uint8_t *buf = (uint8_t *)data; > >>> > >>> - while(bytelen--) > >>> - __arch_putb(*buf++, addr); > >>> + int i; > >>> + for (i = 0; i < bytelen; i++) > >>> + __arch_putb(buf[i], addr); > >>> > >>> } > >> > >> This fixes the problem in these use cases, but leaves the door open. > >> > >> Would it be better to change the __arch_putb macro into an extern inline > >> function instead which would catch these and future cases? > > > > Yes, but you'll need to do that on a much larger scale. Is anyone up for > > doing it ? > > I don't follow that. I found only three (identical) definitions in arm, > sparc and sh. In those three cases __raw_writeb were also (identical) > macro 'aliases' for __arch_putb. Oh if it's only three arches then that's fine. > > I guess you are referring to the testing required for all the boards in > those three arches, or even just arm, with changes to all the > (get|set)(b|w|l) cases? Maybe I see your point now... > > Nick.
Dear Marek Vasut, In message <201109261132.43616.marek.vasut@gmail.com> you wrote: > > > Would it be better to change the __arch_putb macro into an extern inline > > function instead which would catch these and future cases? > > Yes, but you'll need to do that on a much larger scale. Is anyone up for doing > it ? What exactly are the advantages, and what the disadvantages? code size / memory footprint? Best regards, Wolfgang Denk
On Monday, September 26, 2011 01:30:41 PM Wolfgang Denk wrote: > Dear Marek Vasut, > > In message <201109261132.43616.marek.vasut@gmail.com> you wrote: > > > Would it be better to change the __arch_putb macro into an extern > > > inline function instead which would catch these and future cases? > > > > Yes, but you'll need to do that on a much larger scale. Is anyone up for > > doing it ? > > What exactly are the advantages, and what the disadvantages? > > code size / memory footprint? I assume converting those to inline function will yield no memory usage growth at all. > > Best regards, > > Wolfgang Denk
Dear Marek Vasut, In message <201109261937.01263.marek.vasut@gmail.com> you wrote: > > > What exactly are the advantages, and what the disadvantages? > > > > code size / memory footprint? > > I assume converting those to inline function will yield no memory usage growth > at all. Please don't assume, measure it. Thanks. Best regards, Wolfgang Denk
On Monday, September 26, 2011 08:33:58 PM Wolfgang Denk wrote: > Dear Marek Vasut, > > In message <201109261937.01263.marek.vasut@gmail.com> you wrote: > > > What exactly are the advantages, and what the disadvantages? > > > > > > code size / memory footprint? > > > > I assume converting those to inline function will yield no memory usage > > growth at all. > > Please don't assume, measure it. Thanks. Will do ... though I'd prefer to have this patch and the other one separate. Cheers > > Best regards, > > Wolfgang Denk
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 1fbc531..61f4987 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr) extern inline void __raw_writesb(unsigned int addr, const void *data, int bytelen) { uint8_t *buf = (uint8_t *)data; - while(bytelen--) - __arch_putb(*buf++, addr); + int i; + for (i = 0; i < bytelen; i++) + __arch_putb(buf[i], addr); } extern inline void __raw_writesw(unsigned int addr, const void *data, int wordlen) { uint16_t *buf = (uint16_t *)data; - while(wordlen--) - __arch_putw(*buf++, addr); + int i; + for (i = 0; i < wordlen; i++) + __arch_putw(buf[i], addr); } extern inline void __raw_writesl(unsigned int addr, const void *data, int longlen) { uint32_t *buf = (uint32_t *)data; - while(longlen--) - __arch_putl(*buf++, addr); + int i; + for (i = 0; i < longlen; i++) + __arch_putl(buf[i], addr); } extern inline void __raw_readsb(unsigned int addr, void *data, int bytelen) { uint8_t *buf = (uint8_t *)data; - while(bytelen--) - *buf++ = __arch_getb(addr); + int i; + for (i = 0; i < bytelen; i++) + buf[i] = __arch_getb(addr); } extern inline void __raw_readsw(unsigned int addr, void *data, int wordlen) { uint16_t *buf = (uint16_t *)data; - while(wordlen--) - *buf++ = __arch_getw(addr); + int i; + for (i = 0; i < wordlen; i++) + buf[i] = __arch_getw(addr); } extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) { uint32_t *buf = (uint32_t *)data; - while(longlen--) - *buf++ = __arch_getl(addr); + int i; + for (i = 0; i < longlen; i++) + buf[i] = __arch_getl(addr); } #define __raw_writeb(v,a) __arch_putb(v,a)
Signed-off-by: Marek Vasut <marek.vasut@gmail.com> --- arch/arm/include/asm/io.h | 30 ++++++++++++++++++------------ 1 files changed, 18 insertions(+), 12 deletions(-)