Hi Joe,
On 28 January 2015 at 03:22, Joe Hershberger <joe.hershber...@gmail.com> wrote:
> On Tue, Jan 27, 2015 at 8:34 PM, Simon Glass <s...@chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 27 January 2015 at 16:27, Joe Hershberger <joe.hershber...@ni.com>
>> wrote:
>> > First just add support for MAC drivers.
>> >
>>
>> I don't fully understand this partly because my knowledge of the
>> network stack is limited. So I'll make a few comments and we can go
>> from there.
>
> Sounds good.
>
>> > Signed-off-by: Joe Hershberger <joe.hershber...@ni.com>
>> > ---
>> >
>> > common/board_r.c | 4 +-
>> > common/cmd_bdinfo.c | 2 +
>> > include/dm/uclass-id.h | 1 +
>> > include/net.h | 23 ++++
>> > net/eth.c | 320
>> > ++++++++++++++++++++++++++++++++++++++++++++++++-
>> > 5 files changed, 346 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/common/board_r.c b/common/board_r.c
>> > index a301cc2..9a41cae 100644
>> > --- a/common/board_r.c
>> > +++ b/common/board_r.c
>> > @@ -572,7 +572,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: ");
>> > @@ -841,7 +841,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 f17c3c2..b04cbc9 100644
>> > --- a/include/dm/uclass-id.h
>> > +++ b/include/dm/uclass-id.h
>> > @@ -33,6 +33,7 @@ enum uclass_id {
>> > UCLASS_I2C, /* I2C bus */
>> > UCLASS_I2C_GENERIC, /* Generic I2C device */
>> > UCLASS_I2C_EEPROM, /* I2C EEPROM device */
>> > + UCLASS_ETH, /* Network device */
>>
>> Ethernet device?
>
> Sure.
>
>> >
>> > UCLASS_COUNT,
>> > UCLASS_INVALID = -1,
>> > diff --git a/include/net.h b/include/net.h
>> > index 7eef9cc..25636e2 100644
>> > --- a/include/net.h
>> > +++ b/include/net.h
>> > @@ -78,6 +78,29 @@ enum eth_state_t {
>> > ETH_STATE_ACTIVE
>> > };
>> >
>> > +#ifdef CONFIG_DM_ETH
>> > +struct eth_pdata {
>> > + phys_addr_t iobase;
>> > +};
>> > +
>> > +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..d245b65 100644
>> > --- a/net/eth.c
>> > +++ b/net/eth.c
>> > @@ -72,6 +72,321 @@ 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>
>> > +
>> > +struct eth_device_priv {
>> > + unsigned char enetaddr[6];
>> > + int state;
>> > + int index;
>> > + void *priv;
>>
>> Suggestion: you could have per-child platform data as well as
>> per-child private data. See u-boot-dm/master for this. It is used for
>> I2C and SPI to hold the address of the child on the bus.
>>
>> You could use it to hold the MAC address. I think this *might* be
>> better because the MAC address can then be set and retained even when
>> the device is probed/removed.
>
> So in net.h you'll see that I have a platdata structure as well... the
> reason I didn't put the MAC address in there is that the data is not sourced
> from the device tree or the platform data in general. It either comes from
> the env (default) or it is read from the network MAC's eeprom. I'll need a
> little more convincing that the MAC should move.
OK. If it only exists when the device is active (probed) then that is fine.
>
>> > +};
>> > +
>> > +struct eth_uclass_priv {
>> > + struct udevice *current;
>> > + int max_index;
>> > +};
>> > +
>> > +static void eth_set_current_to_next(void)
>> > +{
>> > + struct uclass *uc;
>> > + struct eth_uclass_priv *priv;
>> > +
>> > + uclass_get(UCLASS_ETH, &uc);
>> > + priv = uc->priv;
>> > + uclass_next_device(&(priv->current));
>>
>> Note that this will probe the device. I think you are storing the
>> current ethernet device in the uclass, but you could just as well have
>> a static variable in this file if that is easier.
>
> I started out using a static variable for the current device, but other code
> in u-boot uses the current device (via the eth_get_dev() function) and I
> want it to not require and extern to a non-static variable.
OK.
>
>> (Also remove internal brackets)
>
> Yes... forgot to clean this up.
>
>> If priv->current is NULL, this will die.
>
> This function should not get called if priv->current is NULL.
>
>> Also to avoid confusion I think you should use uc_priv for the uclass
>> priv local variable, to distinguish it from priv.
>
> OK.
>
>> > + if (!priv->current)
>> > + uclass_first_device(UCLASS_ETH, &(priv->current));
>>
>> If I understand this correctly, I think you want:
>>
>> if (priv->current)
>> uclass_next_device(&priv->current)
>> else
>> uclass_first_device(uc, &priv->current)
>
> No... I'm not switching on priv->current being NULL... I get the next, and
> then if THAT is NULL, I start at the beginning. priv->current must never be
> NULL (outside of this transition).
>
> This is emulating the behavior that used to exist where the linked list
> instead of terminating linked back to the head. This way we always cycle
> through all adapters and never stop.
OK I see.
>
>> > +}
>> > +
>> > +struct udevice *eth_get_dev(void)
>> > +{
>> > + struct uclass *uc;
>> > + uclass_get(UCLASS_ETH, &uc);
>> > +
>> > + struct eth_uclass_priv *priv = uc->priv;
>>
>> This is OK so long as the device has been probed. I think it always is
>> because you use uclass_next_device() to get the device.
>
> Why would the device being probed have any relationship with the priv in the
> uclass itself? I would expect the uclass priv to be there regardless. This
> is not the per-device uc_priv.
Yes you are right, I was confused by the 'priv' name.
>
>> > + return priv->current;
>> > +}
>> > +
>> > +static void eth_set_dev(struct udevice *dev)
>> > +{
>> > + struct uclass *uc;
>> > + uclass_get(UCLASS_ETH, &uc);
>> > +
>> > + struct eth_uclass_priv *priv = uc->priv;
>> > + priv->current = dev;
>> > +}
>> > +
>> > +unsigned char *eth_get_ethaddr(void)
>> > +{
>> > + struct eth_device_priv *priv;
>> > + if (eth_get_dev()) {
>> > + priv = eth_get_dev()->uclass_priv;
>> > + if (priv)
>>
>> This check should be unnecessary. Same in other cases below.
>
> So this is guaranteed? In all cases? I ran into some pre-allocated buffers
> that don't get created if post_probe returns an error, for instance.
If the device is probed then it has uclass_priv. If post_probe()
returns an error then the device will be removed (i.e. the probe will
fail).
>
>
>> > + return priv->enetaddr;
>> > + }
>> > + return NULL;
>> > +}
>> > +
>> > +/* Set active state */
>> > +int eth_init_state_only(bd_t *bis)
>> > +{
>> > + struct eth_device_priv *priv;
>> > + 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;
>> > + if (eth_get_dev()) {
>> > + priv = eth_get_dev()->uclass_priv;
>> > + if (priv)
>> > + priv->state = ETH_STATE_PASSIVE;
>> > + }
>> > +}
>> > +
>> > +int eth_get_dev_index(void)
>> > +{
>> > + struct eth_device_priv *priv;
>> > + if (eth_get_dev()) {
>> > + priv = eth_get_dev()->uclass_priv;
>> > + if (priv)
>> > + return priv->index;
>> > + }
>> > + 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;
>> > + }
>> > +
>> > + /* 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_device_priv *priv = dev->uclass_priv;
>> > + if (priv)
>> > + memcpy(priv->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;
>> > + 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 -1;
>>
>> -ENODEV?
>
> OK.
>
>
>> > +}
>> > +
>> > +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;
>> > +
>> > + const struct eth_ops *ops = current->driver->ops;
>> > + return ops->send(current, packet, length);
>> > +}
>> > +
>>
>> As a general comment, there is an implicit assumption that only one
>> device can be active at a time. I suppose that is a U-Boot limitation,
>> but we don't need to keep it for driver model, or at least we could
>> permit multiple devices to be probed at a time.
>
> I agree that longer term we should look for ways to remove that limitation,
> but for now everything that uses the network assumes that the "ethact" env
> var selects the device. There would be a lot of commands to break
> compatibility with to allow them to specify the device. I say we hold off
> on that for now.
OK
One thing to be careful of is probing devices before you use them. By
using uclass_first_device() you will probe the device. If you want to
pick a particular device and probe it, you will need to find the
device through another means - e.g. a helper function in the uclass to
find the device without probing it.
>
>> But still I wonder if you should have functions that are passed a
>> udevice, like eth_rx(struct udevice *dev).
>
> Even if those functions did, the commands would need to change too. Future
> enhancement, I think.
OK
>
>
>> > +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_device_priv *priv = dev->uclass_priv;
>> > + if (!is_zero_ether_addr(env_enetaddr)) {
>> > + if (!is_zero_ether_addr(priv->enetaddr) &&
>> > + memcmp(priv->enetaddr, env_enetaddr, 6)) {
>> > + printf("\nWarning: %s MAC addresses don't
>> > match:\n",
>> > + dev->name);
>> > + printf("Address in SROM is %pM\n",
>> > + priv->enetaddr);
>> > + printf("Address in environment is %pM\n",
>> > + env_enetaddr);
>> > + }
>> > +
>> > + memcpy(priv->enetaddr, env_enetaddr, 6);
>> > + } else if (is_valid_ether_addr(priv->enetaddr)) {
>> > + eth_setenv_enetaddr_by_index(base_name, eth_number,
>> > + priv->enetaddr);
>> > + printf("\nWarning: %s using MAC address from net
>> > device\n",
>> > + dev->name);
>> > + } else if (is_zero_ether_addr(priv->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(priv->enetaddr)) {
>> > + printf("\nError: %s address %pM illegal
>> > value\n",
>> > + dev->name, priv->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);
>> > +
>> > + struct eth_uclass_priv *priv = class->priv;
>> > + priv->max_index = 0;
>>
>> What is max_index used for?
>
> For knowing the number of devices that have been bound and allocating the
> next unused index for the next device.
Is this different from the number of devices in the uclass?
>
>> > +
>> > + eth_env_init();
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int eth_post_bind(struct udevice *dev)
>> > +{
>> > + struct udevice *first;
>> > +
>> > + uclass_first_device(UCLASS_ETH, &first);
>> > + if (first == dev) {
>> > + eth_current_changed();
>> > + eth_set_dev(dev);
>> > + }
>> > +
>> > + struct eth_device_priv *priv = dev->uclass_priv;
>> > + struct eth_uclass_priv *uclass_priv = dev->uclass->priv;
>> > + if (priv) {
>> > + priv->state = ETH_STATE_INIT;
>> > + priv->index = uclass_priv->max_index++;
>>
>> OK I see it is the number of devices. Does this ever decrease?
>
> Never decreases.
>
>> Anyway, struct udevice has a seq member which can give every device a
>> unique sequence number in the uclass automatically (and if you define
>> DM_UC_FLAG_SEQ_ALIAS then the device tree aliases can provide this
>> numbering)
>
> I actually used the seq number originally and that worked fine with
> platdata. When I moved to the device tree, that's where it fell apart. In
> the device tree, it is expected that the eth name will include an
> eth@baseaddr format. The DM code then takes that value that is the baseaddr
> and sets it as the requested seq. That means I get eth268443648 instead of
> eth0. No good. So now I keep track of my own seq (index) to make sure I
> get consistent device names.
That should be fixed now - see u-boot-dm/master. Also the alias
approach is useful since it lets you number the devices within the
uclass as you wish.
>
>> Otherwise I'm not sure what this function is trying to do.
>
> I needs to initialize the priv->current variable so that everything that
> tries to use the network finds a current eth device.
> It also initializes the state variable, which is also important.
OK, also you can't really probe a device in the post_bind() handler.
Probing should only happen when the device is used.
>
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int eth_pre_unbind(struct udevice *dev)
>> > +{
>> > + struct udevice *first;
>> > + struct udevice *current;
>> > +
>> > + current = eth_get_dev();
>> > + uclass_first_device(UCLASS_ETH, &first);
>> > + if (current == dev) {
>> > + if (dev == first)
>> > + uclass_next_device(¤t);
>> > + else
>> > + current = first;
>> > + eth_current_changed();
>> > + eth_set_dev(current);
>> > + }
>>
>> I'm not sure what this function is trying to do.
>
> Again, this is just ensuring the "current" variable is always valid.
OK. Shouldn't this happen when the device is removed rather than when
it is unbound (well I suppose if it happens when removed it doesn't
need to happen when unbound).
>
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int eth_post_probe(struct udevice *dev)
>> > +{
>> > + char *ethprime = getenv("ethprime");
>> > + if (ethprime && strcmp(dev->name, ethprime) == 0)
>> > + eth_set_dev(dev);
>>
>> What does this do?
>
> This allows the initial "current" device to be specified by they "ethprime"
> env var. This is preserving existing behaviors as controlled by these env
> vars.
OK
>
>> > +
>> > + if (strchr(dev->name, ' '))
>> > + printf("\nWarning: eth device name \"%s\" has a
>> > space!\n",
>> > + dev->name);
>>
>> BTW if this is an error you could refuse to bind it - e.g. in the
>> eth_post_bind, return -EINVAL
>
> Good idea.
>
>
>> > +
>> > + struct eth_device_priv *priv = dev->uclass_priv;
>> > + if (priv)
>> > + return eth_write_hwaddr(dev, "eth", priv->index);
>> > + return 1;
>> > +}
>> > +
>> > +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 +738,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 +802,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 +831,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 */
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
