From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Warren <biggerbadderben@gmail.com>
Date: Sun, 09 Aug 2009 22:50:17 -0700
Subject: [U-Boot] [Bug] IXP425 and e1000 network driver
In-Reply-To: <20090805202610.D338C832E416@gemini.denx.de>
References: <gijdmd$vhk$1@ger.gmane.org>
	<20090403211819.5637F83797DC@gemini.denx.de>
	<49D68311.4090807@gmail.com>
	<20090805202610.D338C832E416@gemini.denx.de>
Message-ID: <4A7FB519.6040000@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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