[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[Ivan Mercier]

Hi,

I'm using a ethernet controller intel i210 
(http://www.commell.com.tw/product/Surveillance/MPX-210.htm) on my 
nvidia tegra k1 jetson.

I not an expert with pci, but the only way to make it working in u-boot 
(upstream) is with the workaround below.

E1000 is very common, so finding a critical bug in this driver seems 
weird...
Do you think there is a bug in e1000.c or in tegra pci layer?


diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 2ba03ed..206884d 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -5186,7 +5186,7 @@ static int _e1000_transmit(struct e1000_hw *hw, 
void *txpacket, int length)
      txp = tx_base + tx_tail;
      tx_tail = (tx_tail + 1) % 8;

-    txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, nv_packet));
+    txp->buffer_addr = cpu_to_le64((unsigned long) nv_packet);
      txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
      txp->upper.data = 0;



thank you

-- 
Best regards / Bien cordialement

Ivan Mercier

[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[Stephen Warren]

On 10/30/2015 05:07 AM, Ivan Mercier wrote:
> Hi,
>
> I'm using a ethernet controller intel i210
> (http://www.commell.com.tw/product/Surveillance/MPX-210.htm) on my
> nvidia tegra k1 jetson.

(You didn't actually CC anyone involved with Tegra, so I only 
accidentally noticed this while I was looking at my mailing list folder)

> I not an expert with pci, but the only way to make it working in u-boot
> (upstream) is with the workaround below.
>
> E1000 is very common, so finding a critical bug in this driver seems
> weird...
> Do you think there is a bug in e1000.c or in tegra pci layer?

> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c

> @@ -5186,7 +5186,7 @@ static int _e1000_transmit(struct e1000_hw *hw,
> void *txpacket, int length)
>       txp = tx_base + tx_tail;
>       tx_tail = (tx_tail + 1) % 8;
>
> -    txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, nv_packet));
> +    txp->buffer_addr = cpu_to_le64((unsigned long) nv_packet);
>       txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
>       txp->upper.data = 0;

In order to work out what's going on, perhaps you could print out the 
values of nv_packet and virt_to_bus(hw->pdev, nv_packet).

It's not terribly surprising that removing the call to virt_to_bus 
works, since IIRC U-Boot on Tegra uses the same address setup for the 
PCIe bus as for CPU physical addresses as for CPU virtual addresses.

So, the question is: what is virt_to_bus() doing, and is it the right 
API to call?

I see that virt_to_bus() is defined as:

e1000.c:

#define virt_to_bus(devno, v)   pci_virt_to_mem(devno, (void *) (v))

pci.h:

#define pci_virt_to_mem(dev, addr) \
         pci_virt_to_bus((dev), (addr), PCI_REGION_MEM)

#define pci_virt_to_bus(dev, addr, flags) \
         pci_hose_phys_to_bus(pci_bus_to_hose(PCI_BUS(dev)), \
                              (virt_to_phys(addr)), (flags))

I know that the RTL8169 driver works on the same board (it's soldered 
down and attached to the other PCIe root port on the SoC). For what 
looks like the same "use case", it seems to call pci_mem_to_phys() which is:

pci.h:

#define pci_mem_to_phys(dev, addr) \
         pci_bus_to_phys((dev), (addr), PCI_REGION_MEM)

#define pci_bus_to_phys(dev, addr, flags) \
         pci_hose_bus_to_phys(pci_bus_to_hose(PCI_BUS(dev)), (addr), \
				(flags))

That's odd, since one of those does a phys -> bus translation and the 
other does a bus -> phys translation. That's the opposite direction, so 
both can't possibly be right. I wonder if those mapping functions are 
no-ops on whatever architectures the e1000 driver has been used on (and 
hence have caused no issues), but fail for some reason on ARM?

(The other difference is the call to virt_to_phys(), but that's a no-op 
on ARM as far as I can tell).

