Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

PATCH New Hong Xu 4 participants 14 messages Jul 29, 2008 – Jul 30, 2008

Subsystems (2)

Networking Arm/Microchip (AT91) SoC support

lore

Share & tools

lore

Hong Xu

author

2008-07-29 02:17 UTC #0 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Ben Warren

2008-07-29 04:57 UTC #2 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

In reply to Hong Xu

Hi Hong,

Thanks for all the hard work. One more thing...
<snip>

Review

Quoted 6 lines

(quoted diff) +6 -0

+#if defined(CONFIG_CMD_NET)
+int board_eth_init(bd_t *bis)
+{
+       return at91sam9_eth_initialize(bis);
+}
+#endif

Here's at91sam9_eth_initialize:

#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) void at91sam9_eth_initialize(bd_t *bi)

{
    macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}

#endif

Issues:
1. It returns void
2. It just calls the MACB initializer.

Please change your code to:

extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);

#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) int board_eth_init(bd_t *bi)

{
    return macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}

#endif

or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.

regards,
Ben

Hong Xu

author

2008-07-29 06:19 UTC #3 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Thanks Ben.
I'll wait for more suggestions and re-generate the whole patch again. Hope I do not bother you guys too much. It's my first time saying something in this list. :-)

Quoted 41 lines · Ben Warren

Hi Hong,
Thanks for all the hard work. One more thing...
<snip>
Review
Quoted 6 lines
(quoted diff) +6 -0
+#if defined(CONFIG_CMD_NET)
+int board_eth_init(bd_t *bis)
+{
+       return at91sam9_eth_initialize(bis);
+}
+#endif
Here's at91sam9_eth_initialize:
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) void at91sam9_eth_initialize(bd_t *bi)
{
   macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
Issues:
1. It returns void
2. It just calls the MACB initializer.
Please change your code to:
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) int board_eth_init(bd_t *bi)
{
   return macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.
regards,
Ben

Jean-Christophe PLAGNIOL-VILLARD

2008-07-29 07:25 UTC #4 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

In reply to Ben Warren

Quoted 30 lines · Ben Warren

Hi Hong,
Thanks for all the hard work. One more thing...
<snip>
Review
Quoted 6 lines
(quoted diff) +6 -0
+#if defined(CONFIG_CMD_NET)
+int board_eth_init(bd_t *bis)
+{
+       return at91sam9_eth_initialize(bis);
+}
+#endif
Here's at91sam9_eth_initialize:
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) void at91sam9_eth_initialize(bd_t *bi)
{
    macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
Issues:
1. It returns void
2. It just calls the MACB initializer.
Please change your code to:
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) int board_eth_init(bd_t *bi)

Quoted 7 lines

{
    return macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.

Can we do a cpu_eth_init instead?

Best Regards,
J.

Hong Xu

author

2008-07-29 08:01 UTC #5 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Quoted 36 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 35 lines · Ben Warren
Hi Hong,
Thanks for all the hard work. One more thing...
<snip>
Review
Quoted 6 lines
(quoted diff) +6 -0
+#if defined(CONFIG_CMD_NET)
+int board_eth_init(bd_t *bis)
+{
+       return at91sam9_eth_initialize(bis);
+}
+#endif
Here's at91sam9_eth_initialize:
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) void at91sam9_eth_initialize(bd_t *bi)
{
    macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
Issues:
1. It returns void
2. It just calls the MACB initializer.
Please change your code to:
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) int board_eth_init(bd_t *bi)
{
    return macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif

Quoted 3 lines

Quoted 2 lines
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.
Can we do a cpu_eth_init instead?

Not all arm926ejs series have built-in ethernet controller (e.g. AT91SAM9261 does not have). If we do it in cpu_eth_init, we may need other more #ifdef_s to distinguish. :-)

Quoted 4 lines

Best Regards,
J.

BR,
Eric

Jean-Christophe PLAGNIOL-VILLARD

2008-07-29 09:01 UTC #6 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Quoted 38 lines · Hong Xu

Quoted 36 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 35 lines · Ben Warren
Hi Hong,
Thanks for all the hard work. One more thing...
<snip>
Review
Quoted 6 lines
(quoted diff) +6 -0
+#if defined(CONFIG_CMD_NET)
+int board_eth_init(bd_t *bis)
+{
+       return at91sam9_eth_initialize(bis);
+}
+#endif
Here's at91sam9_eth_initialize:
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) void at91sam9_eth_initialize(bd_t *bi)
{
    macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif
Issues:
1. It returns void
2. It just calls the MACB initializer.
Please change your code to:
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET) int board_eth_init(bd_t *bi)
{
    return macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
}
#endif

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.
Can we do a cpu_eth_init instead?
Not all arm926ejs series have built-in ethernet controller (e.g. AT91SAM9261 does not have). If we do it in cpu_eth_init, we may need other more #ifdef_s to distinguish. :-)

