e1000: fix PCI memory addressing

The Intel E1000 driver was making assumptions about the relationship between some virtual, physical, and PCI addresses.

Also fix some bad usage of the DEBUGOUT macro

---
 drivers/net/e1000.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

index e3c6cea..0f2a5fe 100644
@@ -46,8 +46,7 @@ tested on both gig copper and gig fiber boards
 
 #define TOUT_LOOP   100000
 
-#undef	virt_to_bus
-#define	virt_to_bus(x)	((unsigned long)x)
+#define virt_to_bus(devno, v)	pci_virt_to_mem(devno, (void *) (v))
 #define bus_to_phys(devno, a)	pci_mem_to_phys(devno, a)
 #define mdelay(n)	udelay((n)*1000)
 

@@ -357,7 +356,7 @@ e1000_acquire_eeprom(struct e1000_hw *hw)
 	struct e1000_eeprom_info *eeprom = &hw->eeprom;
 	uint32_t eecd, i = 0;
 
-	DEBUGOUT();
+	DEBUGFUNC();
 
 	if (e1000_swfw_sync_acquire(hw, E1000_SWFW_EEP_SM))
 		return -E1000_ERR_SWFW_SYNC;

@@ -418,7 +417,7 @@ static int32_t e1000_init_eeprom_params(struct e1000_hw *hw)
 	int32_t ret_val = E1000_SUCCESS;
 	uint16_t eeprom_size;
 
-	DEBUGOUT();
+	DEBUGFUNC();
 
 	switch (hw->mac_type) {
 	case e1000_82542_rev2_0:

@@ -2355,7 +2354,7 @@ e1000_copper_link_igp_setup(struct e1000_hw *hw)
 	int32_t ret_val;
 	uint16_t phy_data;
 
-	DEBUGOUT();
+	DEBUGFUNC();
 
 	if (hw->phy_reset_disable)
 		return E1000_SUCCESS;

@@ -5017,7 +5016,7 @@ e1000_transmit(struct eth_device *nic, volatile void *packet, int length)
 	txp = tx_base + tx_tail;
 	tx_tail = (tx_tail + 1) % 8;
 
-	txp->buffer_addr = cpu_to_le64(virt_to_bus(packet));
+	txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, packet));
 	txp->lower.data = cpu_to_le32(hw->txd_cmd | length);
 	txp->upper.data = 0;
 	E1000_WRITE_REG(hw, TDT, tx_tail);

@@ -5145,6 +5144,8 @@ e1000_initialize(bd_t * bis)
 	int idx = 0;
 	u32 PciCommandWord;
 
