[U-Boot] [PATCH 3/3] arm: at91: ether: Prepare for mach-types.h changes

[Igor Grinberg <grinberg@compulab.co.il>]

On 05/01/11 22:38, Reinhard Meyer wrote:

> Dear Igor Grinberg,
>
>> at91 ethernet module used machine_is_cbs337()  macro for board specific
>> Linux compatibility issue.
>> Use compile time defines instead.
>>
>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il>
>> ---
>>   arch/arm/cpu/arm920t/at91rm9200/ether.c |   18 +++++++++---------
>>   1 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm920t/at91rm9200/ether.c b/arch/arm/cpu/arm920t/at91rm9200/ether.c
>> index e1cdeba..4aeb883 100644
>> --- a/arch/arm/cpu/arm920t/at91rm9200/ether.c
>> +++ b/arch/arm/cpu/arm920t/at91rm9200/ether.c
>> @@ -201,15 +201,15 @@ int eth_init (bd_t * bd)
>>        * that MicroMonitor behavior so we avoid needing to make such OS code
>>        * care about which bootloader was used.
>>        */
>> -    if (machine_is_csb337()) {
>> -        p_mac->EMAC_SA2H = (enetaddr[0]<<   8) | (enetaddr[1]);
>> -        p_mac->EMAC_SA2L = (enetaddr[2]<<  24) | (enetaddr[3]<<  16)
>> -                 | (enetaddr[4]<<   8) | (enetaddr[5]);
>> -    } else {
>> -        p_mac->EMAC_SA2L = (enetaddr[3]<<  24) | (enetaddr[2]<<  16)
>> -                 | (enetaddr[1]<<   8) | (enetaddr[0]);
>> -        p_mac->EMAC_SA2H = (enetaddr[5]<<   8) | (enetaddr[4]);
>> -    }
>> +#ifdef CONFIG_MACH_CSB337
>> +    p_mac->EMAC_SA2H = (enetaddr[0]<<   8) | (enetaddr[1]);
>> +    p_mac->EMAC_SA2L = (enetaddr[2]<<  24) | (enetaddr[3]<<  16)
>> +        | (enetaddr[4]<<   8) | (enetaddr[5]);
>> +#else
>> +    p_mac->EMAC_SA2L = (enetaddr[3]<<  24) | (enetaddr[2]<<  16)
>> +        | (enetaddr[1]<<   8) | (enetaddr[0]);
>> +    p_mac->EMAC_SA2H = (enetaddr[5]<<   8) | (enetaddr[4]);
>> +#endif
>>
>>       p_mac->EMAC_RBQP = (long) (&rbfdt[0]);
>>       p_mac->EMAC_RSR&= ~(AT91C_EMAC_RSR_OVR | AT91C_EMAC_REC | AT91C_EMAC_BNA);
>
> There is nothing wrong with your patch itself, but it let me to take a closer look at the
> reasoning of why there is a machine dependency. The full code at this section is:
>
>     eth_getenv_enetaddr("ethaddr", enetaddr);
>
>     /* The CSB337 originally used a version of the MicroMonitor bootloader
>      * which saved Ethernet addresses in the "wrong" order.  Operating
>      * systems (like Linux) know this, and apply a workaround.  Replicate
>      * that MicroMonitor behavior so we avoid needing to make such OS code
>      * care about which bootloader was used.
>      */
>     if (machine_is_csb337()) {
>         p_mac->EMAC_SA2H = (enetaddr[0] <<  8) | (enetaddr[1]);
>         p_mac->EMAC_SA2L = (enetaddr[2] << 24) | (enetaddr[3] << 16)
>                  | (enetaddr[4] <<  8) | (enetaddr[5]);
>     } else {
>         p_mac->EMAC_SA2L = (enetaddr[3] << 24) | (enetaddr[2] << 16)
>                  | (enetaddr[1] <<  8) | (enetaddr[0]);
>         p_mac->EMAC_SA2H = (enetaddr[5] <<  8) | (enetaddr[4]);
>     }
>
> So, for the sake of a(nother) broken bootloader and a workaround in Linux we
> store the MAC address in the wrong order? What if U-Boot itself is used to make
> LAN accesses?

Well, I've read the comment before preparing the patch.
Actually, I felt like: "this should be thrown away!".
Also, I haven't found csb337 board in the tree...
I didn't want to decide for you (If I'm not mistaken,
you are the maintainer of Atmel) what to do with it, so I left it.
Do you think we should remove this?
I would love to send another patch to remove this completely.

>
> Apart from that, it feels entirely wrong to do so. Fix the kernel to NOT do a
> workaround instead should be the better approach.

Yep, I totally agree...

>
> Any opinions by Ben or Wolfgang on this?
>


-- 
Regards,
Igor.