[U-Boot] [PATCH 1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

[U-Boot] [PATCH 1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

[Hans de Goede]

This fixes the following CACHE warnings when using sun8i_emac:

=> dhcp
BOOTP broadcast 1
BOOTP broadcast 2
CACHE: Misaligned operation at range [7bf594a8, 7bf59628]
BOOTP broadcast 3
CACHE: Misaligned operation at range [7bf59c90, 7bf59e10]
CACHE: Misaligned operation at range [7bf5a478, 7bf5a5f8]
DHCP client bound to address 10.42.43.80 (1009 ms)

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/net/sun8i_emac.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 7c088c3..877859c 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -32,7 +32,8 @@
 
 #define CONFIG_TX_DESCR_NUM	32
 #define CONFIG_RX_DESCR_NUM	32
-#define CONFIG_ETH_BUFSIZE	2024
+#define CONFIG_ETH_BUFSIZE	2048
+#define CONFIG_ETH_RXSIZE	2024 /* Note most fit in ETH_BUFSIZE */
 
 #define TX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_TX_DESCR_NUM)
 #define RX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_RX_DESCR_NUM)
@@ -324,7 +325,7 @@ static void rx_descs_init(struct emac_eth_dev *priv)
 		desc_p->buf_addr = (uintptr_t)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
-		desc_p->st |= CONFIG_ETH_BUFSIZE;
+		desc_p->st |= CONFIG_ETH_RXSIZE;
 		desc_p->status = BIT(31);
 	}
 
@@ -506,7 +507,7 @@ static int _sun8i_eth_recv(struct emac_eth_dev *priv, uchar **packetp)
 					roundup(data_end,
 						ARCH_DMA_MINALIGN));
 		if (good_packet) {
-			if (length > CONFIG_ETH_BUFSIZE) {
+			if (length > CONFIG_ETH_RXSIZE) {
 				printf("Received packet is too big (len=%d)\n",
 				       length);
 				return -EMSGSIZE;
-- 
2.7.4

[U-Boot] [PATCH 2/4] sunxi: Ensure that the NIC specific bytes of the mac are not all 0

[Hans de Goede]

On 2 of my H3 boards bytes 13-15 of the SID are all 0 leading to
the NIC specific bytes of the mac all being 0, which leads to the
boards not getting an ipv6 address from the dhcp server.

This commits adds a check to ensure this does not happen.

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 board/sunxi/board.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 36cf963..ef3fe26 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -623,6 +623,10 @@ static void setup_environment(const void *fdt)
 
 	ret = sunxi_get_sid(sid);
 	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
+		/* Ensure the NIC specific bytes of the mac are not all 0 */
+		if ((sid[3] & 0xffffff) == 0)
+			sid[3] |= 0x800000;
+
 		for (i = 0; i < 4; i++) {
 			sprintf(ethaddr, "ethernet%d", i);
 			if (!fdt_get_alias(fdt, ethaddr))
-- 
2.7.4

[U-Boot] [PATCH 2/4] sunxi: Ensure that the NIC specific bytes of the mac are not all 0

[Ian Campbell]

On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
> On 2 of my H3 boards bytes 13-15 of the SID are all 0 leading to
> the NIC specific bytes of the mac all being 0, which leads to the
> boards not getting an ipv6 address from the dhcp server.
> 
> This commits adds a check to ensure this does not happen.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Hans de Goede]

It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
are always 0 on H3 making it a poor candidate to use as source for the
serialnr / mac-address, switch to word1 which seems to be more random.

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 board/sunxi/board.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index ef3fe26..bbe5340 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -620,12 +620,17 @@ static void setup_environment(const void *fdt)
 	uint8_t mac_addr[6];
 	char ethaddr[16];
 	int i, ret;
+#ifdef CONFIG_MACH_SUN8I_H3
+	const int idx0 = 0, idx1 = 1;
+#else
+	const int idx0 = 0, idx1 = 3;
+#endif
 
 	ret = sunxi_get_sid(sid);
-	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
+	if (ret == 0 && sid[idx0] != 0 && sid[idx1] != 0) {
 		/* Ensure the NIC specific bytes of the mac are not all 0 */
-		if ((sid[3] & 0xffffff) == 0)
-			sid[3] |= 0x800000;
+		if ((sid[idx1] & 0xffffff) == 0)
+			sid[idx1] |= 0x800000;
 
 		for (i = 0; i < 4; i++) {
 			sprintf(ethaddr, "ethernet%d", i);
@@ -642,18 +647,18 @@ static void setup_environment(const void *fdt)
 
 			/* Non OUI / registered MAC address */
 			mac_addr[0] = (i << 4) | 0x02;
-			mac_addr[1] = (sid[0] >>  0) & 0xff;
-			mac_addr[2] = (sid[3] >> 24) & 0xff;
-			mac_addr[3] = (sid[3] >> 16) & 0xff;
-			mac_addr[4] = (sid[3] >>  8) & 0xff;
-			mac_addr[5] = (sid[3] >>  0) & 0xff;
+			mac_addr[1] = (sid[idx0] >>  0) & 0xff;
+			mac_addr[2] = (sid[idx1] >> 24) & 0xff;
+			mac_addr[3] = (sid[idx1] >> 16) & 0xff;
+			mac_addr[4] = (sid[idx1] >>  8) & 0xff;
+			mac_addr[5] = (sid[idx1] >>  0) & 0xff;
 
 			eth_setenv_enetaddr(ethaddr, mac_addr);
 		}
 
 		if (!getenv("serial#")) {
 			snprintf(serial_string, sizeof(serial_string),
-				"%08x%08x", sid[0], sid[3]);
+				"%08x%08x", sid[idx0], sid[idx1]);
 
 			setenv("serial#", serial_string);
 		}
-- 
2.7.4

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Ian Campbell]

On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the
> SID
> are always 0 on H3 making it a poor candidate to use as source for
> the
> serialnr / mac-address, switch to word1 which seems to be more
> random.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Siarhei Siamashka]

Hello Hans,

On Wed, 27 Jul 2016 18:10:34 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
> are always 0 on H3 making it a poor candidate to use as source for the
> serialnr / mac-address, switch to word1 which seems to be more random.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  board/sunxi/board.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index ef3fe26..bbe5340 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -620,12 +620,17 @@ static void setup_environment(const void *fdt)
>  	uint8_t mac_addr[6];
>  	char ethaddr[16];
>  	int i, ret;
> +#ifdef CONFIG_MACH_SUN8I_H3
> +	const int idx0 = 0, idx1 = 1;
> +#else
> +	const int idx0 = 0, idx1 = 3;
> +#endif
>  
>  	ret = sunxi_get_sid(sid);
> -	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
> +	if (ret == 0 && sid[idx0] != 0 && sid[idx1] != 0) {
>  		/* Ensure the NIC specific bytes of the mac are not all 0 */
> -		if ((sid[3] & 0xffffff) == 0)
> -			sid[3] |= 0x800000;
> +		if ((sid[idx1] & 0xffffff) == 0)
> +			sid[idx1] |= 0x800000;
>  
>  		for (i = 0; i < 4; i++) {
>  			sprintf(ethaddr, "ethernet%d", i);
> @@ -642,18 +647,18 @@ static void setup_environment(const void *fdt)
>  
>  			/* Non OUI / registered MAC address */
>  			mac_addr[0] = (i << 4) | 0x02;
> -			mac_addr[1] = (sid[0] >>  0) & 0xff;
> -			mac_addr[2] = (sid[3] >> 24) & 0xff;
> -			mac_addr[3] = (sid[3] >> 16) & 0xff;
> -			mac_addr[4] = (sid[3] >>  8) & 0xff;
> -			mac_addr[5] = (sid[3] >>  0) & 0xff;
> +			mac_addr[1] = (sid[idx0] >>  0) & 0xff;
> +			mac_addr[2] = (sid[idx1] >> 24) & 0xff;
> +			mac_addr[3] = (sid[idx1] >> 16) & 0xff;
> +			mac_addr[4] = (sid[idx1] >>  8) & 0xff;
> +			mac_addr[5] = (sid[idx1] >>  0) & 0xff;
>  
>  			eth_setenv_enetaddr(ethaddr, mac_addr);
>  		}
>  
>  		if (!getenv("serial#")) {
>  			snprintf(serial_string, sizeof(serial_string),
> -				"%08x%08x", sid[0], sid[3]);
> +				"%08x%08x", sid[idx0], sid[idx1]);
>  
>  			setenv("serial#", serial_string);
>  		}

Is it really a good idea trying to guess which parts of the SID are
sufficiently unique, depending on the SoC generation?

We can calculate some kind of hash (CRC32? SHA256?) over the whole
SID. This will ensure that all the SID bits are accounted for.

Also changing the MAC address generation algorithm is an invasive
change, which is affecting the end users. So IMHO it's best to have
it implemented properly right from the start, rather than evolving
via a trial and error method. What if it turns out that word1
from the SID is actually not sufficiently random on H3? How large
was your sample set?

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Chen-Yu Tsai]

