[U-Boot] [RFC PATCH] net: rtl8169: allow multiple devices

[Joe Hershberger]

Hi Stephen,

On Wed, Apr 20, 2016 at 6:31 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> From: Stephen Warren <swarren at nvidia.com>
>
> Currently, if multiple rtl8169 devices exist on the PCI bus, they all
> get the same name, which prevents the user from selecting which to use
> via the ethact environment variable. Port the auto-naming code from the
> e1000 driver to solve this.
>
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> ---
> Having to put this code into each Ethernet driver seems a bit odd. Surely
> the core should handle this; should eth_get_dev_by_name() parse "#n" out
> of the ethact value and compare it to the device sequence number?

It does seem at first glance to be the wrong place for it, but I also
don't think that each driver will be completely uninvolved. Likely we
will need to have a core function that sets a unique name, and for any
driver that expects to support PCI, it needs to call that function in
its bind function.

The names just need to be unique. You could certainly name them
uniquely by gluing the sequence number to it instead of a card
counter, but that would still be a bind thing, not in
eth_get_dev_by_name().

> It looks like I should be able to set ethprime to e.g. eth0, eth1, etc.
> and this should work. However, I couldn't get ethprime to behave sensibly,
> and I'm not sure what its semantics are supposed to be.

It is supposed to be the first adapter to try if ethact is not set.
It's a default that lives in a separate variable so that it can
survive env save. It is part of the behavior of fail-over and
auto-trying the next adapter.

> Specifically,
> ethprime seems to only be used if ethact isn't set, yet accessing the
> network (e.g. running "dhcp zImage") seems to set ethact, thus preventing
> any further modification to ethprime from having any effect.

That is the expected behavior.

> Equally,
> simply running e.g. "dhcp zImage" twice in a row doesn't seem to work;
> perhaps the subsequent attempts perform another lookup by name from ethact
> rather than just using the same device pointer from before?

I believe it does work in general. Are you saying that it doesn't work
on your board without this patch?

> Is ethprime
> intended to be functional at present, or is it some legacy feature that's
> bit-rotted and should be removed?

It is functional at present, but I've always thought it to be of
limited use. I'd rather see volatile variables added to the env and
drop things like this.

-Joe

> ---
>  drivers/net/rtl8169.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c
> index 163b9df55c9b..0ccddcabc478 100644
> --- a/drivers/net/rtl8169.c
> +++ b/drivers/net/rtl8169.c
> @@ -1192,6 +1192,22 @@ static int rtl8169_eth_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int rtl8169_eth_bind(struct udevice *dev)
> +{
> +       static int num_cards;
> +       char name[20];
> +
> +       /*
> +        * A simple way to number the devices. When device tree is used this
> +        * is unnecessary, but when the device is just discovered on the PCI
> +        * bus we need a name. We could instead have the uclass figure out
> +        * which devices are different and number them.
> +        */
> +       sprintf(name, "RTL8169#%d", num_cards++);
> +
> +       return device_set_name(dev, name);
> +}
> +
>  static const struct eth_ops rtl8169_eth_ops = {
>         .start  = rtl8169_eth_start,
>         .send   = rtl8169_eth_send,
> @@ -1208,6 +1224,7 @@ U_BOOT_DRIVER(eth_rtl8169) = {
>         .name   = "eth_rtl8169",
>         .id     = UCLASS_ETH,
>         .of_match = rtl8169_eth_ids,
> +       .bind   = rtl8169_eth_bind,
>         .probe  = rtl8169_eth_probe,
>         .ops    = &rtl8169_eth_ops,
>         .priv_auto_alloc_size = sizeof(struct rtl8169_private),
> --
> 2.8.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot