[U-Boot] [RFC PATCH v2 5/8] net: Add basic driver model support to Ethernet stack

[Joe Hershberger <joe.hershberger@gmail.com>]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot