| Message ID | 1461706140-2768-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Commit | dad7b74045929ff10d93ec1dd60d0fd36fd9a527 |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Tue, Apr 26, 2016 at 4:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > The rtl8169 driver uses a global variable to store the register address > of the adapter being operated upon. This is updated to point at the > correct adapter when sending or receiving a packet, or shutting down the > adapter, but not when initializing the adapter. Consequently, switching > between different adapters within the same U-Boot runtime does not work > correctly since the driver programs the wrong registers during > rtl8169_eth_start() -> rtl8169_common_start() -> rtl8169_hw_start(). > > Note that since rtl8169_eth_stop() does set the global variable, the > second consecutive attempt to use the "new" adapter did work even before > this patch, because each time network usage is shut down, the network > core calls stop, which sets the variable so that the next start does > actually initialize the hardware, and the adapter works. > > Equally, rtl8169_eth_probe() calls rtl_init() which sets the global, so > if using only a single device, or if picking the "right" device (based on > probe order) when multiple devices are present, ioaddr will already be set > correctly from the get-go, so the issue does not occur. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> Looks reasonable to me. Acked-by: Joe Hershberger <joe.hershberger@ni.com> > --- > Note that as with almost any patch, I believe NVIDIA copyright applies to > the submission. However, I don't want to block all my work based on that > other conversion, so I'm just going to deliberately ignore our policy of > adding/updating copyright headers to files when editing them. Hopefully > doing so deliberately (rather than the usual case of just forgetting) > doesn't get me fired. Hope you keep your job... at least if you like it. -Joe
On 04/26/2016 04:05 PM, Joe Hershberger wrote: > On Tue, Apr 26, 2016 at 4:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> The rtl8169 driver uses a global variable to store the register address >> of the adapter being operated upon. This is updated to point at the >> correct adapter when sending or receiving a packet, or shutting down the >> adapter, but not when initializing the adapter. Consequently, switching >> between different adapters within the same U-Boot runtime does not work >> correctly since the driver programs the wrong registers during >> rtl8169_eth_start() -> rtl8169_common_start() -> rtl8169_hw_start(). >> >> Note that since rtl8169_eth_stop() does set the global variable, the >> second consecutive attempt to use the "new" adapter did work even before >> this patch, because each time network usage is shut down, the network >> core calls stop, which sets the variable so that the next start does >> actually initialize the hardware, and the adapter works. >> >> Equally, rtl8169_eth_probe() calls rtl_init() which sets the global, so >> if using only a single device, or if picking the "right" device (based on >> probe order) when multiple devices are present, ioaddr will already be set >> correctly from the get-go, so the issue does not occur. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > > Looks reasonable to me. > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Joe, I assume you'll be applying this?
On Wed, Jun 1, 2016 at 11:40 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/26/2016 04:05 PM, Joe Hershberger wrote: >> >> On Tue, Apr 26, 2016 at 4:29 PM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> The rtl8169 driver uses a global variable to store the register address >>> of the adapter being operated upon. This is updated to point at the >>> correct adapter when sending or receiving a packet, or shutting down the >>> adapter, but not when initializing the adapter. Consequently, switching >>> between different adapters within the same U-Boot runtime does not work >>> correctly since the driver programs the wrong registers during >>> rtl8169_eth_start() -> rtl8169_common_start() -> rtl8169_hw_start(). >>> >>> Note that since rtl8169_eth_stop() does set the global variable, the >>> second consecutive attempt to use the "new" adapter did work even before >>> this patch, because each time network usage is shut down, the network >>> core calls stop, which sets the variable so that the next start does >>> actually initialize the hardware, and the adapter works. >>> >>> Equally, rtl8169_eth_probe() calls rtl_init() which sets the global, so >>> if using only a single device, or if picking the "right" device (based on >>> probe order) when multiple devices are present, ioaddr will already be >>> set >>> correctly from the get-go, so the issue does not occur. >>> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> >> >> Looks reasonable to me. >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > > Joe, > > I assume you'll be applying this? Yes.
On 06/02/2016 04:06 PM, Joe Hershberger wrote: > On Wed, Jun 1, 2016 at 11:40 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/26/2016 04:05 PM, Joe Hershberger wrote: >>> >>> On Tue, Apr 26, 2016 at 4:29 PM, Stephen Warren <swarren@wwwdotorg.org> >>> wrote: >>>> >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> The rtl8169 driver uses a global variable to store the register address >>>> of the adapter being operated upon. This is updated to point at the >>>> correct adapter when sending or receiving a packet, or shutting down the >>>> adapter, but not when initializing the adapter. Consequently, switching >>>> between different adapters within the same U-Boot runtime does not work >>>> correctly since the driver programs the wrong registers during >>>> rtl8169_eth_start() -> rtl8169_common_start() -> rtl8169_hw_start(). >>>> >>>> Note that since rtl8169_eth_stop() does set the global variable, the >>>> second consecutive attempt to use the "new" adapter did work even before >>>> this patch, because each time network usage is shut down, the network >>>> core calls stop, which sets the variable so that the next start does >>>> actually initialize the hardware, and the adapter works. >>>> >>>> Equally, rtl8169_eth_probe() calls rtl_init() which sets the global, so >>>> if using only a single device, or if picking the "right" device (based on >>>> probe order) when multiple devices are present, ioaddr will already be >>>> set >>>> correctly from the get-go, so the issue does not occur. >>>> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>> >>> >>> Looks reasonable to me. >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> >> Joe, >> >> I assume you'll be applying this? > > Yes. I don't see this applied anywhere yet.
Hi Stephen, https://patchwork.ozlabs.org/patch/615309/ was applied to u-boot-net.git. Thanks! -Joe
diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c index 163b9df55c9b..843b083f8f90 100644 --- a/drivers/net/rtl8169.c +++ b/drivers/net/rtl8169.c @@ -850,9 +850,11 @@ static void rtl8169_init_ring(pci_dev_t dev) } #ifdef CONFIG_DM_ETH -static void rtl8169_common_start(struct udevice *dev, unsigned char *enetaddr) +static void rtl8169_common_start(struct udevice *dev, unsigned char *enetaddr, + unsigned long dev_iobase) #else -static void rtl8169_common_start(pci_dev_t dev, unsigned char *enetaddr) +static void rtl8169_common_start(pci_dev_t dev, unsigned char *enetaddr, + unsigned long dev_iobase) #endif { int i; @@ -862,6 +864,8 @@ static void rtl8169_common_start(pci_dev_t dev, unsigned char *enetaddr) printf ("%s\n", __FUNCTION__); #endif + ioaddr = dev_iobase; + rtl8169_init_ring(dev); rtl8169_hw_start(dev); /* Construct a perfect filter frame with the mac address as first match @@ -885,8 +889,9 @@ static void rtl8169_common_start(pci_dev_t dev, unsigned char *enetaddr) static int rtl8169_eth_start(struct udevice *dev) { struct eth_pdata *plat = dev_get_platdata(dev); + struct rtl8169_private *priv = dev_get_priv(dev); - rtl8169_common_start(dev, plat->enetaddr); + rtl8169_common_start(dev, plat->enetaddr, priv->iobase); return 0; } @@ -897,7 +902,7 @@ RESET - Finish setting up the ethernet interface static int rtl_reset(struct eth_device *dev, bd_t *bis) { rtl8169_common_start((pci_dev_t)(unsigned long)dev->priv, - dev->enetaddr); + dev->enetaddr, dev->iobase); return 0; }