[U-Boot] [Bug] IXP425 and e1000 network driver
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
-------- --- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\ - writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2)))) -#define E1000_READ_REG_ARRAY(a, reg, offset) ( \ - readl((a)->hw_addr + E1000_##reg + ((offset) << 2))) +#define E1000_WRITE_REG(a, reg, value) \ + (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg))) +#define E1000_READ_REG(a, reg) \ + (le32_to_cpu(readl((a)->hw_addr + E1000_##reg))) +#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \ + (writel(cpu_to_le32(value),\ + ((a)->hw_addr + E1000_##reg + ((offset) << 2)))) +#define E1000_READ_REG_ARRAY(a, reg, offset) \ + (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))) #define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */ ---------
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
Dear Ben,
In message gijdmd$vhk$1@ger.gmane.org Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
--- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
- readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_REG(a, reg, value) \
- (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg)))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
- (writel(cpu_to_le32(value),\
((a)->hw_addr + E1000_##reg + ((offset) << 2))))+#define E1000_READ_REG_ARRAY(a, reg, offset) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2))))
#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
I have never seen any comments on this. Could you please have a look at it?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Ben,
In message gijdmd$vhk$1@ger.gmane.org Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
--- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
- readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_REG(a, reg, value) \
- (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg)))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
- (writel(cpu_to_le32(value),\
((a)->hw_addr + E1000_##reg + ((offset) << 2))))+#define E1000_READ_REG_ARRAY(a, reg, offset) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2))))
#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
I have never seen any comments on this. Could you please have a look at it?
Best regards,
Wolfgang Denk
Sure thing.
regards, Ben
Dear Ben,
In message 49D68311.4090807@gmail.com you wrote:
Wolfgang Denk wrote:
Dear Ben,
In message gijdmd$vhk$1@ger.gmane.org Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
--- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
- readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_REG(a, reg, value) \
- (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg)))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
- (writel(cpu_to_le32(value),\
((a)->hw_addr + E1000_##reg + ((offset) << 2))))+#define E1000_READ_REG_ARRAY(a, reg, offset) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2))))
#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
I have never seen any comments on this. Could you please have a look at it?
Best regards,
Wolfgang Denk
Sure thing.
And what was the result of you having a look at thispatch?
Best regards,
Wolfgang Denk
Dear Ben Warren,
In message 49D68311.4090807@gmail.com you wrote:
Wolfgang Denk wrote:
Dear Ben,
In message gijdmd$vhk$1@ger.gmane.org Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
--- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
- readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_REG(a, reg, value) \
- (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg)))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
- (writel(cpu_to_le32(value),\
((a)->hw_addr + E1000_##reg + ((offset) << 2))))+#define E1000_READ_REG_ARRAY(a, reg, offset) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2))))
#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
I have never seen any comments on this. Could you please have a look at it?
Best regards,
Wolfgang Denk
Sure thing.
That was 4 months ago, but I did not see anything happen. Can you please re-check?
Best regards,
Wolfgang Denk
Hi Wolfgang,
Wolfgang Denk wrote:
Dear Ben Warren,
In message 49D68311.4090807@gmail.com you wrote:
Wolfgang Denk wrote:
Dear Ben,
In message gijdmd$vhk$1@ger.gmane.org Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
--- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -105,12 +105,15 @@ static void e1000_phy_hw_reset(struct e1000_hw *hw); static int e1000_phy_reset(struct e1000_hw *hw); static int e1000_detect_gig_phy(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg))) -#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg)) -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
- readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_REG(a, reg, value) \
- (writel(cpu_to_le32(value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg)))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
- (writel(cpu_to_le32(value),\
((a)->hw_addr + E1000_##reg + ((offset) << 2))))+#define E1000_READ_REG_ARRAY(a, reg, offset) \
- (le32_to_cpu(readl((a)->hw_addr + E1000_##reg + ((offset) << 2))))
#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
#ifndef CONFIG_AP1000 /* remove for warnings */
However, I'm not sure it this is the correct fix.
Is readl supposed to read raw data?
Is le32_to_cpu/cpu_to_le32 a function or a macro? In the later case the code is not save or slow due to multiple argument expansion.
-- Stefan
I have never seen any comments on this. Could you please have a look at it?
Best regards,
Wolfgang Denk
Sure thing.
That was 4 months ago, but I did not see anything happen. Can you please re-check?
Best regards,
Wolfgang Denk
I thought I brought this up already, but maybe not. Won't this break PowerPC? I'm pretty sure (value) != (cpu_to_le32(value)), isn't it? Isn't the problem that writel() and readl() aren't byte-swapped on BE ARM?
regards, Ben
Dear Ben,
In message 4A7FB519.6040000@gmail.com you wrote:
I thought I brought this up already, but maybe not. Won't this break PowerPC? I'm pretty sure (value) != (cpu_to_le32(value)), isn't it?
Right.
Isn't the problem that writel() and readl() aren't byte-swapped on BE ARM?
Right. writel() and readl() are not a good choice for architecture independent code. AFAIK ioreadX(), iowriteX() are supposed to be architecture independent I/O accessors, but unfortunately not all architectures implement these yet.
Best regards,
Wolfgang Denk
On Sat, May 2, 2009 at 4:56 AM, Jean-Christophe PLAGNIOL-VILLARD < plagnioj@jcrosoft.com> wrote:
On 19:31 Sat 20 Dec , Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
Ben Ping
Best Regards, J.
Sorry, I let another month slip by. This looks reasonable to me, but isn't a properly-formatted patch.
Stefan - if you don't have time to re-submit, just let me know and I'll do it for you.
regards, Ben
Dear Ben,
In message f8328f7c0905021956x56c17a13ocee92bceb353d5b6@mail.gmail.com you wrote:
On 19:31 Sat 20 Dec , Stefan Althoefer wrote:
Hi,
I found that IXP425 (big endian ARM) did not work with e1000 network driver. The reason is broken access to controller registers.
I get it working with this patch:
Ben Ping
Best Regards, J.
Sorry, I let another month slip by. This looks reasonable to me, but isn't a properly-formatted patch.
Stefan - if you don't have time to re-submit, just let me know and I'll do it for you.
Seems Stefan has no time. Could you please take care of this, then? Thanks.
Best regards,
Wolfgang Denk
participants (4)
-
Ben Warren -
Jean-Christophe PLAGNIOL-VILLARD -
Stefan Althoefer -
Wolfgang Denk