Hi Joe,

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

The current implementation exposes the eth_device struct to code that needs to access the MAC address. Add a wrapper function for this to abstract away the pointer for this operation.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

arch/mips/cpu/mips32/au1x00/au1x00_eth.c | 2 +- arch/powerpc/cpu/mpc8260/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc85xx/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc8xx/scc.c | 2 +- include/net.h | 8 ++++++++ net/net.c | 2 +- 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c index 4770f56..535d713 100644 --- a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c +++ b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c @@ -238,7 +238,7 @@ static int au1x00_init(struct eth_device* dev, bd_t * bd){ }

    /* Put mac addr in little endian */

-#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() *mac_addr_high = (ea[5] << 8) | (ea[4] ) ; *mac_addr_low = (ea[3] << 24) | (ea[2] << 16) |

I know this is existing code, but (perhaps separately) it might be nice to remove the #define and assign it it to a local variable, i.e.:

unsigned char *ea = eth_get_ethaddr();

            (ea[1] <<  8) | (ea[0]      ) ;

diff --git a/arch/powerpc/cpu/mpc8260/ether_fcc.c b/arch/powerpc/cpu/mpc8260/ether_fcc.c index f9f15b5..f777ba1 100644 --- a/arch/powerpc/cpu/mpc8260/ether_fcc.c +++ b/arch/powerpc/cpu/mpc8260/ether_fcc.c @@ -299,7 +299,7 @@ static int fec_init(struct eth_device* dev, bd_t *bis) * it unique by setting a few bits in the upper byte of the * non-static part of the address. */ -#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->fen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->fen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->fen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/arch/powerpc/cpu/mpc85xx/ether_fcc.c b/arch/powerpc/cpu/mpc85xx/ether_fcc.c index 166dc9e..58d4bfb 100644 --- a/arch/powerpc/cpu/mpc85xx/ether_fcc.c +++ b/arch/powerpc/cpu/mpc85xx/ether_fcc.c @@ -338,7 +338,7 @@ static int fec_init(struct eth_device* dev, bd_t *bis) * it unique by setting a few bits in the upper byte of the * non-static part of the address. */ -#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->fen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->fen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->fen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/arch/powerpc/cpu/mpc8xx/scc.c b/arch/powerpc/cpu/mpc8xx/scc.c index 251966b..66e4014 100644 --- a/arch/powerpc/cpu/mpc8xx/scc.c +++ b/arch/powerpc/cpu/mpc8xx/scc.c @@ -339,7 +339,7 @@ static int scc_init (struct eth_device *dev, bd_t * bis) pram_ptr->sen_gaddr3 = 0x0; /* Group Address Filter 3 (unused) */ pram_ptr->sen_gaddr4 = 0x0; /* Group Address Filter 4 (unused) */

-#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->sen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->sen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->sen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/include/net.h b/include/net.h index 73ea88b..a9579ee 100644 --- a/include/net.h +++ b/include/net.h @@ -111,6 +111,14 @@ struct eth_device *eth_get_dev(void) { return eth_current; }

+static inline unsigned char *eth_get_ethaddr(void) +{

•   if (eth_current)
  

•           return eth_current->enetaddr;
  

•   return NULL;
  

+}

extern struct eth_device *eth_get_dev_by_name(const char *devname); extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ extern int eth_get_dev_index(void); /* get the device index */ diff --git a/net/net.c b/net/net.c index 2bea07b..ddd630c 100644 --- a/net/net.c +++ b/net/net.c @@ -275,7 +275,7 @@ static void NetInitLoop(void) env_changed_id = env_id; } if (eth_get_dev())

•           memcpy(NetOurEther, eth_get_dev()->enetaddr, 6);
  

•           memcpy(NetOurEther, eth_get_ethaddr(), 6);
  
    return;
  

}

1.7.11.5

Regards, Simon

Hi Joe,

On 28 January 2015 at 02:45, Joe Hershberger joe.hershberger@gmail.com wrote:

On Tue, Jan 27, 2015 at 8:33 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Joe,

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

...

The current implementation exposes the eth_device struct to code that needs to access the MAC address. Add a wrapper function for this to abstract away the pointer for this operation.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

arch/mips/cpu/mips32/au1x00/au1x00_eth.c | 2 +- arch/powerpc/cpu/mpc8260/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc85xx/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc8xx/scc.c | 2 +- include/net.h | 8 ++++++++ net/net.c | 2 +- 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c index 4770f56..535d713 100644 --- a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c +++ b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c @@ -238,7 +238,7 @@ static int au1x00_init(struct eth_device* dev, bd_t

• bd){ }

    /* Put mac addr in little endian */
  

-#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() *mac_addr_high = (ea[5] << 8) | (ea[4] ) ; *mac_addr_low = (ea[3] << 24) | (ea[2] << 16) |

I know this is existing code, but (perhaps separately) it might be nice to remove the #define and assign it it to a local variable, i.e.:

unsigned char *ea = eth_get_ethaddr();

I'm sure that if this code is not deleted before it is reworked that this sort of improvement will be made. For this series I just wanted to change as little as possible. I agree that the #define is ugly.

OK sounds good.

Regards, Simon

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

Make it clear that the helper is checking the addr, not setting it.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

net/eth.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/net/eth.c b/net/eth.c index eac4f7b..65e8c77 100644 --- a/net/eth.c +++ b/net/eth.c @@ -153,11 +153,6 @@ static void eth_current_changed(void) setenv("ethact", NULL); }

-static int eth_address_set(unsigned char *addr) -{

•   return memcmp(addr, "\0\0\0\0\0\0", 6);
  

-}

int eth_write_hwaddr(struct eth_device *dev, const char *base_name, int eth_number) { @@ -166,9 +161,9 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,

    eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);

•   if (eth_address_set(env_enetaddr)) {
  

•           if (eth_address_set(dev->enetaddr) &&
  

•                           memcmp(dev->enetaddr, env_enetaddr, 6)) {
  

•   if (!is_zero_ether_addr(env_enetaddr)) {
  

•           if (!is_zero_ether_addr(dev->enetaddr) &&
  

•               memcmp(dev->enetaddr, env_enetaddr, 6)) {
                    printf("\nWarning: %s MAC addresses don't match:\n",
                            dev->name);
                    printf("Address in SROM is         %pM\n",
  

@@ -183,7 +178,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->enetaddr); printf("\nWarning: %s using MAC address from net device\n", dev->name);

•   } else if (!(eth_address_set(dev->enetaddr))) {
  

•   } else if (is_zero_ether_addr(dev->enetaddr)) {
            printf("\nError: %s address not set.\n",
                   dev->name);
            return -EINVAL;
  

-- 1.7.11.5

Hi Joe,

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

First just add support for MAC drivers.

I don't fully understand this partly because my knowledge of the network stack is limited. So I'll make a few comments and we can go from there.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

common/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 23 ++++ net/eth.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 346 insertions(+), 4 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c index a301cc2..9a41cae 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -572,7 +572,7 @@ static int initr_bbmii(void) } #endif

-#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) static int initr_net(void) { puts("Net: "); @@ -841,7 +841,7 @@ init_fnc_t init_sequence_r[] = { #ifdef CONFIG_BITBANGMII initr_bbmii, #endif -#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) INIT_FUNC_WATCHDOG_RESET initr_net, #endif diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c index e6d8a7a..8688cf9 100644 --- a/common/cmd_bdinfo.c +++ b/common/cmd_bdinfo.c @@ -34,6 +34,7 @@ static void print_eth(int idx) printf("%-12s= %s\n", name, val); }

+#ifndef CONFIG_DM_ETH __maybe_unused static void print_eths(void) { @@ -52,6 +53,7 @@ static void print_eths(void) printf("current eth = %s\n", eth_get_name()); printf("ip_addr = %s\n", getenv("ipaddr")); } +#endif

__maybe_unused static void print_lnum(const char *name, unsigned long long value) diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index f17c3c2..b04cbc9 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -33,6 +33,7 @@ enum uclass_id { UCLASS_I2C, /* I2C bus */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */

•   UCLASS_ETH,             /* Network device */
  

Ethernet device?

    UCLASS_COUNT,
    UCLASS_INVALID = -1,

diff --git a/include/net.h b/include/net.h index 7eef9cc..25636e2 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,29 @@ enum eth_state_t { ETH_STATE_ACTIVE };

+#ifdef CONFIG_DM_ETH +struct eth_pdata {

•   phys_addr_t iobase;
  

+};

+struct eth_ops {

•   int (*init)(struct udevice *dev, bd_t *bis);
  

•   int (*send)(struct udevice *dev, void *packet, int length);
  

•   int (*recv)(struct udevice *dev);
  

•   void (*halt)(struct udevice *dev);
  

+#ifdef CONFIG_MCAST_TFTP

•   int (*mcast)(struct udevice *dev, const u8 *enetaddr, u8 set);
  

+#endif

•   int (*write_hwaddr)(struct udevice *dev);
  

+};

+struct udevice *eth_get_dev(void); /* get the current device */ +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */ +int eth_init_state_only(bd_t *bis); /* Set active state */ +void eth_halt_state_only(void); /* Set passive state */ +#endif

+#ifndef CONFIG_DM_ETH struct eth_device { char name[16]; unsigned char enetaddr[6]; diff --git a/net/eth.c b/net/eth.c index c02548c..d245b65 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,321 @@ static int eth_mac_skip(int index) return ((skip_state = getenv(enetvar)) != NULL); }

+static void eth_current_changed(void);

+#ifdef CONFIG_DM_ETH +#include <dm.h>

+struct eth_device_priv {

•   unsigned char enetaddr[6];
  

•   int state;
  

•   int index;
  

•   void *priv;
  

Suggestion: you could have per-child platform data as well as per-child private data. See u-boot-dm/master for this. It is used for I2C and SPI to hold the address of the child on the bus.

You could use it to hold the MAC address. I think this *might* be better because the MAC address can then be set and retained even when the device is probed/removed.

+};

+struct eth_uclass_priv {

•   struct udevice *current;
  

•   int max_index;
  

+};

+static void eth_set_current_to_next(void) +{

•   struct uclass *uc;
  

•   struct eth_uclass_priv *priv;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   priv = uc->priv;
  

•   uclass_next_device(&(priv->current));
  

Note that this will probe the device. I think you are storing the current ethernet device in the uclass, but you could just as well have a static variable in this file if that is easier.

(Also remove internal brackets)

If priv->current is NULL, this will die.

Also to avoid confusion I think you should use uc_priv for the uclass priv local variable, to distinguish it from priv.

•   if (!priv->current)
  

•           uclass_first_device(UCLASS_ETH, &(priv->current));
  

If I understand this correctly, I think you want:

if (priv->current) uclass_next_device(&priv->current) else uclass_first_device(uc, &priv->current)

+}

+struct udevice *eth_get_dev(void) +{

•   struct uclass *uc;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *priv = uc->priv;
  

This is OK so long as the device has been probed. I think it always is because you use uclass_next_device() to get the device.

•   return priv->current;
  

+}

+static void eth_set_dev(struct udevice *dev) +{

•   struct uclass *uc;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *priv = uc->priv;
  

•   priv->current = dev;
  

+}

+unsigned char *eth_get_ethaddr(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

This check should be unnecessary. Same in other cases below.

•                   return priv->enetaddr;
  

•   }
  

•   return NULL;
  

+}

+/* Set active state */ +int eth_init_state_only(bd_t *bis) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_ACTIVE;
  

•   }
  

•   return 0;
  

+} +/* Set passive state */ +void eth_halt_state_only(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_PASSIVE;
  

•   }
  

