From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Date: Fri, 14 Jun 2019 11:55:56 +0200 Subject: [U-Boot] rtl8169: use dm_pci_map_bar In-Reply-To: <20190613231632.GA43759@nyx.fritz.box> References: <20190611111500.GA53008@thor.local> <20190613164325.GD3258@ulmo> <20190613231632.GA43759@nyx.fritz.box> Message-ID: <20190614095556.GD15526@ulmo> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Fri, Jun 14, 2019 at 01:16:32AM +0200, Patrick Wildt wrote: > On Thu, Jun 13, 2019 at 06:43:25PM +0200, Thierry Reding wrote: > > On Thu, Jun 13, 2019 at 03:16:10PM +0800, Bin Meng wrote: > > > Hi Stefan, > > > > > > On Thu, Jun 13, 2019 at 1:40 PM Stefan Roese wrote: > > > > > > > > Added Bin, Joe and Thierry to Cc > > > > > > > > On 11.06.19 13:15, Patrick Wildt wrote: > > > > > Hi, > > > > > > > > > > I have an rtl8169 on a macchiatobin and that card has a 64-bit > > > > > memory address. The current code only reads a single word, which > > > > > means it can only support a 32-bit address. By using dm_pci_map_bar > > > > > we don't need to manually parse the register, we can just have it do > > > > > its job. > > > > > > > > > > I'm not sure though if this works for all devices since the previous > > > > > version had an explicit check for the device. > > > > > > > > > > Patrick > > > > > > > > > > Signed-off-by: Patrick Wildt > > > > > > > > > > diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c > > > > > index 521e5909a2..f1d2ade253 100644 > > > > > --- a/drivers/net/rtl8169.c > > > > > +++ b/drivers/net/rtl8169.c > > > > > @@ -1182,22 +1182,11 @@ static int rtl8169_eth_probe(struct udevice *dev) > > > > > struct pci_child_platdata *pplat = dev_get_parent_platdata(dev); > > > > > struct rtl8169_private *priv = dev_get_priv(dev); > > > > > struct eth_pdata *plat = dev_get_platdata(dev); > > > > > - u32 iobase; > > > > > - int region; > > > > > int ret; > > > > > > > > > > - debug("rtl8169: REALTEK RTL8169 @0x%x\n", iobase); > > > > > - switch (pplat->device) { > > > > > - case 0x8168: > > > > > - region = 2; > > > > > - break; > > > > > - default: > > > > > - region = 1; > > > > > - break; > > > > > - } > > > > > - dm_pci_read_config32(dev, PCI_BASE_ADDRESS_0 + region * 4, &iobase); > > > > > - iobase &= ~0xf; > > > > > - priv->iobase = (int)dm_pci_mem_to_phys(dev, iobase); > > > > > + priv->iobase = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_2, > > > > > + PCI_REGION_MEM); > > > > > + printf("rtl8169: REALTEK RTL8169 @0x%x\n", priv->iobase); > > > > > > > > > > ret = rtl_init(priv->iobase, dev->name, plat->enetaddr); > > > > > if (ret < 0) { > > > > > > > > Bin, Joe, Thierry, > > > > > > > > do you have any comments on this patch? Moving unconditionally to one > > > > BAR instead of BAR1/2 depending on the chip version seems a bit > > > > "brave". > > > > > > Agreed that blinding setting one BAR for the iobase is not a good idea. > > > > Agreed. I don't know whether it's actually required to differentiate > > based on version, but I suppose the code is like that for a reason, so > > better keep that. > > > > Also, looking at dm_pci_map_bar() it doesn't look like that does > > anything different than the existing code. It merely reads a single > > 32-bit register, so it doesn't properly deal with 64-bit BARs either. > > > > I suppose that could be fixed in dm_pci_map_bar(), and then the fix > > would automatically propagate to all users of that, which is good. But I > > don't think it currently works correctly. > > > > Thierry > > Oh, I'm very sorry to have wasted your time. Yes, I agree, the > differentiation must come back. > > To be honest it was about half a year ago, so I think I forgot the > actual issue. Maybe dm_pci_mem_to_phys() returned 0 for whatever > reason, but I don't remember. The address is 32-bit, 0xf6000000, > so it definitely wasn't the 64-bit think I rambled about. Sorry. > > I will try to find out what was wrong and report back. > > By the way, the debug() line in the current code prints iobase which at > that time isn't even initialized. Yeah, that's definitely a bug. I think that warrants a separate patch. Mind sending out a patch with just that change? Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: