From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hershberger Date: Wed, 11 Feb 2015 00:25:19 -0600 Subject: [U-Boot] [RFC PATCH v2 5/8] net: Add basic driver model support to Ethernet stack In-Reply-To: References: <1422401245-8452-1-git-send-email-joe.hershberger@ni.com> <1422923925-5572-1-git-send-email-joe.hershberger@ni.com> <1422923925-5572-6-git-send-email-joe.hershberger@ni.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass wrote: > > Hi Joe, > > On 2 February 2015 at 17:38, Joe Hershberger wrote: > > First just add support for MAC drivers. > > > > Signed-off-by: Joe Hershberger > > > > --- > > > > 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 | 24 ++++ > > net/eth.c | 319 ++++++++++++++++++++++++++++++++++++++++++++++++- > > 5 files changed, 346 insertions(+), 4 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 7eef9cc..4d21d91 100644 > > --- a/include/net.h > > +++ b/include/net.h > > @@ -78,6 +78,30 @@ enum eth_state_t { > > ETH_STATE_ACTIVE > > }; > > > > +#ifdef CONFIG_DM_ETH > > You may not need this? I need it for the function prototypes that have a different device pointer parameter. > > +struct eth_pdata { > > + phys_addr_t iobase; > > + unsigned char enetaddr[6]; > > +}; > > + > > +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..1b5a169 100644 > > --- a/net/eth.c > > +++ b/net/eth.c > > @@ -72,6 +72,320 @@ static int eth_mac_skip(int index) > > return ((skip_state = getenv(enetvar)) != NULL); > > } > > > > +static void eth_current_changed(void); > > + > > +#ifdef CONFIG_DM_ETH > > +#include > > +#include > > These should go at the top of the file - should be OK to always > include them (i.e. no #ifdef) OK... Moved them. > > + > > +struct eth_device_priv { > > + int state; > > + void *priv; > > +}; > > + > > +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); > > This can actually return an error, although I agree there is little > point in handling it. So I suppose this is OK. I think this should be a given. > > + uc_priv = uc->priv; > > + uclass_next_device(&uc_priv->current); > > + if (!uc_priv->current) > > + uclass_first_device(UCLASS_ETH, &uc_priv->current); > > +} > > + > > +struct udevice *eth_get_dev(void) > > +{ > > + struct uclass *uc; > > blank line here OK on all of these. > > + uclass_get(UCLASS_ETH, &uc); > > + > > + struct eth_uclass_priv *uc_priv = uc->priv; > > + return uc_priv->current; > > +} > > + > > +static void eth_set_dev(struct udevice *dev) > > +{ > > + struct uclass *uc; > > blank line here > > > + uclass_get(UCLASS_ETH, &uc); > > + > > + struct eth_uclass_priv *uc_priv = uc->priv; > > + uc_priv->current = dev; > > +} > > + > > +unsigned char *eth_get_ethaddr(void) > > +{ > > + struct eth_pdata *pdata; > > blank line here > > > + 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) > > +{ > > + struct eth_device_priv *priv; > > blank line here > > > + 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; > > blank line here > > > + 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; > > + struct uclass *uc; > > + > > + current = eth_get_dev(); > > + if (!current) { > > + puts("No ethernet found.\n"); > > + return -1; > > + } > > + device_probe(current); > > Check error OK. > > + > > + /* 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_pdata *pdata = dev->platdata; > > blank line > > Do all devices have the same platdata by design? What if a particular > device wants its own? That's a good question. I imagine some devices may have something unique, but I would expect they would read that data into the priv data that they define. How do other subsystems handle platform data for unique devices? > > + if (pdata) > > What do you need this check? No. This was left over from when enetaddr was in priv. > > + memcpy(pdata->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; > > blank line > > > + 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 -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; > > Seems like eth_get_dev() should return an error code if current or > current->driver is NULL. It's returning a pointer, so I prefer it to be NULL or a valid pointer. Perhaps you meant to say that eth_send() should return an error code? > > + > > + const struct eth_ops *ops = current->driver->ops; > > + return ops->send(current, packet, length); > > +} > > + > > +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_pdata *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; > > + } > > + > > + const struct eth_ops *ops = dev->driver->ops; > > + if (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); > > + } > > The above code seems mostly duplicated but I suppose it is hard to > make it common? It is, but I look forward to the day when we delete the other copy. It would be difficult to combine, for sure. > > + > > + 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; > > + } > > + > > + if (!eth_get_dev()) { > > + eth_set_dev(dev); > > + eth_current_changed(); > > + } > > I still don't really get what you are doing here. Perhaps you could > add some comments? There should be no probed devices at this stage > since you are binding only. This has changed a fair amount in v3. Please comment there if it's more clear now. > > + > > + char *ethprime = getenv("ethprime"); > > + if (ethprime && strcmp(dev->name, ethprime) == 0) { > > + eth_set_dev(dev); > > + eth_current_changed(); > > + } > > + > > + /* > > + * 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. > > + */ > > + eth_write_hwaddr(dev, "eth", uclass_resolve_seq(dev)); > > + > > + return 0; > > +} > > + > > +static int eth_pre_unbind(struct udevice *dev) > > +{ > > + struct udevice *first = NULL; > > + struct udevice *current; > > + struct uclass *uc; > > + > > + uclass_get(UCLASS_ETH, &uc); > > + uclass_foreach_dev(current, uc) { > > + if (!first) > > + first = current; > > + if (current == dev) { > > + if (dev == first) { > > + current = list_entry(current->uclass_node.next, > > + struct udevice, uclass_node); > > + } else { > > + current = first; > > + } > > + eth_set_dev(current); > > + eth_current_changed(); > > + } > > + } > > If this is turning down a device it really should happen in a remove() > method. Maybe in post_remove()? This has changed a fair amount in v3. Please comment there if it's more clear now. > > + > > + return 0; > > +} > > + > > +static int eth_post_probe(struct udevice *dev) > > +{ > > + struct eth_device_priv *priv = dev->uclass_priv; > > + if (priv) > > + priv->state = ETH_STATE_INIT; > > + > > + 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 +737,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 +801,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 +830,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