[U-Boot] [PATCH] [83xx] Adds two more ethernet interface to 83xx
Jerry Van Baren
gvb.uboot at gmail.com
Fri Sep 26 05:44:23 CEST 2008
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.
> 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.
> 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.)
> 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).
> 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.
> 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!
> #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
More information about the U-Boot
mailing list