It already the case, I'll send a patch obout it

Best Regards,
J.

Jean-Christophe PLAGNIOL-VILLARD

2008-07-29 11:03 UTC #7 lore raw

PATCH 1/1 at91sam9: merge ether driver to cpu_eth_init

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

cpu/arm926ejs/at91/ether.c +2 -2

index 7e11fe4..c54f33b 100644
@@ -25,10 +25,10 @@
 #include <common.h>
 #include <asm/arch/hardware.h>
 
+#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET)
 extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
 
-#if defined(CONFIG_MACB) && defined(CONFIG_CMD_NET)
-void at91sam9_eth_initialize(bd_t *bi)
+void cpu_eth_init(bd_t *bi)
 {
 	macb_eth_initialize(0, (void *)AT91_BASE_EMAC, 0x00);
 }

net/eth.c +0 -5

index 38979aa..53c6622 100644
@@ -74,7 +74,6 @@ extern int greth_initialize(bd_t *);
 extern int atngw100_eth_initialize(bd_t *);
 extern int mcffec_initialize(bd_t*);
 extern int mcdmafec_initialize(bd_t*);
-extern int at91sam9_eth_initialize(bd_t *);
 
 #ifdef CONFIG_API
 extern void (*push_packet)(volatile void *, int);

@@ -286,10 +285,6 @@ int eth_initialize(bd_t *bis)
 #if defined(CONFIG_FSLDMAFEC)
 	mcdmafec_initialize(bis);
 #endif
-#if defined(CONFIG_AT91CAP9) || defined(CONFIG_AT91SAM9260) || \
-    defined(CONFIG_AT91SAM9263)
-	at91sam9_eth_initialize(bis);
-#endif
 
 	if (!eth_devices) {
 		puts ("No ethernet found.\n");

Signature · 1 line

1.5.6.2

Thanks. This won't apply against the net/testing branch because I've already removed a bunch of the other initializations. If you don't mind, I'll take care of this one along with many others in the coming few days.

Contradicting what I mentioned in another e-mail, this one does appear to make more sense as a cpu_eth_init, since all three (plus the new one) call only one MACB controller. Not a strong opinion, though, so if anybody wants it as board_eth_init, speak up.

regards,
Ben

Jean-Christophe PLAGNIOL-VILLARD

2008-07-29 14:53 UTC #9 lore raw

PATCH 1/1 at91sam9: merge ether driver to cpu_eth_init

Quoted 4 lines

Thanks. This won't apply against the net/testing branch because I've already removed a bunch of the other initializations. If you don't mind, I'll take care of this one along with many others in the coming few days.

Go ahead

Quoted 6 lines

Contradicting what I mentioned in another e-mail, this one does appear to make more sense as a cpu_eth_init, since all three (plus the new one) call only one MACB controller. Not a strong opinion, though, so if anybody wants it as board_eth_init, speak up.

Personnaly I prefer it in cpu_eth_init because it's cpu dependant not board dependant

Best Regards,
J.

Haavard Skinnemoen

2008-07-29 09:15 UTC #10 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

In reply to Hong Xu

Quoted 6 lines · Hong Xu

Quoted 3 lines
Quoted 2 lines
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.
Can we do a cpu_eth_init instead?
Not all arm926ejs series have built-in ethernet controller (e.g. AT91SAM9261 does not have). If we do it in cpu_eth_init, we may need other more #ifdef_s to distinguish. :-)

In _my_ opinion, I think it's much cleaner to just let the board code decide which and how many ethernet controllers to initialize instead of having some #ifdef maze in the CPU code. But it's not may call.

Oh, and another thing...

Quoted 1 line

Quoted 1 line
Quoted 1 line
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);

