[U-Boot] [PATCH 2/2] 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
Signed-off-by: Timur Tabi timur@freescale.com --- drivers/net/e1000.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c index e3c6cea..0f2a5fe 100644 --- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -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 */
FYI, there is no patch 1/2. It's just this one.
On Mon, Aug 17, 2009 at 3:55 PM, Timur Tabitimur@freescale.com wrote:
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
Signed-off-by: Timur Tabi timur@freescale.com
On Aug 17, 2009, at 3:55 PM, Timur Tabi wrote:
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
Signed-off-by: Timur Tabi timur@freescale.com
drivers/net/e1000.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
Acked-by: Kumar Gala galak@kernel.crashing.org
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
On Aug 17, 2009, at 6:25 PM, Kumar Gala wrote:
On Aug 17, 2009, at 3:55 PM, Timur Tabi wrote:
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
Signed-off-by: Timur Tabi timur@freescale.com
drivers/net/e1000.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
Acked-by: Kumar Gala galak@kernel.crashing.org
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.
Ben,
Any update on pulling this in for v2009.08
- k
Timur,
Timur Tabi wrote:
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
Signed-off-by: Timur Tabi timur@freescale.com
drivers/net/e1000.c | 17 +++++++++--------
Applied to net repo.
thanks, Ben
Dear Timur Tabi,
In message 1250542538-5717-1-git-send-email-timur@freescale.com you wrote:
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
Signed-off-by: Timur Tabi timur@freescale.com
drivers/net/e1000.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
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:
@@ -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();
...
These changes are unrelated to the change and not even mentioned in the commit message. This is a bad thing to do. Please make sure NOT to do this again. Please split this into two orthogonal patches next time. Thanks.
Best regards,
Wolfgang Denk
On Sat, Aug 22, 2009 at 4:12 AM, Wolfgang Denkwd@denx.de wrote:
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
That's odd. I had those warnings at one point on my system and fixed them before I submitted the patch.
- DEBUGOUT();
- DEBUGFUNC();
...
These changes are unrelated to the change and not even mentioned in the commit message.
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.
Dear Timur Tabi,
In message ed82fe3e0908220440u26990f80x9db39de082996868@mail.gmail.com you wrote:
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
That's odd. I had those warnings at one point on my system and fixed them before I submitted the patch.
Can you please look into this?
- DEBUGOUT();
- DEBUGFUNC();
...
These changes are unrelated to the change and not even mentioned in the commit message.
I do mention them:
"Also fix some bad usage of the DEBUGOUT macro"
Ah, indeed.
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.
Well, it is a code change, and an unrelated one. As such, it belongs into a separate commit.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Timur Tabi,
In message1250542538-5717-1-git-send-email-timur@freescale.com you wrote:
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
Signed-off-by: Timur Tabitimur@freescale.com
drivers/net/e1000.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
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
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,
In message 4A8FEA69.9080408@freescale.com you wrote:
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
I can't reproduce this problem.
...
$ ${CROSS_COMPILE}gcc --version powerpc-linux-gnu-gcc (Sourcery G++ Lite 4.3-74) 4.3.2
I used ELDK 4.2 (gcc-4.2.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:
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.
#define virt_to_bus(devno, v) pci_virt_to_mem(devno, (void *) (v))
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).
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 *)".
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
On Sat, Aug 22, 2009 at 8:17 AM, Wolfgang Denkwd@denx.de wrote:
#define virt_to_bus(devno, v) pci_virt_to_mem(devno, (void *) (v))
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).
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).
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.
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.
participants (4)
-
Ben Warren -
Kumar Gala -
Timur Tabi -
Wolfgang Denk