Hi Gary,
Gary Jennejohn wrote:
Hi Ben,
Ben Warren biggerbadderben@gmail.com wrote:
Gary Jennejohn wrote:
[snip]
#if defined(CONFIG_ETHER_ON_SCC) && defined(CONFIG_CMD_NET)
While you're mucking around with this file, please settle on a single CONFIG that can allow conditional compilation from the Makefile, then get rid of this stuff.
You mean get rid of CONFIG_ETHER_ON_SCC and CONFIG_CMD_NET? But isn't at least CONFIG_CMD_NET required to get networking support in other parts of U-Boot, which would make it a prerequisite for compiling this?
And eliminating or supplementing CONFIG_ETHER_ON_SCC with a new CONFIG would mean changing a whole slew of configuration files, not to mention include/net.h.
I don't mean get rid of them completely, just move the conditionality to the Makefile. IMHO the CONFIG_ETHER_ON_SCC conditional is enough, you don't need to check for CONFIG_CMD_NET. It the user doesn' t have it set, problems will show up all over the place, and very quickly. I'd prefer to change the name of CONFIG_ETHER_ON_SCC to something indicating 82xx-ness, since this driver is specific to the 82xx family of SOCs, but SCCs have been around longer than PowerPC. One thing you'll notice is that most of the config files that mention this option are #undef'ing it only.
-int eth_send(volatile void *packet, int length) +int sec_send(struct eth_device *dev, volatile void *packet, int length)
Please give all these functions, except initialize(), file scope (i.e. make them static). I'm not crazy about the name 'sec', but if it's static the objection doesn't carry much weight. I also can't think of a better name.
Yeah, I should have thought of this when I did the mods.
[snip]
+int sec_initialize(bd_t *bis)
For this function with global namespace, please pick a more descriptive name. Maybe 82xx_scc_initialize() or something?
I called it 82xx_scc_enet_initialize() to make its function clear.
Perfect.
--- a/net/eth.c +++ b/net/eth.c @@ -48,6 +48,8 @@ extern int ppc_4xx_eth_initialize(bd_t *); extern int scc_initialize(bd_t*); extern int npe_initialize(bd_t *); extern int uec_initialize(int); +extern int sec_initialize(bd_t *); +extern int keymile_hdlc_enet_initialize(bd_t *);
#ifdef CONFIG_API extern void (*push_packet)(volatile void *, int); @@ -196,6 +198,12 @@ int eth_initialize(bd_t *bis) #if defined(CONFIG_IXP4XX_NPE) npe_initialize(bis); #endif +#if defined(CONFIG_ETHER_ON_SCC) && defined(CONFIG_MPC8260)
- sec_initialize(bis);
+#endif +#if defined(CONFIG_KEYMILE_HDLC_ENET)
- keymile_hdlc_enet_initialize(bis);
+#endif if (!eth_devices) { puts ("No ethernet found.\n"); show_boot_progress (-64);
Please don't add anything to this file. All initializations now go in cpu_eth_init()/board_eth_init(). There are plenty of examples you can draw from. Don't forget to add your initializer function to include/netdev.h
OK, that should be easy enough. I've now done it for keymile.
You get bonus points if you move this driver to drivers/net
I'll look into it but make no promises.
That's OK, do what you can. We all have priorities.
regards, Ben