Patchwork [U-Boot] at91_emac: fix compile warning

login
register
mail settings
Submitter Andreas Bießmann
Date June 9, 2011, 10:22 a.m.
Message ID <1307614974-15516-1-git-send-email-andreas.devel@gmail.com>
Download mbox | patch
Permalink /patch/99718/
State Accepted
Commit 2321bfe425385494da8739d7d7e9dd3a5ca18a84
Delegated to: Reinhard Meyer
Headers show

Comments

Andreas Bießmann - June 9, 2011, 10:22 a.m.
This patch removes the warning

---8<---
at91_emac.c: In function 'at91emac_write_hwaddr':
at91_emac.c:487:2: warning: dereferencing type-punned pointer will break strict-aliasing rules
--->8---

Signed-off-by: Andreas Bießmann <andreas.devel@gmail.com>
---
BEWARE! This patch is only compile tested!

It is possible, that there is an endianess problem. It would be great, if one
could test it on real hardware!

Reinhard, you could adopt the macb driver also to get the warning there fixed.
If it works on at91sam this way it would also work on at91rm9200 cause they have the
same endianess.

 drivers/net/at91_emac.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
Reinhard Meyer - June 9, 2011, 11:08 a.m.
Dear Andreas Bießmann,
> This patch removes the warning
> 
> ---8<---
> at91_emac.c: In function 'at91emac_write_hwaddr':
> at91_emac.c:487:2: warning: dereferencing type-punned pointer will break strict-aliasing rules
> --->8---
> 
> Signed-off-by: Andreas Bießmann <andreas.devel@gmail.com>
> ---
> BEWARE! This patch is only compile tested!
> 
> It is possible, that there is an endianess problem. It would be great, if one
> could test it on real hardware!

If you have the time, can you make the same patch for macb? I can test it later on our
hardware.

> 
> Reinhard, you could adopt the macb driver also to get the warning there fixed.
> If it works on at91sam this way it would also work on at91rm9200 cause they have the
> same endianess.

See above ;)

> 
>  drivers/net/at91_emac.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/at91_emac.c b/drivers/net/at91_emac.c
> index b09ff8c..97d2739 100644
> --- a/drivers/net/at91_emac.c
> +++ b/drivers/net/at91_emac.c
> @@ -481,11 +481,13 @@ static int at91emac_write_hwaddr(struct eth_device *netdev)
>  	dev = (emac_device *) netdev->priv;
>  
>  	writel(1 << ATMEL_ID_EMAC, &pmc->pcer);
> -	DEBUG_AT91EMAC("init MAC-ADDR %x%x \n",
> -		cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))),
> -		cpu_to_le32(*((u32 *)netdev->enetaddr)));
> -	writel(cpu_to_le32(*((u32 *)netdev->enetaddr)), &emac->sa2l);
> -	writel(cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))), &emac->sa2h);
> +	DEBUG_AT91EMAC("init MAC-ADDR %02x:%02x:%02x:%02x:%02x:%02x\n",
> +		netdev->enetaddr[5], netdev->enetaddr[4], netdev->enetaddr[3],
> +		netdev->enetaddr[2], netdev->enetaddr[1], netdev->enetaddr[0]);
> +	writel( (netdev->enetaddr[0] | netdev->enetaddr[1] << 8 |
> +			netdev->enetaddr[2] << 16 | netdev->enetaddr[3] << 24),
> +			&emac->sa2l);

OUCH, I would think by precedence rules, | comes before << !?!?!
Can you verify and supply a new patch if that holds true?

> +	writel((netdev->enetaddr[4] | netdev->enetaddr[5] << 8), &emac->sa2h);
>  	DEBUG_AT91EMAC("init MAC-ADDR %x%x \n",

If you touch the file next time, can you remove that space before the \n ?

>  		readl(&emac->sa2h), readl(&emac->sa2l));
>  	return 0;

Already applied to u-boot-atmel/master,
Thanks,
Reinhard
Andreas Bießmann - June 9, 2011, 11:26 a.m.
Dear Reinahrd Meyer,

Am 09.06.2011 13:08, schrieb Reinhard Meyer:
> Dear Andreas Bießmann,
>> This patch removes the warning
>>
>> ---8<---
>> at91_emac.c: In function 'at91emac_write_hwaddr':
>> at91_emac.c:487:2: warning: dereferencing type-punned pointer will break strict-aliasing rules
>> --->8---
>>
>> Signed-off-by: Andreas Bießmann <andreas.devel@gmail.com>
>> ---
>> BEWARE! This patch is only compile tested!
>>
>> It is possible, that there is an endianess problem. It would be great, if one
>> could test it on real hardware!
> 
> If you have the time, can you make the same patch for macb? I can test it later on our
> hardware.

will do ..

>>
>> Reinhard, you could adopt the macb driver also to get the warning there fixed.
>> If it works on at91sam this way it would also work on at91rm9200 cause they have the
>> same endianess.
> 
> See above ;)

great ...