Hi,

On Thu, Jul 28, 2016 at 3:14 AM, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> Hello Hans,
>
> On Wed, 27 Jul 2016 18:10:34 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
>> are always 0 on H3 making it a poor candidate to use as source for the
>> serialnr / mac-address, switch to word1 which seems to be more random.
>>
>> Cc: Chen-Yu Tsai <wens@csie.org>
>> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
>> Cc: Amit Singh Tomar <amittomer25@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  board/sunxi/board.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index ef3fe26..bbe5340 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -620,12 +620,17 @@ static void setup_environment(const void *fdt)
>>       uint8_t mac_addr[6];
>>       char ethaddr[16];
>>       int i, ret;
>> +#ifdef CONFIG_MACH_SUN8I_H3
>> +     const int idx0 = 0, idx1 = 1;
>> +#else
>> +     const int idx0 = 0, idx1 = 3;
>> +#endif
>>
>>       ret = sunxi_get_sid(sid);
>> -     if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
>> +     if (ret == 0 && sid[idx0] != 0 && sid[idx1] != 0) {
>>               /* Ensure the NIC specific bytes of the mac are not all 0 */
>> -             if ((sid[3] & 0xffffff) == 0)
>> -                     sid[3] |= 0x800000;
>> +             if ((sid[idx1] & 0xffffff) == 0)
>> +                     sid[idx1] |= 0x800000;
>>
>>               for (i = 0; i < 4; i++) {
>>                       sprintf(ethaddr, "ethernet%d", i);
>> @@ -642,18 +647,18 @@ static void setup_environment(const void *fdt)
>>
>>                       /* Non OUI / registered MAC address */
>>                       mac_addr[0] = (i << 4) | 0x02;
>> -                     mac_addr[1] = (sid[0] >>  0) & 0xff;
>> -                     mac_addr[2] = (sid[3] >> 24) & 0xff;
>> -                     mac_addr[3] = (sid[3] >> 16) & 0xff;
>> -                     mac_addr[4] = (sid[3] >>  8) & 0xff;
>> -                     mac_addr[5] = (sid[3] >>  0) & 0xff;
>> +                     mac_addr[1] = (sid[idx0] >>  0) & 0xff;
>> +                     mac_addr[2] = (sid[idx1] >> 24) & 0xff;
>> +                     mac_addr[3] = (sid[idx1] >> 16) & 0xff;
>> +                     mac_addr[4] = (sid[idx1] >>  8) & 0xff;
>> +                     mac_addr[5] = (sid[idx1] >>  0) & 0xff;
>>
>>                       eth_setenv_enetaddr(ethaddr, mac_addr);
>>               }
>>
>>               if (!getenv("serial#")) {
>>                       snprintf(serial_string, sizeof(serial_string),
>> -                             "%08x%08x", sid[0], sid[3]);
>> +                             "%08x%08x", sid[idx0], sid[idx1]);
>>
>>                       setenv("serial#", serial_string);
>>               }
>
> Is it really a good idea trying to guess which parts of the SID are
> sufficiently unique, depending on the SoC generation?
>
> We can calculate some kind of hash (CRC32? SHA256?) over the whole
> SID. This will ensure that all the SID bits are accounted for.
>
> Also changing the MAC address generation algorithm is an invasive
> change, which is affecting the end users. So IMHO it's best to have
> it implemented properly right from the start, rather than evolving
> via a trial and error method. What if it turns out that word1
> from the SID is actually not sufficiently random on H3? How large
> was your sample set?