+}

+int eth_get_dev_index(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   return priv->index;
  

•   }
  

•   return -1;
  

+}

+int eth_init(bd_t *bis) +{

•   struct udevice *current, *old_current, *dev;
  

•   struct uclass *uc;
  

•   current = eth_get_dev();
  

•   if (!current) {
  

•           puts("No ethernet found.\n");
  

•           return -1;
  

•   }
  

•   /* Sync environment with network devices */
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   uclass_foreach_dev(dev, uc) {
  

•           uchar env_enetaddr[6];
  

•           if (eth_getenv_enetaddr_by_index("eth", dev->seq,
  

•                                            env_enetaddr)) {
  

•                   struct eth_device_priv *priv = dev->uclass_priv;
  

•                   if (priv)
  

•                           memcpy(priv->enetaddr, env_enetaddr, 6);
  

•           }
  

•   };
  

•   old_current = current;
  

•   do {
  

•           debug("Trying %s\n", current->name);
  

•           if (current->driver) {
  

•                   const struct eth_ops *ops = current->driver->ops;
  

•                   if (ops->init(current, bis) >= 0) {
  

•                           struct eth_device_priv *priv =
  

•                                   current->uclass_priv;
  

•                           if (priv)
  

•                                   priv->state = ETH_STATE_ACTIVE;
  

•                           return 0;
  

•                   }
  

•           }
  

•           debug("FAIL\n");
  

•           eth_try_another(0);
  

•           current = eth_get_dev();
  

•   } while (old_current != current);
  

•   return -1;
  

-ENODEV?

+}

+void eth_halt(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return;
  

•   if (!current->driver)
  

•           return;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   ops->halt(current);
  

•   struct eth_device_priv *priv = current->uclass_priv;
  

•   if (priv)
  

•           priv->state = ETH_STATE_PASSIVE;
  

+}

+int eth_send(void *packet, int length) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->send(current, packet, length);
  

+}

As a general comment, there is an implicit assumption that only one device can be active at a time. I suppose that is a U-Boot limitation, but we don't need to keep it for driver model, or at least we could permit multiple devices to be probed at a time.

But still I wonder if you should have functions that are passed a udevice, like eth_rx(struct udevice *dev).

+int eth_rx(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->recv(current);
  

+}

+static int eth_write_hwaddr(struct udevice *dev, const char *base_name,

•              int eth_number)
  

+{

•   unsigned char env_enetaddr[6];
  

•   int ret = 0;
  

•   eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
  

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   if (!is_zero_ether_addr(env_enetaddr)) {
  

•           if (!is_zero_ether_addr(priv->enetaddr) &&
  

•               memcmp(priv->enetaddr, env_enetaddr, 6)) {
  

•                   printf("\nWarning: %s MAC addresses don't match:\n",
  

•                          dev->name);
  

•                   printf("Address in SROM is         %pM\n",
  

•                          priv->enetaddr);
  

•                   printf("Address in environment is  %pM\n",
  

•                          env_enetaddr);
  

•           }
  

•           memcpy(priv->enetaddr, env_enetaddr, 6);
  

•   } else if (is_valid_ether_addr(priv->enetaddr)) {
  

•           eth_setenv_enetaddr_by_index(base_name, eth_number,
  

•                                        priv->enetaddr);
  

•           printf("\nWarning: %s using MAC address from net device\n",
  

•                  dev->name);
  

•   } else if (is_zero_ether_addr(priv->enetaddr)) {
  

•           printf("\nError: %s address not set.\n",
  

•                  dev->name);
  

•           return -EINVAL;
  

•   }
  

•   const struct eth_ops *ops = dev->driver->ops;
  

•   if (ops->write_hwaddr && !eth_mac_skip(eth_number)) {
  

•           if (!is_valid_ether_addr(priv->enetaddr)) {
  

•                   printf("\nError: %s address %pM illegal value\n",
  

•                          dev->name, priv->enetaddr);
  

•                   return -EINVAL;
  

•           }
  

•           ret = ops->write_hwaddr(dev);
  

•           if (ret)
  

•                   printf("\nWarning: %s failed to set MAC address\n",
  

•                          dev->name);
  

•   }
  

•   return ret;
  

+}

+static int eth_uclass_init(struct uclass *class) +{

•   bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
  

•   struct eth_uclass_priv *priv = class->priv;
  

•   priv->max_index = 0;
  

What is max_index used for?

•   eth_env_init();
  

•   return 0;
  

+}

+static int eth_post_bind(struct udevice *dev) +{

•   struct udevice *first;
  

•   uclass_first_device(UCLASS_ETH, &first);
  

•   if (first == dev) {
  

•           eth_current_changed();
  

•           eth_set_dev(dev);
  

•   }
  

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   struct eth_uclass_priv *uclass_priv = dev->uclass->priv;
  

•   if (priv) {
  

•           priv->state = ETH_STATE_INIT;
  

•           priv->index = uclass_priv->max_index++;
  

OK I see it is the number of devices. Does this ever decrease?

Anyway, struct udevice has a seq member which can give every device a unique sequence number in the uclass automatically (and if you define DM_UC_FLAG_SEQ_ALIAS then the device tree aliases can provide this numbering)

Otherwise I'm not sure what this function is trying to do.

•   }
  

•   return 0;
  

+}

+static int eth_pre_unbind(struct udevice *dev) +{

•   struct udevice *first;
  

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   uclass_first_device(UCLASS_ETH, &first);
  

•   if (current == dev) {
  

•           if (dev == first)
  

•                   uclass_next_device(&current);
  

•           else
  

•                   current = first;
  

•           eth_current_changed();
  

•           eth_set_dev(current);
  

•   }
  

I'm not sure what this function is trying to do.

•   return 0;
  

+}

+static int eth_post_probe(struct udevice *dev) +{

•   char *ethprime = getenv("ethprime");
  

•   if (ethprime && strcmp(dev->name, ethprime) == 0)
  

•           eth_set_dev(dev);
  

What does this do?

•   if (strchr(dev->name, ' '))
  

•           printf("\nWarning: eth device name \"%s\" has a space!\n",
  

•                  dev->name);
  

BTW if this is an error you could refuse to bind it - e.g. in the eth_post_bind, return -EINVAL

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   if (priv)
  

•           return eth_write_hwaddr(dev, "eth", priv->index);
  

•   return 1;
  

+}

+UCLASS_DRIVER(eth) = {

•   .name           = "eth",
  

•   .id             = UCLASS_ETH,
  

•   .post_bind      = eth_post_bind,
  

•   .pre_unbind     = eth_pre_unbind,
  

•   .post_probe     = eth_post_probe,
  

•   .init           = eth_uclass_init,
  

•   .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
  

•   .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
  

+}; +#endif

+#ifndef CONFIG_DM_ETH /*

• CPU and board-specific Ethernet initializations. Aliased function
• signals caller to move on

@@ -423,6 +738,7 @@ int eth_rx(void)

    return eth_current->recv(eth_current);

} +#endif /* ifndef CONFIG_DM_ETH */

#ifdef CONFIG_API static void eth_save_packet(void *packet, int length) @@ -486,7 +802,7 @@ static void eth_current_changed(void)

void eth_try_another(int first_restart) {

•   static struct eth_device *first_failed;
  

•   static void *first_failed;
    char *ethrotate;
  
    /*
  

@@ -515,7 +831,7 @@ void eth_set_current(void) { static char *act; static int env_changed_id;

•   struct eth_device *old_current;
  

•   void *old_current;
    int     env_id;
  
    if (!eth_get_dev())     /* XXX no current */
  

-- 1.7.11.5

Regards, Simon

Hi Joe,

On 28 January 2015 at 03:22, Joe Hershberger joe.hershberger@gmail.com wrote:

On Tue, Jan 27, 2015 at 8:34 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Joe,

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

...

First just add support for MAC drivers.

I don't fully understand this partly because my knowledge of the network stack is limited. So I'll make a few comments and we can go from there.

Sounds good.

...

...

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

common/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 23 ++++ net/eth.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 346 insertions(+), 4 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c index a301cc2..9a41cae 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -572,7 +572,7 @@ static int initr_bbmii(void) } #endif

-#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) static int initr_net(void) { puts("Net: "); @@ -841,7 +841,7 @@ init_fnc_t init_sequence_r[] = { #ifdef CONFIG_BITBANGMII initr_bbmii, #endif -#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) INIT_FUNC_WATCHDOG_RESET initr_net, #endif diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c index e6d8a7a..8688cf9 100644 --- a/common/cmd_bdinfo.c +++ b/common/cmd_bdinfo.c @@ -34,6 +34,7 @@ static void print_eth(int idx) printf("%-12s= %s\n", name, val); }

+#ifndef CONFIG_DM_ETH __maybe_unused static void print_eths(void) { @@ -52,6 +53,7 @@ static void print_eths(void) printf("current eth = %s\n", eth_get_name()); printf("ip_addr = %s\n", getenv("ipaddr")); } +#endif

__maybe_unused static void print_lnum(const char *name, unsigned long long value) diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index f17c3c2..b04cbc9 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -33,6 +33,7 @@ enum uclass_id { UCLASS_I2C, /* I2C bus */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */

•   UCLASS_ETH,             /* Network device */
  

Ethernet device?

Sure.

...

...

    UCLASS_COUNT,
    UCLASS_INVALID = -1,

diff --git a/include/net.h b/include/net.h index 7eef9cc..25636e2 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,29 @@ enum eth_state_t { ETH_STATE_ACTIVE };

+#ifdef CONFIG_DM_ETH +struct eth_pdata {

•   phys_addr_t iobase;
  

+};

+struct eth_ops {

•   int (*init)(struct udevice *dev, bd_t *bis);
  

•   int (*send)(struct udevice *dev, void *packet, int length);
  

•   int (*recv)(struct udevice *dev);
  

•   void (*halt)(struct udevice *dev);
  

+#ifdef CONFIG_MCAST_TFTP

•   int (*mcast)(struct udevice *dev, const u8 *enetaddr, u8 set);
  

+#endif

•   int (*write_hwaddr)(struct udevice *dev);
  

+};

