[U-Boot] [RFC PATCH v3 07/14] dm: eth: Add basic driver model support to Ethernet stack
Joe Hershberger
joe.hershberger at gmail.com
Tue Feb 17 05:37:52 CET 2015
Hi Simon,
On Sun, Feb 15, 2015 at 9:49 AM, Simon Glass <sjg at chromium.org> wrote:
>
> 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.
I'm a bit leery to merge this until I've actually got more of the
real-world implementation for a board done. I guess it could always be
corrected in the future, but at the same time, I think it should be fairly
complete. Do you prefer that it go in as smaller parts? There's still no
actual board supported and the MDIO / PHY support is not done yet.
> > 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?
Legacy. I can kill it if you like.
> > + 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.
Again, legacy. I just copied the former function prototypes and changed
the first parameter to udevice*.
> > +#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.
OK.
> > +
> > +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).
Legacy. Same function prototypes as original. As for active and passive
state, that basically is what the Ethernet stack uses to keep track of if
the network driver is enabled. These state-only functions were added to
improve NetConsole performance on certain hardware without overhauling the
network stack state transitions.
> > +#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;
> }
OK.
> 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.
Correct.
> > +{
> > + 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.
OK
> > + }
> > + 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().
Sorry... once again a copy of the previous code with eth_device switched to
udevice.
> > + 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.
It should already be probed before this function can be called.
> > + 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)
OK
> 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
OK
> > + 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?
I'll have to make a new function for this (explained in the other patch
comments).
> 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.
OK... That makes sense. I'll move it over.
> > +{
> > + 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
OK
> > + 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
> >
More information about the U-Boot
mailing list