From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 17 Feb 2015 22:02:12 -0700 Subject: [U-Boot] [RFC PATCH v3 07/14] dm: eth: Add basic driver model support to Ethernet stack In-Reply-To: References: <1422923925-5572-1-git-send-email-joe.hershberger@ni.com> <1423618233-11397-1-git-send-email-joe.hershberger@ni.com> <1423618233-11397-8-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 16 February 2015 at 21:37, Joe Hershberger wrote: > Hi Simon, > > On Sun, Feb 15, 2015 at 9:49 AM, Simon Glass wrote: >> >> Hi Joe, >> >> On 10 February 2015 at 18:30, Joe Hershberger >> 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 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 >> > >> > --- >> > >> > 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 >> > #include >> > +#include >> > +#include >> > #include >> > #include >> > #include >> > @@ -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