| Message ID | 1304244632-8714-3-git-send-email-grinberg@compulab.co.il |
|---|---|
| State | Superseded |
| Headers | show |
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? Apart from that, it feels entirely wrong to do so. Fix the kernel to NOT do a workaround instead should be the better approach. Any opinions by Ben or Wolfgang on this? Best Regards, Reinhard
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? >
Hi Igor, > 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. I'd say remove it. Why do I say that? [dzu@pollux u-boot-testing (master)]$ make csb337_config make: *** No rule to make target `csb337_config'. Stop. make: *** [csb337_config] Error 1 [dzu@pollux u-boot-testing (master)]$ grep -i csb337 Makefile [dzu@pollux u-boot-testing (master)]$ grep -i csb337 boards.cfg [dzu@pollux u-boot-testing (master)]$ Cheers Detlev
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);
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(-)