From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Warren <biggerbadderben@gmail.com>
Date: Tue, 29 Apr 2008 07:43:10 -0700
Subject: [U-Boot-Users] [PATCH] net: add 16 bit support for smc911x
In-Reply-To: <20080429100913.23049.90329.stgit@tq-sewsrv-4.tq-net.de>
References: <20080429100913.23049.90329.stgit@tq-sewsrv-4.tq-net.de>
Message-ID: <481733FE.2070502@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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