[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[Bin Meng]

On Thu, Nov 5, 2015 at 2:54 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/30/2015 05:07 AM, Ivan Mercier wrote:
>>
>> Hi,
>>
>> I'm using a ethernet controller intel i210
>> (http://www.commell.com.tw/product/Surveillance/MPX-210.htm) on my
>> nvidia tegra k1 jetson.
>
>
> (You didn't actually CC anyone involved with Tegra, so I only accidentally
> noticed this while I was looking at my mailing list folder)
>
>> I not an expert with pci, but the only way to make it working in u-boot
>> (upstream) is with the workaround below.
>>
>> E1000 is very common, so finding a critical bug in this driver seems
>> weird...
>> Do you think there is a bug in e1000.c or in tegra pci layer?
>
>
>> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
>
>
>> @@ -5186,7 +5186,7 @@ static int _e1000_transmit(struct e1000_hw *hw,
>> void *txpacket, int length)
>>       txp = tx_base + tx_tail;
>>       tx_tail = (tx_tail + 1) % 8;
>>
>> -    txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, nv_packet));
>> +    txp->buffer_addr = cpu_to_le64((unsigned long) nv_packet);
>>       txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
>>       txp->upper.data = 0;
>
>
> In order to work out what's going on, perhaps you could print out the values
> of nv_packet and virt_to_bus(hw->pdev, nv_packet).
>
> It's not terribly surprising that removing the call to virt_to_bus works,
> since IIRC U-Boot on Tegra uses the same address setup for the PCIe bus as
> for CPU physical addresses as for CPU virtual addresses.
>
> So, the question is: what is virt_to_bus() doing, and is it the right API to
> call?

virt_to_bus() is to translate cpu virtual address to pci bus physical
address. Basically two levels of translation.

>
> I see that virt_to_bus() is defined as:
>
> e1000.c:
>
> #define virt_to_bus(devno, v)   pci_virt_to_mem(devno, (void *) (v))
>
> pci.h:
>
> #define pci_virt_to_mem(dev, addr) \
>         pci_virt_to_bus((dev), (addr), PCI_REGION_MEM)
>
> #define pci_virt_to_bus(dev, addr, flags) \
>         pci_hose_phys_to_bus(pci_bus_to_hose(PCI_BUS(dev)), \
>                              (virt_to_phys(addr)), (flags))
>
> I know that the RTL8169 driver works on the same board (it's soldered down
> and attached to the other PCIe root port on the SoC). For what looks like
> the same "use case", it seems to call pci_mem_to_phys() which is:
>
> pci.h:
>
> #define pci_mem_to_phys(dev, addr) \
>         pci_bus_to_phys((dev), (addr), PCI_REGION_MEM)
>
> #define pci_bus_to_phys(dev, addr, flags) \
>         pci_hose_bus_to_phys(pci_bus_to_hose(PCI_BUS(dev)), (addr), \
>                                 (flags))
>
> That's odd, since one of those does a phys -> bus translation and the other
> does a bus -> phys translation. That's the opposite direction, so both can't
> possibly be right. I wonder if those mapping functions are no-ops on

Indeed these address translation macros in pci.h are confusing. But
most SoC defines 1:1:1 mapping so these are not needed. The reason why
it fails on Tegra is probably due to the addresses are not 1:1:1
mapped?

> whatever architectures the e1000 driver has been used on (and hence have
> caused no issues), but fail for some reason on ARM?
>
> (The other difference is the call to virt_to_phys(), but that's a no-op on
> ARM as far as I can tell).

Regards,
Bin

