[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Timur Tabi]

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 */
-- 
1.6.0.6

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Timur Tabi]

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

On Mon, Aug 17, 2009 at 3:55 PM, Timur Tabi<timur@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>

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Kumar Gala]

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

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Kumar Gala]

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

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Ben Warren]

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

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Wolfgang Denk]

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

-- 
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

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Timur Tabi]

On Sat, Aug 22, 2009 at 4:12 AM, Wolfgang Denk<wd@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.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Wolfgang Denk]

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

-- 
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)

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Timur Tabi]

Wolfgang Denk wrote:
> 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

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 *)".

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Wolfgang Denk]

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

-- 
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

[U-Boot] [PATCH 2/2] e1000: fix PCI memory addressing

[Timur Tabi]

On Sat, Aug 22, 2009 at 8:17 AM, Wolfgang Denk<wd@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.

-- 
Timur Tabi
Linux kernel developer at Freescale