I've added the SID values from whatever H3 boards I have to:

    http://linux-sunxi.org/SID_Register_Guide#Currently_known_SID.27s

And it seems word1 is more like a batch number. Note the last 3 boards,
which have identical word1's. These were given to me by Steven at the
same time. word2 seems to follow the pattern 5xxxxxxe.

In short there are quite a few bits that are the same.

Regards
ChenYu

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Hans de Goede]

Hi,

On 28-07-16 05:13, Chen-Yu Tsai wrote:
> Hi,
>
> On Thu, Jul 28, 2016 at 3:14 AM, Siarhei Siamashka
> <siarhei.siamashka@gmail.com> wrote:
>> Hello Hans,
>>
>> On Wed, 27 Jul 2016 18:10:34 +0200
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
>>> are always 0 on H3 making it a poor candidate to use as source for the
>>> serialnr / mac-address, switch to word1 which seems to be more random.
>>>
>>> Cc: Chen-Yu Tsai <wens@csie.org>
>>> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
>>> Cc: Amit Singh Tomar <amittomer25@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  board/sunxi/board.c | 23 ++++++++++++++---------
>>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>> index ef3fe26..bbe5340 100644
>>> --- a/board/sunxi/board.c
>>> +++ b/board/sunxi/board.c
>>> @@ -620,12 +620,17 @@ static void setup_environment(const void *fdt)
>>>       uint8_t mac_addr[6];
>>>       char ethaddr[16];
>>>       int i, ret;
>>> +#ifdef CONFIG_MACH_SUN8I_H3
>>> +     const int idx0 = 0, idx1 = 1;
>>> +#else
>>> +     const int idx0 = 0, idx1 = 3;
>>> +#endif
>>>
>>>       ret = sunxi_get_sid(sid);
>>> -     if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
>>> +     if (ret == 0 && sid[idx0] != 0 && sid[idx1] != 0) {
>>>               /* Ensure the NIC specific bytes of the mac are not all 0 */
>>> -             if ((sid[3] & 0xffffff) == 0)
>>> -                     sid[3] |= 0x800000;
>>> +             if ((sid[idx1] & 0xffffff) == 0)
>>> +                     sid[idx1] |= 0x800000;
>>>
>>>               for (i = 0; i < 4; i++) {
>>>                       sprintf(ethaddr, "ethernet%d", i);
>>> @@ -642,18 +647,18 @@ static void setup_environment(const void *fdt)
>>>
>>>                       /* Non OUI / registered MAC address */
>>>                       mac_addr[0] = (i << 4) | 0x02;
>>> -                     mac_addr[1] = (sid[0] >>  0) & 0xff;
>>> -                     mac_addr[2] = (sid[3] >> 24) & 0xff;
>>> -                     mac_addr[3] = (sid[3] >> 16) & 0xff;
>>> -                     mac_addr[4] = (sid[3] >>  8) & 0xff;
>>> -                     mac_addr[5] = (sid[3] >>  0) & 0xff;
>>> +                     mac_addr[1] = (sid[idx0] >>  0) & 0xff;
>>> +                     mac_addr[2] = (sid[idx1] >> 24) & 0xff;
>>> +                     mac_addr[3] = (sid[idx1] >> 16) & 0xff;
>>> +                     mac_addr[4] = (sid[idx1] >>  8) & 0xff;
>>> +                     mac_addr[5] = (sid[idx1] >>  0) & 0xff;
>>>
>>>                       eth_setenv_enetaddr(ethaddr, mac_addr);
>>>               }
>>>
>>>               if (!getenv("serial#")) {
>>>                       snprintf(serial_string, sizeof(serial_string),
>>> -                             "%08x%08x", sid[0], sid[3]);
>>> +                             "%08x%08x", sid[idx0], sid[idx1]);
>>>
>>>                       setenv("serial#", serial_string);
>>>               }
>>
>> Is it really a good idea trying to guess which parts of the SID are
>> sufficiently unique, depending on the SoC generation?

