[U-Boot] [RFC PATCH 5/7] net: Add basic driver model support to Ethernet stack
Simon Glass
sjg at chromium.org
Wed Jan 28 03:34:20 CET 2015
Hi Joe,
On 27 January 2015 at 16:27, Joe Hershberger <joe.hershberger at ni.com> wrote:
> First just add support for MAC drivers.
>
I don't fully understand this partly because my knowledge of the
network stack is limited. So I'll make a few comments and we can go
from there.
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
> ---
>
> common/board_r.c | 4 +-
> common/cmd_bdinfo.c | 2 +
> include/dm/uclass-id.h | 1 +
> include/net.h | 23 ++++
> net/eth.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 346 insertions(+), 4 deletions(-)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index a301cc2..9a41cae 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -572,7 +572,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: ");
> @@ -841,7 +841,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 f17c3c2..b04cbc9 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -33,6 +33,7 @@ enum uclass_id {
> UCLASS_I2C, /* I2C bus */
> UCLASS_I2C_GENERIC, /* Generic I2C device */
> UCLASS_I2C_EEPROM, /* I2C EEPROM device */
> + UCLASS_ETH, /* Network device */
Ethernet device?
>
> UCLASS_COUNT,
> UCLASS_INVALID = -1,
> diff --git a/include/net.h b/include/net.h
> index 7eef9cc..25636e2 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -78,6 +78,29 @@ enum eth_state_t {
> ETH_STATE_ACTIVE
> };
>
> +#ifdef CONFIG_DM_ETH
> +struct eth_pdata {
> + phys_addr_t iobase;
> +};
> +
> +struct eth_ops {
> + int (*init)(struct udevice *dev, bd_t *bis);
> + 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);
> +#endif
> + int (*write_hwaddr)(struct udevice *dev);
> +};
> +
> +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 */
> +#endif
> +
> +#ifndef CONFIG_DM_ETH
> struct eth_device {
> char name[16];
> unsigned char enetaddr[6];
> diff --git a/net/eth.c b/net/eth.c
> index c02548c..d245b65 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -72,6 +72,321 @@ static int eth_mac_skip(int index)
> return ((skip_state = getenv(enetvar)) != NULL);
> }
>
> +static void eth_current_changed(void);
> +
> +#ifdef CONFIG_DM_ETH
> +#include <dm.h>
> +
> +struct eth_device_priv {
> + unsigned char enetaddr[6];
> + int state;
> + int index;
> + void *priv;
Suggestion: you could have per-child platform data as well as
per-child private data. See u-boot-dm/master for this. It is used for
I2C and SPI to hold the address of the child on the bus.
You could use it to hold the MAC address. I think this *might* be
better because the MAC address can then be set and retained even when
the device is probed/removed.
> +};
> +
> +struct eth_uclass_priv {
> + struct udevice *current;
> + int max_index;
> +};
> +
> +static void eth_set_current_to_next(void)
> +{
> + struct uclass *uc;
> + struct eth_uclass_priv *priv;
> +
> + uclass_get(UCLASS_ETH, &uc);
> + priv = uc->priv;
> + uclass_next_device(&(priv->current));
Note that this will probe the device. I think you are storing the
current ethernet device in the uclass, but you could just as well have
a static variable in this file if that is easier.
(Also remove internal brackets)
If priv->current is NULL, this will die.
Also to avoid confusion I think you should use uc_priv for the uclass
priv local variable, to distinguish it from priv.
> + if (!priv->current)
> + uclass_first_device(UCLASS_ETH, &(priv->current));
If I understand this correctly, I think you want:
if (priv->current)
uclass_next_device(&priv->current)
else
uclass_first_device(uc, &priv->current)
> +}
> +
> +struct udevice *eth_get_dev(void)
> +{
> + struct uclass *uc;
> + uclass_get(UCLASS_ETH, &uc);
> +
> + struct eth_uclass_priv *priv = uc->priv;
This is OK so long as the device has been probed. I think it always is
because you use uclass_next_device() to get the device.
> + return priv->current;
> +}
> +
> +static void eth_set_dev(struct udevice *dev)
> +{
> + struct uclass *uc;
> + uclass_get(UCLASS_ETH, &uc);
> +
> + struct eth_uclass_priv *priv = uc->priv;
> + priv->current = dev;
> +}
> +
> +unsigned char *eth_get_ethaddr(void)
> +{
> + struct eth_device_priv *priv;
> + if (eth_get_dev()) {
> + priv = eth_get_dev()->uclass_priv;
> + if (priv)
This check should be unnecessary. Same in other cases below.
> + return priv->enetaddr;
> + }
> + return NULL;
> +}
> +
> +/* Set active state */
> +int eth_init_state_only(bd_t *bis)
> +{
> + struct eth_device_priv *priv;
> + if (eth_get_dev()) {
> + priv = eth_get_dev()->uclass_priv;
> + if (priv)
> + priv->state = ETH_STATE_ACTIVE;
> + }
> +
> + 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)
> +{
> + struct eth_device_priv *priv;
> + if (eth_get_dev()) {
> + priv = eth_get_dev()->uclass_priv;
> + if (priv)
> + return priv->index;
> + }
> + return -1;
> +}
> +
> +int eth_init(bd_t *bis)
> +{
> + struct udevice *current, *old_current, *dev;
> + struct uclass *uc;
> +
> + current = eth_get_dev();
> + if (!current) {
> + puts("No ethernet found.\n");
> + return -1;
> + }
> +
> + /* Sync environment with network devices */
> + uclass_get(UCLASS_ETH, &uc);
> + uclass_foreach_dev(dev, uc) {
> + uchar env_enetaddr[6];
> +
> + if (eth_getenv_enetaddr_by_index("eth", dev->seq,
> + env_enetaddr)) {
> + struct eth_device_priv *priv = dev->uclass_priv;
> + if (priv)
> + memcpy(priv->enetaddr, env_enetaddr, 6);
> + }
> + };
> +
> + old_current = current;
> + do {
> + debug("Trying %s\n", current->name);
> +
> + if (current->driver) {
> + const struct eth_ops *ops = current->driver->ops;
> + if (ops->init(current, bis) >= 0) {
> + struct eth_device_priv *priv =
> + current->uclass_priv;
> + if (priv)
> + priv->state = ETH_STATE_ACTIVE;
> +
> + return 0;
> + }
> + }
> + debug("FAIL\n");
> +
> + eth_try_another(0);
> + current = eth_get_dev();
> + } while (old_current != current);
> +
> + return -1;
-ENODEV?
> +}
> +
> +void eth_halt(void)
> +{
> + struct udevice *current;
> +
> + current = eth_get_dev();
> + if (!current)
> + return;
> + if (!current->driver)
> + return;
> +
> + const struct eth_ops *ops = current->driver->ops;
> + ops->halt(current);
> +
> + struct eth_device_priv *priv = current->uclass_priv;
> + if (priv)
> + priv->state = ETH_STATE_PASSIVE;
> +}
> +
> +int eth_send(void *packet, int length)
> +{
> + struct udevice *current;
> +
> + current = eth_get_dev();
> + if (!current)
> + return -1;
> + if (!current->driver)
> + return -1;
> +
> + const struct eth_ops *ops = current->driver->ops;
> + return ops->send(current, packet, length);
> +}
> +
As a general comment, there is an implicit assumption that only one
device can be active at a time. I suppose that is a U-Boot limitation,
but we don't need to keep it for driver model, or at least we could
permit multiple devices to be probed at a time.
But still I wonder if you should have functions that are passed a
udevice, like eth_rx(struct udevice *dev).
> +int eth_rx(void)
> +{
> + struct udevice *current;
> +
> + current = eth_get_dev();
> + if (!current)
> + return -1;
> + if (!current->driver)
> + return -1;
> +
> + const struct eth_ops *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;
> +
> + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
> +
> + struct eth_device_priv *priv = dev->uclass_priv;
> + if (!is_zero_ether_addr(env_enetaddr)) {
> + if (!is_zero_ether_addr(priv->enetaddr) &&
> + memcmp(priv->enetaddr, env_enetaddr, 6)) {
> + printf("\nWarning: %s MAC addresses don't match:\n",
> + dev->name);
> + printf("Address in SROM is %pM\n",
> + priv->enetaddr);
> + printf("Address in environment is %pM\n",
> + env_enetaddr);
> + }
> +
> + memcpy(priv->enetaddr, env_enetaddr, 6);
> + } else if (is_valid_ether_addr(priv->enetaddr)) {
> + eth_setenv_enetaddr_by_index(base_name, eth_number,
> + priv->enetaddr);
> + printf("\nWarning: %s using MAC address from net device\n",
> + dev->name);
> + } else if (is_zero_ether_addr(priv->enetaddr)) {
> + printf("\nError: %s address not set.\n",
> + dev->name);
> + return -EINVAL;
> + }
> +
> + const struct eth_ops *ops = dev->driver->ops;
> + if (ops->write_hwaddr && !eth_mac_skip(eth_number)) {
> + if (!is_valid_ether_addr(priv->enetaddr)) {
> + printf("\nError: %s address %pM illegal value\n",
> + dev->name, priv->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);
> +
> + struct eth_uclass_priv *priv = class->priv;
> + priv->max_index = 0;
What is max_index used for?
> +
> + eth_env_init();
> +
> + return 0;
> +}
> +
> +static int eth_post_bind(struct udevice *dev)
> +{
> + struct udevice *first;
> +
> + uclass_first_device(UCLASS_ETH, &first);
> + if (first == dev) {
> + eth_current_changed();
> + eth_set_dev(dev);
> + }
> +
> + struct eth_device_priv *priv = dev->uclass_priv;
> + struct eth_uclass_priv *uclass_priv = dev->uclass->priv;
> + if (priv) {
> + priv->state = ETH_STATE_INIT;
> + priv->index = uclass_priv->max_index++;
OK I see it is the number of devices. Does this ever decrease?
Anyway, struct udevice has a seq member which can give every device a
unique sequence number in the uclass automatically (and if you define
DM_UC_FLAG_SEQ_ALIAS then the device tree aliases can provide this
numbering)
Otherwise I'm not sure what this function is trying to do.
> + }
> +
> + return 0;
> +}
> +
> +static int eth_pre_unbind(struct udevice *dev)
> +{
> + struct udevice *first;
> + struct udevice *current;
> +
> + current = eth_get_dev();
> + uclass_first_device(UCLASS_ETH, &first);
> + if (current == dev) {
> + if (dev == first)
> + uclass_next_device(¤t);
> + else
> + current = first;
> + eth_current_changed();
> + eth_set_dev(current);
> + }
I'm not sure what this function is trying to do.
> +
> + return 0;
> +}
> +
> +static int eth_post_probe(struct udevice *dev)
> +{
> + char *ethprime = getenv("ethprime");
> + if (ethprime && strcmp(dev->name, ethprime) == 0)
> + eth_set_dev(dev);
What does this do?
> +
> + if (strchr(dev->name, ' '))
> + printf("\nWarning: eth device name \"%s\" has a space!\n",
> + dev->name);
BTW if this is an error you could refuse to bind it - e.g. in the
eth_post_bind, return -EINVAL
> +
> + struct eth_device_priv *priv = dev->uclass_priv;
> + if (priv)
> + return eth_write_hwaddr(dev, "eth", priv->index);
> + return 1;
> +}
> +
> +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 +738,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 +802,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,7 +831,7 @@ 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 */
> --
> 1.7.11.5
>
Regards,
Simon
More information about the U-Boot
mailing list