[U-Boot] [Bug] IXP425 and e1000 network driver

[U-Boot] [Bug] IXP425 and e1000 network driver

[Stefan Althoefer]

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

[U-Boot] [Bug] IXP425 and e1000 network driver

[Wolfgang Denk]

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

-- 
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
Unix is like a toll road on which you have to stop every 50  feet  to
pay another nickel. But hey! You only feel 5 cents poorer each time.
                 - Larry Wall in <1992Aug13.192357.15731@netlabs.com>

[U-Boot] [Bug] IXP425 and e1000 network driver

[Ben Warren]

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

[U-Boot] [Bug] IXP425 and e1000 network driver

[Wolfgang Denk]

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

-- 
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
In the beginning there was nothing.
And the Lord said "Let There Be Light!"
And still there was nothing, but at least now you could see it.

[U-Boot] [Bug] IXP425 and e1000 network driver

[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

-- 
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
Niklaus Wirth has lamented that, whereas Europeans pronounce his name
correctly  (Ni-klows  Virt),  Americans  invariably  mangle  it  into
(Nick-les  Worth).  Which  is to say that Europeans call him by name,
but Americans call him by value.

[U-Boot] [Bug] IXP425 and e1000 network driver

[Ben Warren]

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

[U-Boot] [Bug] IXP425 and e1000 network driver

[Wolfgang Denk]

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

-- 
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
Computers make excellent and efficient servants, but I have  no  wish
to  serve under them. Captain, a starship also runs on loyalty to one
man. And nothing can replace it or him.
	-- Spock, "The Ultimate Computer", stardate 4729.4

[U-Boot] [Bug] IXP425 and e1000 network driver

[Jean-Christophe PLAGNIOL-VILLARD]

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.

[U-Boot] [Bug] IXP425 and e1000 network driver

[Ben Warren]

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

[U-Boot] [Bug] IXP425 and e1000 network driver

[Wolfgang Denk]

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

-- 
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
A direct quote from the Boss: "We passed over a lot of good people to
get the ones we hired."