Probably not, but that is what we've been doing so far.

>> We can calculate some kind of hash (CRC32? SHA256?) over the whole
>> SID. This will ensure that all the SID bits are accounted for.

That seems like a good idea.

I'm thinking about doing the following (for H3 at least, and probably
also for other new SoCs):

	sid[3] ^= sid[1] ^ sid[2];

No need to use a crpytho graphically secure hash, and AFAIK we
don't have enough data for that anyways (without using some seed).

Before using sid[3] to get the low 4 bytes of the mac / serial.

I believe that using sid[0] as the first 4 bytes of the serial
is still a good idea, as it gives us info on which SoC is being used.


>>
>> Also changing the MAC address generation algorithm is an invasive
>> change, which is affecting the end users. So IMHO it's best to have
>> it implemented properly right from the start, rather than evolving
>> via a trial and error method. What if it turns out that word1
>> from the SID is actually not sufficiently random on H3? How large
>> was your sample set?
>
> I've added the SID values from whatever H3 boards I have to:
>
>     http://linux-sunxi.org/SID_Register_Guide#Currently_known_SID.27s

I had done the same for my boards yesterday, but apparently never saved
the preview :|

I've just done this a second time.

> And it seems word1 is more like a batch number. Note the last 3 boards,
> which have identical word1's. These were given to me by Steven at the
> same time. word2 seems to follow the pattern 5xxxxxxe.
>
> In short there are quite a few bits that are the same.

