[U-Boot] [PATCH] [83xx] Adds two more ethernet interface to 83xx

[Ben Warren]

Jerry Van Baren wrote:
> OK, critique v2 (thanks to Wolfgang calling BS on my previous critique :-).
>
> richardretanubun wrote:
>   
>> Added for convenience for other platforms that uses MPC8360 (has 8 UCC).
>> 6 eth interface is chosen because the platform I am using combines
>> UCC1&2 and UCC3&4 as gigEth and the other 4 UCC as 10/100 Eth.
>>     
>
> [snip]
>
>   
>> diff --git a/README b/README
>> index ccd839c..8802304 100644
>> --- a/README
>> +++ b/README
>> @@ -1095,8 +1095,11 @@ The following options need to be configured:
>>  
>>  - Ethernet address:
>>  		CONFIG_ETHADDR
>> +		CONFIG_ETH1ADDR
>>  		CONFIG_ETH2ADDR
>>  		CONFIG_ETH3ADDR
>> +		CONFIG_ETH4ADDR
>> +		CONFIG_ETH5ADDR
>>  
>>  		Define a default value for Ethernet address to use
>>  		for the respective Ethernet interface, in case this
>>     
>
> OK, the above is unavoidable unless...
>
> A more major change but conceptually slick alternative would be a (weak) 
> function generating appropriate MAC addresses since on most/all boards 
> the MAC addresses of the etherspiggots are mathematically related.  The 
> generation function would use MAXCONTROLLERS (CONFIG_NUM_ETH?) to 
> generate the appropriate number of MAC address env variables.
>
>   
Maybe most/all of the boards worked on by gvb, but I sure wouldn't want 
to generalize much beyond that. In my case, boards usually have one or 
more public ports, requiring registered addresses. These would probably 
be sequential. The other ports usually have private MAC addresses based 
on uniquish things like board serial numbers. And I'm sure there are 
lots of other schemes out there.
>> diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
>> index f4d9d40..67cc64f 100644
>> --- a/common/cmd_bdinfo.c
>> +++ b/common/cmd_bdinfo.c
>> @@ -91,11 +91,12 @@ int do_bdinfo ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>>  	print_str ("pevfreq",	    strmhz(buf, bd->bi_pevfreq));
>>  #endif
>>  
>> +#if defined(CONFIG_HAS_ETH0)
>>  	puts ("ethaddr     =");
>>  	for (i=0; i<6; ++i) {
>>  		printf ("%c%02X", i ? ':' : ' ', bd->bi_enetaddr[i]);
>>  	}
>> -
>> +#endif
>>  #if defined(CONFIG_HAS_ETH1)
>>  	puts ("\neth1addr    =");
>>  	for (i=0; i<6; ++i) {
>> @@ -117,6 +118,20 @@ int do_bdinfo ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>>  	}
>>  #endif
>>  
>> +#if defined(CONFIG_HAS_ETH4)
>> +       puts ("\neth4addr    =");
>> +       for (i=0; i<6; ++i) {
>> +		printf ("%c%02X", i ? ':' : ' ', bd->bi_enet4addr[i]);
>> +	}
>> +#endif
>> +
>> +#if defined(CONFIG_HAS_ETH5)
>> +       puts ("\neth5addr    =");
>> +       for (i=0; i<6; ++i) {
>> +		printf ("%c%02X", i ? ':' : ' ', bd->bi_enet5addr[i]);
>> +	}
>> +#endif
>> +
>>     
>
> Here is where a modification Kumar's loop (I would loop over 
> /aliases/ethernet[N] instead of the env variables) would cut out 5 
> copies of the same code and would scale infinitely.  Hmmm, we have 
> MAXCONTROLLERS (CONFIG_NUM_ETH?) to tell us how many etherspiggots we 
> have, can we loop on that?
>
> I would also *strongly* prefer eliminating bd->bi_enet[N]addr variables, 
> are they needed at all in a fdt-enabled system?  Are there any other 
> places that bd->bi_enet[N]addr is used?  If the only use is here, to 
> print out the MAC addresses, we can do Kumar's loop and print the env 
> variables directly instead of converting the env variable to 
> bd->enet[N]addr and then back into ASCII.  Bleah.
>
>   
Let's try to back away from fdt for a minute. I know this particular 
controller is PowerPC and so fdt is relevant, but the concept of having 
oodles of network ports is hardly PowerPC-specific and so we need to 
think in more general terms.
>> diff --git a/common/env_common.c b/common/env_common.c
>> index 77f9944..0fee3af 100644
>> --- a/common/env_common.c
>> +++ b/common/env_common.c
>> @@ -91,6 +91,12 @@ uchar default_environment[] = {
>>  #ifdef	CONFIG_ETH3ADDR
>>  	"eth3addr="	MK_STR(CONFIG_ETH3ADDR)		"\0"
>>  #endif
>> +#ifdef	CONFIG_ETH4ADDR
>> +	"eth4addr="	MK_STR(CONFIG_ETH4ADDR)		"\0"
>> +#endif
>> +#ifdef	CONFIG_ETH5ADDR
>> +	"eth5addr="	MK_STR(CONFIG_ETH5ADDR)		"\0"
>> +#endif
>>  #ifdef	CONFIG_IPADDR
>>  	"ipaddr="	MK_STR(CONFIG_IPADDR)		"\0"
>>  #endif
>> diff --git a/common/env_embedded.c b/common/env_embedded.c
>> index 77e5619..e79f843 100644
>> --- a/common/env_embedded.c
>> +++ b/common/env_embedded.c
>> @@ -135,6 +135,12 @@ env_t environment __PPCENV__ = {
>>  #ifdef	CONFIG_ETH3ADDR
>>  	"eth3addr="	MK_STR(CONFIG_ETH3ADDR)		"\0"
>>  #endif
>> +#ifdef	CONFIG_ETH4ADDR
>> +	"eth4addr="	MK_STR(CONFIG_ETH4ADDR)		"\0"
>> +#endif
>> +#ifdef	CONFIG_ETH5ADDR
>> +	"eth5addr="	MK_STR(CONFIG_ETH5ADDR)		"\0"
>> +#endif
>>  #ifdef	CONFIG_ETHPRIME
>>  	"ethprime="	CONFIG_ETHPRIME			"\0"
>>  #endif
>>     
>
> OK, the above is unavoidable unless we use a function to generate the 
> sequence of MAC addresses.  (Caution: undeveloped concept.)
>
>   
Since we don't allow anyone to define CONFIG_ETHxADDR in board configs, 
do they really need to exist at all? Sorry, this comment isn't related 
to this particular code snippet, but my fingers won't stop moving.
>> diff --git a/cpu/mpc83xx/fdt.c b/cpu/mpc83xx/fdt.c
>> index 39bd9dc..3e3e1c8 100644
>> --- a/cpu/mpc83xx/fdt.c
>> +++ b/cpu/mpc83xx/fdt.c
>> @@ -52,7 +52,8 @@ void ft_cpu_setup(void *blob, bd_t *bd)
>>  		fdt_fixup_crypto_node(blob, 0x0204);
>>  
>>  #if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) ||\
>> -    defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
>> +    defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3) ||\
>> +    defined(CONFIG_HAS_ETH4) || defined(CONFIG_HAS_ETH5)
>>  	fdt_fixup_ethernet(blob);
>>  #endif
>>     
>
> This is horrible.  We should use CONFIG_HAS_MANY_ETH or something like 
> that.  Maybe we can create CONFIG_NUM_ETH and #define it to be the 
> number of etherspiggots we have (replacing MAXCONTROLLERS - not a good 
> name if it ends up being more widely visible).
>
>   
How about creating a bitmap of present controllers. e.g.

<some header file>
#define ETH0 1<<0
#define ETH1 1<<1
#define ETH2 1<<2
...

<board config file>
#define ETH_PRESENT (ETH0 | ETH2)

and then you check for non-zero or count the bits. It doesn't scale 
well, as we found with CMDs, but 32 for now seems pretty big.
>> diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
>> index 344c649..e1dec5e 100644
>> --- a/drivers/qe/uec.c
>> +++ b/drivers/qe/uec.c
>> @@ -123,8 +123,54 @@ static uec_info_t eth4_uec_info = {
>>  	.enet_interface		= CFG_UEC4_INTERFACE_MODE,
>>  };
>>  #endif
>> +#ifdef CONFIG_UEC_ETH5
>> +static uec_info_t eth5_uec_info = {
>> +	.uf_info		= {
>> +		.ucc_num	= CFG_UEC5_UCC_NUM,
>> +		.rx_clock	= CFG_UEC5_RX_CLK,
>> +		.tx_clock	= CFG_UEC5_TX_CLK,
>> +		.eth_type	= CFG_UEC5_ETH_TYPE,
>> +	},
>> +#if (CFG_UEC5_ETH_TYPE == FAST_ETH)
>> +	.num_threads_tx		= UEC_NUM_OF_THREADS_1,
>> +	.num_threads_rx		= UEC_NUM_OF_THREADS_1,
>> +#else
>> +	.num_threads_tx		= UEC_NUM_OF_THREADS_4,
>> +	.num_threads_rx		= UEC_NUM_OF_THREADS_4,
>> +#endif
>> +	.riscTx			= QE_RISC_ALLOCATION_RISC1_AND_RISC2,
>> +	.riscRx			= QE_RISC_ALLOCATION_RISC1_AND_RISC2,
>> +	.tx_bd_ring_len		= 16,
>> +	.rx_bd_ring_len		= 16,
>> +	.phy_address		= CFG_UEC5_PHY_ADDR,
>> +	.enet_interface		= CFG_UEC5_INTERFACE_MODE,
>> +};
>> +#endif
>> +#ifdef CONFIG_UEC_ETH6
>> +static uec_info_t eth6_uec_info = {
>> +	.uf_info		= {
>> +		.ucc_num	= CFG_UEC6_UCC_NUM,
>> +		.rx_clock	= CFG_UEC6_RX_CLK,
>> +		.tx_clock	= CFG_UEC6_TX_CLK,
>> +		.eth_type	= CFG_UEC6_ETH_TYPE,
>> +	},
>> +#if (CFG_UEC6_ETH_TYPE == FAST_ETH)
>> +	.num_threads_tx		= UEC_NUM_OF_THREADS_1,
>> +	.num_threads_rx		= UEC_NUM_OF_THREADS_1,
>> +#else
>> +	.num_threads_tx		= UEC_NUM_OF_THREADS_4,
>> +	.num_threads_rx		= UEC_NUM_OF_THREADS_4,
>> +#endif
>> +	.riscTx			= QE_RISC_ALLOCATION_RISC1_AND_RISC2,
>> +	.riscRx			= QE_RISC_ALLOCATION_RISC1_AND_RISC2,
>> +	.tx_bd_ring_len		= 16,
>> +	.rx_bd_ring_len		= 16,
>> +	.phy_address		= CFG_UEC6_PHY_ADDR,
>> +	.enet_interface		= CFG_UEC6_INTERFACE_MODE,
>> +};
>> +#endif
>>     
>
> Here I think my previous observation is still applicable: Is this 
> information in the fdt blob?  If not, we should add it (possibly 
> coordinating with the kernel folks.
>
>   
>> -#define MAXCONTROLLERS	(4)
>> +#define MAXCONTROLLERS	(6)
>>     
>
> CONFIG_NUM_ETH? (see above for discussion)
>
>   
>>  static struct eth_device *devlist[MAXCONTROLLERS];
>>  
>> diff --git a/include/asm-ppc/u-boot.h b/include/asm-ppc/u-boot.h
>> index 54ac01d..7451905 100644
>> --- a/include/asm-ppc/u-boot.h
>> +++ b/include/asm-ppc/u-boot.h
>> @@ -111,6 +111,12 @@ typedef struct bd_info {
>>  #ifdef CONFIG_HAS_ETH3
>>  	unsigned char   bi_enet3addr[6];
>>  #endif
>> +#ifdef CONFIG_HAS_ETH4
>> +	unsigned char   bi_enet4addr[6];
>> +#endif
>> +#ifdef CONFIG_HAS_ETH5
>> +	unsigned char   bi_enet5addr[6];
>> +#endif
>>     
>
> Bleah.  I would like to see the bd_info struct go away.  Please check
> if anybody really needs bi_enet[0-5]addr after looking into my
> suggestions/objections above.  If we cannot make bd_info go away, we
> should at least be able to stop making it bigger.
>
>   
It is becoming less and less relevant, but I expect strong resistance...
>> diff --git a/lib_ppc/board.c b/lib_ppc/board.c
>> index c02ac62..d440722 100644
>> --- a/lib_ppc/board.c
>> +++ b/lib_ppc/board.c
>> @@ -944,6 +944,36 @@ void board_init_r (gd_t *id, ulong dest_addr)
>>  	}
>>  #endif
>>  
>> +#ifdef CONFIG_HAS_ETH4
>> +	/* handle 5th ethernet address */
>> +	s = getenv("eth4addr");
>> +#if defined(CONFIG_XPEDITE1K) || defined(CONFIG_METROBOX) || defined(CONFIG_KAREF)
>> +	if (s == NULL)
>> +		board_get_enetaddr(bd->bi_enet4addr);
>> +	else
>> +#endif
>> +	for (i = 0; i < 6; ++i) {
>> +		bd->bi_enet4addr[i] = s ? simple_strtoul (s, &e, 16) : 0;
>> +		if (s)
>> +			s = (*e) ? e + 1 : e;
>> +	}
>> +#endif
>>     
>
> This is a different version of the repetitive code I complain about 
> above (common/cmd_bdinfo.c).  I would like to see a variant of Kumar's 
> loop instead of the #if defined() explosion.
>
> [snip duplicate but with different [N] code]
>
>   
>> diff --git a/net/eth.c b/net/eth.c
>> index 432dd60..ccd871a 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -180,6 +180,12 @@ int eth_initialize(bd_t *bis)
>>  #if defined(CONFIG_UEC_ETH4)
>>  	uec_initialize(3);
>>  #endif
>> +#if defined(CONFIG_UEC_ETH5)
>> +	uec_initialize(4);
>> +#endif
>> +#if defined(CONFIG_UEC_ETH6)
>> +	uec_initialize(5);
>> +#endif
>>     
>
> My kingdom for a loop!
>
>   
Don't worry, this will be gone soon.
>>  #if defined(FEC_ENET) || defined(CONFIG_ETHER_ON_FCC)
>>  	fec_initialize(bis);
>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>> index 46747d3..6e9c34f 100644
>> --- a/tools/env/fw_env.c
>> +++ b/tools/env/fw_env.c
>> @@ -157,6 +157,12 @@ static char default_environment[] = {
>>  #ifdef	CONFIG_ETH3ADDR
>>  	"eth3addr=" MK_STR (CONFIG_ETH3ADDR) "\0"
>>  #endif
>> +#ifdef	CONFIG_ETH4ADDR
>> +	"eth4addr=" MK_STR (CONFIG_ETH4ADDR) "\0"
>> +#endif
>> +#ifdef	CONFIG_ETH5ADDR
>> +	"eth5addr=" MK_STR (CONFIG_ETH5ADDR) "\0"
>> +#endif
>>  #ifdef	CONFIG_ETHPRIME
>>  	"ethprime=" CONFIG_ETHPRIME "\0"
>>  #endif
>>     
>
> Curmudgeonly yours,
> gvb
cheers,
Ben