+struct udevice *eth_get_dev(void); /* get the current device */ +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */ +int eth_init_state_only(bd_t *bis); /* Set active state */ +void eth_halt_state_only(void); /* Set passive state */ +#endif

+#ifndef CONFIG_DM_ETH struct eth_device { char name[16]; unsigned char enetaddr[6]; diff --git a/net/eth.c b/net/eth.c index c02548c..d245b65 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,321 @@ static int eth_mac_skip(int index) return ((skip_state = getenv(enetvar)) != NULL); }

+static void eth_current_changed(void);

+#ifdef CONFIG_DM_ETH +#include <dm.h>

+struct eth_device_priv {

•   unsigned char enetaddr[6];
  

•   int state;
  

•   int index;
  

•   void *priv;
  

Suggestion: you could have per-child platform data as well as per-child private data. See u-boot-dm/master for this. It is used for I2C and SPI to hold the address of the child on the bus.

You could use it to hold the MAC address. I think this *might* be better because the MAC address can then be set and retained even when the device is probed/removed.

So in net.h you'll see that I have a platdata structure as well... the reason I didn't put the MAC address in there is that the data is not sourced from the device tree or the platform data in general. It either comes from the env (default) or it is read from the network MAC's eeprom. I'll need a little more convincing that the MAC should move.

OK. If it only exists when the device is active (probed) then that is fine.

...

...

+};

+struct eth_uclass_priv {

•   struct udevice *current;
  

•   int max_index;
  

+};

+static void eth_set_current_to_next(void) +{

•   struct uclass *uc;
  

•   struct eth_uclass_priv *priv;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   priv = uc->priv;
  

•   uclass_next_device(&(priv->current));
  

Note that this will probe the device. I think you are storing the current ethernet device in the uclass, but you could just as well have a static variable in this file if that is easier.

I started out using a static variable for the current device, but other code in u-boot uses the current device (via the eth_get_dev() function) and I want it to not require and extern to a non-static variable.

OK.

...

(Also remove internal brackets)

Yes... forgot to clean this up.

...

If priv->current is NULL, this will die.

This function should not get called if priv->current is NULL.

...

Also to avoid confusion I think you should use uc_priv for the uclass priv local variable, to distinguish it from priv.

OK.

...

...

•   if (!priv->current)
  

•           uclass_first_device(UCLASS_ETH, &(priv->current));
  

If I understand this correctly, I think you want:

if (priv->current) uclass_next_device(&priv->current) else uclass_first_device(uc, &priv->current)

No... I'm not switching on priv->current being NULL... I get the next, and then if THAT is NULL, I start at the beginning. priv->current must never be NULL (outside of this transition).

This is emulating the behavior that used to exist where the linked list instead of terminating linked back to the head. This way we always cycle through all adapters and never stop.

OK I see.

...

...

+}

+struct udevice *eth_get_dev(void) +{

•   struct uclass *uc;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *priv = uc->priv;
  

This is OK so long as the device has been probed. I think it always is because you use uclass_next_device() to get the device.

Why would the device being probed have any relationship with the priv in the uclass itself? I would expect the uclass priv to be there regardless. This is not the per-device uc_priv.

Yes you are right, I was confused by the 'priv' name.

...

...

•   return priv->current;
  

+}

+static void eth_set_dev(struct udevice *dev) +{

•   struct uclass *uc;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *priv = uc->priv;
  

•   priv->current = dev;
  

+}

+unsigned char *eth_get_ethaddr(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

This check should be unnecessary. Same in other cases below.

So this is guaranteed? In all cases? I ran into some pre-allocated buffers that don't get created if post_probe returns an error, for instance.

If the device is probed then it has uclass_priv. If post_probe() returns an error then the device will be removed (i.e. the probe will fail).

...

...

•                   return priv->enetaddr;
  

•   }
  

•   return NULL;
  

+}

+/* Set active state */ +int eth_init_state_only(bd_t *bis) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_ACTIVE;
  

•   }
  

•   return 0;
  

+} +/* Set passive state */ +void eth_halt_state_only(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_PASSIVE;
  

•   }
  

+}

+int eth_get_dev_index(void) +{

•   struct eth_device_priv *priv;
  

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   return priv->index;
  

•   }
  

•   return -1;
  

+}

+int eth_init(bd_t *bis) +{

•   struct udevice *current, *old_current, *dev;
  

•   struct uclass *uc;
  

•   current = eth_get_dev();
  

•   if (!current) {
  

•           puts("No ethernet found.\n");
  

•           return -1;
  

•   }
  

•   /* Sync environment with network devices */
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   uclass_foreach_dev(dev, uc) {
  

•           uchar env_enetaddr[6];
  

•           if (eth_getenv_enetaddr_by_index("eth", dev->seq,
  

•                                            env_enetaddr)) {
  

•                   struct eth_device_priv *priv = dev->uclass_priv;
  

•                   if (priv)
  

•                           memcpy(priv->enetaddr, env_enetaddr, 6);
  

•           }
  

•   };
  

•   old_current = current;
  

•   do {
  

•           debug("Trying %s\n", current->name);
  

•           if (current->driver) {
  

•                   const struct eth_ops *ops =
  

current->driver->ops;

•                   if (ops->init(current, bis) >= 0) {
  

•                           struct eth_device_priv *priv =
  

•                                   current->uclass_priv;
  

•                           if (priv)
  

•                                   priv->state = ETH_STATE_ACTIVE;
  

•                           return 0;
  

•                   }
  

•           }
  

•           debug("FAIL\n");
  

•           eth_try_another(0);
  

•           current = eth_get_dev();
  

•   } while (old_current != current);
  

•   return -1;
  

-ENODEV?

OK.

...

...

+}

+void eth_halt(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return;
  

•   if (!current->driver)
  

•           return;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   ops->halt(current);
  

•   struct eth_device_priv *priv = current->uclass_priv;
  

•   if (priv)
  

•           priv->state = ETH_STATE_PASSIVE;
  

+}

+int eth_send(void *packet, int length) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->send(current, packet, length);
  

+}

As a general comment, there is an implicit assumption that only one device can be active at a time. I suppose that is a U-Boot limitation, but we don't need to keep it for driver model, or at least we could permit multiple devices to be probed at a time.

I agree that longer term we should look for ways to remove that limitation, but for now everything that uses the network assumes that the "ethact" env var selects the device. There would be a lot of commands to break compatibility with to allow them to specify the device. I say we hold off on that for now.

OK

One thing to be careful of is probing devices before you use them. By using uclass_first_device() you will probe the device. If you want to pick a particular device and probe it, you will need to find the device through another means - e.g. a helper function in the uclass to find the device without probing it.

...

But still I wonder if you should have functions that are passed a udevice, like eth_rx(struct udevice *dev).

Even if those functions did, the commands would need to change too. Future enhancement, I think.

OK

...

...

+int eth_rx(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->recv(current);
  

+}

+static int eth_write_hwaddr(struct udevice *dev, const char *base_name,

•              int eth_number)
  

+{

•   unsigned char env_enetaddr[6];
  

•   int ret = 0;
  

•   eth_getenv_enetaddr_by_index(base_name, eth_number,
  

env_enetaddr);

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   if (!is_zero_ether_addr(env_enetaddr)) {
  

•           if (!is_zero_ether_addr(priv->enetaddr) &&
  

•               memcmp(priv->enetaddr, env_enetaddr, 6)) {
  

•                   printf("\nWarning: %s MAC addresses don't
  

match:\n",

•                          dev->name);
  

•                   printf("Address in SROM is         %pM\n",
  

•                          priv->enetaddr);
  

•                   printf("Address in environment is  %pM\n",
  

•                          env_enetaddr);
  

•           }
  

•           memcpy(priv->enetaddr, env_enetaddr, 6);
  

•   } else if (is_valid_ether_addr(priv->enetaddr)) {
  

•           eth_setenv_enetaddr_by_index(base_name, eth_number,
  

•                                        priv->enetaddr);
  

•           printf("\nWarning: %s using MAC address from net
  

device\n",

•                  dev->name);
  

•   } else if (is_zero_ether_addr(priv->enetaddr)) {
  

•           printf("\nError: %s address not set.\n",
  

•                  dev->name);
  

•           return -EINVAL;
  

•   }
  

•   const struct eth_ops *ops = dev->driver->ops;
  

•   if (ops->write_hwaddr && !eth_mac_skip(eth_number)) {
  

•           if (!is_valid_ether_addr(priv->enetaddr)) {
  

•                   printf("\nError: %s address %pM illegal
  

value\n",

•                          dev->name, priv->enetaddr);
  

•                   return -EINVAL;
  

•           }
  

•           ret = ops->write_hwaddr(dev);
  

•           if (ret)
  

•                   printf("\nWarning: %s failed to set MAC
  

address\n",

•                          dev->name);
  

•   }
  

•   return ret;
  

+}

+static int eth_uclass_init(struct uclass *class) +{

•   bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
  

•   struct eth_uclass_priv *priv = class->priv;
  

•   priv->max_index = 0;
  

What is max_index used for?

For knowing the number of devices that have been bound and allocating the next unused index for the next device.

Is this different from the number of devices in the uclass?

...

...

•   eth_env_init();
  

•   return 0;
  

+}

+static int eth_post_bind(struct udevice *dev) +{

•   struct udevice *first;
  

•   uclass_first_device(UCLASS_ETH, &first);
  

•   if (first == dev) {
  

•           eth_current_changed();
  

•           eth_set_dev(dev);
  

•   }
  

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   struct eth_uclass_priv *uclass_priv = dev->uclass->priv;
  

•   if (priv) {
  

•           priv->state = ETH_STATE_INIT;
  

•           priv->index = uclass_priv->max_index++;
  

OK I see it is the number of devices. Does this ever decrease?

Never decreases.

...

Anyway, struct udevice has a seq member which can give every device a unique sequence number in the uclass automatically (and if you define DM_UC_FLAG_SEQ_ALIAS then the device tree aliases can provide this numbering)

I actually used the seq number originally and that worked fine with platdata. When I moved to the device tree, that's where it fell apart. In the device tree, it is expected that the eth name will include an eth@baseaddr format. The DM code then takes that value that is the baseaddr and sets it as the requested seq. That means I get eth268443648 instead of eth0. No good. So now I keep track of my own seq (index) to make sure I get consistent device names.

That should be fixed now - see u-boot-dm/master. Also the alias approach is useful since it lets you number the devices within the uclass as you wish.

...

Otherwise I'm not sure what this function is trying to do.

I needs to initialize the priv->current variable so that everything that tries to use the network finds a current eth device. It also initializes the state variable, which is also important.

OK, also you can't really probe a device in the post_bind() handler. Probing should only happen when the device is used.

...

...

•   }
  

•   return 0;
  

+}