>>  	writel(1 << ATMEL_ID_EMAC, &pmc->pcer);
>> -	DEBUG_AT91EMAC("init MAC-ADDR %x%x \n",
>> -		cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))),
>> -		cpu_to_le32(*((u32 *)netdev->enetaddr)));
>> -	writel(cpu_to_le32(*((u32 *)netdev->enetaddr)), &emac->sa2l);
>> -	writel(cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))), &emac->sa2h);
>> +	DEBUG_AT91EMAC("init MAC-ADDR %02x:%02x:%02x:%02x:%02x:%02x\n",
>> +		netdev->enetaddr[5], netdev->enetaddr[4], netdev->enetaddr[3],
>> +		netdev->enetaddr[2], netdev->enetaddr[1], netdev->enetaddr[0]);
>> +	writel( (netdev->enetaddr[0] | netdev->enetaddr[1] << 8 |
>> +			netdev->enetaddr[2] << 16 | netdev->enetaddr[3] << 24),
>> +			&emac->sa2l);
> 
> OUCH, I would think by precedence rules, | comes before << !?!?!

You are wrong, try the following:

---8<---
# cat main.c
#include <stdio.h>

int main(void)
{
    unsigned int test1 = (1 << 8 | 1);
    unsigned int test2 = ((1 << 8) | 1);
    printf("test1 = %x\ntest2 = %x\n", test1, test2);
}

# gcc main.c
# ./a.out
test1 = 101
test2 = 101
--->8---

> Can you verify and supply a new patch if that holds true?

Is the test ok?

> Already applied to u-boot-atmel/master,

But why do you apply the patch, if there are questions?

regards

Andreas Bießmann
Reinhard Meyer - June 9, 2011, 12:06 p.m.
Dear Andreas Bießmann,
>>> +	writel( (netdev->enetaddr[0] | netdev->enetaddr[1] << 8 |
>>> +			netdev->enetaddr[2] << 16 | netdev->enetaddr[3] << 24),
>>> +			&emac->sa2l);
>>
>> OUCH, I would think by precedence rules, | comes before << !?!?!
> 
> You are wrong, try the following:

Yep, just checked the table ;) -
+ comes before <<, but << comes before |, hence the mixup :)

>> Already applied to u-boot-atmel/master,
> 
> But why do you apply the patch, if there are questions?

At first, the  << | seemed ok to me, so I applied ;)

Regards,
Reinhard
Andreas Bießmann - June 9, 2011, 12:13 p.m.
Dear Reinhard Meyer,

Am 09.06.2011 14:06, schrieb Reinhard Meyer:
> Dear Andreas Bießmann,

>>> Already applied to u-boot-atmel/master,
>>
>> But why do you apply the patch, if there are questions?
> 
> At first, the  << | seemed ok to me, so I applied ;)

Well that is ok, but this have never run on real hardware. Therefore it
would be great if one (Jens or Eric) could add their tested-by ..

So please test the v2 for macb and I'm fine with that.

regards

Andreas Bießmann
Andreas Bießmann - June 12, 2011, 10:08 a.m.
Dear Andreas Bießmann,

Am 09.06.2011 um 12:22 schrieb Andreas Bießmann:

> This patch removes the warning
> 
> ---8<---
> at91_emac.c: In function 'at91emac_write_hwaddr':
> at91_emac.c:487:2: warning: dereferencing type-punned pointer will break strict-aliasing rules
> --->8---
> 
> Signed-off-by: Andreas Bießmann <andreas.devel@gmail.com>
> ---
> BEWARE! This patch is only compile tested!

The patch was tested on real hardware and it works.

regards

Andreas Bießmann

Patch

diff --git a/drivers/net/at91_emac.c b/drivers/net/at91_emac.c
index b09ff8c..97d2739 100644
--- a/drivers/net/at91_emac.c
+++ b/drivers/net/at91_emac.c
@@ -481,11 +481,13 @@  static int at91emac_write_hwaddr(struct eth_device *netdev)
 	dev = (emac_device *) netdev->priv;
 
 	writel(1 << ATMEL_ID_EMAC, &pmc->pcer);
-	DEBUG_AT91EMAC("init MAC-ADDR %x%x \n",
-		cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))),
-		cpu_to_le32(*((u32 *)netdev->enetaddr)));
-	writel(cpu_to_le32(*((u32 *)netdev->enetaddr)), &emac->sa2l);
-	writel(cpu_to_le16(*((u16 *)(netdev->enetaddr + 4))), &emac->sa2h);
+	DEBUG_AT91EMAC("init MAC-ADDR %02x:%02x:%02x:%02x:%02x:%02x\n",
+		netdev->enetaddr[5], netdev->enetaddr[4], netdev->enetaddr[3],
+		netdev->enetaddr[2], netdev->enetaddr[1], netdev->enetaddr[0]);
+	writel( (netdev->enetaddr[0] | netdev->enetaddr[1] << 8 |
+			netdev->enetaddr[2] << 16 | netdev->enetaddr[3] << 24),
+			&emac->sa2l);
+	writel((netdev->enetaddr[4] | netdev->enetaddr[5] << 8), &emac->sa2h);
 	DEBUG_AT91EMAC("init MAC-ADDR %x%x \n",
 		readl(&emac->sa2h), readl(&emac->sa2l));
 	return 0;