[U-Boot] [RFC PATCH v2 5/8] net: Add basic driver model support to Ethernet stack
Simon Glass
sjg at chromium.org
Fri Feb 13 06:20:46 CET 2015
Hi Joe,
On 10 February 2015 at 23:25, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>
>
> On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 2 February 2015 at 17:38, Joe Hershberger <joe.hershberger at ni.com>
>> wrote:
>> > First just add support for MAC drivers.
>> >
>> > Signed-off-by: Joe Hershberger <joe.hershberger at 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
More information about the U-Boot
mailing list