Patchwork ARM: mxs: read correct values when setting up MAC

login
register
mail settings
Submitter Wolfram Sang
Date Jan. 24, 2012, 6:57 p.m.
Message ID <1327431447-5031-1-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/137611/
State New
Headers show

Comments

Wolfram Sang - Jan. 24, 2012, 6:57 p.m.
Currently, the MAC address from the second ethernet is generated from the
crypto-key (and not a customer reg) because of a wrong index to the ocotp
array.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
---

This looks clearly wrong to me; probably ocotp was a u8 somewhen in the
process? I wonder how it works for these boards. Am I missing something?

 arch/arm/mach-mxs/mach-m28evk.c  |    2 +-
 arch/arm/mach-mxs/mach-mx28evk.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Stefano Babic - Jan. 25, 2012, 2:49 p.m.
On 24/01/2012 19:57, Wolfram Sang wrote:
> Currently, the MAC address from the second ethernet is generated from the
> crypto-key (and not a customer reg) because of a wrong index to the ocotp
> array.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
> 

Hi Wolfram,

> This looks clearly wrong to me; probably ocotp was a u8 somewhen in the
> process? I wonder how it works for these boards. Am I missing something?

Maybe you're right, but let's check with the manual:

> 
>  arch/arm/mach-mxs/mach-m28evk.c  |    2 +-
>  arch/arm/mach-mxs/mach-mx28evk.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-m28evk.c b/arch/arm/mach-mxs/mach-m28evk.c
> index 2f27582..8d1459d 100644
> --- a/arch/arm/mach-mxs/mach-m28evk.c
> +++ b/arch/arm/mach-mxs/mach-m28evk.c
> @@ -258,7 +258,7 @@ static int __init m28evk_fec_get_mac(void)
>  	 * so hard-code DENX OUI (C0:E5:4E) here.
>  	 */
>  	for (i = 0; i < 2; i++) {
> -		val = ocotp[i * 4];
> +		val = ocotp[i];

I can read that HW_OCOTP_CUST0 has the phisycal address 0x8002_C020h,
and HW_OCOTP_CUST1 ist at 8002_C030h. So the two registers are not
consecutive and there are some reserved fields between the two registers
- this is the reason for the i * 4.

HW_OCOTP_CRYPTO0 is at 8002_C060h, so it is not true that the address is
currently read from the crypto-key. At least, this is my interpretation...

Best regards,
Stefano Babic
Wolfram Sang - Jan. 25, 2012, 3:35 p.m.
> > --- a/arch/arm/mach-mxs/mach-m28evk.c
> > +++ b/arch/arm/mach-mxs/mach-m28evk.c
> > @@ -258,7 +258,7 @@ static int __init m28evk_fec_get_mac(void)
> >  	 * so hard-code DENX OUI (C0:E5:4E) here.
> >  	 */
> >  	for (i = 0; i < 2; i++) {
> > -		val = ocotp[i * 4];
> > +		val = ocotp[i];
> 
> I can read that HW_OCOTP_CUST0 has the phisycal address 0x8002_C020h,
> and HW_OCOTP_CUST1 ist at 8002_C030h. So the two registers are not
> consecutive and there are some reserved fields between the two registers
> - this is the reason for the i * 4.
> 
> HW_OCOTP_CRYPTO0 is at 8002_C060h, so it is not true that the address is
> currently read from the crypto-key. At least, this is my interpretation...

Ah, that explains... that you probably never tested the code? What about
this in ocotp.c?

 75         for (i = 0; i < OCOTP_WORD_COUNT; i++)
 76                 ocotp_words[i] = __raw_readl(ocotp_base + OCOTP_WORD_OFFSET +
 77                                                 i * 0x10);
Stefano Babic - Jan. 25, 2012, 4:12 p.m.
On 25/01/2012 16:35, Wolfram Sang wrote:
> 
>>> --- a/arch/arm/mach-mxs/mach-m28evk.c +++
>>> b/arch/arm/mach-mxs/mach-m28evk.c @@ -258,7 +258,7 @@ static
>>> int __init m28evk_fec_get_mac(void) * so hard-code DENX OUI
>>> (C0:E5:4E) here. */ for (i = 0; i < 2; i++) { -		val = ocotp[i
>>> * 4]; +		val = ocotp[i];
>> 
>> I can read that HW_OCOTP_CUST0 has the phisycal address
>> 0x8002_C020h, and HW_OCOTP_CUST1 ist at 8002_C030h. So the two
>> registers are not consecutive and there are some reserved fields
>> between the two registers - this is the reason for the i * 4.
>> 
>> HW_OCOTP_CRYPTO0 is at 8002_C060h, so it is not true that the
>> address is currently read from the crypto-key. At least, this is
>> my interpretation...
> 
> Ah, that explains... that you probably never tested the code?

..and that I don't remember very well...

> What about this in ocotp.c?
> 
> 75         for (i = 0; i < OCOTP_WORD_COUNT; i++) 76
> ocotp_words[i] = __raw_readl(ocotp_base + OCOTP_WORD_OFFSET + 77
> i * 0x10);

You are right - and thanks for fixing it !

Stefano
Wolfram Sang - Jan. 31, 2012, 2:21 p.m.
On Tue, Jan 24, 2012 at 07:57:27PM +0100, Wolfram Sang wrote:
> Currently, the MAC address from the second ethernet is generated from the
> crypto-key (and not a customer reg) because of a wrong index to the ocotp
> array.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Stefano Babic <sbabic@denx.de>
> ---

Ping.

> 
> This looks clearly wrong to me; probably ocotp was a u8 somewhen in the
> process? I wonder how it works for these boards. Am I missing something?
> 
>  arch/arm/mach-mxs/mach-m28evk.c  |    2 +-
>  arch/arm/mach-mxs/mach-mx28evk.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-m28evk.c b/arch/arm/mach-mxs/mach-m28evk.c
> index 2f27582..8d1459d 100644
> --- a/arch/arm/mach-mxs/mach-m28evk.c
> +++ b/arch/arm/mach-mxs/mach-m28evk.c
> @@ -258,7 +258,7 @@ static int __init m28evk_fec_get_mac(void)
>  	 * so hard-code DENX OUI (C0:E5:4E) here.
>  	 */
>  	for (i = 0; i < 2; i++) {
> -		val = ocotp[i * 4];
> +		val = ocotp[i];
>  		mx28_fec_pdata[i].mac[0] = 0xC0;
>  		mx28_fec_pdata[i].mac[1] = 0xE5;
>  		mx28_fec_pdata[i].mac[2] = 0x4E;
> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index fdb0a56..df318ef 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -285,7 +285,7 @@ static int __init mx28evk_fec_get_mac(void)
>  	 * so hard-code Freescale OUI (00:04:9f) here.
>  	 */
>  	for (i = 0; i < 2; i++) {
> -		val = ocotp[i * 4];
> +		val = ocotp[i];
>  		mx28_fec_pdata[i].mac[0] = 0x00;
>  		mx28_fec_pdata[i].mac[1] = 0x04;
>  		mx28_fec_pdata[i].mac[2] = 0x9f;
> -- 
> 1.7.2.5
>
Shawn Guo - Jan. 31, 2012, 3:21 p.m.
On Tue, Jan 31, 2012 at 03:21:55PM +0100, Wolfram Sang wrote:
> On Tue, Jan 24, 2012 at 07:57:27PM +0100, Wolfram Sang wrote:
> > Currently, the MAC address from the second ethernet is generated from the
> > crypto-key (and not a customer reg) because of a wrong index to the ocotp
> > array.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> 
> Ping.
> 
Just applied, thanks.

Patch

diff --git a/arch/arm/mach-mxs/mach-m28evk.c b/arch/arm/mach-mxs/mach-m28evk.c
index 2f27582..8d1459d 100644
--- a/arch/arm/mach-mxs/mach-m28evk.c
+++ b/arch/arm/mach-mxs/mach-m28evk.c
@@ -258,7 +258,7 @@  static int __init m28evk_fec_get_mac(void)
 	 * so hard-code DENX OUI (C0:E5:4E) here.
 	 */
 	for (i = 0; i < 2; i++) {
-		val = ocotp[i * 4];
+		val = ocotp[i];
 		mx28_fec_pdata[i].mac[0] = 0xC0;
 		mx28_fec_pdata[i].mac[1] = 0xE5;
 		mx28_fec_pdata[i].mac[2] = 0x4E;
diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index fdb0a56..df318ef 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -285,7 +285,7 @@  static int __init mx28evk_fec_get_mac(void)
 	 * so hard-code Freescale OUI (00:04:9f) here.
 	 */
 	for (i = 0; i < 2; i++) {
-		val = ocotp[i * 4];
+		val = ocotp[i];
 		mx28_fec_pdata[i].mac[0] = 0x00;
 		mx28_fec_pdata[i].mac[1] = 0x04;
 		mx28_fec_pdata[i].mac[2] = 0x9f;