Ack, so nack to my own patch as using word1 is worse then using word3,
I suggest we simply ex-or word1-3 together and use that, any comments
on that ?

Regards,

Hans

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Siarhei Siamashka]

Hi,

On Thu, 28 Jul 2016 20:35:21 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 28-07-16 05:13, Chen-Yu Tsai wrote:
> > Hi,
> >
> > On Thu, Jul 28, 2016 at 3:14 AM, Siarhei Siamashka
> > <siarhei.siamashka@gmail.com> wrote:  
> >> Hello Hans,
> >>
> >> On Wed, 27 Jul 2016 18:10:34 +0200
> >> Hans de Goede <hdegoede@redhat.com> wrote:
> >>  
> >>> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
> >>> are always 0 on H3 making it a poor candidate to use as source for the
> >>> serialnr / mac-address, switch to word1 which seems to be more random.
> >>>
> >>> Cc: Chen-Yu Tsai <wens@csie.org>
> >>> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> >>> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>  board/sunxi/board.c | 23 ++++++++++++++---------
> >>>  1 file changed, 14 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >>> index ef3fe26..bbe5340 100644
> >>> --- a/board/sunxi/board.c
> >>> +++ b/board/sunxi/board.c
> >>> @@ -620,12 +620,17 @@ static void setup_environment(const void *fdt)
> >>>       uint8_t mac_addr[6];
> >>>       char ethaddr[16];
> >>>       int i, ret;
> >>> +#ifdef CONFIG_MACH_SUN8I_H3
> >>> +     const int idx0 = 0, idx1 = 1;
> >>> +#else
> >>> +     const int idx0 = 0, idx1 = 3;
> >>> +#endif
> >>>
> >>>       ret = sunxi_get_sid(sid);
> >>> -     if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
> >>> +     if (ret == 0 && sid[idx0] != 0 && sid[idx1] != 0) {
> >>>               /* Ensure the NIC specific bytes of the mac are not all 0 */
> >>> -             if ((sid[3] & 0xffffff) == 0)
> >>> -                     sid[3] |= 0x800000;
> >>> +             if ((sid[idx1] & 0xffffff) == 0)
> >>> +                     sid[idx1] |= 0x800000;
> >>>
> >>>               for (i = 0; i < 4; i++) {
> >>>                       sprintf(ethaddr, "ethernet%d", i);
> >>> @@ -642,18 +647,18 @@ static void setup_environment(const void *fdt)
> >>>
> >>>                       /* Non OUI / registered MAC address */
> >>>                       mac_addr[0] = (i << 4) | 0x02;
> >>> -                     mac_addr[1] = (sid[0] >>  0) & 0xff;
> >>> -                     mac_addr[2] = (sid[3] >> 24) & 0xff;
> >>> -                     mac_addr[3] = (sid[3] >> 16) & 0xff;
> >>> -                     mac_addr[4] = (sid[3] >>  8) & 0xff;
> >>> -                     mac_addr[5] = (sid[3] >>  0) & 0xff;
> >>> +                     mac_addr[1] = (sid[idx0] >>  0) & 0xff;
> >>> +                     mac_addr[2] = (sid[idx1] >> 24) & 0xff;
> >>> +                     mac_addr[3] = (sid[idx1] >> 16) & 0xff;
> >>> +                     mac_addr[4] = (sid[idx1] >>  8) & 0xff;
> >>> +                     mac_addr[5] = (sid[idx1] >>  0) & 0xff;
> >>>
> >>>                       eth_setenv_enetaddr(ethaddr, mac_addr);
> >>>               }
> >>>
> >>>               if (!getenv("serial#")) {
> >>>                       snprintf(serial_string, sizeof(serial_string),
> >>> -                             "%08x%08x", sid[0], sid[3]);
> >>> +                             "%08x%08x", sid[idx0], sid[idx1]);
> >>>
> >>>                       setenv("serial#", serial_string);
> >>>               }  
> >>
> >> Is it really a good idea trying to guess which parts of the SID are
> >> sufficiently unique, depending on the SoC generation?  
> 
> Probably not, but that is what we've been doing so far.
> 
> >> We can calculate some kind of hash (CRC32? SHA256?) over the whole
> >> SID. This will ensure that all the SID bits are accounted for.  
> 
> That seems like a good idea.
> 
> I'm thinking about doing the following (for H3 at least, and probably
> also for other new SoCs):
> 
> 	sid[3] ^= sid[1] ^ sid[2];

This is not a very good idea. For example, if one of the words is a
encoding the batch number and another word is encoding the serial
number in a batch, then there will be MAC address collisions:

    0x0000000 ^ 0x00000000 = 0x00000000
    0x0000000 ^ 0x00000001 = 0x00000001
    0x0000001 ^ 0x00000000 = 0x00000001
    0x0000001 ^ 0x00000001 = 0x00000000

There is a reason why CRC32 is used for calculating checksums instead
of doing simple XOR or ADD. And even added as a special instruction on
x86 and ARM. It just ensures that changing one bit in the input affects
many bits in the output and can detect single bit or multiple bit flips
in the corrupted data. 

    https://en.wikipedia.org/wiki/Avalanche_effect

I would say that if we only want to combine the SID data into 32
deterministic pseudo-random bits, then the use of CRC32 is a much
better choice than XOR. U-Boot should already have the crc32 function
linked in and it's only a single function call to do this. But if we
want more than 32 bits, than taking any N bits of any cryptographic
hash would do the job. As an additional bonus, if some users have
privacy concerns, than a cryptographic hash would make it difficult
to recover the original SID value from the MAC address.

> No need to use a crpytho graphically secure hash, and AFAIK we
> don't have enough data for that anyways (without using some seed).
> 
> Before using sid[3] to get the low 4 bytes of the mac / serial.
> 
> I believe that using sid[0] as the first 4 bytes of the serial
> is still a good idea, as it gives us info on which SoC is being used.

It might be a waste of the 8 bits of space in the MAC address for
encoding something like only 4 bits of real data. Moreover, the
current code is only taking the lowest 8 bits of the sid[0], which
do not contain the SoC type on older Allwinner chips (A10/A13/A20).
And we are in the assumptions territory again for the newer chips
(H3/A83/A64/....), but making assumptions is not very reliable.

With using a good hash for combining the SID data, we only have a
risk of random collisions of the generated MAC addresses:

    http://preshing.com/20110504/hash-collision-probabilities/

Using more bits for the deterministic pseudo-random part of the MAC
address significantly reduces the probability of having collisions.

With 32 bits of the pseudo-random part, we can expect 1% probability
of a collision between at least one pair of devices in a set of 9292
devices.

With 44 bits of the pseudo-random part, we can expect 1% probability
of a collision between at least one pair of devices in a set of 594656
devices.
 
> >>
> >> Also changing the MAC address generation algorithm is an invasive
> >> change, which is affecting the end users. So IMHO it's best to have
> >> it implemented properly right from the start, rather than evolving
> >> via a trial and error method. What if it turns out that word1
> >> from the SID is actually not sufficiently random on H3? How large
> >> was your sample set?  
> >
> > I've added the SID values from whatever H3 boards I have to:
> >
> >     http://linux-sunxi.org/SID_Register_Guide#Currently_known_SID.27s  
> 
> I had done the same for my boards yesterday, but apparently never saved
> the preview :|
> 
> I've just done this a second time.
> 
> > And it seems word1 is more like a batch number. Note the last 3 boards,
> > which have identical word1's. These were given to me by Steven at the
> > same time. word2 seems to follow the pattern 5xxxxxxe.
> >
> > In short there are quite a few bits that are the same.  
> 
> Ack, so nack to my own patch as using word1 is worse then using word3,
> I suggest we simply ex-or word1-3 together and use that, any comments
> on that ?

I still think that it's best to use SHA1 or SHA256 and calculate it
over the whole SID data. The self-test code included in the sources

    http://git.denx.de/?p=u-boot.git;a=blob;f=lib/sha1.c
    http://git.denx.de/?p=u-boot.git;a=blob;f=lib/sha256.c

can serve as a usage example. There is really nothing difficult. And
then we can use as many bits as necessary for the pseudo-random part
of the MAC.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH 3/4] sun8i: On H3 Use word 1 instead of word 3 from the SID

[Hans de Goede]

Hi,

On 27-07-16 21:14, Siarhei Siamashka wrote:
> Hello Hans,

<Snip>

I just realized I forgot to reply to this bit:

