[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Jens Gehrlein]

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));
+
+	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);
+}
 #else
-#error "SMC911X: Only 32-bit bus is supported"
+#error "SMC911X: undefined buswidth"
 #endif
 
 #define mdelay(n)       udelay((n)*1000)

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Ben Warren]

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

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Magnus Lilja]

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


I'll try it later this week, definitely not today though.


/Magnus

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Ben Warren]

Magnus Lilja wrote:
>>  Thanks for doing this!  Hopefully Magnus will get a chance to try on
>>  real hardware and we can put this issue to rest.
>>     
>
>
> I'll try it later this week, definitely not today though.
>
>
> /Magnus
>
>   
Cool!  Thanks Magnus.

regards,
Ben

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Guennadi Liakhovetski]

On Tue, 29 Apr 2008, 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
> +

Looks like an obvious thing - you'll remove this define from the patch for 
final submission, right?

Thanks
Guennadi
---
Guennadi Liakhovetski

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Jens Gehrlein]

Guennadi Liakhovetski schrieb:
> On Tue, 29 Apr 2008, 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
>> +
> 
> Looks like an obvious thing - you'll remove this define from the patch for 
> final submission, right?

Oh, ah. You're right. I inserted this to do a compile test and to 
inspect the generated assembler code. This define should be later moved 
to the board specific header file.

Sorry.

Well spotted. Thank you.

Magnus, are you going to fix the code as proposed by Ben and Guennadi or 
  should I resubmit the patch?

Best Regards,
Jens

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Ben Warren]

Hi Jens,

Jens Gehrlein wrote:
> Guennadi Liakhovetski schrieb:
>   
>> On Tue, 29 Apr 2008, 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
>>> +
>>>       
>> Looks like an obvious thing - you'll remove this define from the patch for 
>> final submission, right?
>>     
>
> Oh, ah. You're right. I inserted this to do a compile test and to 
> inspect the generated assembler code. This define should be later moved 
> to the board specific header file.
>
> Sorry.
>
> Well spotted. Thank you.
>
> Magnus, are you going to fix the code as proposed by Ben and Guennadi or 
>   should I resubmit the patch?
>   
Ultimately, somebody needs to submit a patch.  Since Magnus won't try it 
til later this week, it would be best if you post a patch incorporating 
changes.  Then if it works, we're ready to go.

regards,
Ben

[U-Boot-Users] [PATCH] net: add 16 bit support for smc911x

[Magnus Lilja]

Hi all

>  > Magnus, are you going to fix the code as proposed by Ben and Guennadi or
>  >   should I resubmit the patch?
>  >
>  Ultimately, somebody needs to submit a patch.  Since Magnus won't try it
>  til later this week, it would be best if you post a patch incorporating
>  changes.  Then if it works, we're ready to go.

May I suggest adding something like this to the smc911x.c-patch as well:

#if defined(CONFIG_DRIVER_SMC911X_32_BIT) &&
defined(CONFIG_DRIVER_SMC911X_16_BIT)
#error "SMC911X: Only one of CONFIG_DRIVER_SMC911X_32_BIT and
CONFIG_DRIVER_SMC911X_16_BIT shall be set"
#endif

(but with the correct line wrapping, the above is written directly in
gmail and I don't know how gmail will mangle the code)

Just in case someone accidentally defines both in the config file.


Also, I tested the patch with Ben's modifications and it seems to work
on the i.MX31 Litekit board. Tried both ping and tftp file transfer
with the 16 bit option (and 32 bit option).


Regards, Magnus