+static int eth_pre_unbind(struct udevice *dev) +{

•   struct udevice *first;
  

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   uclass_first_device(UCLASS_ETH, &first);
  

•   if (current == dev) {
  

•           if (dev == first)
  

•                   uclass_next_device(&current);
  

•           else
  

•                   current = first;
  

•           eth_current_changed();
  

•           eth_set_dev(current);
  

•   }
  

I'm not sure what this function is trying to do.

Again, this is just ensuring the "current" variable is always valid.

OK. Shouldn't this happen when the device is removed rather than when it is unbound (well I suppose if it happens when removed it doesn't need to happen when unbound).

...

...

•   return 0;
  

+}

+static int eth_post_probe(struct udevice *dev) +{

•   char *ethprime = getenv("ethprime");
  

•   if (ethprime && strcmp(dev->name, ethprime) == 0)
  

•           eth_set_dev(dev);
  

What does this do?

This allows the initial "current" device to be specified by they "ethprime" env var. This is preserving existing behaviors as controlled by these env vars.

OK

...

...

•   if (strchr(dev->name, ' '))
  

•           printf("\nWarning: eth device name \"%s\" has a
  

space!\n",

•                  dev->name);
  

BTW if this is an error you could refuse to bind it - e.g. in the eth_post_bind, return -EINVAL

Good idea.

...

...

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   if (priv)
  

•           return eth_write_hwaddr(dev, "eth", priv->index);
  

•   return 1;
  

+}

+UCLASS_DRIVER(eth) = {

•   .name           = "eth",
  

•   .id             = UCLASS_ETH,
  

•   .post_bind      = eth_post_bind,
  

•   .pre_unbind     = eth_pre_unbind,
  

•   .post_probe     = eth_post_probe,
  

•   .init           = eth_uclass_init,
  

•   .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
  

•   .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
  

+}; +#endif

+#ifndef CONFIG_DM_ETH /*

• CPU and board-specific Ethernet initializations. Aliased function
• signals caller to move on

@@ -423,6 +738,7 @@ int eth_rx(void)

    return eth_current->recv(eth_current);

} +#endif /* ifndef CONFIG_DM_ETH */

#ifdef CONFIG_API static void eth_save_packet(void *packet, int length) @@ -486,7 +802,7 @@ static void eth_current_changed(void)

void eth_try_another(int first_restart) {

•   static struct eth_device *first_failed;
  

•   static void *first_failed;
    char *ethrotate;
  
    /*
  

@@ -515,7 +831,7 @@ void eth_set_current(void) { static char *act; static int env_changed_id;

•   struct eth_device *old_current;
  

•   void *old_current;
    int     env_id;
  
    if (!eth_get_dev())     /* XXX no current */
  

Regards, Simon

Hi Joe,

On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:

Add basic network support to sandbox which includes a network driver.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

arch/sandbox/dts/sandbox.dts | 4 ++ drivers/net/Makefile | 2 + drivers/net/sandbox.c | 91 ++++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h | 14 ++++--- 4 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 drivers/net/sandbox.c

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 11748ae..a1d3199 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -174,4 +174,8 @@ }; };

•   eth@10002000 {
  

•           compatible = "sandbox,eth";
  

•           reg = <0x10002000 0x1000>;
  

•   };
  

}; diff --git a/drivers/net/Makefile b/drivers/net/Makefile index 46c4ac6..2659a8a 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -50,6 +50,8 @@ obj-$(CONFIG_NS8382X) += ns8382x.o obj-$(CONFIG_PCNET) += pcnet.o obj-$(CONFIG_RTL8139) += rtl8139.o obj-$(CONFIG_RTL8169) += rtl8169.o +obj-$(CONFIG_ETH_SANDBOX) += sandbox.o +obj-$(CONFIG_ETH_SANDBOX_RAW) += sandbox-raw.o obj-$(CONFIG_SH_ETHER) += sh_eth.o obj-$(CONFIG_SMC91111) += smc91111.o obj-$(CONFIG_SMC911X) += smc911x.o diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c new file mode 100644 index 0000000..e1ee69b --- /dev/null +++ b/drivers/net/sandbox.c @@ -0,0 +1,91 @@ +/*

• • Copyright (c) 2015 National Instruments
• • (C) Copyright 2015
• • Joe Hershberger joe.hershberger@ni.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <fdtdec.h> +#include <malloc.h> +#include <net.h>

+DECLARE_GLOBAL_DATA_PTR;

+struct eth_sandbox_priv {

•   void *device;
  

•   int sd;
  

I'm not sure what these are for.

+};

+int sb_eth_init(struct udevice *dev, bd_t *bis) +{

•   printf("eth_sandbox: Init\n");
  

debug()?

•   return 0;
  

+}

+int sb_eth_send(struct udevice *dev, void *packet, int length) +{

•   printf("eth_sandbox: Send packet %d\n", length);
  

•   return 0;
  

+#endif +}

+int sb_eth_recv(struct udevice *dev) +{

•   return 0;
  

+}

+void sb_eth_halt(struct udevice *dev) +{

•   printf("eth_sandbox: Halt\n");
  

+}

+int sb_eth_write_hwaddr(struct udevice *dev) +{

•   printf("eth_sandbox: Write HW ADDR\n");
  

•   return 0;
  

+}

+static const struct eth_ops eth_sandbox_ops = {

•   .init                   = sb_eth_init,
  

•   .send                   = sb_eth_send,
  

•   .recv                   = sb_eth_recv,
  

•   .halt                   = sb_eth_halt,
  

•   .write_hwaddr           = sb_eth_write_hwaddr,
  

+};

+static int eth_sandbox_remove(struct udevice *dev) +{

•   return 0;
  

+}

+#ifdef CONFIG_OF_CONTROL +static int sandbox_eth_ofdata_to_platdata(struct udevice *dev) +{

•   struct eth_pdata *pdata = dev->platdata;
  

•   pdata->iobase = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
  

•   return 0;
  

+}

+static const struct udevice_id sandbox_eth_ids[] = {

•   { .compatible = "sandbox,eth" },
  

•   { }
  

+}; +#endif

+U_BOOT_DRIVER(eth_sandbox) = {

•   .name   = "eth_sandbox",
  

•   .id     = UCLASS_ETH,
  

•   .of_match = of_match_ptr(sandbox_eth_ids),
  

•   .ofdata_to_platdata = of_match_ptr(sandbox_eth_ofdata_to_platdata),
  

•   .remove = eth_sandbox_remove,
  

•   .ops    = &eth_sandbox_ops,
  

•   .priv_auto_alloc_size = sizeof(struct eth_sandbox_priv),
  

•   .platdata_auto_alloc_size = sizeof(struct eth_pdata),
  

+}; diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 657f751..67bfc52 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -143,9 +143,9 @@ /* include default commands */ #include <config_cmd_default.h>

-/* We don't have networking support yet */ -#undef CONFIG_CMD_NET -#undef CONFIG_CMD_NFS +#define CONFIG_DM_ETH +#define CONFIG_ETH_SANDBOX +#define CONFIG_CMD_PING

#define CONFIG_CMD_HASH #define CONFIG_HASH_VERIFY @@ -188,12 +188,16 @@

#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial,cros-ec-keyb\0" \ "stdout=serial,lcd\0" \

•                                   "stderr=serial,lcd\0"
  

•                                   "stderr=serial,lcd\0" \
  

•                                   "ethaddr=00:00:11:22:33:44\0" \
  

•                                   "ipaddr=1.2.3.4\0"
  

#else

#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial\0" \ "stdout=serial,lcd\0" \

•                                   "stderr=serial,lcd\0"
  

•                                   "stderr=serial,lcd\0" \
  

•                                   "ethaddr=00:00:11:22:33:44\0" \
  

•                                   "ipaddr=1.2.3.4\0"
  

#endif

#define CONFIG_GZIP_COMPRESSED

1.7.11.5

Regards, Simon

The current implementation exposes the eth_device struct to code that needs to access the MAC address. Add a wrapper function for this to abstract away the pointer for this operation.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---

Changes in v2: None

arch/mips/cpu/mips32/au1x00/au1x00_eth.c | 2 +- arch/powerpc/cpu/mpc8260/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc85xx/ether_fcc.c | 2 +- arch/powerpc/cpu/mpc8xx/scc.c | 2 +- include/net.h | 8 ++++++++ net/net.c | 2 +- 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c index 4770f56..535d713 100644 --- a/arch/mips/cpu/mips32/au1x00/au1x00_eth.c +++ b/arch/mips/cpu/mips32/au1x00/au1x00_eth.c @@ -238,7 +238,7 @@ static int au1x00_init(struct eth_device* dev, bd_t * bd){ }

/* Put mac addr in little endian */ -#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() *mac_addr_high = (ea[5] << 8) | (ea[4] ) ; *mac_addr_low = (ea[3] << 24) | (ea[2] << 16) | (ea[1] << 8) | (ea[0] ) ; diff --git a/arch/powerpc/cpu/mpc8260/ether_fcc.c b/arch/powerpc/cpu/mpc8260/ether_fcc.c index f9f15b5..f777ba1 100644 --- a/arch/powerpc/cpu/mpc8260/ether_fcc.c +++ b/arch/powerpc/cpu/mpc8260/ether_fcc.c @@ -299,7 +299,7 @@ static int fec_init(struct eth_device* dev, bd_t *bis) * it unique by setting a few bits in the upper byte of the * non-static part of the address. */ -#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->fen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->fen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->fen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/arch/powerpc/cpu/mpc85xx/ether_fcc.c b/arch/powerpc/cpu/mpc85xx/ether_fcc.c index 166dc9e..58d4bfb 100644 --- a/arch/powerpc/cpu/mpc85xx/ether_fcc.c +++ b/arch/powerpc/cpu/mpc85xx/ether_fcc.c @@ -338,7 +338,7 @@ static int fec_init(struct eth_device* dev, bd_t *bis) * it unique by setting a few bits in the upper byte of the * non-static part of the address. */ -#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->fen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->fen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->fen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/arch/powerpc/cpu/mpc8xx/scc.c b/arch/powerpc/cpu/mpc8xx/scc.c index 251966b..66e4014 100644 --- a/arch/powerpc/cpu/mpc8xx/scc.c +++ b/arch/powerpc/cpu/mpc8xx/scc.c @@ -339,7 +339,7 @@ static int scc_init (struct eth_device *dev, bd_t * bis) pram_ptr->sen_gaddr3 = 0x0; /* Group Address Filter 3 (unused) */ pram_ptr->sen_gaddr4 = 0x0; /* Group Address Filter 4 (unused) */

