* [U-Boot] rtl8169: use dm_pci_map_bar
@ 2019-06-11 11:15 Patrick Wildt
2019-06-13 5:40 ` Stefan Roese
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Wildt @ 2019-06-11 11:15 UTC (permalink / raw)
To: u-boot
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) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] rtl8169: use dm_pci_map_bar
2019-06-11 11:15 [U-Boot] rtl8169: use dm_pci_map_bar Patrick Wildt
@ 2019-06-13 5:40 ` Stefan Roese
2019-06-13 7:16 ` Bin Meng
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2019-06-13 5:40 UTC (permalink / raw)
To: u-boot
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".
Thanks,
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] rtl8169: use dm_pci_map_bar
2019-06-13 5:40 ` Stefan Roese
@ 2019-06-13 7:16 ` Bin Meng
2019-06-13 16:43 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Bin Meng @ 2019-06-13 7:16 UTC (permalink / raw)
To: u-boot
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.
Regards,
Bin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] rtl8169: use dm_pci_map_bar
2019-06-13 7:16 ` Bin Meng
@ 2019-06-13 16:43 ` Thierry Reding
2019-06-13 23:16 ` Patrick Wildt
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2019-06-13 16:43 UTC (permalink / raw)
To: u-boot
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
-------------- 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] rtl8169: use dm_pci_map_bar
2019-06-13 16:43 ` Thierry Reding
@ 2019-06-13 23:16 ` Patrick Wildt
2019-06-14 9:55 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Wildt @ 2019-06-13 23:16 UTC (permalink / raw)
To: u-boot
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.
Patrick
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] rtl8169: use dm_pci_map_bar
2019-06-13 23:16 ` Patrick Wildt
@ 2019-06-14 9:55 ` Thierry Reding
0 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2019-06-14 9:55 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-14 9:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11 11:15 [U-Boot] rtl8169: use dm_pci_map_bar Patrick Wildt
2019-06-13 5:40 ` Stefan Roese
2019-06-13 7:16 ` Bin Meng
2019-06-13 16:43 ` Thierry Reding
2019-06-13 23:16 ` Patrick Wildt
2019-06-14 9:55 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.