[U-Boot] [RFC PATCH v3 07/14] dm: eth: Add basic driver model support to Ethernet stack
Simon Glass
sjg at chromium.org
Sun Feb 15 16:49:47 CET 2015
Hi Joe,
On 10 February 2015 at 18:30, Joe Hershberger <joe.hershberger at ni.com> wrote:
> First just add support for MAC drivers.
It has taken me a while to get through all this unfortunately.
This seems OK to me but needs a clean-up with more comments, etc. If
you like these could go in a separate patch, so if you want to do that
please add my Reviewed-by: Simon Glass <sjg at chromium.org> to this one.
I would prefer that we sort out the bind/probe problem before this is
merged but I understand you now have quite a bit of work built on top,
and the problems can be separated.
So if you like we could do one more version, merge it, and continue
with refinements after that.
>
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
>
> ---
>
> 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/board_r.c | 4 +-
> common/cmd_bdinfo.c | 2 +
> include/dm/uclass-id.h | 1 +
> include/net.h | 25 ++++
> net/eth.c | 336 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 361 insertions(+), 7 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 4d7575e..11471bd 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);
Why do we pass in bd_t? Isn't that available through gd->bd?
> + 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);
s/u8/bool/ or maybe int? On ARM at least it is inefficient to keep
having to mask the parameters.
> +#endif
> + int (*write_hwaddr)(struct udevice *dev);
> +};
Can you please add interface comments on all of these plus the four
below? I'm trying to make driver model an opportunity to improve the
code as we go. Things like what the function does, what packet
contains.
> +
> +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 */
Can you expand these a bit? The first one can return NULL in some
situations. The second returns a pointer to 6 bytes I think (perhaps
we should define a struct for this in a future patch?) What are
active and passive state? Why does one function get passed bd_t and
not the other (better if neither did).
> +#endif
> +
> +#ifndef CONFIG_DM_ETH
> struct eth_device {
> char name[16];
> unsigned char enetaddr[6];
> @@ -145,6 +169,7 @@ 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 */
> diff --git a/net/eth.c b/net/eth.c
> index c02548c..e84b948 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -1,12 +1,15 @@
> /*
> * (C) Copyright 2001-2010
> * 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>
> #include <net.h>
> #include <miiphy.h>
> #include <phy.h>
> @@ -72,6 +75,331 @@ 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: ...
* @priv: ...
/
> +struct eth_device_priv {
> + int state;
> + void *priv;
> +};
> +
Structure attached to the uclass itself.
> +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;
> + if (uc_priv->current)
> + uclass_next_device(&uc_priv->current);
> + if (!uc_priv->current)
> + uclass_first_device(UCLASS_ETH, &uc_priv->current);
> +}
> +
I think you should have something like this to avoid this duplication:
static struct eth_uclass_priv *get_uclass_priv(void)
{
struct uclass *uc;
uclass_get(UCLASS_ETH, &uc);
assert(uc);
return uc->priv;
}
At some point we should add a uclass_get_priv() function in uclass.h.
> +struct udevice *eth_get_dev(void)
> +{
> + struct uclass *uc;
> + struct eth_uclass_priv *uc_priv;
> +
> + uclass_get(UCLASS_ETH, &uc);
> + uc_priv = uc->priv;
> +
> + return uc_priv->current;
> +}
> +
> +static void eth_set_dev(struct udevice *dev)
> +{
> + struct uclass *uc;
> + struct eth_uclass_priv *uc_priv;
> +
> + uclass_get(UCLASS_ETH, &uc);
> + 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)
This doesn't seem to use bis, so I suspect it is for backwards compatibility.
> +{
> + struct eth_device_priv *priv;
> +
> + if (eth_get_dev()) {
> + priv = eth_get_dev()->uclass_priv;
> + if (priv)
> + priv->state = ETH_STATE_ACTIVE;
It looks like state uses an enum, so that should be described in the
comment I mentioned earlier.
> + }
> + 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;
> + int retval;
> + struct uclass *uc;
> +
> + current = eth_get_dev();
> + if (!current) {
> + puts("No ethernet found.\n");
printf() as I believe we are trying to avoid puts().
> + return -1;
> + }
> + retval = device_probe(current);
> + if (retval)
> + return retval;
> +
> + /* Sync environment with network devices */
> + uclass_get(UCLASS_ETH, &uc);
> + uclass_foreach_dev(dev, uc) {
> + uchar env_enetaddr[6];
> + struct eth_pdata *pdata = dev->platdata;
> +
ret = device_probe(dev);
if (ret) ...
here since you can't use dev->seq otherwise.
> + if (eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr))
> + memcpy(pdata->enetaddr, env_enetaddr, 6);
> + else
> + memset(pdata->enetaddr, 0, 6);
> + }
> +
> + old_current = current;
> + do {
> + debug("Trying %s\n", current->name);
> +
> + if (current->driver && (current->flags & DM_FLAG_ACTIVATED)) {
There is no need to check current->driver (here and elsewhere)
device_active(current)
> + const struct eth_ops *ops = current->driver->ops;
> +
> + if (ops->init(current, bis) >= 0) {
> + struct eth_device_priv *priv =
> + current->uclass_priv;
> + if (priv)
Remove this check too
> + 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;
> + const struct eth_ops *ops;
> + struct eth_device_priv *priv;
> +
> + current = eth_get_dev();
> + if (!current)
> + return;
> + if (!current->driver)
> + return;
Remove these checks
> +
> + ops = current->driver->ops;
Define this in your header file:
#define eth_get_ops(dev) ((struct eth_ops *)(dev)->driver->ops)
then one day we can add checks on dev, etc.
> + ops->halt(current);
If you like you can drop the local variable and use:
eth_get_ops(dev)->halt(current)
> +
> + priv = current->uclass_priv;
> + if (priv)
> + priv->state = ETH_STATE_PASSIVE;
> +}
> +
> +int eth_send(void *packet, int length)
> +{
> + struct udevice *current;
> + const struct eth_ops *ops;
> +
> + current = eth_get_dev();
> + if (!current)
> + return -1;
> + if (!current->driver)
> + return -1;
> + ops = current->driver->ops;
> +
> + return ops->send(current, packet, length);
> +}
> +
> +int eth_rx(void)
> +{
> + struct udevice *current;
> + const struct eth_ops *ops;
> +
> + current = eth_get_dev();
> + if (!current)
> + return -1;
> + if (!current->driver)
> + return -1;
> + 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;
> + struct eth_pdata *pdata;
> + const struct eth_ops *ops;
> +
> + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
> +
> + 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;
> + }
> +
> + ops = dev->driver->ops;
> + if (dev->driver && ops && 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;
> + }
> +
> + /*
> + * 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.
> + */
> + dev->seq = uclass_resolve_seq(dev);
> + eth_write_hwaddr(dev, "eth", dev->seq);
I still don't like this sorry. I don't see why you can't do this in
eth_init() above?
If you really want to do this, please add #ifndef CONFIG_DM_NET around
the check you have taken out and we can sort it out later.
> +
> + return 0;
> +}
> +
> +static int eth_pre_unbind(struct udevice *dev)
It still feels like this is using binding as the presence/absence of a
device rather than probing. When a device is removed with the remove()
method, it is no longer available for use, so things like
eth_try_another() should ignore them. By the time we come to unbind a
device, it should already be removed.
One way to think of this is that the probe()/remove() corresponds to
the same idea as in Linux, and bind()/unbind() is a new thing meaning
that we are aware of the device but it may not currently be available.
> +{
> + struct udevice *first = NULL;
> + struct udevice *active;
> + struct udevice *it;
> + struct uclass *uc;
> +
> + uclass_get(UCLASS_ETH, &uc);
> + uclass_foreach_dev(it, uc) {
> + if (!first)
> + first = it;
> + if (it == dev) {
> + if (dev == first) {
> + active = list_entry(it->uclass_node.next,
> + struct udevice, uclass_node);
> + if (&active->uclass_node == &uc->dev_head)
> + active = NULL;
> + } else {
> + active = first;
> + }
> + eth_set_dev(active);
> + eth_current_changed();
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int eth_post_probe(struct udevice *dev)
> +{
> + struct eth_device_priv *priv = dev->uclass_priv;
> + struct eth_pdata *pdata = dev->platdata;
> +
> + if (priv)
no need for this check
> + priv->state = ETH_STATE_INIT;
> +
> + if (is_zero_ether_addr(pdata->enetaddr))
> + return -EINVAL;
> +
> + 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 +751,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 +815,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,12 +844,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