[U-Boot] [RFC PATCH v3 07/14] dm: eth: Add basic driver model support to Ethernet stack
Simon Glass
sjg at chromium.org
Wed Feb 18 06:02:12 CET 2015
Hi Joe,
On 16 February 2015 at 21:37, Joe Hershberger <joe.hershberger at gmail.com> wrote:
> 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.
It's up to you, but I know what it is like when you have a lot of
patches backed up. A real board certainly helps though.
>
>> > 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.
OK, that's fine, it can die later.
>
>> > + 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.
OK
>
>> > +#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.
OK, so it could be an oppty to tidy it up, but it's fine to leave it.
>
>> > + 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.
OK, can you add a comment about that here?
>
>> > + 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
>> >
>
Regards,
Simon
More information about the U-Boot
mailing list