-#define ea eth_get_dev()->enetaddr +#define ea eth_get_ethaddr() pram_ptr->sen_paddrh = (ea[5] << 8) + ea[4]; pram_ptr->sen_paddrm = (ea[3] << 8) + ea[2]; pram_ptr->sen_paddrl = (ea[1] << 8) + ea[0]; diff --git a/include/net.h b/include/net.h index 73ea88b..a9579ee 100644 --- a/include/net.h +++ b/include/net.h @@ -111,6 +111,14 @@ struct eth_device *eth_get_dev(void) { return eth_current; } + +static inline unsigned char *eth_get_ethaddr(void) +{ + if (eth_current) + return eth_current->enetaddr; + return NULL; +} + extern struct eth_device *eth_get_dev_by_name(const char *devname); extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ extern int eth_get_dev_index(void); /* get the device index */ diff --git a/net/net.c b/net/net.c index 2bea07b..ddd630c 100644 --- a/net/net.c +++ b/net/net.c @@ -275,7 +275,7 @@ static void NetInitLoop(void) env_changed_id = env_id; } if (eth_get_dev()) - memcpy(NetOurEther, eth_get_dev()->enetaddr, 6); + memcpy(NetOurEther, eth_get_ethaddr(), 6);

return; }

Make it clear that the helper is checking the addr, not setting it.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---

Changes in v2: None

net/eth.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/eth.c b/net/eth.c index eac4f7b..65e8c77 100644 --- a/net/eth.c +++ b/net/eth.c @@ -153,11 +153,6 @@ static void eth_current_changed(void) setenv("ethact", NULL); }

-static int eth_address_set(unsigned char *addr) -{ - return memcmp(addr, "\0\0\0\0\0\0", 6); -} - int eth_write_hwaddr(struct eth_device *dev, const char *base_name, int eth_number) { @@ -166,9 +161,9 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,

eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);

- if (eth_address_set(env_enetaddr)) { - if (eth_address_set(dev->enetaddr) && - memcmp(dev->enetaddr, env_enetaddr, 6)) { + if (!is_zero_ether_addr(env_enetaddr)) { + if (!is_zero_ether_addr(dev->enetaddr) && + memcmp(dev->enetaddr, env_enetaddr, 6)) { printf("\nWarning: %s MAC addresses don't match:\n", dev->name); printf("Address in SROM is %pM\n", @@ -183,7 +178,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->enetaddr); printf("\nWarning: %s using MAC address from net device\n", dev->name); - } else if (!(eth_address_set(dev->enetaddr))) { + } else if (is_zero_ether_addr(dev->enetaddr)) { printf("\nError: %s address not set.\n", dev->name); return -EINVAL;

Many of the functions in net.h were preceded extern needlessly. Removing them to limit the number of checkpatch.pl complaints.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---

Changes in v2: None

include/net.h | 96 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/include/net.h b/include/net.h index a9579ee..ff8b7af 100644 --- a/include/net.h +++ b/include/net.h @@ -97,11 +97,11 @@ struct eth_device { void *priv; };

-extern int eth_initialize(bd_t *bis); /* Initialize network subsystem */ -extern int eth_register(struct eth_device* dev);/* Register network device */ -extern int eth_unregister(struct eth_device *dev);/* Remove network device */ -extern void eth_try_another(int first_restart); /* Change the device */ -extern void eth_set_current(void); /* set nterface to ethcur var */ +int eth_initialize(bd_t *bis); /* Initialize network subsystem */ +int eth_register(struct eth_device *dev);/* Register network device */ +int eth_unregister(struct eth_device *dev);/* Remove network device */ +void eth_try_another(int first_restart); /* Change the device */ +void eth_set_current(void); /* set nterface to ethcur var */

/* get the current device MAC */ extern struct eth_device *eth_current; @@ -119,12 +119,12 @@ static inline unsigned char *eth_get_ethaddr(void) return NULL; }

-extern struct eth_device *eth_get_dev_by_name(const char *devname); -extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ -extern int eth_get_dev_index(void); /* get the device index */ -extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); -extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); -extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); +struct eth_device *eth_get_dev_by_name(const char *devname); +struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ +int eth_get_dev_index(void); /* get the device index */ +void eth_parse_enetaddr(const char *addr, uchar *enetaddr); +int eth_getenv_enetaddr(char *name, uchar *enetaddr); +int eth_setenv_enetaddr(char *name, const uchar *enetaddr);

/* * Get the hardware address for an ethernet interface . @@ -135,20 +135,20 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); * Returns: * Return true if the address is valid. */ -extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, - uchar *enetaddr); +int eth_getenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr);

-extern int usb_eth_initialize(bd_t *bi); -extern int eth_init(bd_t *bis); /* Initialize the device */ -extern int eth_send(void *packet, int length); /* Send a packet */ +int usb_eth_initialize(bd_t *bi); +int eth_init(bd_t *bis); /* Initialize the device */ +int eth_send(void *packet, int length); /* Send a packet */

#ifdef CONFIG_API -extern int eth_receive(void *packet, int length); /* Receive a packet*/ -extern void (*push_packet)(void *packet, int length); +int eth_receive(void *packet, int length); /* Receive a packet*/ +void (*push_packet)(void *packet, int length); #endif -extern int eth_rx(void); /* Check for received packets */ -extern void eth_halt(void); /* stop SCC */ -extern char *eth_get_name(void); /* get name of current device */ +int eth_rx(void); /* Check for received packets */ +void eth_halt(void); /* stop SCC */ +char *eth_get_name(void); /* get name of current device */

/* Set active state */ static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis) @@ -469,25 +469,25 @@ extern IPaddr_t Mcast_addr; #endif

/* Initialize the network adapter */ -extern void net_init(void); -extern int NetLoop(enum proto_t); +void net_init(void); +int NetLoop(enum proto_t);

/* Shutdown adapters and cleanup */ -extern void NetStop(void); +void NetStop(void);

/* Load failed. Start again. */ -extern void NetStartAgain(void); +void NetStartAgain(void);

/* Get size of the ethernet header when we send */ -extern int NetEthHdrSize(void); +int NetEthHdrSize(void);

/* Set ethernet header; returns the size of the header */ -extern int NetSetEther(uchar *, uchar *, uint); -extern int net_update_ether(struct ethernet_hdr *et, uchar *addr, uint prot); +int NetSetEther(uchar *, uchar *, uint); +int net_update_ether(struct ethernet_hdr *et, uchar *addr, uint prot);

/* Set IP header */ -extern void net_set_ip_header(uchar *pkt, IPaddr_t dest, IPaddr_t source); -extern void net_set_udp_header(uchar *pkt, IPaddr_t dest, int dport, +void net_set_ip_header(uchar *pkt, IPaddr_t dest, IPaddr_t source); +void net_set_udp_header(uchar *pkt, IPaddr_t dest, int dport, int sport, int len);

/** @@ -521,16 +521,16 @@ unsigned add_ip_checksums(unsigned offset, unsigned sum, unsigned new_sum); int ip_checksum_ok(const void *addr, unsigned nbytes);

/* Checksum */ -extern int NetCksumOk(uchar *, int); /* Return true if cksum OK */ -extern uint NetCksum(uchar *, int); /* Calculate the checksum */ +int NetCksumOk(uchar *, int); /* Return true if cksum OK */ +uint NetCksum(uchar *, int); /* Calculate the checksum */

/* Callbacks */ -extern rxhand_f *net_get_udp_handler(void); /* Get UDP RX packet handler */ -extern void net_set_udp_handler(rxhand_f *); /* Set UDP RX packet handler */ -extern rxhand_f *net_get_arp_handler(void); /* Get ARP RX packet handler */ -extern void net_set_arp_handler(rxhand_f *); /* Set ARP RX packet handler */ -extern void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */ -extern void NetSetTimeout(ulong, thand_f *);/* Set timeout handler */ +rxhand_f *net_get_udp_handler(void); /* Get UDP RX packet handler */ +void net_set_udp_handler(rxhand_f *); /* Set UDP RX packet handler */ +rxhand_f *net_get_arp_handler(void); /* Get ARP RX packet handler */ +void net_set_arp_handler(rxhand_f *); /* Set ARP RX packet handler */ +void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */ +void NetSetTimeout(ulong, thand_f *);/* Set timeout handler */

/* Network loop state */ enum net_loop_state { @@ -563,11 +563,11 @@ static inline void NetSendPacket(uchar *pkt, int len) * @param sport Source UDP port * @param payload_len Length of data after the UDP header */ -extern int NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, +int NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int payload_len);

/* Processes a received packet */ -extern void NetReceive(uchar *, int); +void NetReceive(uchar *, int);

#ifdef CONFIG_NETCONSOLE void NcStart(void); @@ -715,28 +715,28 @@ static inline void eth_random_addr(uchar *addr) }

/* Convert an IP address to a string */ -extern void ip_to_string(IPaddr_t x, char *s); +void ip_to_string(IPaddr_t x, char *s);

/* Convert a string to ip address */ -extern IPaddr_t string_to_ip(const char *s); +IPaddr_t string_to_ip(const char *s);

/* Convert a VLAN id to a string */ -extern void VLAN_to_string(ushort x, char *s); +void VLAN_to_string(ushort x, char *s);

/* Convert a string to a vlan id */ -extern ushort string_to_VLAN(const char *s); +ushort string_to_VLAN(const char *s);

/* read a VLAN id from an environment variable */ -extern ushort getenv_VLAN(char *); +ushort getenv_VLAN(char *);

/* copy a filename (allow for "..." notation, limit length) */ -extern void copy_filename(char *dst, const char *src, int size); +void copy_filename(char *dst, const char *src, int size);

/* get a random source port */ -extern unsigned int random_port(void); +unsigned int random_port(void);

/* Update U-Boot over TFTP */ -extern int update_tftp(ulong addr); +int update_tftp(ulong addr);

/**********************************************************************/

Move some things around and organize things so that the driver model implementation will fit in more easily.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---

Changes in v2: None

include/net.h | 63 +++++++++++++++++++++++++------------------------ net/eth.c | 75 +++++++++++++++++++++++++++++++---------------------------- 2 files changed, 73 insertions(+), 65 deletions(-)

diff --git a/include/net.h b/include/net.h index ff8b7af..7eef9cc 100644 --- a/include/net.h +++ b/include/net.h @@ -100,10 +100,7 @@ struct eth_device { int eth_initialize(bd_t *bis); /* Initialize network subsystem */ int eth_register(struct eth_device *dev);/* Register network device */ int eth_unregister(struct eth_device *dev);/* Remove network device */ -void eth_try_another(int first_restart); /* Change the device */ -void eth_set_current(void); /* set nterface to ethcur var */

-/* get the current device MAC */ extern struct eth_device *eth_current;

static inline __attribute__((always_inline)) @@ -112,6 +109,7 @@ struct eth_device *eth_get_dev(void) return eth_current; }

+/* get the current device MAC */ static inline unsigned char *eth_get_ethaddr(void) { if (eth_current) @@ -119,6 +117,37 @@ static inline unsigned char *eth_get_ethaddr(void) return NULL; }

+/* Set active state */ +static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis) +{ + eth_get_dev()->state = ETH_STATE_ACTIVE; + + return 0; +} +/* Set passive state */ +static inline __attribute__((always_inline)) void eth_halt_state_only(void) +{ + eth_get_dev()->state = ETH_STATE_PASSIVE; +} + +/* + * Set the hardware address for an ethernet interface based on 'eth%daddr' + * environment variable (or just 'ethaddr' if eth_number is 0). + * Args: + * base_name - base name for device (normally "eth") + * eth_number - value of %d (0 for first device of this type) + * Returns: + * 0 is success, non-zero is error status from driver. + */ +int eth_write_hwaddr(struct eth_device *dev, const char *base_name, + int eth_number); + +int usb_eth_initialize(bd_t *bi); +#endif + +void eth_try_another(int first_restart); /* Change the device */ +void eth_set_current(void); /* set nterface to ethcur var */ + struct eth_device *eth_get_dev_by_name(const char *devname); struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ int eth_get_dev_index(void); /* get the device index */ @@ -138,7 +167,6 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr); int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr);

