Hi Jens,

Jens Gehrlein wrote:

Hi Ben, Magnus,

here is my patch to enable 2x16 bit accesses to the LAN9x1x. I still not have a HW, so may I ask you to test it and, if applicable, fix the code?

Best Regards, Jens

---

drivers/net/smc911x.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c index d22c889..841a64d 100644 --- a/drivers/net/smc911x.c +++ b/drivers/net/smc911x.c @@ -30,6 +30,8 @@ #include <net.h> #include <miiphy.h>

+#define CONFIG_DRIVER_SMC911X_16_BIT 1

#ifdef CONFIG_DRIVER_SMC911X_32_BIT static inline u32 reg_read(u32 addr) { @@ -39,8 +41,21 @@ static inline void reg_write(u32 addr, u32 val) { *(volatile u32*)addr = val; } +#elif CONFIG_DRIVER_SMC911X_16_BIT +static inline u32 reg_read(u32 addr) +{

• u32 val_lo = (u32)(*(volatile u16*)addr);
• u32 val_hi = (u32)(*(volatile u16*)(addr | 2));

This won't work. Presumably addr will already be a multiple of 2, so the bit-wise OR will make val_lo and val_hi identical. Here's how I'd do it:

volatile u16 *addr_16 = (u16 *)addr; return ((*addr_16 & 0x0000ffff) | (*(addr_16 + 1) << 16));

Of course, this could be done as a 1-liner with castings, but it becomes even more unreadable that way. The mask here might be unnecessary too.

• return (val_hi << 16) | val_lo;

+} +static inline void reg_write(u32 addr, u32 val) +{

• *(volatile u16*)addr = (u16)val;
• *(volatile u16*)(addr | 2) = (u16)(val >> 16);

Same problem here with the bitwise OR. Second line should be: *(volatile u16*)(addr + 2) = (u16)(val >> 16);

+} #else -#error "SMC911X: Only 32-bit bus is supported" +#error "SMC911X: undefined buswidth" #endif

#define mdelay(n) udelay((n)*1000)

Thanks for doing this! Hopefully Magnus will get a chance to try on real hardware and we can put this issue to rest.

regards, Ben