Patchwork [U-Boot,NET,2/2] Net: clarify board/cpu_eth_init calls

login
register
mail settings
Submitter Ben Warren
Date Sept. 1, 2010, 6:05 a.m.
Message ID <1283321104-953-2-git-send-email-biggerbadderben@gmail.com>
Download mbox | patch
Permalink /patch/71837/
State Accepted
Commit 8ad25bf8d9233eb7d0b614612108622a59069354
Headers show

Comments

Ben Warren - Sept. 1, 2010, 6:05 a.m.
This has always been confusing, and the idea of these functions returning the
number of interfaces initialized was half-baked and ultimately pointless.
Instead, act more like regular functions and return < 0 on failure, >= 0 on
success.

This change shouldn't break anything.

Signed-off-by: Ben Warren <biggerbadderben@gmail.com>
---
 net/eth.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)
Mike Frysinger - Sept. 1, 2010, 5:04 p.m.
On Wednesday, September 01, 2010 02:05:04 Ben Warren wrote:
> +	if (board_eth_init != __def_eth_init) {
> +	} else if (cpu_eth_init != __def_eth_init) {

i'm not sure these changes are useful.  the resolution of the symbols happens 
at link time, so it isnt like gcc will be able to optimize away the default.

if anything, it'd make more sense to declare the functions as external/weak, 
and then check that the pointer is not NULL.  that'd save on the overhead of 
having uncalled stub functions that merely return 0 in the final linked image.
-mike
Ben Warren - Sept. 1, 2010, 7:13 p.m.
Hi Mike,

On 9/1/2010 10:04 AM, Mike Frysinger wrote:
> On Wednesday, September 01, 2010 02:05:04 Ben Warren wrote:
>> +	if (board_eth_init != __def_eth_init) {
>> +	} else if (cpu_eth_init != __def_eth_init) {
> i'm not sure these changes are useful.  the resolution of the symbols happens
> at link time, so it isnt like gcc will be able to optimize away the default.
>
> if anything, it'd make more sense to declare the functions as external/weak,
> and then check that the pointer is not NULL.  that'd save on the overhead of
> having uncalled stub functions that merely return 0 in the final linked image.
> -mike
This did work as I hoped on my PPC eval board, but maybe not globally.  
I remember that initially, with the functions defined as weak but with 
no body, check for NULL didn't work.  I've never tried declaring them as 
external, though.

thanks,
Ben

Patch

diff --git a/net/eth.c b/net/eth.c
index 5c70d4f..6082c90 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -204,10 +204,18 @@  int eth_initialize(bd_t *bis)
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
 	miiphy_init();
 #endif
-	/* Try board-specific initialization first.  If it fails or isn't
-	 * present, try the cpu-specific initialization */
-	if (board_eth_init(bis) < 0)
-		cpu_eth_init(bis);
+	/*
+	 * If board-specific initialization exists, call it.
+	 * If not, call a CPU-specific one
+	 */
+	if (board_eth_init != __def_eth_init) {
+		if (board_eth_init(bis) < 0)
+			printf("Board Net Initialization Failed\n");
+	} else if (cpu_eth_init != __def_eth_init) {
+		if (cpu_eth_init(bis) < 0)
+			printf("CPU Net Initialization Failed\n");
+	} else
+		printf("Net Initialization Skipped\n");
 
 #if defined(CONFIG_DB64360) || defined(CONFIG_CPCI750)
 	mv6436x_eth_initialize(bis);