[U-Boot] [RFC PATCH v4 12/23] dm: eth: Add basic driver model support to Ethernet stack
Simon Glass
sjg at chromium.org
Sun Mar 1 19:07:26 CET 2015
Hi Joe,
On 24 February 2015 at 17:02, Joe Hershberger <joe.hershberger at ni.com> wrote:
> First just add support for MAC drivers.
>
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
Reviewed-by: Simon Glass <sjg at 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.
>
> ---
>
> 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.
> 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
> + */
> +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.
> #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?
I suggest that it only returns probed devices, as it simplifies the code.
> +{
> + if (!eth_get_uclass_priv()->current)
> + uclass_first_device(UCLASS_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.
> + 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.
> +}
> +
> +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.
So I think this function should return an error.
> + 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?
> +
> + 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);
}
> + 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?
> +
> +#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
More information about the U-Boot
mailing list