Hi Joe, On 10 February 2015 at 23:25, Joe Hershberger <joe.hershber...@gmail.com> wrote: > > > On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass <s...@chromium.org> wrote: >> >> Hi Joe, >> >> On 2 February 2015 at 17:38, Joe Hershberger <joe.hershber...@ni.com> >> wrote: >> > First just add support for MAC drivers. >> > >> > Signed-off-by: Joe Hershberger <joe.hershber...@ni.com> >> > >> > --- >> > >> > 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(-) >> >
[snip] >> > 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 <dm.h> >> > +#include <dm/device-internal.h> >> >> 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? There isn't great support for it. Typically the device has its own platform data. For buses there is per-child platform data, which the uclass can define, but we don't have uclass-defined platform data for normal devices (which are not children). > >> > + 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? Well yes that too. But -1 is not a valid error anymore. You need to use -EPERM or something. And whatever went wrong with eth_get_dev() should really be returned. So you could do: ret = eth_get_dev(¤t); if (ret) return ret; or if you prefer: current = eth_get_dev(); if (IS_ERR(current)) return PTR_ERR(current); > > >> > + >> > + const struct eth_ops *ops = current->driver->ops; >> > + return ops->send(current, packet, length); >> > +} >> > + > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot