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

login
register
mail settings
Submitter Dan Murphy
Date July 17, 2013, 8:16 p.m.
Message ID <1374092167-27645-6-git-send-email-dmurphy@ti.com>
Download mbox | patch
Permalink /patch/259754/
State Awaiting Upstream
Delegated to: Marek Vasut
Headers show

Comments

Dan Murphy - July 17, 2013, 8:16 p.m.
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(+)
Marek Vasut - July 18, 2013, 4:28 a.m.
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]
 */


> +	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.

> 
>  	auxclk = readl((*prcm)->scrm_auxclk1);
>  	/* Request auxilary clock */
> diff --git a/include/configs/omap5_uevm.h b/include/configs/omap5_uevm.h
> index 0740a32..53b86ec 100644
> --- a/include/configs/omap5_uevm.h
> +++ b/include/configs/omap5_uevm.h
> @@ -79,5 +79,7 @@
> 
>  #define CONSOLEDEV		"ttyO2"
> 
> +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
> +
>  #define CONFIG_OMAP_PLATFORM_RESET_TIME_MAX_USEC	16296
>  #endif /* __CONFIG_OMAP5_EVM_H */

[1] http://www.denx.de/wiki/U-Boot/CodingStyle

Best regards,
Marek Vasut
Dan Murphy - July 18, 2013, 3:40 p.m.
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?
>
>>  	auxclk = readl((*prcm)->scrm_auxclk1);
>>  	/* Request auxilary clock */
>> diff --git a/include/configs/omap5_uevm.h b/include/configs/omap5_uevm.h
>> index 0740a32..53b86ec 100644
>> --- a/include/configs/omap5_uevm.h
>> +++ b/include/configs/omap5_uevm.h
>> @@ -79,5 +79,7 @@
>>
>>  #define CONSOLEDEV		"ttyO2"
>>
>> +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
>> +
>>  #define CONFIG_OMAP_PLATFORM_RESET_TIME_MAX_USEC	16296
>>  #endif /* __CONFIG_OMAP5_EVM_H */
> [1] http://www.denx.de/wiki/U-Boot/CodingStyle
>
> Best regards,
> Marek Vasut
Marek Vasut - July 18, 2013, 3:47 p.m.
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

Patch

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.
+	 */
+	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
 
 	auxclk = readl((*prcm)->scrm_auxclk1);
 	/* Request auxilary clock */
diff --git a/include/configs/omap5_uevm.h b/include/configs/omap5_uevm.h
index 0740a32..53b86ec 100644
--- a/include/configs/omap5_uevm.h
+++ b/include/configs/omap5_uevm.h
@@ -79,5 +79,7 @@ 
 
 #define CONSOLEDEV		"ttyO2"
 
+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
+
 #define CONFIG_OMAP_PLATFORM_RESET_TIME_MAX_USEC	16296
 #endif /* __CONFIG_OMAP5_EVM_H */