[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[Stephen Warren]

On 11/04/2015 08:49 PM, Bin Meng wrote:
> On Thu, Nov 5, 2015 at 2:54 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/30/2015 05:07 AM, Ivan Mercier wrote:
>>>
>>> Hi,
>>>
>>> I'm using a ethernet controller intel i210
>>> (http://www.commell.com.tw/product/Surveillance/MPX-210.htm) on my
>>> nvidia tegra k1 jetson.
>>
>>
>> (You didn't actually CC anyone involved with Tegra, so I only accidentally
>> noticed this while I was looking at my mailing list folder)
>>
>>> I not an expert with pci, but the only way to make it working in u-boot
>>> (upstream) is with the workaround below.
>>>
>>> E1000 is very common, so finding a critical bug in this driver seems
>>> weird...
>>> Do you think there is a bug in e1000.c or in tegra pci layer?
>>
>>
>>> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
>>
>>
>>> @@ -5186,7 +5186,7 @@ static int _e1000_transmit(struct e1000_hw *hw,
>>> void *txpacket, int length)
>>>        txp = tx_base + tx_tail;
>>>        tx_tail = (tx_tail + 1) % 8;
>>>
>>> -    txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, nv_packet));
>>> +    txp->buffer_addr = cpu_to_le64((unsigned long) nv_packet);
>>>        txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
>>>        txp->upper.data = 0;
>>
>>
>> In order to work out what's going on, perhaps you could print out the values
>> of nv_packet and virt_to_bus(hw->pdev, nv_packet).
>>
>> It's not terribly surprising that removing the call to virt_to_bus works,
>> since IIRC U-Boot on Tegra uses the same address setup for the PCIe bus as
>> for CPU physical addresses as for CPU virtual addresses.
>>
>> So, the question is: what is virt_to_bus() doing, and is it the right API to
>> call?
>
> virt_to_bus() is to translate cpu virtual address to pci bus physical
> address. Basically two levels of translation.
>
>> I see that virt_to_bus() is defined as:
>>
>> e1000.c:
>>
>> #define virt_to_bus(devno, v)   pci_virt_to_mem(devno, (void *) (v))
>>
>> pci.h:
>>
>> #define pci_virt_to_mem(dev, addr) \
>>          pci_virt_to_bus((dev), (addr), PCI_REGION_MEM)
>>
>> #define pci_virt_to_bus(dev, addr, flags) \
>>          pci_hose_phys_to_bus(pci_bus_to_hose(PCI_BUS(dev)), \
>>                               (virt_to_phys(addr)), (flags))
>>
>> I know that the RTL8169 driver works on the same board (it's soldered down
>> and attached to the other PCIe root port on the SoC). For what looks like
>> the same "use case", it seems to call pci_mem_to_phys() which is:
>>
>> pci.h:
>>
>> #define pci_mem_to_phys(dev, addr) \
>>          pci_bus_to_phys((dev), (addr), PCI_REGION_MEM)
>>
>> #define pci_bus_to_phys(dev, addr, flags) \
>>          pci_hose_bus_to_phys(pci_bus_to_hose(PCI_BUS(dev)), (addr), \
>>                                  (flags))
>>
>> That's odd, since one of those does a phys -> bus translation and the other
>> does a bus -> phys translation. That's the opposite direction, so both can't
>> possibly be right. I wonder if those mapping functions are no-ops on
>
> Indeed these address translation macros in pci.h are confusing. But
> most SoC defines 1:1:1 mapping so these are not needed. The reason why
> it fails on Tegra is probably due to the addresses are not 1:1:1
> mapped?

I'm pretty sure everything is 1:1. That's why removing the call to any 
translation macro made the e1000 driver work for Ivan.

[U-Boot] Ethernet i210 (e1000 driver) on tegra K1

[Marcel Ziswiler]

Hi Stephen

By accident I stumbled over this post and by incident now that we at
Toradex are working on an Apalis TK1 module design using the same
i210/i211 gigabit Ethernet chip as already widely deployed on Apalis
T30 we are facing this as well.

On Mon, 2015-11-09 at 10:21 -0700, Stephen Warren wrote:
> On 11/04/2015 08:49 PM, Bin Meng wrote:
> > On Thu, Nov 5, 2015 at 2:54 AM, Stephen Warren <swarren@wwwdotorg.o
> > rg> wrote:
> > > On 10/30/2015 05:07 AM, Ivan Mercier wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I'm using a ethernet controller intel i210
> > > > (http://www.commell.com.tw/product/Surveillance/MPX-210.htm) on
> > > > my
> > > > nvidia tegra k1 jetson.
> > > 
> > > 
> > > (You didn't actually CC anyone involved with Tegra, so I only
> > > accidentally
> > > noticed this while I was looking at my mailing list folder)

Exactly. Looking through the e1000 git log one should also have seen
that me and others actually worked on it and that we are indeed
successfully using it on Tegra albeit T30 only so far.

> > > > I not an expert with pci, but the only way to make it working
> > > > in u-boot
> > > > (upstream) is with the workaround below.
> > > > 
> > > > E1000 is very common, so finding a critical bug in this driver
> > > > seems
> > > > weird...
> > > > Do you think there is a bug in e1000.c or in tegra pci layer?
> > > 
> > > 
> > > > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
> > > 
> > > 
> > > > @@ -5186,7 +5186,7 @@ static int _e1000_transmit(struct
> > > > e1000_hw *hw,
> > > > void *txpacket, int length)
> > > > ???????txp = tx_base + tx_tail;
> > > > ???????tx_tail = (tx_tail + 1) % 8;
> > > > 
> > > > -????txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev,
> > > > nv_packet));
> > > > +????txp->buffer_addr = cpu_to_le64((unsigned long) nv_packet);
> > > > ???????txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
> > > > ???????txp->upper.data = 0;
> > > 
> > > 
> > > In order to work out what's going on, perhaps you could print out
> > > the values
> > > of nv_packet and virt_to_bus(hw->pdev, nv_packet).

