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 <s...@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 <patr...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