-int usb_eth_initialize(bd_t *bi); int eth_init(bd_t *bis); /* Initialize the device */ int eth_send(void *packet, int length); /* Send a packet */

@@ -148,32 +176,7 @@ void (*push_packet)(void *packet, int length); #endif int eth_rx(void); /* Check for received packets */ void eth_halt(void); /* stop SCC */ -char *eth_get_name(void); /* get name of current device */ - -/* Set active state */ -static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis) -{ - eth_get_dev()->state = ETH_STATE_ACTIVE; - - return 0; -} -/* Set passive state */ -static inline __attribute__((always_inline)) void eth_halt_state_only(void) -{ - eth_get_dev()->state = ETH_STATE_PASSIVE; -} - -/* - * Set the hardware address for an ethernet interface based on 'eth%daddr' - * environment variable (or just 'ethaddr' if eth_number is 0). - * Args: - * base_name - base name for device (normally "eth") - * eth_number - value of %d (0 for first device of this type) - * Returns: - * 0 is success, non-zero is error status from driver. - */ -int eth_write_hwaddr(struct eth_device *dev, const char *base_name, - int eth_number); +const char *eth_get_name(void); /* get name of current device */

#ifdef CONFIG_MCAST_TFTP int eth_mcast_join(IPaddr_t mcast_addr, u8 join); diff --git a/net/eth.c b/net/eth.c index 65e8c77..c02548c 100644 --- a/net/eth.c +++ b/net/eth.c @@ -55,6 +55,14 @@ static inline int eth_setenv_enetaddr_by_index(const char *base_name, int index, return eth_setenv_enetaddr(enetvar, enetaddr); }

+static void eth_env_init(void) +{ + const char *s; + + s = getenv("bootfile"); + if (s != NULL) + copy_filename(BootFile, s, sizeof(BootFile)); +}

static int eth_mac_skip(int index) { @@ -87,6 +95,11 @@ static unsigned int eth_rcv_current, eth_rcv_last; static struct eth_device *eth_devices; struct eth_device *eth_current;

+static void eth_set_current_to_next(void) +{ + eth_current = eth_current->next; +} + struct eth_device *eth_get_dev_by_name(const char *devname) { struct eth_device *dev, *target_dev; @@ -137,22 +150,6 @@ int eth_get_dev_index(void) return eth_current->index; }

-static void eth_current_changed(void) -{ - char *act = getenv("ethact"); - /* update current ethernet name */ - if (eth_current) { - if (act == NULL || strcmp(act, eth_current->name) != 0) - setenv("ethact", eth_current->name); - } - /* - * remove the variable completely if there is no active - * interface - */ - else if (act != NULL) - setenv("ethact", NULL); -} - int eth_write_hwaddr(struct eth_device *dev, const char *base_name, int eth_number) { @@ -251,14 +248,6 @@ int eth_unregister(struct eth_device *dev) return 0; }

-static void eth_env_init(bd_t *bis) -{ - const char *s; - - if ((s = getenv("bootfile")) != NULL) - copy_filename(BootFile, s, sizeof(BootFile)); -} - int eth_initialize(bd_t *bis) { int num_devices = 0; @@ -479,6 +468,22 @@ int eth_receive(void *packet, int length) } #endif /* CONFIG_API */

+static void eth_current_changed(void) +{ + char *act = getenv("ethact"); + /* update current ethernet name */ + if (eth_get_dev()) { + if (act == NULL || strcmp(act, eth_get_name()) != 0) + setenv("ethact", eth_get_name()); + } + /* + * remove the variable completely if there is no active + * interface + */ + else if (act != NULL) + setenv("ethact", NULL); +} + void eth_try_another(int first_restart) { static struct eth_device *first_failed; @@ -492,17 +497,17 @@ void eth_try_another(int first_restart) if ((ethrotate != NULL) && (strcmp(ethrotate, "no") == 0)) return;

- if (!eth_current) + if (!eth_get_dev()) return;

if (first_restart) - first_failed = eth_current; + first_failed = eth_get_dev();

- eth_current = eth_current->next; + eth_set_current_to_next();

eth_current_changed();

- if (first_failed == eth_current) + if (first_failed == eth_get_dev()) NetRestartWrap = 1; }

@@ -513,7 +518,7 @@ void eth_set_current(void) struct eth_device *old_current; int env_id;

- if (!eth_current) /* XXX no current */ + if (!eth_get_dev()) /* XXX no current */ return;

env_id = get_env_id(); @@ -522,18 +527,18 @@ void eth_set_current(void) env_changed_id = env_id; } if (act != NULL) { - old_current = eth_current; + old_current = eth_get_dev(); do { - if (strcmp(eth_current->name, act) == 0) + if (strcmp(eth_get_name(), act) == 0) return; - eth_current = eth_current->next; - } while (old_current != eth_current); + eth_set_current_to_next(); + } while (old_current != eth_get_dev()); }

eth_current_changed(); }

-char *eth_get_name(void) +const char *eth_get_name(void) { - return eth_current ? eth_current->name : "unknown"; + return eth_get_dev() ? eth_get_dev()->name : "unknown"; }

First just add support for MAC drivers.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

---

Changes in v2: -Updated comments -Removed extra parentheses -Changed eth_uclass_priv local var names to be uc_priv -Update error codes -Cause an invalid name to fail binding -Rebase on top of dm/master -Stop maintaining our own index and use DM seq now that it works for our needs -Move the hwaddr to platdata so that its memory is allocated at bind when we need it -Prevent device from being probed before used by a command (i.e. before eth_init()).

common/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 24 ++++ net/eth.c | 319 ++++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 346 insertions(+), 4 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c index 68a9448..75147b7 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -556,7 +556,7 @@ static int initr_bbmii(void) } #endif

-#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) static int initr_net(void) { puts("Net: "); @@ -825,7 +825,7 @@ init_fnc_t init_sequence_r[] = { #ifdef CONFIG_BITBANGMII initr_bbmii, #endif -#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) INIT_FUNC_WATCHDOG_RESET initr_net, #endif diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c index e6d8a7a..8688cf9 100644 --- a/common/cmd_bdinfo.c +++ b/common/cmd_bdinfo.c @@ -34,6 +34,7 @@ static void print_eth(int idx) printf("%-12s= %s\n", name, val); }

+#ifndef CONFIG_DM_ETH __maybe_unused static void print_eths(void) { @@ -52,6 +53,7 @@ static void print_eths(void) printf("current eth = %s\n", eth_get_name()); printf("ip_addr = %s\n", getenv("ipaddr")); } +#endif

