[U-Boot] rtl8169: use dm_pci_map_bar

[Thierry Reding <thierry.reding@gmail.com>]

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 <sr@denx.de> 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 <patrick@blueri.se>
> > > > >
> > > > > 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: <http://lists.denx.de/pipermail/u-boot/attachments/20190614/7859aac6/attachment-0001.sig>