[U-Boot] [PATCH] mgcoge make ether_scc.c work with CONFIG_NET_MULTI

[Ben Warren]

Hi Gary,

Gary Jennejohn wrote:
> Hi Ben,
>
> Ben Warren <biggerbadderben at 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