> Also changing the MAC address generation algorithm is an invasive
> change, which is affecting the end users.

Agreed, which is why I'm only changing it for the H3 and why I'm
changing it now before ethernet support hits the mainline kernel.

Unfortunately if you look at:

http://linux-sunxi.org/SID_Register_Guide#Currently_known_SID.27s

You will see there are many H3 samples where sid[3] only differs
a single bit with the sid[3] from other boards, and bytes 1-2 of
sid[3] are always 0, so it really is a poor candidate.

 > So IMHO it's best to have
> it implemented properly right from the start, rather than evolving
> via a trial and error method.

Agreed, as I said in my previous mail on this I will go with your
suggestion to hash (well simply ex-or actuallu) 3 of the 4 sid words
and use that for the unique part of the mac / serial.

Thanks for your input on this!

Regards,

Hans

[U-Boot] [PATCH 4/4] sunxi: Re-enable h3 emac support

[Hans de Goede]

With the recent bug fixes for the sun8i_emac driver all known issues
are resolved, so we can re-enable the driver.

While at it, also enable the emac on the Orange Pi One.

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/dts/sun8i-h3-orangepi-one.dts | 11 +++++++++++
 configs/orangepi_one_defconfig         |  1 +
 configs/orangepi_pc_defconfig          |  1 +
 configs/orangepi_pc_plus_defconfig     |  1 +
 configs/pine64_plus_defconfig          |  1 +
 5 files changed, 15 insertions(+)

diff --git a/arch/arm/dts/sun8i-h3-orangepi-one.dts b/arch/arm/dts/sun8i-h3-orangepi-one.dts
index 0adf932..8df5c74 100644
--- a/arch/arm/dts/sun8i-h3-orangepi-one.dts
+++ b/arch/arm/dts/sun8i-h3-orangepi-one.dts
@@ -94,6 +94,17 @@
 	status = "okay";
 };
 
+&emac {
+	phy = <&phy1>;
+	phy-mode = "mii";
+	allwinner,use-internal-phy;
+	allwinner,leds-active-low;
+	status = "okay";
+	phy1: ethernet-phy at 1 {
+		reg = <1>;
+	};
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
diff --git a/configs/orangepi_one_defconfig b/configs/orangepi_one_defconfig
index be8afca..81299e6 100644
--- a/configs/orangepi_one_defconfig
+++ b/configs/orangepi_one_defconfig
@@ -13,3 +13,4 @@ CONFIG_SPL=y
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_FPGA is not set
 CONFIG_USB_EHCI_HCD=y
+CONFIG_SUN8I_EMAC=y
diff --git a/configs/orangepi_pc_defconfig b/configs/orangepi_pc_defconfig
index 366fa08..2281aed 100644
--- a/configs/orangepi_pc_defconfig
+++ b/configs/orangepi_pc_defconfig
@@ -14,3 +14,4 @@ CONFIG_SPL=y
 # CONFIG_CMD_FPGA is not set
 CONFIG_SY8106A_POWER=y
 CONFIG_USB_EHCI_HCD=y
+CONFIG_SUN8I_EMAC=y
diff --git a/configs/orangepi_pc_plus_defconfig b/configs/orangepi_pc_plus_defconfig
index c78aec3..3ec6dac 100644
--- a/configs/orangepi_pc_plus_defconfig
+++ b/configs/orangepi_pc_plus_defconfig
@@ -15,3 +15,4 @@ CONFIG_SPL=y
 # CONFIG_CMD_FPGA is not set
 CONFIG_SY8106A_POWER=y
 CONFIG_USB_EHCI_HCD=y
+CONFIG_SUN8I_EMAC=y
diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig
index 0bf79bf..5c97de1 100644
--- a/configs/pine64_plus_defconfig
+++ b/configs/pine64_plus_defconfig
@@ -10,3 +10,4 @@ CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_FPGA is not set
 CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y
+CONFIG_SUN8I_EMAC=y
-- 
2.7.4

[U-Boot] [PATCH 4/4] sunxi: Re-enable h3 emac support

[Ian Campbell]

On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
> With the recent bug fixes for the sun8i_emac driver all known issues
> are resolved, so we can re-enable the driver.
> 
> While at it, also enable the emac on the Orange Pi One.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH 4/4] sunxi: Re-enable h3 emac support

[Jagan Teki]

