[U-Boot] [RFC PATCH v3 11/14] dm: eth: Add support for aliases

[Simon Glass <sjg@chromium.org>]

Hi Joe,

On 16 February 2015 at 22:04, Joe Hershberger <joe.hershberger@gmail.com> wrote:
>
>
> On Sun, Feb 15, 2015 at 9:50 AM, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 10 February 2015 at 18:30, Joe Hershberger <joe.hershberger@ni.com>
>> wrote:
>> > Allow network devices to be referred to as "eth0" instead of
>> > "eth at 12345678" when specified in ethact.
>> >
>> > Add tests to verify this behavior.
>> >
>> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> >
>> > ---
>> >
>> > Changes in v3:
>> > -Added support for aliases
>> >
>> > Changes in v2: None
>> >
>> >  include/configs/sandbox.h |  4 ++--
>> >  include/fdtdec.h          |  1 +
>> >  include/net.h             |  5 +++++
>> >  lib/fdtdec.c              |  1 +
>> >  net/eth.c                 | 53
>> > +++++++++++++++++++++++++++++++++++++++--------
>> >  test/dm/eth.c             | 25 ++++++++++++++++++++++
>> >  test/dm/test.dts          | 10 +++++----
>> >  7 files changed, 84 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
>> > index fdba1c8..9df5f74 100644
>> > --- a/include/configs/sandbox.h
>> > +++ b/include/configs/sandbox.h
>> > @@ -187,7 +187,7 @@
>> >                                         "stderr=serial,lcd\0" \
>> >                                         "ethaddr=00:00:11:22:33:44\0" \
>> >                                         "eth1addr=00:00:11:22:33:45\0" \
>> > -                                       "eth2addr=00:00:11:22:33:46\0" \
>> > +                                       "eth5addr=00:00:11:22:33:46\0" \
>> >                                         "ipaddr=1.2.3.4\0"
>> >  #else
>> >
>> > @@ -196,7 +196,7 @@
>> >                                         "stderr=serial,lcd\0" \
>> >                                         "ethaddr=00:00:11:22:33:44\0" \
>> >                                         "eth1addr=00:00:11:22:33:45\0" \
>> > -                                       "eth2addr=00:00:11:22:33:46\0" \
>> > +                                       "eth5addr=00:00:11:22:33:46\0" \
>> >                                         "ipaddr=1.2.3.4\0"
>> >  #endif
>> >
>> > diff --git a/include/fdtdec.h b/include/fdtdec.h
>> > index 231eed7..e945baa 100644
>> > --- a/include/fdtdec.h
>> > +++ b/include/fdtdec.h
>> > @@ -167,6 +167,7 @@ enum fdt_compat_id {
>> >         COMPAT_INTEL_GMA,               /* Intel Graphics Media
>> > Accelerator */
>> >         COMPAT_AMS_AS3722,              /* AMS AS3722 PMIC */
>> >         COMPAT_INTEL_ICH_SPI,           /* Intel ICH7/9 SPI controller
>> > */
>> > +       COMPAT_ETHERNET,                /* Ethernet devices */
>>
>> SANDBOX_ETHERNET
>
> This is not limited to sandbox.  This is needed for all Ethernet MACs.  Is
> there some other way that I should be identifying with all devices in the
> device tree of a certain class?

Not that I know of. But each Ethernet driver would need its own
compatible string, since otherwise driver model won't use the right
driver.

>
>> >
>> >         COMPAT_COUNT,
>> >  };
>> > diff --git a/include/net.h b/include/net.h
>> > index 11471bd..4e98850 100644
>> > --- a/include/net.h
>> > +++ b/include/net.h
>> > @@ -38,6 +38,8 @@
>> >
>> >  #define PKTALIGN       ARCH_DMA_MINALIGN
>> >
>> > +#define ETH_MAX_DEVS   32
>> > +
>> >  /* IPv4 addresses are always 32 bits in size */
>> >  typedef __be32         IPaddr_t;
>> >
>> > @@ -79,6 +81,8 @@ enum eth_state_t {
>> >  };
>> >
>> >  #ifdef CONFIG_DM_ETH
>> > +#define ETH_ALIAS_ROOT "eth"
>> > +
>> >  struct eth_pdata {
>> >         phys_addr_t iobase;
>> >         unsigned char enetaddr[6];
>> > @@ -96,6 +100,7 @@ struct eth_ops {
>> >  };
>> >
>> >  struct udevice *eth_get_dev(void); /* get the current device */
>> > +struct udevice *eth_get_dev_by_name(const char *devname);
>> >  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 */
>> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> > index 5bf8f29..33b0a53 100644
>> > --- a/lib/fdtdec.c
>> > +++ b/lib/fdtdec.c
>> > @@ -75,6 +75,7 @@ static const char * const compat_names[COMPAT_COUNT] =
>> > {
>> >         COMPAT(INTEL_GMA, "intel,gma"),
>> >         COMPAT(AMS_AS3722, "ams,as3722"),
>> >         COMPAT(INTEL_ICH_SPI, "intel,ich-spi"),
>> > +       COMPAT(ETHERNET, "eth"),
>>
>> sandbox,eth
>
> Again, this is used to identify all Ethernet controllers. Perhaps
> fdtdec_find_aliases_for_id() should not be limiting the alias search to
> those that have a certain "compatible" string?

I think you want 'sandbox,ethernet' here, or similar, but you probably
don't want to use that fdtdec function ultimately. Driver model should
do it all for you.

>
>
>> >  };
>> >
>> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
>> > diff --git a/net/eth.c b/net/eth.c
>> > index e84b948..762effe 100644
>> > --- a/net/eth.c
>> > +++ b/net/eth.c
>> > @@ -10,11 +10,14 @@
>> >  #include <command.h>
>> >  #include <dm.h>
>> >  #include <dm/device-internal.h>
>> > +#include <fdtdec.h>
>> >  #include <net.h>
>> >  #include <miiphy.h>
>> >  #include <phy.h>
>> >  #include <asm/errno.h>
>> >
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> >  void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>> >  {
>> >         char *end;
>> > @@ -121,6 +124,39 @@ static void eth_set_dev(struct udevice *dev)
>> >         uc_priv->current = dev;
>> >  }
>> >
>> > +/*
>> > + * Find the udevice that either has the name passed in as devname or
>> > has an
>> > + * alias named devname.
>> > + */
>> > +struct udevice *eth_get_dev_by_name(const char *devname)
>> > +{
>> > +       int node_list[ETH_MAX_DEVS];
>> > +       int count;
>> > +       int seq;
>> > +       char *endp = NULL;
>> > +       const char *true_name = devname;
>> > +       struct udevice *it;
>> > +       struct uclass *uc;
>> > +
>> > +       count = fdtdec_find_aliases_for_id(gd->fdt_blob, ETH_ALIAS_ROOT,
>> > +                                          COMPAT_ETHERNET, node_list,
>> > +                                          ETH_MAX_DEVS);
>> > +
>> > +       seq = simple_strtoul(devname + strlen(ETH_ALIAS_ROOT), &endp,
>> > 10);
>> > +
>> > +       if (endp > devname + strlen(ETH_ALIAS_ROOT) && count > seq &&
>> > +           node_list[seq])
>> > +               true_name = fdt_get_name(gd->fdt_blob, node_list[seq],
>> > NULL);
>> > +
>> > +       uclass_get(UCLASS_ETH, &uc);
>> > +       uclass_foreach_dev(it, uc) {
>> > +               if (strcmp(it->name, true_name) == 0 || it->seq == seq)
>> > +                       return it;
>> > +       }
>>
>> Is it possible for eth_get_dev_by_name() to just look up the name
>> provided?
>>
>> You already have this:
>>
>> uclass_foreach_dev(it, uc) {
>> if (strcmp(it->name, true_name) == 0 || it->seq == seq)
>> return it;
>> }
>>
>> It feels like you just need to through through the devices in the
>> uclass and strcmp(dev->name, find_name).
>
> That works fine for looking up the exact name, but that's not the point of
> this patch.  Such code was already there and you can see it below.

OK

>
>> I guess I am suffering because I don't understand what you are trying
>> to do, so am not sure if there is an easier way with driver model. We
>> really should not go looking in the device tree in
>> eth_get_dev_by_name()...driver is responsible for parsing that.
>
> Perhaps this is a limitation of fdtdec_find_aliases_for_id().  I need to be
> able to look up the device based on its alias.  It doesn't seem to me that
> I'm digging around in the device tree at random places that should be done
> by the driver.  I'm simply wanting to ask the device tree what the alias
> that the user passed in is pointing at.  Potentially is it not an alias, but
> a full name.  Either way, I want to return the udevice.

If you want to know the device's number, use dev->req_seq. This will
hold the alias number. If it is -1 then the device is not mentioned in
/aliases in the device tree.

But please see if you can use the existing driver model features to
find devices, their numbers, etc.

>
>
>> > +
>> > +       return NULL;
>> > +}
>> > +
>> >  unsigned char *eth_get_ethaddr(void)
>> >  {
>> >         struct eth_pdata *pdata;
>> > @@ -396,6 +432,7 @@ UCLASS_DRIVER(eth) = {
>> >         .init           = eth_uclass_init,
>> >         .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
>> >         .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
>> > +       .flags          = DM_UC_FLAG_SEQ_ALIAS,
>> >  };
>> >  #endif
>> >
>> > @@ -428,6 +465,11 @@ static void eth_set_current_to_next(void)
>> >         eth_current = eth_current->next;
>> >  }
>> >
>> > +static void eth_set_dev(struct eth_device *dev)
>> > +{
>> > +       eth_current = dev;
>> > +}
>> > +
>> >  struct eth_device *eth_get_dev_by_name(const char *devname)
>> >  {
>> >         struct eth_device *dev, *target_dev;
>> > @@ -844,7 +886,6 @@ void eth_set_current(void)
>> >  {
>> >         static char *act;
>> >         static int  env_changed_id;
>> > -       void *old_current;
>> >         int     env_id;
>> >
>> >         env_id = get_env_id();
>> > @@ -852,14 +893,8 @@ void eth_set_current(void)
>> >                 act = getenv("ethact");
>> >                 env_changed_id = env_id;
>> >         }
>> > -       if (act != NULL) {
>> > -               old_current = eth_get_dev();
>> > -               do {
>> > -                       if (strcmp(eth_get_name(), act) == 0)
>> > -                               return;
>> > -                       eth_set_current_to_next();
>> > -               } while (old_current != eth_get_dev());
>> > -       }
>
> This is the simple compare that is being replaced.
>
>> > +       if (act != NULL)
>> > +               eth_set_dev(eth_get_dev_by_name(act));
>> >
>> >         eth_current_changed();
>> >  }
>> > diff --git a/test/dm/eth.c b/test/dm/eth.c
>> > index 2b29fa2..c0a8ab5 100644
>> > --- a/test/dm/eth.c
>> > +++ b/test/dm/eth.c
>> > @@ -37,3 +37,28 @@ static int dm_test_eth(struct dm_test_state *dms)
>> >  }
>> >
>> >  DM_TEST(dm_test_eth, DM_TESTF_SCAN_FDT);
>> > +
>> > +static int dm_test_eth_alias(struct dm_test_state *dms)
>> > +{
>> > +       NetPingIP = string_to_ip("1.1.2.2");
>> > +       setenv("ethact", "eth0");
>> > +       ut_assertok(NetLoop(PING));
>> > +       ut_asserteq_str("eth at 10002000", getenv("ethact"));
>> > +
>> > +       setenv("ethact", "eth1");
>> > +       ut_assertok(NetLoop(PING));
>> > +       ut_asserteq_str("eth at 10004000", getenv("ethact"));
>> > +
>> > +       /* Expected to fail since eth2 is not defined in the device tree
>> > */
>> > +       setenv("ethact", "eth2");
>> > +       ut_asserteq(-1, NetLoop(PING));
>> > +       ut_asserteq_ptr(NULL, getenv("ethact"));
>> > +
>> > +       setenv("ethact", "eth5");
>> > +       ut_assertok(NetLoop(PING));
>> > +       ut_asserteq_str("eth at 10003000", getenv("ethact"));
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +DM_TEST(dm_test_eth_alias, DM_TESTF_SCAN_FDT);
>> > diff --git a/test/dm/test.dts b/test/dm/test.dts
>> > index 2f68cdf..c5008c3 100644
>> > --- a/test/dm/test.dts
>> > +++ b/test/dm/test.dts
>> > @@ -17,6 +17,8 @@
>> >                 testfdt3 = "/b-test";
>> >                 testfdt5 = "/some-bus/c-test at 5";
>> >                 testfdt8 = "/a-test";
>> > +               eth0 = "/eth at 10002000";
>> > +               eth5 = &eth_5;
>> >         };
>> >
>> >         uart0: serial {
>> > @@ -150,19 +152,19 @@
>> >         };
>> >
>> >         eth at 10002000 {
>> > -               compatible = "sandbox,eth";
>> > +               compatible = "sandbox,eth", "eth";
>> >                 reg = <0x10002000 0x1000>;
>> >                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x00>;
>> >         };
>> >
>> > -       eth at 10003000 {
>> > -               compatible = "sandbox,eth";
>> > +       eth_5: eth at 10003000 {
>> > +               compatible = "sandbox,eth", "eth";
>> >                 reg = <0x10003000 0x1000>;
>> >                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x11>;
>> >         };
>> >
>> >         eth at 10004000 {
>> > -               compatible = "sandbox,eth";
>> > +               compatible = "sandbox,eth", "eth";
>> >                 reg = <0x10004000 0x1000>;
>> >                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x22>;
>> >         };
>> > --
>> > 1.7.11.5

Regards,
Simon