+	DEBUGFUNC();
+
 	while (1) {		/* Find PCI device(s) */
 		if ((devno = pci_find_devices(supported, idx++)) < 0) {
 			break;

@@ -5170,7 +5171,6 @@ e1000_initialize(bd_t * bis)
 		hw = (struct e1000_hw *) malloc(sizeof (*hw));
 		hw->pdev = devno;
 		nic->priv = hw;
-		nic->iobase = bus_to_phys(devno, iobase);
 
 		sprintf(nic->name, "e1000#%d", card_number);
 

@@ -5180,7 +5180,8 @@ e1000_initialize(bd_t * bis)
 		hw->autoneg_failed = 0;
 		hw->autoneg = 1;
 		hw->get_link_status = TRUE;
-		hw->hw_addr = (typeof(hw->hw_addr)) iobase;
+		hw->hw_addr =
+			pci_map_bar(devno, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
 		hw->mac_type = e1000_undefined;
 
 		/* MAC and Phy settings */

1.6.0.6

FYI, there is no patch 1/2. It's just this one.

Timur Tabi
Linux kernel developer at Freescale

Note: This is related to the 85xx pull request I have for v2009.08. In testing e1000 w/out 36-bit address maps we ran into some issues addressed by this patch.

- k

Ben,

Any update on pulling this in for v2009.08

- k

Timur,

Applied to net repo.

thanks,
Ben

Dear Timur Tabi,

This patch causes compiler warnings:

Configuring for MVBC_P board...
e1000.c: In function 'e1000_transmit':
e1000.c:5019: warning: passing argument 1 of 'virt_to_phys' discards qualifiers from pointer target type
...
Configuring for MPC8536DS board...
e1000.c: In function 'e1000_transmit':
e1000.c:5019: warning: passing argument 1 of 'virt_to_phys' discards qualifiers from pointer target type
...
Configuring for MPC8544DS board...
e1000.c: In function 'e1000_transmit':
e1000.c:5019: warning: passing argument 1 of 'virt_to_phys' discards qualifiers from pointer target type
...

etc.

Please provide a fix!

Also, I tonticed this:

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Very ugly or very beautiful women should be flattered on their
understanding, and mediocre ones on their beauty.
                                       -- Philip Earl of Chesterfield

That's odd. I had those warnings at one point on my system and fixed them before I submitted the patch.

I do mention them:

"Also fix some bad usage of the DEBUGOUT macro"

Unless you're talking about the summary. I figured the changes were harmless and just added them as a freebie. It's not worth submitting a different patch for just these.

Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

Can you please look into this?

Ah, indeed.

Well, it is a code change, and an unrelated one. As such, it belongs into a separate commit.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first thing we do is kill all the lawyers.
(Shakespeare. II Henry VI, Act IV, scene ii)

I can't reproduce this problem.

powerpc-linux-gnu-gcc -g -Os -fPIC -ffixed-r14 -meabi -D__KERNEL__
-DTEXT_BASE=0xFF800000 -I/home/b04825/git/u-boot.e1000/include
-fno-builtin -ffreestanding -nostdinc -isystem /opt/freescale/usr/local/gcc-4.3.74-eglibc-2.8.74-2/powerpc-linux-gnu/bin/../lib/gcc/powerpc-linux-gnu/4.3.2/include -pipe -DCONFIG_PPC -D__powerpc__ -DCONFIG_MPC5xxx -ffixed-r2 -mstring
-mcpu=603e -mmultiple -DTEXT_BASE=0xFF800000
-I/home/b04825/git/u-boot.e1000/board -Wall -Wstrict-prototypes
-fno-stack-protector -o e1000.o e1000.c -c
powerpc-linux-gnu-ar crv libnet.a e1000.o

$ ${CROSS_COMPILE}gcc --version
powerpc-linux-gnu-gcc (Sourcery G++ Lite 4.3-74) 4.3.2

If you look at the definition of virt_to_bus, you'll see that I added a pointer cast specifically to address this warning:

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

If I remove the (void *) cast, I get this:

e1000.c: In function 'e1000_transmit':
e1000.c:5019: warning: passing argument 1 of 'virt_to_phys' discards qualifiers from pointer target type

So somehow, you're compiler is ignoring the "(void *)".

Dear Timur Tabi,

...

I used ELDK 4.2 (gcc-4.2.2).

Well, are you aware how this simple statement gets expanded by the preprocessor?

Out of

txp->buffer_addr = cpu_to_le64(virt_to_bus(hw->pdev, packet));

becomes:

txp->buffer_addr = (__builtin_constant_p((__u64)((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) ? ((__u64)( (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x00000000000000ffULL) << 56) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x000000000000ff00ULL) << 40) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x0000000000ff0000ULL) << 24) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x00000000ff000000ULL) << 8) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >>
16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x000000ff00000000ULL) >> 8) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x0000ff0000000000ULL) >> 24) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0x00ff000000000000ULL) >> 40) | (__u64)(((__u64)(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))) & (__u64)0xff00000000000000ULL) >> 56) )) : __fswab64(((pci_hose_phys_to_bus(pci_bus_to_hose(((((hw->pdev)) >> 16) & 0xff)), (virt_to_phys(((void *) (packet)))), (0x00000000))))));

I take my hat off to you if you manage to understand the consequences and results of all this casting going on here.

Also, I have to admit that I really dislike such casts as they just suppress compiler warnings which are usually valuable - I'd rather see you fixing the original problem (i. e. the type incompatibilities).

I would not go so far to say it is ignoring it. Let's say this cast is insufficient (or simply the wrong approach) to silence the compiler.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Maintain an awareness for contribution --  to  your  schedule,  your
project, our company."                         - A Group of Employees

The cast is there because it doesn't matter if the passed-in pointer is volatile or not, but the compiler complains anyway. Although apparently, it complains even with the cast. I don't know what to do about that, other than to modify struct eth_device so that the 'send' function doesn't take a virtual pointer (which it probably shouldn't anyway).

Well, since I can't reproduce the problem, how can I know if I've fixed it? I just can't see what's wrong with the code. My only suggestion is to do this:

	u64 temp;

	temp = virt_to_bus(hw->pdev, packet);
	txp->buffer_addr = cpu_to_le64(temp);

That should eliminate the inefficient macro expansion, but it won't fix the warning.

Timur Tabi
Linux kernel developer at Freescale