[U-Boot] [RFC PATCH 5/7] net: Add basic driver model support to Ethernet stack
Joe Hershberger
joe.hershberger at gmail.com
Wed Jan 28 11:22:05 CET 2015
On Tue, Jan 27, 2015 at 8:34 PM, Simon Glass <sjg at chromium.org> wrote:
>
> 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.
Sounds good.
> > 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?
Sure.
> >
> > 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.
So in net.h you'll see that I have a platdata structure as well... the
reason I didn't put the MAC address in there is that the data is not
sourced from the device tree or the platform data in general. It either
comes from the env (default) or it is read from the network MAC's eeprom.
I'll need a little more convincing that the MAC should move.
> > +};
> > +
> > +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.
I started out using a static variable for the current device, but other
code in u-boot uses the current device (via the eth_get_dev() function) and
I want it to not require and extern to a non-static variable.
> (Also remove internal brackets)
Yes... forgot to clean this up.
> If priv->current is NULL, this will die.
This function should not get called if priv->current is NULL.
> Also to avoid confusion I think you should use uc_priv for the uclass
> priv local variable, to distinguish it from priv.
OK.
> > + 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)
No... I'm not switching on priv->current being NULL... I get the next, and
then if THAT is NULL, I start at the beginning. priv->current must never
be NULL (outside of this transition).
This is emulating the behavior that used to exist where the linked list
instead of terminating linked back to the head. This way we always cycle
through all adapters and never stop.
> > +}
> > +
> > +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.
Why would the device being probed have any relationship with the priv in
the uclass itself? I would expect the uclass priv to be there regardless.
This is not the per-device uc_priv.
> > + 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.
So this is guaranteed? In all cases? I ran into some pre-allocated
buffers that don't get created if post_probe returns an error, for instance.
> > + 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?
OK.
> > +}
> > +
> > +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.
I agree that longer term we should look for ways to remove that limitation,
but for now everything that uses the network assumes that the "ethact" env
var selects the device. There would be a lot of commands to break
compatibility with to allow them to specify the device. I say we hold off
on that for now.
> But still I wonder if you should have functions that are passed a
> udevice, like eth_rx(struct udevice *dev).
Even if those functions did, the commands would need to change too. Future
enhancement, I think.
> > +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?
For knowing the number of devices that have been bound and allocating the
next unused index for the next device.
> > +
> > + 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?
Never decreases.
> 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)
I actually used the seq number originally and that worked fine with
platdata. When I moved to the device tree, that's where it fell apart. In
the device tree, it is expected that the eth name will include an
eth at baseaddr format. The DM code then takes that value that is the
baseaddr and sets it as the requested seq. That means I get eth268443648
instead of eth0. No good. So now I keep track of my own seq (index) to
make sure I get consistent device names.
> Otherwise I'm not sure what this function is trying to do.
I needs to initialize the priv->current variable so that everything that
tries to use the network finds a current eth device.
It also initializes the state variable, which is also important.
> > + }
> > +
> > + 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.
Again, this is just ensuring the "current" variable is always valid.
> > +
> > + 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?
This allows the initial "current" device to be specified by they "ethprime"
env var. This is preserving existing behaviors as controlled by these env
vars.
> > +
> > + 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
Good idea.
> > +
> > + 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list