__maybe_unused static void print_lnum(const char *name, unsigned long long value) diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 91bb90d..ad96682 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -34,6 +34,7 @@ enum uclass_id { UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_MOD_EXP, /* RSA Mod Exp device */ + UCLASS_ETH, /* Ethernet device */

UCLASS_COUNT, UCLASS_INVALID = -1, diff --git a/include/net.h b/include/net.h index 7eef9cc..4d21d91 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,30 @@ enum eth_state_t { ETH_STATE_ACTIVE };

+#ifdef CONFIG_DM_ETH +struct eth_pdata { + phys_addr_t iobase; + unsigned char enetaddr[6]; +}; + +struct eth_ops { + int (*init)(struct udevice *dev, bd_t *bis); + int (*send)(struct udevice *dev, void *packet, int length); + int (*recv)(struct udevice *dev); + void (*halt)(struct udevice *dev); +#ifdef CONFIG_MCAST_TFTP + int (*mcast)(struct udevice *dev, const u8 *enetaddr, u8 set); +#endif + int (*write_hwaddr)(struct udevice *dev); +}; + +struct udevice *eth_get_dev(void); /* get the current device */ +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */ +int eth_init_state_only(bd_t *bis); /* Set active state */ +void eth_halt_state_only(void); /* Set passive state */ +#endif + +#ifndef CONFIG_DM_ETH struct eth_device { char name[16]; unsigned char enetaddr[6]; diff --git a/net/eth.c b/net/eth.c index c02548c..1b5a169 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,320 @@ static int eth_mac_skip(int index) return ((skip_state = getenv(enetvar)) != NULL); }

+static void eth_current_changed(void); + +#ifdef CONFIG_DM_ETH +#include <dm.h> +#include <dm/device-internal.h> + +struct eth_device_priv { + int state; + void *priv; +}; + +struct eth_uclass_priv { + struct udevice *current; +}; + +static void eth_set_current_to_next(void) +{ + struct uclass *uc; + struct eth_uclass_priv *uc_priv; + + uclass_get(UCLASS_ETH, &uc); + uc_priv = uc->priv; + uclass_next_device(&uc_priv->current); + if (!uc_priv->current) + uclass_first_device(UCLASS_ETH, &uc_priv->current); +} + +struct udevice *eth_get_dev(void) +{ + struct uclass *uc; + uclass_get(UCLASS_ETH, &uc); + + struct eth_uclass_priv *uc_priv = uc->priv; + return uc_priv->current; +} + +static void eth_set_dev(struct udevice *dev) +{ + struct uclass *uc; + uclass_get(UCLASS_ETH, &uc); + + struct eth_uclass_priv *uc_priv = uc->priv; + uc_priv->current = dev; +} + +unsigned char *eth_get_ethaddr(void) +{ + struct eth_pdata *pdata; + if (eth_get_dev()) { + pdata = eth_get_dev()->platdata; + if (pdata) + return pdata->enetaddr; + } + return NULL; +} + +/* Set active state */ +int eth_init_state_only(bd_t *bis) +{ + struct eth_device_priv *priv; + if (eth_get_dev()) { + priv = eth_get_dev()->uclass_priv; + if (priv) + priv->state = ETH_STATE_ACTIVE; + } + + return 0; +} +/* Set passive state */ +void eth_halt_state_only(void) +{ + struct eth_device_priv *priv; + if (eth_get_dev()) { + priv = eth_get_dev()->uclass_priv; + if (priv) + priv->state = ETH_STATE_PASSIVE; + } +} + +int eth_get_dev_index(void) +{ + if (eth_get_dev()) + return eth_get_dev()->seq; + return -1; +} + +int eth_init(bd_t *bis) +{ + struct udevice *current, *old_current, *dev; + struct uclass *uc; + + current = eth_get_dev(); + if (!current) { + puts("No ethernet found.\n"); + return -1; + } + device_probe(current); + + /* Sync environment with network devices */ + uclass_get(UCLASS_ETH, &uc); + uclass_foreach_dev(dev, uc) { + uchar env_enetaddr[6]; + + if (eth_getenv_enetaddr_by_index("eth", dev->seq, + env_enetaddr)) { + struct eth_pdata *pdata = dev->platdata; + if (pdata) + memcpy(pdata->enetaddr, env_enetaddr, 6); + } + }; + + old_current = current; + do { + debug("Trying %s\n", current->name); + + if (current->driver) { + const struct eth_ops *ops = current->driver->ops; + if (ops->init(current, bis) >= 0) { + struct eth_device_priv *priv = + current->uclass_priv; + if (priv) + priv->state = ETH_STATE_ACTIVE; + + return 0; + } + } + debug("FAIL\n"); + + eth_try_another(0); + current = eth_get_dev(); + } while (old_current != current); + + return -ENODEV; +} + +void eth_halt(void) +{ + struct udevice *current; + + current = eth_get_dev(); + if (!current) + return; + if (!current->driver) + return; + + const struct eth_ops *ops = current->driver->ops; + ops->halt(current); + + struct eth_device_priv *priv = current->uclass_priv; + if (priv) + priv->state = ETH_STATE_PASSIVE; +} + +int eth_send(void *packet, int length) +{ + struct udevice *current; + + current = eth_get_dev(); + if (!current) + return -1; + if (!current->driver) + return -1; + + const struct eth_ops *ops = current->driver->ops; + return ops->send(current, packet, length); +} + +int eth_rx(void) +{ + struct udevice *current; + + current = eth_get_dev(); + if (!current) + return -1; + if (!current->driver) + return -1; + + const struct eth_ops *ops = current->driver->ops; + return ops->recv(current); +} + +static int eth_write_hwaddr(struct udevice *dev, const char *base_name, + int eth_number) +{ + unsigned char env_enetaddr[6]; + int ret = 0; + + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr); + + struct eth_pdata *pdata = dev->platdata; + if (!is_zero_ether_addr(env_enetaddr)) { + if (!is_zero_ether_addr(pdata->enetaddr) && + memcmp(pdata->enetaddr, env_enetaddr, 6)) { + printf("\nWarning: %s MAC addresses don't match:\n", + dev->name); + printf("Address in SROM is %pM\n", + pdata->enetaddr); + printf("Address in environment is %pM\n", + env_enetaddr); + } + + memcpy(pdata->enetaddr, env_enetaddr, 6); + } else if (is_valid_ether_addr(pdata->enetaddr)) { + eth_setenv_enetaddr_by_index(base_name, eth_number, + pdata->enetaddr); + printf("\nWarning: %s using MAC address from net device\n", + dev->name); + } else if (is_zero_ether_addr(pdata->enetaddr)) { + printf("\nError: %s address not set.\n", + dev->name); + return -EINVAL; + } + + const struct eth_ops *ops = dev->driver->ops; + if (ops->write_hwaddr && !eth_mac_skip(eth_number)) { + if (!is_valid_ether_addr(pdata->enetaddr)) { + printf("\nError: %s address %pM illegal value\n", + dev->name, pdata->enetaddr); + return -EINVAL; + } + + ret = ops->write_hwaddr(dev); + if (ret) + printf("\nWarning: %s failed to set MAC address\n", + dev->name); + } + + return ret; +} + +static int eth_uclass_init(struct uclass *class) +{ + bootstage_mark(BOOTSTAGE_ID_NET_ETH_START); + + eth_env_init(); + + return 0; +} + +static int eth_post_bind(struct udevice *dev) +{ + if (strchr(dev->name, ' ')) { + printf("\nError: eth device name "%s" has a space!\n", + dev->name); + return -EINVAL; + } + + if (!eth_get_dev()) { + eth_set_dev(dev); + eth_current_changed(); + } + + char *ethprime = getenv("ethprime"); + if (ethprime && strcmp(dev->name, ethprime) == 0) { + eth_set_dev(dev); + eth_current_changed(); + } + + /* + * Devices need to write the hwaddr even if not probed so that Linux + * will have access to the hwaddr that u-boot stored for the device. + */ + eth_write_hwaddr(dev, "eth", uclass_resolve_seq(dev)); + + return 0; +} + +static int eth_pre_unbind(struct udevice *dev) +{ + struct udevice *first = NULL; + struct udevice *current; + struct uclass *uc; + + uclass_get(UCLASS_ETH, &uc); + uclass_foreach_dev(current, uc) { + if (!first) + first = current; + if (current == dev) { + if (dev == first) { + current = list_entry(current->uclass_node.next, + struct udevice, uclass_node); + } else { + current = first; + } + eth_set_dev(current); + eth_current_changed(); + } + } + + return 0; +} + +static int eth_post_probe(struct udevice *dev) +{ + struct eth_device_priv *priv = dev->uclass_priv; + if (priv) + priv->state = ETH_STATE_INIT; + + return 0; +} + +UCLASS_DRIVER(eth) = { + .name = "eth", + .id = UCLASS_ETH, + .post_bind = eth_post_bind, + .pre_unbind = eth_pre_unbind, + .post_probe = eth_post_probe, + .init = eth_uclass_init, + .priv_auto_alloc_size = sizeof(struct eth_uclass_priv), + .per_device_auto_alloc_size = sizeof(struct eth_device_priv), +}; +#endif + +#ifndef CONFIG_DM_ETH /* * CPU and board-specific Ethernet initializations. Aliased function * signals caller to move on @@ -423,6 +737,7 @@ int eth_rx(void)

return eth_current->recv(eth_current); } +#endif /* ifndef CONFIG_DM_ETH */

#ifdef CONFIG_API static void eth_save_packet(void *packet, int length) @@ -486,7 +801,7 @@ static void eth_current_changed(void)

void eth_try_another(int first_restart) { - static struct eth_device *first_failed; + static void *first_failed; char *ethrotate;

/* @@ -515,7 +830,7 @@ void eth_set_current(void) { static char *act; static int env_changed_id; - struct eth_device *old_current; + void *old_current; int env_id;

if (!eth_get_dev()) /* XXX no current */

On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass sjg@chromium.org wrote:

Hi Joe,

On 2 February 2015 at 17:38, Joe Hershberger joe.hershberger@ni.com

wrote:

...

First just add support for MAC drivers.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

---

Changes in v2: -Updated comments -Removed extra parentheses -Changed eth_uclass_priv local var names to be uc_priv -Update error codes -Cause an invalid name to fail binding -Rebase on top of dm/master -Stop maintaining our own index and use DM seq now that it works for

our needs

...

-Move the hwaddr to platdata so that its memory is allocated at bind

when we need it

...

-Prevent device from being probed before used by a command (i.e. before

eth_init()).

...

common/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 24 ++++ net/eth.c | 319

++++++++++++++++++++++++++++++++++++++++++++++++-

...

5 files changed, 346 insertions(+), 4 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c index 68a9448..75147b7 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -556,7 +556,7 @@ static int initr_bbmii(void) } #endif

-#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) static int initr_net(void) { puts("Net: "); @@ -825,7 +825,7 @@ init_fnc_t init_sequence_r[] = { #ifdef CONFIG_BITBANGMII initr_bbmii, #endif -#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) INIT_FUNC_WATCHDOG_RESET initr_net, #endif diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c index e6d8a7a..8688cf9 100644 --- a/common/cmd_bdinfo.c +++ b/common/cmd_bdinfo.c @@ -34,6 +34,7 @@ static void print_eth(int idx) printf("%-12s= %s\n", name, val); }

+#ifndef CONFIG_DM_ETH __maybe_unused static void print_eths(void) { @@ -52,6 +53,7 @@ static void print_eths(void) printf("current eth = %s\n", eth_get_name()); printf("ip_addr = %s\n", getenv("ipaddr")); } +#endif

__maybe_unused static void print_lnum(const char *name, unsigned long long value) diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 91bb90d..ad96682 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -34,6 +34,7 @@ enum uclass_id { UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_MOD_EXP, /* RSA Mod Exp device */

•   UCLASS_ETH,             /* Ethernet device */
  
    UCLASS_COUNT,
    UCLASS_INVALID = -1,
  

diff --git a/include/net.h b/include/net.h index 7eef9cc..4d21d91 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,30 @@ enum eth_state_t { ETH_STATE_ACTIVE };

+#ifdef CONFIG_DM_ETH

You may not need this?

I need it for the function prototypes that have a different device pointer parameter.

...

+struct eth_pdata {

•   phys_addr_t iobase;
  

•   unsigned char enetaddr[6];
  

+};

+struct eth_ops {

•   int (*init)(struct udevice *dev, bd_t *bis);
  

•   int (*send)(struct udevice *dev, void *packet, int length);
  

•   int (*recv)(struct udevice *dev);
  

•   void (*halt)(struct udevice *dev);
  

+#ifdef CONFIG_MCAST_TFTP

•   int (*mcast)(struct udevice *dev, const u8 *enetaddr, u8 set);
  

+#endif

•   int (*write_hwaddr)(struct udevice *dev);
  

+};

+struct udevice *eth_get_dev(void); /* get the current device */ +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */ +int eth_init_state_only(bd_t *bis); /* Set active state */ +void eth_halt_state_only(void); /* Set passive state */ +#endif

+#ifndef CONFIG_DM_ETH struct eth_device { char name[16]; unsigned char enetaddr[6]; diff --git a/net/eth.c b/net/eth.c index c02548c..1b5a169 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,320 @@ static int eth_mac_skip(int index) return ((skip_state = getenv(enetvar)) != NULL); }

+static void eth_current_changed(void);

+#ifdef CONFIG_DM_ETH +#include <dm.h> +#include <dm/device-internal.h>

These should go at the top of the file - should be OK to always include them (i.e. no #ifdef)

OK... Moved them.

...

+struct eth_device_priv {

•   int state;
  

•   void *priv;
  

+};

+struct eth_uclass_priv {

•   struct udevice *current;
  

+};

+static void eth_set_current_to_next(void) +{

•   struct uclass *uc;
  

•   struct eth_uclass_priv *uc_priv;
  

•   uclass_get(UCLASS_ETH, &uc);
  

This can actually return an error, although I agree there is little point in handling it. So I suppose this is OK.

I think this should be a given.

...

•   uc_priv = uc->priv;
  

•   uclass_next_device(&uc_priv->current);
  

•   if (!uc_priv->current)
  

•           uclass_first_device(UCLASS_ETH, &uc_priv->current);
  

+}

+struct udevice *eth_get_dev(void) +{

•   struct uclass *uc;
  

blank line here

OK on all of these.

...

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *uc_priv = uc->priv;
  

•   return uc_priv->current;
  

+}

+static void eth_set_dev(struct udevice *dev) +{

•   struct uclass *uc;
  

blank line here

...

•   uclass_get(UCLASS_ETH, &uc);
  

•   struct eth_uclass_priv *uc_priv = uc->priv;
  

•   uc_priv->current = dev;
  

+}

+unsigned char *eth_get_ethaddr(void) +{

•   struct eth_pdata *pdata;
  

blank line here

...

•   if (eth_get_dev()) {
  

•           pdata = eth_get_dev()->platdata;
  

•           if (pdata)
  

•                   return pdata->enetaddr;
  

•   }
  

•   return NULL;
  

+}

+/* Set active state */ +int eth_init_state_only(bd_t *bis) +{

•   struct eth_device_priv *priv;
  

blank line here

...

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_ACTIVE;
  

•   }
  

•   return 0;
  

+} +/* Set passive state */ +void eth_halt_state_only(void) +{

•   struct eth_device_priv *priv;
  

blank line here

...

•   if (eth_get_dev()) {
  

•           priv = eth_get_dev()->uclass_priv;
  

•           if (priv)
  

•                   priv->state = ETH_STATE_PASSIVE;
  

•   }
  

+}

+int eth_get_dev_index(void) +{

•   if (eth_get_dev())
  

•           return eth_get_dev()->seq;
  

•   return -1;
  

+}

+int eth_init(bd_t *bis) +{

•   struct udevice *current, *old_current, *dev;
  

•   struct uclass *uc;
  

•   current = eth_get_dev();
  

•   if (!current) {
  

•           puts("No ethernet found.\n");
  

•           return -1;
  

•   }
  

•   device_probe(current);
  

Check error

OK.

...

•   /* Sync environment with network devices */
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   uclass_foreach_dev(dev, uc) {
  

•           uchar env_enetaddr[6];
  

•           if (eth_getenv_enetaddr_by_index("eth", dev->seq,
  

•                                            env_enetaddr)) {
  

•                   struct eth_pdata *pdata = dev->platdata;
  

blank line

Do all devices have the same platdata by design? What if a particular device wants its own?

That's a good question. I imagine some devices may have something unique, but I would expect they would read that data into the priv data that they define. How do other subsystems handle platform data for unique devices?

...

•                   if (pdata)
  

What do you need this check?

No. This was left over from when enetaddr was in priv.

...

•                           memcpy(pdata->enetaddr, env_enetaddr,
  

6);

...

•           }
  

•   };
  

•   old_current = current;
  

•   do {
  

•           debug("Trying %s\n", current->name);
  

•           if (current->driver) {
  

•                   const struct eth_ops *ops =
  

current->driver->ops;

blank line

...

•                   if (ops->init(current, bis) >= 0) {
  

•                           struct eth_device_priv *priv =
  

•                                   current->uclass_priv;
  

•                           if (priv)
  

•                                   priv->state = ETH_STATE_ACTIVE;
  

•                           return 0;
  

•                   }
  

•           }
  

•           debug("FAIL\n");
  

•           eth_try_another(0);
  

•           current = eth_get_dev();
  

•   } while (old_current != current);
  

•   return -ENODEV;
  

+}

+void eth_halt(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return;
  

•   if (!current->driver)
  

•           return;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   ops->halt(current);
  

•   struct eth_device_priv *priv = current->uclass_priv;
  

•   if (priv)
  

•           priv->state = ETH_STATE_PASSIVE;
  

+}

+int eth_send(void *packet, int length) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

Seems like eth_get_dev() should return an error code if current or current->driver is NULL.

It's returning a pointer, so I prefer it to be NULL or a valid pointer.

Perhaps you meant to say that eth_send() should return an error code?

...

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->send(current, packet, length);
  

+}

+int eth_rx(void) +{

•   struct udevice *current;
  

•   current = eth_get_dev();
  

•   if (!current)
  

•           return -1;
  

•   if (!current->driver)
  

•           return -1;
  

•   const struct eth_ops *ops = current->driver->ops;
  

•   return ops->recv(current);
  

+}

+static int eth_write_hwaddr(struct udevice *dev, const char *base_name,

•              int eth_number)
  

+{

•   unsigned char env_enetaddr[6];
  

•   int ret = 0;
  

•   eth_getenv_enetaddr_by_index(base_name, eth_number,
  

env_enetaddr);

...

•   struct eth_pdata *pdata = dev->platdata;
  

•   if (!is_zero_ether_addr(env_enetaddr)) {
  

•           if (!is_zero_ether_addr(pdata->enetaddr) &&
  

•               memcmp(pdata->enetaddr, env_enetaddr, 6)) {
  

•                   printf("\nWarning: %s MAC addresses don't
  

match:\n",

...

•                          dev->name);
  

•                   printf("Address in SROM is         %pM\n",
  

•                          pdata->enetaddr);
  

•                   printf("Address in environment is  %pM\n",
  

•                          env_enetaddr);
  

•           }
  

•           memcpy(pdata->enetaddr, env_enetaddr, 6);
  

•   } else if (is_valid_ether_addr(pdata->enetaddr)) {
  

•           eth_setenv_enetaddr_by_index(base_name, eth_number,
  

•                                        pdata->enetaddr);
  

•           printf("\nWarning: %s using MAC address from net
  

device\n",

...

•                  dev->name);
  

•   } else if (is_zero_ether_addr(pdata->enetaddr)) {
  

•           printf("\nError: %s address not set.\n",
  

•                  dev->name);
  

•           return -EINVAL;
  

•   }
  

•   const struct eth_ops *ops = dev->driver->ops;
  

•   if (ops->write_hwaddr && !eth_mac_skip(eth_number)) {
  

•           if (!is_valid_ether_addr(pdata->enetaddr)) {
  

•                   printf("\nError: %s address %pM illegal
  

value\n",

...

•                          dev->name, pdata->enetaddr);
  

•                   return -EINVAL;
  

•           }
  

•           ret = ops->write_hwaddr(dev);
  

•           if (ret)
  

•                   printf("\nWarning: %s failed to set MAC
  

address\n",

...

•                          dev->name);
  

•   }
  

The above code seems mostly duplicated but I suppose it is hard to make it common?

It is, but I look forward to the day when we delete the other copy. It would be difficult to combine, for sure.

...

•   return ret;
  

+}

+static int eth_uclass_init(struct uclass *class) +{

•   bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
  

•   eth_env_init();
  

•   return 0;
  

+}

+static int eth_post_bind(struct udevice *dev) +{

•   if (strchr(dev->name, ' ')) {
  

•           printf("\nError: eth device name \"%s\" has a space!\n",
  

•                  dev->name);
  

•           return -EINVAL;
  

•   }
  

•   if (!eth_get_dev()) {
  

•           eth_set_dev(dev);
  

•           eth_current_changed();
  

•   }
  

I still don't really get what you are doing here. Perhaps you could add some comments? There should be no probed devices at this stage since you are binding only.

This has changed a fair amount in v3. Please comment there if it's more clear now.

...

•   char *ethprime = getenv("ethprime");
  

•   if (ethprime && strcmp(dev->name, ethprime) == 0) {
  

•           eth_set_dev(dev);
  

•           eth_current_changed();
  

•   }
  

•   /*
  

•    * Devices need to write the hwaddr even if not probed so that
  

Linux

...

•    * will have access to the hwaddr that u-boot stored for the
  

device.

...

•    */
  

•   eth_write_hwaddr(dev, "eth", uclass_resolve_seq(dev));
  

•   return 0;
  

+}

+static int eth_pre_unbind(struct udevice *dev) +{

•   struct udevice *first = NULL;
  

•   struct udevice *current;
  

•   struct uclass *uc;
  

•   uclass_get(UCLASS_ETH, &uc);
  

•   uclass_foreach_dev(current, uc) {
  

•           if (!first)
  

•                   first = current;
  

•           if (current == dev) {
  

•                   if (dev == first) {
  

•                           current =
  

list_entry(current->uclass_node.next,

...

•                                      struct udevice, uclass_node);
  

•                   } else {
  

•                           current = first;
  

•                   }
  

•                   eth_set_dev(current);
  

•                   eth_current_changed();
  

•           }
  

•   }
  

If this is turning down a device it really should happen in a remove() method. Maybe in post_remove()?

This has changed a fair amount in v3. Please comment there if it's more clear now.

...

•   return 0;
  

+}

+static int eth_post_probe(struct udevice *dev) +{

•   struct eth_device_priv *priv = dev->uclass_priv;
  

•   if (priv)
  

•           priv->state = ETH_STATE_INIT;
  

•   return 0;
  

+}

+UCLASS_DRIVER(eth) = {

•   .name           = "eth",
  

•   .id             = UCLASS_ETH,
  

•   .post_bind      = eth_post_bind,
  

•   .pre_unbind     = eth_pre_unbind,
  

•   .post_probe     = eth_post_probe,
  

•   .init           = eth_uclass_init,
  

•   .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
  

•   .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
  

+}; +#endif

+#ifndef CONFIG_DM_ETH /*

• CPU and board-specific Ethernet initializations. Aliased function
• signals caller to move on

@@ -423,6 +737,7 @@ int eth_rx(void)

    return eth_current->recv(eth_current);

} +#endif /* ifndef CONFIG_DM_ETH */

#ifdef CONFIG_API static void eth_save_packet(void *packet, int length) @@ -486,7 +801,7 @@ static void eth_current_changed(void)

void eth_try_another(int first_restart) {

•   static struct eth_device *first_failed;
  

•   static void *first_failed;
    char *ethrotate;
  
    /*
  

@@ -515,7 +830,7 @@ void eth_set_current(void) { static char *act; static int env_changed_id;

•   struct eth_device *old_current;
  

•   void *old_current;
    int     env_id;
  
    if (!eth_get_dev())     /* XXX no current */
  

-- 1.7.11.5

Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi Simon,

On Thu, Feb 12, 2015 at 11:20 PM, Simon Glass sjg@chromium.org wrote:

Hi Joe,

On 10 February 2015 at 23:25, Joe Hershberger joe.hershberger@gmail.com

wrote:

...

On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Joe,

On 2 February 2015 at 17:38, Joe Hershberger joe.hershberger@ni.com wrote:

...

First just add support for MAC drivers.

Signed-off-by: Joe Hershberger joe.hershberger@ni.com

---

Changes in v2: -Updated comments -Removed extra parentheses -Changed eth_uclass_priv local var names to be uc_priv -Update error codes -Cause an invalid name to fail binding -Rebase on top of dm/master -Stop maintaining our own index and use DM seq now that it works for

our

...

...

...

needs -Move the hwaddr to platdata so that its memory is allocated at bind when we need it -Prevent device from being probed before used by a command (i.e.

before