net: rtl8169: allow multiple devices
RFC PATCH 1/1 net: rtl8169: allow multiple devices
From: Stephen Warren <[email protected]>
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.
---
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 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. 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. 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? Is ethprime intended to be functional at present, or is it some legacy feature that's bit-rotted and should be removed?
--- drivers/net/rtl8169.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/net/rtl8169.c +17 -0
@@ -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),
Signature · 1 line
2.8.1
RFC PATCH 1/1 net: rtl8169: allow multiple devices
Quoted 13 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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 <[email protected]>---
Having to put this code into each Ethernet driver seems a bit odd. Surely the core should handle this; shouldeth_get_dev_by_name()parse "#n" out of the ethact value and compare it to the device sequence number?
Quoted 11 lines
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. 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. 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? Is ethprime intended to be functional at present, or is it some legacy feature that's bit-rotted and should be removed?
Simon, Joe, any thoughts on those questions re: this patch?
RFC PATCH 1/1 net: rtl8169: allow multiple devices
In reply to Stephen WarrenHi Stephen,
Quoted 7 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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.
Quoted 5 lines
Signed-off-by Stephen Warren <[email protected]>---
Having to put this code into each Ethernet driver seems a bit odd. Surely the core should handle this; shouldeth_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().
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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?
Quoted 3 lines
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
Quoted 46 lines
--- drivers/net/rtl8169.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)drivers/net/rtl8169.c +17
-1@@ -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
RFC PATCH 1/1 net: rtl8169: allow multiple devices
Quoted 9 lines · Joe Hershberger
Hi Stephen,
Quoted 6 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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.
... (issues with ethprime/ethact)
Quoted 7 lines
Quoted 4 lines
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?
Ah. The problem is that modifying ethprime after it's been used the first time has no effect. So, the following work:
pci enum
setenv ethprime 'RTL8169#0'
dhcp zImage
or:
pci enum
setenv ethprime 'RTL8169#1'
dhcp zImage
but this doesn't:
pci enum
setenv ethprime 'RTL8169#0'
dhcp zImage
# Move Ethernet cable
setenv ethprime 'RTL8169#1'
dhcp zImage
I guess that makes sense given how ethprime is supposed to work, but was a bit confusing when I wasn't aware.
RFC PATCH 1/1 net: rtl8169: allow multiple devices
Quoted 44 lines · Stephen Warren
Quoted 10 lines · Joe Hershberger
Hi Stephen,
Quoted 6 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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.
... (issues with ethprime/ethact)
Quoted 8 lines
Quoted 5 lines
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?
Ah. The problem is that modifying ethprime after it's been used the first time has no effect. So, the following work:
pci enum
setenv ethprime 'RTL8169#0'
dhcp zImageor:
pci enum
setenv ethprime 'RTL8169#1'
dhcp zImagebut this doesn't:
pci enum
setenv ethprime 'RTL8169#0'
dhcp zImage
# Move Ethernet cable
setenv ethprime 'RTL8169#1'
dhcp zImage
Quoted 2 lines
I guess that makes sense given how ethprime is supposed to work, but was a bit confusing when I wasn't aware.
There is one problematic case in the current code-base, without this patch, again because there can be multiple Ethernet adapters with the same name.
eth-uclass.c's eth_get_dev_by_name() allows devices to be looked up either by name, or via string "ethN" which will find the Nth adapter. That enables the user to select between even identically-named adapters. However, eth_current_changed() then over-writes ethact to the adapter name, which potentially causes a different adapter to be found next time, which likely causes failures, e.g. if the user only plugged a cable into one of the two.
Quoted 3 lines
pci enum
setenv ethact eth1
dhcp zImage
<<succeeds>
Quoted 1 line
echo $ethact
eth_rtl8169
Quoted 1 line
dhcp zImage
<<fails>>
This patches solves that problem too, since the value in the over-written ethact is unique so everything works.