[U-Boot] [PATCH] Using I/O accessor instead of volatile pointers in SMC911x driver
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"
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
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@denx.de Dear Lord: I just want *one* one-armed manager so I never have to hear "On the other hand", again.
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
participants (4)
-
Ben Warren -
Matthias Weisser -
Mike Frysinger -
Wolfgang Denk