[U-Boot] [RFC PATCH v4 12/23] dm: eth: Add basic driver model support to Ethernet stack

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

Hi Simon,

On Sun, Mar 1, 2015 at 12:07 PM, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Joe,
>
> On 24 February 2015 at 17:02, Joe Hershberger <joe.hershberger@ni.com>
wrote:
> > First just add support for MAC drivers.
> >
> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> This looks right to me. I still have some comments on error handling,
> but I'm OK with you addressing these in a follow-on patch if you like.

Thanks for going back and forth on this to make it right.

> > ---
> >
> > Changes in v4:
> > -Redo the seq / probe implementation
> > --Don't prevent eth_initialize on driver model
> > --Use eth_initialize to probe all devices and write_hwaddr
> > --Look up MAC address in post-probe
> > --Include ethprime handling in eth_initialize
> > --If current == NULL, always check if there is a device available in
eth_get_dev
> > --Move env init call from uclass init to eth_initialize
> > --Print the alias in eth_initialize
> > -Stop handling selecting a new "current" in pre-unbind as it will now
work itself out by clearing the pointer
> > -Change -1 returns to error constants
> > -Remove bd_t *bis from dm eth_ops init function
> > -Add documentation to the structures
> > -Add a helper function for eth_uclass_priv
> > -Change puts to printf
> > -Add eth_get_ops helper
> > -Rename init() to start() in ops
> > -Rename halt() to stop() in ops
> > -Remove checks for driver==NULL
> > -Remove priv pointer in per-device priv struct (drivers already get
their own directly from DM)
> >
> > Changes in v3:
> > -Correct the pre_unbind logic
> > -Correct failure chaining from bind to probe to init
> > --Fail init if not activated
> > --Fail probe if ethaddr not set
> > -Update ethaddr from env unconditionally on init
> > -Use set current to select the current device regardless of the
previous selection
> > -Allow current eth dev to be NULL
> > -Fixed blank line formatting for variable declaration
> >
> > 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/cmd_bdinfo.c    |   2 +
> >  drivers/net/Kconfig    |   5 +
> >  include/dm/uclass-id.h |   1 +
> >  include/net.h          |  52 ++++++++
> >  net/eth.c              | 345
++++++++++++++++++++++++++++++++++++++++++++++++-
> >  5 files changed, 399 insertions(+), 6 deletions(-)
> >
> > diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
> > index aa81da2..b4cce25 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/drivers/net/Kconfig b/drivers/net/Kconfig
> > index e69de29..bdd0f05 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -0,0 +1,5 @@
> > +config DM_ETH
> > +       bool "Enable Driver Model for Ethernet drivers"
> > +       depends on DM
> > +       help
> > +         Enable driver model for Ethernet.
>
> Here you could mention that the eth_...() interface is then
> implemented by the Ethernet uclass. Perhaps a few other notes too? See
> for example drivers/spi/Kconfig or drivers/gpio/Kconfig.

OK

