From mboxrd@z Thu Jan  1 00:00:00 1970
From: Amit Virdi <amit.virdi@st.com>
Date: Mon, 26 Mar 2012 17:11:09 +0530
Subject: [U-Boot] [PATCH 04/25] SPEAr: Configure network support for
	spear SoCs
In-Reply-To: <201203071429.34080.sr@denx.de>
References: <1331121854-20494-1-git-send-email-amit.virdi@st.com>
	<1331121854-20494-5-git-send-email-amit.virdi@st.com>
	<201203071429.34080.sr@denx.de>
Message-ID: <4F7055D5.2070505@st.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

Hello Stefan,

On 3/7/2012 6:59 PM, Stefan Roese wrote:
> On Wednesday 07 March 2012 13:03:53 Amit Virdi wrote:
>> From: Vipin KUMAR<vipin.kumar@st.com>
>
> Please find some comments below.
>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> Signed-off-by: Amit Virdi<amit.virdi@st.com>
>> diff --git a/arch/arm/include/asm/arch-spear/hardware.h
>> b/arch/arm/include/asm/arch-spear/hardware.h index a6517b2..2b9cb0e 100644
>> --- a/arch/arm/include/asm/arch-spear/hardware.h
>> +++ b/arch/arm/include/asm/arch-spear/hardware.h
>> @@ -31,6 +31,7 @@
>>   #define CONFIG_SPEAR_SYSCNTLBASE		(0xFCA00000)
>>   #define CONFIG_SPEAR_TIMERBASE			(0xFC800000)
>>   #define CONFIG_SPEAR_MISCBASE			(0xFCA80000)
>> +#define CONFIG_SPEAR_ETHBASE			(0xE0800000)
>
> I would prefer if you removed these unneeded parentheses here:
>
> #define CONFIG_SPEAR_ETHBASE			0xE0800000
>
> Perhaps best done by doing a cosmetic cleanup patch before the newly added
> defines. I know that checkpatch doesn't complain about this, but these
> parentheses really distract me. Not sure how other feel about it.
>

ok

>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +#if defined(CONFIG_DESIGNWARE_ETH)
>> +	return designware_initialize(0, CONFIG_SPEAR_ETHBASE, CONFIG_DW0_PHY);
>> +#else
>> +	return -1;
>> +#endif
>> +}
>
> This routine is added multiple times in this patch. Perhaps this can be moved
> to a "common/" file instead?
>

[...]

>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +#if defined(CONFIG_DESIGNWARE_ETH)
>> +	return designware_initialize(0, CONFIG_SPEAR_ETHBASE, CONFIG_DW0_PHY);
>> +#else
>> +	return -1;
>> +#endif
>> +}
>
> Here again.
>

[...]

>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +#if defined(CONFIG_DESIGNWARE_ETH)
>> +	return designware_initialize(0, CONFIG_SPEAR_ETHBASE, CONFIG_DW0_PHY);
>> +#else
>> +	return -1;
>> +#endif
>> +}
>
> And again.
>

[...]

>> diff --git a/board/spear/spear600/spear600.c
>> b/board/spear/spear600/spear600.c index 7cf63d6..5a32b7f 100644
>> --- a/board/spear/spear600/spear600.c
>> +++ b/board/spear/spear600/spear600.c
>> @@ -22,6 +22,7 @@
>>    */
>>
>>   #include<common.h>
>> +#include<netdev.h>
>>   #include<nand.h>
>>   #include<asm/io.h>
>>   #include<linux/mtd/fsmc_nand.h>
>> @@ -52,3 +53,12 @@ int board_nand_init(struct nand_chip *nand)
>>   #endif
>>   	return -1;
>>   }
>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +#if defined(CONFIG_DESIGNWARE_ETH)
>> +	return designware_initialize(0, CONFIG_SPEAR_ETHBASE, CONFIG_DW0_PHY);
>> +#else
>> +	return -1;
>> +#endif
>> +}
>
> and again.
>

Yes Stefan, it is planned. There's a lot of cleanup that is needed in 
the SPEAr platform code. I shall be sending another patchset after this 
patchset that adds new functionality and does the cleanup.

Can you accept this patch for the time being?

>>
>> +/* Ethernet driver configuration */
>> +#define CONFIG_MII
>> +#define CONFIG_DESIGNWARE_ETH
>> +#define CONFIG_DW_SEARCH_PHY
>> +#define CONFIG_DW0_PHY				1
>> +#define CONFIG_NET_MULTI
>> +#define CONFIG_PHY_RESET_DELAY			(10000)		/*
> in usec */
>
> Again, please remove these parentheses.
>

Sure!


Thanks
Amit Virdi