[U-Boot] [PATCH v3 5/7] ARM: OMAP5-uevm: Add USB MAC ethernet address

[Marek Vasut <marex@denx.de>]

Dear Dan Murphy,

> On 07/17/2013 11:28 PM, Marek Vasut wrote:
> > Dear Dan Murphy,
> > 
> >> Set the usbethaddr based on the OMAP DIE_ID registers
> >> which should be unique for each processor.
> >> 
> >> Then set this as the usb ethernet MAC address.
> >> 
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >> ---
> >> v3 - new patch
> >> 
> >>  board/ti/omap5_uevm/evm.c    |   26 ++++++++++++++++++++++++++
> >>  include/configs/omap5_uevm.h |    2 ++
> >>  2 files changed, 28 insertions(+)
> >> 
> >> diff --git a/board/ti/omap5_uevm/evm.c b/board/ti/omap5_uevm/evm.c
> >> index bf14cd2..9add0fd 100644
> >> --- a/board/ti/omap5_uevm/evm.c
> >> +++ b/board/ti/omap5_uevm/evm.c
> >> @@ -33,6 +33,11 @@
> >> 
> >>  #include <usb.h>
> >>  #include <asm/arch/ehci.h>
> >>  #include <asm/ehci-omap.h>
> >> 
> >> +
> >> +#define MAX_DEVICE_MAC		20
> >> +#define DIE_ID_REG_BASE         (OMAP54XX_L4_CORE_BASE + 0x2000)
> >> +#define DIE_ID_REG_OFFSET	0x200
> >> +
> >> 
> >>  #endif
> >>  
> >>  DECLARE_GLOBAL_DATA_PTR;
> >> 
> >> @@ -121,6 +126,27 @@ int ehci_hcd_init(int index, struct ehci_hccr
> >> **hccr, struct ehci_hcor **hcor) {
> >> 
> >>  	int ret;
> >>  	int auxclk;
> >> 
> >> +	int val[4] = { 0 };
> >> +	int reg;
> >> +	char device_mac[MAX_DEVICE_MAC];
> >> +
> >> +	reg = DIE_ID_REG_BASE + DIE_ID_REG_OFFSET;
> >> +
> >> +	val[0] = readl(reg);
> >> +	val[1] = readl(reg + 0x8);
> >> +	val[2] = readl(reg + 0xC);
> >> +	val[3] = readl(reg + 0x10);
> >> +
> >> +	/* create a fake MAC address from the processor ID code.
> >> +	 * first byte is 0x02 to signify locally administered.
> >> +	 */
> > 
> > /*
> > 
> >  * valid
> >  * multiline
> >  * comment ... has that leading newline on the top. [1]
> >  */
> 
> Will fix
> 
> >> +	snprintf(device_mac, MAX_DEVICE_MAC, "%02X:%02X:%02X:%02X:%02X:%02X",
> >> +				0x02, val[3] & 0xff, val[2] & 0xff, val[1] &
> > 
> > 0xff,
> > 
> >> +				val[0] & 0xff, (val[0] >> 8 & 0xff));
> >> +
> >> +#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
> >> +	setenv("usbethaddr", device_mac);
> >> +#endif
> > 
> > Urgh ... this is a big fat NAK. No, just don't do this, please. A kitten
> > will die if you set variables in the code like this. Another one will
> > die for such a config option.
> > 
> > You might work around this problem by setting a different env variable
> > (yet I am still unhappy to see this) and then in the board environment
> > have a command to load the USB mac address from that other variable. But
> > do not enforce the mac address on users.
> 
> I will modify the usbethaddr only if the user has not already set the
> variable. This is the way I see it in other code as well.
> 
> Is that acceptable?

That'd also make sense, yeah.

Best regards,
Marek Vasut