diff mbox series

[v5,04/10] ti: common: board_detect: commodify ethaddr environment setting code

Message ID f93b5b10-b71f-0927-af23-e3a881875aa9@ti.com
State Accepted
Commit 38f719ea5e9d69e69c56b92fc723564194b755e6
Headers show
Series None | expand

Commit Message

Roger Quadros March 14, 2017, 1:04 p.m. UTC 
Keystone and OMAP platforms will need this to set ethernet
MAC addresses from board EEPROM.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Reviewed-by: Lokesh Vutla <lokeshvutla@ti.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
v5:
- Set upto 50 MAC addresses if more than 50 are provided in board EEPROM.

 board/ti/common/board_detect.c | 62 ++++++++++++++++++++++++++++++++++++++++++
 board/ti/common/board_detect.h | 12 ++++++++
 2 files changed, 74 insertions(+)

Comments

Felipe Balbi March 14, 2017, 3:54 p.m. UTC | #1 
Hi,

On Tue, Mar 14, 2017 at 3:05 PM Roger Quadros <rogerq@ti.com> wrote:

+void board_ti_set_ethaddr(int index)
+{
+       uint8_t mac_addr[6];
+       int i;
+       u64 mac1, mac2;
+       u8 mac_addr1[6], mac_addr2[6];
+       int num_macs;
+       /*
+        * Export any Ethernet MAC addresses from EEPROM.
+        * The 2 MAC addresses in EEPROM define the address range.
+        */
+       board_ti_get_eth_mac_addr(0, mac_addr1);
+       board_ti_get_eth_mac_addr(1, mac_addr2);
+
+       if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
+               mac1 = mac_to_u64(mac_addr1);
+               mac2 = mac_to_u64(mac_addr2);
+
+               /* must contain an address range */
+               num_macs = mac2 - mac1 + 1;
+               if (num_macs <= 0)
+                       return;


seems like there's still one minor improvement here:

If num_macs < 0, then it could be that mac1 and mac2 are swapped.

Perhaps test for that ?

if (num_macs == 0)
     bail();

if (num_macs < 0) {
        if ((mac1 - mac2 + 1) < 50) {
                num_macs = mac1 - mac2 + 1;
                mac1 ^= mac2;
                mac2 ^= mac1;
                mac1 ^= mac2;
[...]

don't know how much this may help, it's a judgment call, I suppose
Roger Quadros March 15, 2017, 1:53 p.m. UTC | #2 
Hi,

On 14/03/17 17:54, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Mar 14, 2017 at 3:05 PM Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote:
> 
>     +void board_ti_set_ethaddr(int index)
>     +{
>     +       uint8_t mac_addr[6];
>     +       int i;
>     +       u64 mac1, mac2;
>     +       u8 mac_addr1[6], mac_addr2[6];
>     +       int num_macs;
>     +       /*
>     +        * Export any Ethernet MAC addresses from EEPROM.
>     +        * The 2 MAC addresses in EEPROM define the address range.
>     +        */
>     +       board_ti_get_eth_mac_addr(0, mac_addr1);
>     +       board_ti_get_eth_mac_addr(1, mac_addr2);
>     +
>     +       if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
>     +               mac1 = mac_to_u64(mac_addr1);
>     +               mac2 = mac_to_u64(mac_addr2);
>     +
>     +               /* must contain an address range */
>     +               num_macs = mac2 - mac1 + 1;
>     +               if (num_macs <= 0)
>     +                       return;
> 
> 
> seems like there's still one minor improvement here:
> 
> If num_macs < 0, then it could be that mac1 and mac2 are swapped.
> 
> Perhaps test for that ?
> 
> if (num_macs == 0)
>      bail();
> 
> if (num_macs < 0) {
>         if ((mac1 - mac2 + 1) < 50) {
>                 num_macs = mac1 - mac2 + 1;
>                 mac1 ^= mac2;
>                 mac2 ^= mac1;
>                 mac1 ^= mac2;
> [...]
> 
> don't know how much this may help, it's a judgment call, I suppose

We don't have any boards with swapped mac1 and mac2. We'd like to
keep it that way and catch any new boards with eeprom errors early so I'd
rather not have this enhancement.
Felipe Balbi March 15, 2017, 2:46 p.m. UTC | #3 
Hi,

Roger Quadros <rogerq@ti.com> writes:
>> On Tue, Mar 14, 2017 at 3:05 PM Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote:
>> 
>>     +void board_ti_set_ethaddr(int index)
>>     +{
>>     +       uint8_t mac_addr[6];
>>     +       int i;
>>     +       u64 mac1, mac2;
>>     +       u8 mac_addr1[6], mac_addr2[6];
>>     +       int num_macs;
>>     +       /*
>>     +        * Export any Ethernet MAC addresses from EEPROM.
>>     +        * The 2 MAC addresses in EEPROM define the address range.
>>     +        */
>>     +       board_ti_get_eth_mac_addr(0, mac_addr1);
>>     +       board_ti_get_eth_mac_addr(1, mac_addr2);
>>     +
>>     +       if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
>>     +               mac1 = mac_to_u64(mac_addr1);
>>     +               mac2 = mac_to_u64(mac_addr2);
>>     +
>>     +               /* must contain an address range */
>>     +               num_macs = mac2 - mac1 + 1;
>>     +               if (num_macs <= 0)
>>     +                       return;
>> 
>> 
>> seems like there's still one minor improvement here:
>> 
>> If num_macs < 0, then it could be that mac1 and mac2 are swapped.
>> 
>> Perhaps test for that ?
>> 
>> if (num_macs == 0)
>>      bail();
>> 
>> if (num_macs < 0) {
>>         if ((mac1 - mac2 + 1) < 50) {
>>                 num_macs = mac1 - mac2 + 1;
>>                 mac1 ^= mac2;
>>                 mac2 ^= mac1;
>>                 mac1 ^= mac2;
>> [...]
>> 
>> don't know how much this may help, it's a judgment call, I suppose
>
> We don't have any boards with swapped mac1 and mac2. We'd like to

in that case, why would (mac2-mac1+1) ever be < 0?
Roger Quadros March 15, 2017, 3:04 p.m. UTC | #4 
On 15/03/17 16:46, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> On Tue, Mar 14, 2017 at 3:05 PM Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote:
>>>
>>>     +void board_ti_set_ethaddr(int index)
>>>     +{
>>>     +       uint8_t mac_addr[6];
>>>     +       int i;
>>>     +       u64 mac1, mac2;
>>>     +       u8 mac_addr1[6], mac_addr2[6];
>>>     +       int num_macs;
>>>     +       /*
>>>     +        * Export any Ethernet MAC addresses from EEPROM.
>>>     +        * The 2 MAC addresses in EEPROM define the address range.
>>>     +        */
>>>     +       board_ti_get_eth_mac_addr(0, mac_addr1);
>>>     +       board_ti_get_eth_mac_addr(1, mac_addr2);
>>>     +
>>>     +       if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
>>>     +               mac1 = mac_to_u64(mac_addr1);
>>>     +               mac2 = mac_to_u64(mac_addr2);
>>>     +
>>>     +               /* must contain an address range */
>>>     +               num_macs = mac2 - mac1 + 1;
>>>     +               if (num_macs <= 0)
>>>     +                       return;
>>>
>>>
>>> seems like there's still one minor improvement here:
>>>
>>> If num_macs < 0, then it could be that mac1 and mac2 are swapped.
>>>
>>> Perhaps test for that ?
>>>
>>> if (num_macs == 0)
>>>      bail();
>>>
>>> if (num_macs < 0) {
>>>         if ((mac1 - mac2 + 1) < 50) {
>>>                 num_macs = mac1 - mac2 + 1;
>>>                 mac1 ^= mac2;
>>>                 mac2 ^= mac1;
>>>                 mac1 ^= mac2;
>>> [...]
>>>
>>> don't know how much this may help, it's a judgment call, I suppose
>>
>> We don't have any boards with swapped mac1 and mac2. We'd like to
> 
> in that case, why would (mac2-mac1+1) ever be < 0?
> 

For incorrectly programmed boards in pre-production probably.
Tom Rini March 21, 2017, 6:08 p.m. UTC | #5 
On Tue, Mar 14, 2017 at 03:04:19PM +0200, Roger Quadros wrote:

> Keystone and OMAP platforms will need this to set ethernet

> MAC addresses from board EEPROM.

> 

> Signed-off-by: Roger Quadros <rogerq@ti.com>

> Reviewed-by: Lokesh Vutla <lokeshvutla@ti.com>

> Reviewed-by: Tom Rini <trini@konsulko.com>


Applied to u-boot/master, thanks!

-- 
Tom
diff mbox series

Patch

diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
index a5dba94..c55e24e 100644
--- a/board/ti/common/board_detect.c
+++ b/board/ti/common/board_detect.c
@@ -314,3 +314,65 @@  void __maybe_unused set_board_info_env(char *name)
 	else
 		setenv("board_serial", unknown);
 }
+
+static u64 mac_to_u64(u8 mac[6])
+{
+	int i;
+	u64 addr = 0;
+
+	for (i = 0; i < 6; i++) {
+		addr <<= 8;
+		addr |= mac[i];
+	}
+
+	return addr;
+}
+
+static void u64_to_mac(u64 addr, u8 mac[6])
+{
+	mac[5] = addr;
+	mac[4] = addr >> 8;
+	mac[3] = addr >> 16;
+	mac[2] = addr >> 24;
+	mac[1] = addr >> 32;
+	mac[0] = addr >> 40;
+}
+
+void board_ti_set_ethaddr(int index)
+{
+	uint8_t mac_addr[6];
+	int i;
+	u64 mac1, mac2;
+	u8 mac_addr1[6], mac_addr2[6];
+	int num_macs;
+	/*
+	 * Export any Ethernet MAC addresses from EEPROM.
+	 * The 2 MAC addresses in EEPROM define the address range.
+	 */
+	board_ti_get_eth_mac_addr(0, mac_addr1);
+	board_ti_get_eth_mac_addr(1, mac_addr2);
+
+	if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
+		mac1 = mac_to_u64(mac_addr1);
+		mac2 = mac_to_u64(mac_addr2);
+
+		/* must contain an address range */
+		num_macs = mac2 - mac1 + 1;
+		if (num_macs <= 0)
+			return;
+
+		if (num_macs > 50) {
+			printf("%s: Too many MAC addresses: %d. Limiting to 50\n",
+			       __func__, num_macs);
+			num_macs = 50;
+		}
+
+		for (i = 0; i < num_macs; i++) {
+			u64_to_mac(mac1 + i, mac_addr);
+			if (is_valid_ethaddr(mac_addr)) {
+				eth_setenv_enetaddr_by_index("eth", i + index,
+							     mac_addr);
+			}
+		}
+	}
+}
diff --git a/board/ti/common/board_detect.h b/board/ti/common/board_detect.h
index 52b0075..88b0a59 100644
--- a/board/ti/common/board_detect.h
+++ b/board/ti/common/board_detect.h
@@ -193,4 +193,16 @@  u64 board_ti_get_emif2_size(void);
  */
 void set_board_info_env(char *name);
 
+/**
+ * board_ti_set_ethaddr- Sets the ethaddr environment from EEPROM
+ * @index: The first eth<index>addr environment variable to set
+ *
+ * EEPROM should be already read before calling this function.
+ * The EEPROM contains 2 MAC addresses which define the MAC address
+ * range (i.e. first and last MAC address).
+ * This function sets the ethaddr environment variable for all
+ * the available MAC addresses starting from eth<index>addr.
+ */
+void board_ti_set_ethaddr(int index);
+
 #endif	/* __BOARD_DETECT_H */