From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Fri, 6 Feb 2015 18:25:47 -0700 Subject: [U-Boot] [RFC PATCH v2 5/8] net: Add basic driver model support to Ethernet stack In-Reply-To: <1422923925-5572-6-git-send-email-joe.hershberger@ni.com> 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 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? > +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) > + > +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. > + 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 > + 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 > + > + /* 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? > + if (pdata) What do you need this check? > + 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. > + > + 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? > + > + 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. > + > + 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()? > + > + 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