[U-Boot] [PATCH] [83xx] Adds two more ethernet interface to 83xx
Jerry Van Baren
gerald.vanbaren at ge.com
Thu Sep 25 22:42:17 CEST 2008
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
More information about the U-Boot
mailing list