[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver

[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver

[Matthias Weisser]

Volatile pointer usage caused lockup with arm-gcc 4.3.2
Using I/O accessor fixed that.

Signed-off-by: Matthias Weisser <matthias.weisser@graf-syteco.de>
---
 drivers/net/smc911x.h |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/smc911x.h b/drivers/net/smc911x.h
index 80d2ce0..877b905 100644
--- a/drivers/net/smc911x.h
+++ b/drivers/net/smc911x.h
@@ -26,6 +26,7 @@
 #define _SMC911X_H_
 
 #include <linux/types.h>
+#include <asm/io.h>
 
 #if defined (CONFIG_DRIVER_SMC911X_32_BIT) && \
 	defined (CONFIG_DRIVER_SMC911X_16_BIT)
@@ -36,25 +37,29 @@
 #if defined (CONFIG_DRIVER_SMC911X_32_BIT)
 static inline u32 __smc911x_reg_read(u32 addr)
 {
-	return *(volatile u32*)addr;
+	return readl(addr);
 }
 u32 smc911x_reg_read(u32 addr) __attribute__((weak, alias("__smc911x_reg_read")));
 
 static inline void __smc911x_reg_write(u32 addr, u32 val)
 {
-	*(volatile u32*)addr = val;
+	writel(val, addr);
 }
 void smc911x_reg_write(u32 addr, u32 val) __attribute__((weak, alias("__smc911x_reg_write")));
 #elif defined (CONFIG_DRIVER_SMC911X_16_BIT)
 static inline u32 smc911x_reg_read(u32 addr)
 {
-	volatile u16 *addr_16 = (u16 *)addr;
-	return ((*addr_16 & 0x0000ffff) | (*(addr_16 + 1) << 16));
+	u32 res;
+
+	res = readw(addr) & 0xFFFF;
+	res |= ((u32)(readw(addr + 2) & 0xFFFF) << 16);
+
+	return res;
 }
 static inline void smc911x_reg_write(u32 addr, u32 val)
 {
-	*(volatile u16*)addr = (u16)val;
-	*(volatile u16*)(addr + 2) = (u16)(val >> 16);
+	writew(val & 0xFFFF, addr);
+	writew(val >> 16, addr + 2);
 }
 #else
 #error "SMC911X: undefined bus width"
-- 
1.5.6.3

[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver

[Mike Frysinger]

On Wednesday 22 July 2009 04:14:33 Matthias Weisser wrote:
>  static inline u32 smc911x_reg_read(u32 addr)
>  {
> -	volatile u16 *addr_16 = (u16 *)addr;
> -	return ((*addr_16 & 0x0000ffff) | (*(addr_16 + 1) << 16));
> +	u32 res;
> +
> +	res = readw(addr) & 0xFFFF;
> +	res |= ((u32)(readw(addr + 2) & 0xFFFF) << 16);
> +
> +	return res;
>  }
>  static inline void smc911x_reg_write(u32 addr, u32 val)
>  {
> -	*(volatile u16*)addr = (u16)val;
> -	*(volatile u16*)(addr + 2) = (u16)(val >> 16);
> +	writew(val & 0xFFFF, addr);
> +	writew(val >> 16, addr + 2);
>  }

i think your masking and casting are unnecessary.  you're dealing with 
unsigned values which means there wont be sign extension.  the code would look 
a lot simpler without it:
static inline u32 smc911x_reg_read(u32 addr)
{
	return readw(addr) | (readw(addr + 2) << 16);
}
static inline void smc911x_reg_write(u32 addr, u32 val)
{
	writew(val, addr);
	writew(val >> 16, addr + 2);
}
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090722/0f6545c9/attachment.pgp 

[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver

[Wolfgang Denk]

Dear Matthias Weisser,

In message <1248250473-12694-1-git-send-email-matthias.weisser@graf-syteco.de> you wrote:
> Volatile pointer usage caused lockup with arm-gcc 4.3.2
> Using I/O accessor fixed that.

Hm...

> -	return *(volatile u32*)addr;
> +	return readl(addr);

On big-endian systems like PowerPC, readl() is a byte-swapping
(little-endian) input function, which means that your patch changes
the byte order of all I/O operations.

Is this intentional? Has your patch been tested on both BE and LE
systems?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Dear Lord: I just want *one* one-armed manager so  I  never  have  to
hear "On the other hand", again.

[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver

[Ben Warren]

Wolfgang Denk wrote:
> Dear Matthias Weisser,
>
> In message <1248250473-12694-1-git-send-email-matthias.weisser@graf-syteco.de> you wrote:
>   
>> Volatile pointer usage caused lockup with arm-gcc 4.3.2
>> Using I/O accessor fixed that.
>>     
>
> Hm...
>
>   
>> -	return *(volatile u32*)addr;
>> +	return readl(addr);
>>     
>
> On big-endian systems like PowerPC, readl() is a byte-swapping
> (little-endian) input function, which means that your patch changes
> the byte order of all I/O operations.
>
>   
Yeah, that's why I left these alone when re-working this driver.  I 
don't know what the correct approach is when using memory-mapped devices 
that could in theory go on the local bus of any processor.  Most people 
I know would hook them up according to the native endianness.

regards,
Ben