On 27 July 2016 at 23:55, Ian Campbell <ijc+uboot@hellion.org.uk> wrote:
> On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
>> With the recent bug fixes for the sun8i_emac driver all known issues
>> are resolved, so we can re-enable the driver.
>>
>> While at it, also enable the emac on the Orange Pi One.
>>
>> Cc: Chen-Yu Tsai <wens@csie.org>
>> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
>> Cc: Amit Singh Tomar <amittomer25@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Ian Campbell <ijc@hellion.org.uk>

Acked-by: Jagan Teki <jteki@openedev.com>

-- 
Jagan.

[U-Boot] [PATCH 1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

[Amit Tomer]

Hello,

> index 7c088c3..877859c 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -32,7 +32,8 @@
>
>  #define CONFIG_TX_DESCR_NUM    32
>  #define CONFIG_RX_DESCR_NUM    32
> -#define CONFIG_ETH_BUFSIZE     2024
> +#define CONFIG_ETH_BUFSIZE     2048
> +#define CONFIG_ETH_RXSIZE      2024 /* Note most fit in ETH_BUFSIZE */

As per the following comment made in Linux driver.

/* The datasheet said that each descriptor can transfers up to 4096bytes
+ * But latter, a register documentation reduce that value to 2048
+ * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
+ */

Can we keep CONFIG_ETH_BUFSIZE to 2044 ?

Thanks,
Amit.

[U-Boot] [PATCH 1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

[Hans de Goede]

Hi,

On 27-07-16 19:11, Amit Tomer wrote:
> Hello,
>
>> index 7c088c3..877859c 100644
>> --- a/drivers/net/sun8i_emac.c
>> +++ b/drivers/net/sun8i_emac.c
>> @@ -32,7 +32,8 @@
>>
>>  #define CONFIG_TX_DESCR_NUM    32
>>  #define CONFIG_RX_DESCR_NUM    32
>> -#define CONFIG_ETH_BUFSIZE     2024
>> +#define CONFIG_ETH_BUFSIZE     2048
>> +#define CONFIG_ETH_RXSIZE      2024 /* Note most fit in ETH_BUFSIZE */
>
> As per the following comment made in Linux driver.
>
> /* The datasheet said that each descriptor can transfers up to 4096bytes
> + * But latter, a register documentation reduce that value to 2048
> + * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
> + */
>
> Can we keep CONFIG_ETH_BUFSIZE to 2044 ?

Note that my patch is introducing a CONFIG_ETH_RXSIZE and is using that
where ever CONFIG_ETH_BUFSIZE is not used to actually allocate
or cache-flush a buffer.

And no we cannot use 2044 as then the 2nd buffer in the array
still is not cache-aligned.

But using 2048 is fine, as long as we only use it to allocate
buffers, and not actually as the value passed to the hardware
(which is what the new CONFIG_ETH_RXSIZE is for).

I started with just using 2048 and that does indeed not work,
but with the addition of CONFIG_ETH_RXSIZE things work fine.

Regards,

Hans

[U-Boot] [PATCH 1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

[Ian Campbell]

On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
> This fixes the following CACHE warnings when using sun8i_emac:
> 
> => dhcp
> BOOTP broadcast 1
> BOOTP broadcast 2
> CACHE: Misaligned operation at range [7bf594a8, 7bf59628]
> BOOTP broadcast 3
> CACHE: Misaligned operation at range [7bf59c90, 7bf59e10]
> CACHE: Misaligned operation at range [7bf5a478, 7bf5a5f8]
> DHCP client bound to address 10.42.43.80 (1009 ms)
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> ?drivers/net/sun8i_emac.c | 7 ++++---
> ?1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 7c088c3..877859c 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -32,7 +32,8 @@
> ?
> ?#define CONFIG_TX_DESCR_NUM	32
> ?#define CONFIG_RX_DESCR_NUM	32
> -#define CONFIG_ETH_BUFSIZE	2024
> +#define CONFIG_ETH_BUFSIZE	2048
> +#define CONFIG_ETH_RXSIZE	2024 /* Note most fit in ETH_BUFSIZE */

s/most/must/?

A comment (perhaps in the commit message rather than the code) as to
how/why RXSIZE and BUFSIZE interact to affect the alignment in the
desired fasion would be useful, since it is non-obvious to me at
least.?

I was about to speculate on the difference of 14 bytes relating to the
Ethernet frame header, but then I realised it's 24 not 14 and deleted
those paragraphs, which I think underscores the need for a comment ;-)

Ian.