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

[Jerry Van Baren <gerald.vanbaren@ge.com>]

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.

We are about to fall into a never ending explosion of #define 
CONFIG_ETHnnnADDR.  This is bad.  Very bad.  :-(

Kumar solved this problem WRT cpu/mpc83xx/fdt.c fdt_fixup_ethernet(void 
*fdt) (and other CPUs) by using the device tree to find all the 
ethernets and configure them.
   <http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/45554>

Is bdinfo relevant? If not, return;  /* that was easy */

If bdinfo is still relevant, look at how Kumar solved the #define 
explosion in cpu/*/fdt.c.  You should be able to adapt it in the code 
you augmented with CONFIG_ETH[45]ADDR.

Suggestion: Look at changing CONFIG_ETH*ADDR to CONFIG_ETH_FDT where 
that notation indicates the code should find the ethernet info in the 
fdt blob rather than #defines/env variables.

One fly in the ointment is if we don't have a fdt blob to pick the 
ethernet parameters out of.  Perhaps this is because we are going to 
load a fdt blob over the ethernet.  We discussed having a default blob 
(probably the more elegant solution).  A simpler and more "the way it is 
now" solution would be to define a limited number of #define 
CONFIG_ETH*ADDR values (I think 2 would be adequate, but at least limit 
ourselves to the current 4 max).  Then bare board fdt blob loading would 
have to be over one/two/four fixed ethernet ports, but once a valid fdt 
blob was loaded all of the ethernets would be configured properly by u-boot.

> ---
>  README                   |    3 ++
>  common/cmd_bdinfo.c      |   17 +++++++++++++++-
>  common/env_common.c      |    6 +++++
>  common/env_embedded.c    |    6 +++++
>  cpu/mpc83xx/fdt.c        |    3 +-
>  drivers/qe/uec.c         |   48 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/asm-ppc/u-boot.h |    6 +++++
>  lib_ppc/board.c          |   30 ++++++++++++++++++++++++++++
>  net/eth.c                |    6 +++++
>  tools/env/fw_env.c       |    6 +++++
>  10 files changed, 128 insertions(+), 3 deletions(-)
> 
> 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

Bleah!  Oh, right, I said that already.

[snip]

> +#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
> +

No, don't add more cut'n'pastes.  Bleah!  Use a fdt_*() loop (a'la 
Kumar's code).  This would have to be conditional on CONFIG_LIBFDT since 
not all boards use LIBFDT.  If !defined(CONFIG_LIBFDT), use the existing 
code.

> 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

Bleah!  I would prefer to cut our losses at CONFIG_ETH1ADDR (or 
CONFIG_ETH3ADDR), see lead-in discussion.

> 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

Bleah! ...but I repeat myself.

Do we need eth[1-5]?addr env variables?  I don't think so.

If we really do need them, they can be generated from the fdt blob a'la 
Kumar's loop.

> 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)

Bleah!  I counter-propose a single CONFIG_ETH_FDT.  Alternatively, if 
defined(CONFIG_HAS_ETH0), it is a good bet that you want to call 
fdt_fixup_ethernet(blob); and *that* routine handles up all defined 
ethernet spiggots (thanks, Kumar!).

>  	fdt_fixup_ethernet(blob);
>  #endif
>  
> 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

Is this in the fdt blob?  If not, we should add it (possibly 
coordinating with the kernel folks.

> -#define MAXCONTROLLERS	(4)
> +#define MAXCONTROLLERS	(6)
>  
>  static struct eth_device *devlist[MAXCONTROLLERS];

Hmmm, I don't have an clever answer for this one.  Looks like we will 
need a MAXCONTROLLERS to make our devlist[].

> 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

More evil.  I would like to se 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 be able to stop making it bigger.

[snipped remainder of the same]

Thanks,
gvb