[U-Boot] rtl8169: use dm_pci_map_bar

[Thierry Reding]

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 at 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 at 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
-------------- 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/20190613/81205155/attachment.sig>