> > 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 10d38f8..508c572 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -78,6 +78,57 @@ enum eth_state_t {
> >         ETH_STATE_ACTIVE
> >  };
> >
> > +#ifdef CONFIG_DM_ETH
> > +/**
> > + * struct eth_pdata - Platform data for Ethernet MAC controllers
> > + *
> > + * @iobase: The base address of the hardware registers
> > + * @enetaddr: The Ethernet MAC address that is loaded from EEPROM or
env
> > + */
> > +struct eth_pdata {
> > +       phys_addr_t iobase;
> > +       unsigned char enetaddr[6];
> > +};
> > +
> > +/**
> > + * struct eth_ops - functions of Ethernet MAC controllers
> > + *
> > + * start: Prepare the hardware to send and receive packets
> > + * send: Send the bytes passed in "packet" as a packet on the wire
> > + * recv: Check if the hardware received a packet. Call the network
stack if so
> > + * stop: Stop the hardware from looking for packets - may be called
even if
> > + *      state == PASSIVE
> > + * mcast: Join or leave a multicast group (for TFTP) - optional
> > + * write_hwaddr: Write a MAC address to the hardware (used to pass it
to Linux
> > + *              on some platforms like ARM). This function expects the
> > + *              eth_pdata::enetaddr field to be populated - optional
> > + * read_rom_hwaddr: Some devices have a backup of the MAC address
stored in a
> > + *                 ROM on the board. This is how the driver should
expose it
> > + *                 to the network stack. This function should fill in
the
> > + *                 eth_pdata::enetaddr field - optional

I consider this one of the primary purposes that board-specific init exists
for Ethernet. I think this will help to eliminate them. I'm interested in
your thoughts about how to generically expose this to a board / SoC /
driver. For now I was thinking that it would be up to the driver to fan out
if needed, meaning that if the driver doesn't know the MAC, it has a
board-hook function call, but if it does, like USB or PCI adapters, it
won't have board hooks. [sorry for the run-on sentence] I feel it's
non-ideal, but can work for now.

> > + */
> > +struct eth_ops {
> > +       int (*start)(struct udevice *dev);
> > +       int (*send)(struct udevice *dev, void *packet, int length);
> > +       int (*recv)(struct udevice *dev);
> > +       void (*stop)(struct udevice *dev);
> > +#ifdef CONFIG_MCAST_TFTP
> > +       int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> > +#endif
> > +       int (*write_hwaddr)(struct udevice *dev);
> > +       int (*read_rom_hwaddr)(struct udevice *dev);
> > +};
> > +
> > +#define eth_get_ops(dev) ((struct eth_ops *)(dev)->driver->ops)
> > +
> > +struct udevice *eth_get_dev(void); /* get the current device */
> > +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */
> > +/* Used only when NetConsole is enabled */
> > +int eth_init_state_only(void); /* 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];
> > @@ -144,6 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
char *base_name,
> >                      int eth_number);
> >
> >  int usb_eth_initialize(bd_t *bi);
> > +#endif
> >
> >  int eth_initialize(void);              /* Initialize network subsystem
*/
> >  void eth_try_another(int first_restart);       /* Change the device */
> > diff --git a/net/eth.c b/net/eth.c
> > index 7bbaac4..9c2dfb9 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -1,12 +1,15 @@
> >  /*
> > - * (C) Copyright 2001-2010
> > + * (C) Copyright 2001-2015
> >   * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > + * Joe Hershberger, National Instruments
> >   *
> >   * SPDX-License-Identifier:    GPL-2.0+
> >   */
> >
> >  #include <common.h>
> >  #include <command.h>
> > +#include <dm.h>
> > +#include <dm/device-internal.h>
>
> The dm/ headers should go after the asm/ header below.

What in the asm headers affects the dm headers? Or is it an aesthetics
request?