I did exactly that which gives:

Loading: nv_packet = 0xfffecb00
pci_hose_phys_to_bus: invalid physical address
virt_to_bus(hw->pdev, nv_packet) = 0x00000000

So that looks OK but does not yet give us much further clues.

Looking at where the actual translation happens
in?drivers/pci/pci_common.c:

int __pci_hose_phys_to_bus(struct pci_controller *hose,
...

		if (bus_addr >= res->bus_start &&
		????bus_addr < res->bus_start + res->size) {
			*ba = bus_addr;
...

So I added some more prints there giving:

bus_addr = 0xfffecb00
phys_addr = 0xfffecb00
res->phys_start = 0x80000000
res->bus_start = 0x80000000
res->size = 0x80000000

Ah, OK. That makes sense now. Just a classical overflow happening in
the address range check (e.g. 0x80000000 + 0x80000000 giving 0x0 in 32-
bits).

Strange that this works for others but e.g. the Realtek driver seems to
do the inverse conversion:

int __pci_hose_bus_to_phys(struct pci_controller *hose,
...

		if (bus_addr >= res->bus_start &&
		????(bus_addr - res->bus_start) < res->size) {
			*pa = (bus_addr - res->bus_start + res-
>phys_start);

Ah, they worked around it by changing the order of the addition vs.
subtraction in the range check. That will work.

I will be sending out a proper patch to fix this shortly.

But why does it work on T30?

bus_addr = 0xffee92e0
phys_addr = 0xffee92e0
res->phys_start = 0x80000000
res->bus_start = 0x80000000
res->size = 0x7ff00000

Ah funny, ooks like we got lucky due to the size being slightly below
0x80000000 (;-p).

> > > It's not terribly surprising that removing the call to
> > > virt_to_bus works,
> > > since IIRC U-Boot on Tegra uses the same address setup for the
> > > PCIe bus as
> > > for CPU physical addresses as for CPU virtual addresses.
> > > 
> > > So, the question is: what is virt_to_bus() doing, and is it the
> > > right API to
> > > call?
> > 
> > virt_to_bus() is to translate cpu virtual address to pci bus
> > physical
> > address. Basically two levels of translation.
> > 
> > > I see that virt_to_bus() is defined as:
> > > 
> > > e1000.c:
> > > 
> > > #define virt_to_bus(devno, v)???pci_virt_to_mem(devno, (void *)
> > > (v))
> > > 
> > > pci.h:
> > > 
> > > #define pci_virt_to_mem(dev, addr) \
> > > ?????????pci_virt_to_bus((dev), (addr), PCI_REGION_MEM)
> > > 
> > > #define pci_virt_to_bus(dev, addr, flags) \
> > > ?????????pci_hose_phys_to_bus(pci_bus_to_hose(PCI_BUS(dev)), \
> > > ??????????????????????????????(virt_to_phys(addr)), (flags))
> > > 
> > > I know that the RTL8169 driver works on the same board (it's
> > > soldered down
> > > and attached to the other PCIe root port on the SoC). For what
> > > looks like
> > > the same "use case", it seems to call pci_mem_to_phys() which is:
> > > 
> > > pci.h:
> > > 
> > > #define pci_mem_to_phys(dev, addr) \
> > > ?????????pci_bus_to_phys((dev), (addr), PCI_REGION_MEM)
> > > 
> > > #define pci_bus_to_phys(dev, addr, flags) \
> > > ?????????pci_hose_bus_to_phys(pci_bus_to_hose(PCI_BUS(dev)),
> > > (addr), \
> > > ?????????????????????????????????(flags))
> > > 
> > > That's odd, since one of those does a phys -> bus translation and
> > > the other
> > > does a bus -> phys translation. That's the opposite direction, so
> > > both can't
> > > possibly be right. I wonder if those mapping functions are no-ops 
> > > on
> > 
> > Indeed these address translation macros in pci.h are confusing. But
> > most SoC defines 1:1:1 mapping so these are not needed. The reason
> > why
> > it fails on Tegra is probably due to the addresses are not 1:1:1
> > mapped?
> 
> I'm pretty sure everything is 1:1. That's why removing the call to
> any 
> translation macro made the e1000 driver work for Ivan.

Yes, it's just 1:1:1 mapped.

@Ivan: Thank you very much for having reported this. Saves me some
nightmare on bringing up our Apalis TK1 when time will be short next
year couple weeks before the Embedded World show (;-p).

Cheers

Marcel