We're getting more and more of these. Does anyone know an appropriate header to put it in?

Haavard

Ben Warren

2008-07-29 14:13 UTC #11 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Quoted 12 lines · Haavard Skinnemoen

Quoted 6 lines · Hong Xu
Quoted 3 lines
Quoted 2 lines
or something like this. I don't know if AT91_BASE_EMAC is visible from this code, so you may need to modify slightly.
Can we do a cpu_eth_init instead?
Not all arm926ejs series have built-in ethernet controller (e.g. AT91SAM9261 does not have). If we do it in cpu_eth_init, we may need other more #ifdef_s to distinguish. :-)
In _my_ opinion, I think it's much cleaner to just let the board code decide which and how many ethernet controllers to initialize instead of having some #ifdef maze in the CPU code. But it's not may call.

Agreed

Quoted 7 lines

Oh, and another thing...
Quoted 1 line
Quoted 1 line
Quoted 1 line
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
We're getting more and more of these. Does anyone know an appropriate header to put it in?

At one point I had a netdev.h file, but removed it because it only contained this type of definition. Maybe it's time to resurrect.

regards,
Ben

Haavard Skinnemoen

2008-07-30 07:44 UTC #12 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Quoted 7 lines · Ben Warren

Quoted 5 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
We're getting more and more of these. Does anyone know an appropriate header to put it in?
At one point I had a netdev.h file, but removed it because it only contained this type of definition. Maybe it's time to resurrect.

Yeah, perhaps that would be nice. I get the feeling this isn't the only instance of this kind of thing:

~/work/u-boot/upstream$ find -name '*.c' | xargs grep '^extern' | wc -l 1042

All of those instances are potentially dangerous. The macb_eth_initialize() thing has certainly blown up in my face before...

Btw, shouldn't board_eth_init() and cpu_eth_init() be declared in some header file as well? It might catch the wrong function definitions you've pointed out quite a few times already.

Haavard

Ben Warren

2008-07-30 14:20 UTC #13 lore raw

PATCH 1/1 Re-Submit: Add support for ATMEL AT91SAM9G20EK board.

Quoted 6 lines · Haavard Skinnemoen

Quoted 5 lines · Ben Warren
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);
We're getting more and more of these. Does anyone know an appropriate header to put it in?

Quoted 9 lines

Quoted 2 lines
At one point I had a netdev.h file, but removed it because it only contained this type of definition. Maybe it's time to resurrect.
Yeah, perhaps that would be nice. I get the feeling this isn't the only instance of this kind of thing:
~/work/u-boot/upstream$ find -name '*.c' | xargs grep '^extern' | wc -l 1042

Yikes. I don't mean to be an ass, but did you know that modern grep has a '-R' (recurse) switch?
e.g. $ grep -R --include *.c '^extern' * | wc -l

Quoted 7 lines

All of those instances are potentially dangerous. The macb_eth_initialize() thing has certainly blown up in my face before...
Btw, shouldn't board_eth_init() and cpu_eth_init() be declared in some header file as well? It might catch the wrong function definitions you've pointed out quite a few times already.

Yeah, the fact that I've corrected several wrong return types in one day makes me think it's time.

thanks,
Ben