> >  #include <net.h>
> >  #include <miiphy.h>
> >  #include <phy.h>
> > @@ -74,6 +77,338 @@ static int eth_mac_skip(int index)
> >         return ((skip_state = getenv(enetvar)) != NULL);
> >  }
> >
> > +static void eth_current_changed(void);
> > +
> > +#ifdef CONFIG_DM_ETH
> > +/**
> > + * struct eth_device_priv - private structure for each Ethernet device
> > + *
> > + * @state: The state of the Ethernet MAC driver (defined by enum
eth_state_t)
> > + */
> > +struct eth_device_priv {
> > +       enum eth_state_t state;
> > +};
> > +
> > +/**
> > + * struct eth_uclass_priv - The structure attached to the uclass itself
> > + *
> > + * @current: The Ethernet device that the network functions are using
> > + */
> > +struct eth_uclass_priv {
> > +       struct udevice *current;
> > +};
> > +
> > +static struct eth_uclass_priv *eth_get_uclass_priv(void)
> > +{
> > +       struct uclass *uc;
> > +
> > +       uclass_get(UCLASS_ETH, &uc);
> > +       assert(uc);
> > +       return uc->priv;
> > +}
> > +
> > +static void eth_set_current_to_next(void)
> > +{
> > +       struct eth_uclass_priv *uc_priv;
> > +
> > +       uc_priv = eth_get_uclass_priv();
> > +       if (uc_priv->current)
> > +               uclass_next_device(&uc_priv->current);
> > +       if (!uc_priv->current)
> > +               uclass_first_device(UCLASS_ETH, &uc_priv->current);
> > +}
> > +
> > +struct udevice *eth_get_dev(void)
>
> This function needs a comment. It isn't clear what is it supposed to
> return. Does it only return probed devices (in which case the check in
> eth_halt() etc. for device_active() is redundant? Or can it return
> devices which cannot be probed?

In the current implementation it will return any device (probe-able or
not), as it is the caller that has the logic to decide how to move on or
not.

> I suggest that it only returns probed devices, as it simplifies the code.

I can evaluate that, but it is not currently expected anywhere.

> > +{
> > +       if (!eth_get_uclass_priv()->current)
> > +               uclass_first_device(UCLASS_ETH,
> > +                                   &eth_get_uclass_priv()->current);
>
> This can return an error. You will then eat it and return the device
> anyway, even though uclass_first_device() will not change ->current.

This can return the error from the probe(), but is the probe fails it also
return a NULL pointer, which is what I'm using to determine success. I
can't return the device in the error case because uclass_first_device()
doesn't give it to me.

> > +       return eth_get_uclass_priv()->current;
>
> Also I think it would be better to have a local variable here for
> uc_priv, as in the above functino.

OK

> > +}
> > +
> > +static void eth_set_dev(struct udevice *dev)
> > +{
> > +       device_probe(dev);
>
> This needs an error check, since if it fails you cannot use the
> device. Also in eth_pre_unbind() you call this function with NULL.

The device_probe() function already has that check, so I didn't bother
adding it again here.

> So I think this function should return an error.

I'm not sure what other error it would return. If it's just the NULL
pointer, the caller knows what it passed in. NULL pointer is valid here.
Essentially asking to unset this device as current (as in pre_unbind
handler).

> > +       eth_get_uclass_priv()->current = dev;
> > +}
> > +
> > +unsigned char *eth_get_ethaddr(void)
> > +{
> > +       struct eth_pdata *pdata;
> > +
> > +       if (eth_get_dev()) {
> > +               pdata = eth_get_dev()->platdata;
> > +               return pdata->enetaddr;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +/* Set active state without calling start on the driver */
> > +int eth_init_state_only(void)
> > +{
> > +       struct udevice *current;
> > +       struct eth_device_priv *priv;
> > +
> > +       current = eth_get_dev();
> > +       if (!current || !device_active(current))
> > +               return -EINVAL;
> > +
> > +       priv = current->uclass_priv;
> > +       priv->state = ETH_STATE_ACTIVE;
> > +
> > +       return 0;
> > +}
> > +
> > +/* Set passive state without calling stop on the driver */
> > +void eth_halt_state_only(void)
> > +{
> > +       struct udevice *current;
> > +       struct eth_device_priv *priv;
> > +
> > +       current = eth_get_dev();
> > +       if (!current || !device_active(current))
> > +               return;
> > +
> > +       priv = current->uclass_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(void)
> > +{
> > +       struct udevice *current;
> > +       struct udevice *old_current;
> > +
> > +       current = eth_get_dev();
> > +       if (!current) {
> > +               printf("No ethernet found.\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       old_current = current;
> > +       do {
> > +               debug("Trying %s\n", current->name);
> > +
> > +               if (device_active(current)) {
> > +                       uchar env_enetaddr[6];
> > +                       struct eth_pdata *pdata = current->platdata;
> > +
> > +                       /* Sync environment with network device */
> > +                       if (eth_getenv_enetaddr_by_index("eth",
current->seq,
> > +                                                        env_enetaddr))
> > +                               memcpy(pdata->enetaddr, env_enetaddr,
6);
> > +                       else
> > +                               memset(pdata->enetaddr, 0, 6);
> > +
> > +                       if (eth_get_ops(current)->start(current) >= 0) {
> > +                               struct eth_device_priv *priv =
> > +                                       current->uclass_priv;
> > +
> > +                               priv->state = ETH_STATE_ACTIVE;
> > +                               return 0;
> > +                       }
> > +               }
> > +               debug("FAIL\n");
> > +
> > +               /* This will ensure the new "current" attempted to
probe */
> > +               eth_try_another(0);
> > +               current = eth_get_dev();
> > +       } while (old_current != current);
> > +
> > +       return -ENODEV;
> > +}
> > +
> > +void eth_halt(void)
> > +{
> > +       struct udevice *current;
> > +       struct eth_device_priv *priv;
> > +
> > +       current = eth_get_dev();
> > +       if (!current || !device_active(current))
> > +               return;
>
> As above, probably this device_active() check is redundant?

There is a case where it is not redundant. If the eth_set_dev() is called
with a device that fails to probe, then it will be called the current
device, but not be active. This is needed to be able to skip over a device
that won't probe to get to one that will.

> > +
> > +       eth_get_ops(current)->stop(current);
>
> How about adding error checking in these functions? Then eventually we
> can plumb it through. Driver authors are not going to motivated to
> deal with errors if they know that their caller ignores them.
>
> ret = eth_get_ops...
> if (ret) {
>    /* We cannot return the error at present */
>   debug("%s: stop() returned error %d\n", __func__, ret);
> }

OK

> > +       priv = current->uclass_priv;
> > +       priv->state = ETH_STATE_PASSIVE;
> > +}
> > +
> > +int eth_send(void *packet, int length)
> > +{
> > +       struct udevice *current;
> > +
> > +       current = eth_get_dev();
> > +       if (!current)
> > +               return -ENODEV;
> > +
> > +       if (!device_active(current))
> > +               return -EINVAL;
> > +
> > +       return eth_get_ops(current)->send(current, packet, length);
> > +}
> > +
> > +int eth_rx(void)
> > +{
> > +       struct udevice *current;
> > +
> > +       current = eth_get_dev();
> > +       if (!current)
> > +               return -ENODEV;
> > +
> > +       if (!device_active(current))
> > +               return -EINVAL;
> > +
> > +       return eth_get_ops(current)->recv(current);
> > +}
> > +
> > +static int eth_write_hwaddr(struct udevice *dev)
> > +{
> > +       struct eth_pdata *pdata = dev->platdata;
> > +       int ret = 0;
> > +
> > +       if (!dev || !device_active(dev))
> > +               return -EINVAL;
> > +
> > +       /* seq is valid since the device is active */
> > +       if (eth_get_ops(dev)->write_hwaddr && !eth_mac_skip(dev->seq)) {
> > +               if (!is_valid_ether_addr(pdata->enetaddr)) {
> > +                       printf("\nError: %s address %pM illegal
value\n",
> > +                              dev->name, pdata->enetaddr);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               ret = eth_get_ops(dev)->write_hwaddr(dev);
> > +               if (ret)
> > +                       printf("\nWarning: %s failed to set MAC
address\n",
> > +                              dev->name);
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +int eth_initialize(void)
> > +{
> > +       int num_devices = 0;
> > +       struct udevice *dev;
> > +
> > +       bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
> > +       eth_env_init();
> > +
> > +       /*
> > +        * Devices need to write the hwaddr even if not started so that
Linux
> > +        * will have access to the hwaddr that u-boot stored for the
device.
> > +        * This is accomplished by attempting to probe each device and
calling
> > +        * their write_hwaddr() operation.
> > +        */
> > +       uclass_first_device(UCLASS_ETH, &dev);
> > +       if (!dev) {
> > +               printf("No ethernet found.\n");
> > +               bootstage_error(BOOTSTAGE_ID_NET_ETH_START);
> > +       } else {
> > +               bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
> > +               do {
> > +                       if (num_devices)
> > +                               printf(", ");
> > +
> > +                       printf("eth%d: %s", dev->seq, dev->name);
> > +
> > +                       eth_write_hwaddr(dev);
> > +
> > +                       uclass_next_device(&dev);
> > +                       num_devices++;
> > +               } while (dev);
> > +
> > +               putc('\n');
> > +       }
> > +
> > +       return num_devices;
> > +}
> > +
> > +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;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int eth_pre_unbind(struct udevice *dev)
> > +{
> > +       /* Don't hang onto a pointer that is going away */
> > +       if (dev == eth_get_uclass_priv()->current)
> > +               eth_set_dev(NULL);
> > +
> > +       return 0;
> > +}
> > +
> > +static int eth_post_probe(struct udevice *dev)
> > +{
> > +       struct eth_device_priv *priv = dev->uclass_priv;
> > +       struct eth_pdata *pdata = dev->platdata;
> > +       unsigned char env_enetaddr[6];
> > +
> > +       priv->state = ETH_STATE_INIT;
> > +
> > +       /* Check if the device has a MAC address in ROM */
> > +       if (eth_get_ops(dev)->read_rom_hwaddr)
> > +               eth_get_ops(dev)->read_rom_hwaddr(dev);
> > +
> > +       eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
> > +       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);
> > +               }
> > +
> > +               /* Override the ROM MAC address */
> > +               memcpy(pdata->enetaddr, env_enetaddr, 6);
> > +       } else if (is_valid_ether_addr(pdata->enetaddr)) {
> > +               eth_setenv_enetaddr_by_index("eth", dev->seq,
pdata->enetaddr);
> > +               printf("\nWarning: %s using MAC address from ROM\n",
> > +                      dev->name);
> > +       } else if (is_zero_ether_addr(pdata->enetaddr)) {
> > +               printf("\nError: %s address not set.\n",
> > +                      dev->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int eth_pre_remove(struct udevice *dev)
> > +{
> > +       eth_get_ops(dev)->stop(dev);
> > +
> > +       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,
> > +       .pre_remove     = eth_pre_remove,
> > +       .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
> > +       .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
> > +};
> > +#endif
>
> Could we put the uclass in its own eth-uclass.c file at some point?

At some point, yes.

> > +
> > +#ifndef CONFIG_DM_ETH
> >  /*
> >   * CPU and board-specific Ethernet initializations.  Aliased function
> >   * signals caller to move on
> > @@ -425,6 +760,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)
> > @@ -488,7 +824,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;
> >
> >         /*
> > @@ -517,12 +853,9 @@ 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 */
> > -               return;
> > -
> >         env_id = get_env_id();
> >         if ((act == NULL) || (env_changed_id != env_id)) {
> >                 act = getenv("ethact");
> > --
> > 1.7.11.5
> >
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot