[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Heiko Schocher]

Check the presence of the PIGGY on the keymile boards mgcoge,
mgsuvd and kmeter1. If the PIGGY is not present, dont register
this Ethernet device.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 board/keymile/common/common.c   |   16 ++++++++++++++--
 board/keymile/kmeter1/Makefile  |    6 ++++--
 board/keymile/kmeter1/kmeter1.c |    7 ++++++-
 board/keymile/mgcoge/mgcoge.c   |    9 +++++++--
 board/keymile/mgsuvd/mgsuvd.c   |    8 +++++++-
 cpu/mpc8260/ether_scc.c         |    8 ++++++++
 cpu/mpc8xx/scc.c                |    6 ++++++
 drivers/qe/uec.c                |    7 +++++++
 include/configs/kmeter1.h       |   10 ++++++++++
 include/configs/mgcoge.h        |   11 +++++++++++
 include/configs/mgsuvd.h        |   10 ++++++++++
 include/net.h                   |    4 +++-
 12 files changed, 93 insertions(+), 9 deletions(-)

diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c
index a4cf24c..54c8730 100644
--- a/board/keymile/common/common.c
+++ b/board/keymile/common/common.c
@@ -22,10 +22,14 @@
  */

 #include <common.h>
+#if defined(CONFIG_MGCOGE)
 #include <mpc8260.h>
+#endif
 #include <ioports.h>
 #include <malloc.h>
 #include <hush.h>
+#include <net.h>
+#include <asm/io.h>

 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
 #include <libfdt.h>
@@ -33,8 +37,6 @@

 #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
 #include <i2c.h>
-#endif
-#include <asm/io.h>

 extern int i2c_soft_read_pin (void);

@@ -495,6 +497,7 @@ void i2c_init_board(void)
 #endif
 }
 #endif
+#endif

 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
 int fdt_set_node_and_value (void *blob,
@@ -521,3 +524,12 @@ int fdt_set_node_and_value (void *blob,
 	return ret;
 }
 #endif
+
+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+int ethernet_present (int index)
+{
+	int	ret;
+	ret = in_8((u8 *)CONFIG_SYS_PIGGY_BASE + CONFIG_SYS_SLOT_ID_OFF) & 0x80;
+	return ret;
+}
+#endif
diff --git a/board/keymile/kmeter1/Makefile b/board/keymile/kmeter1/Makefile
index 88b79f3..12a1518 100644
--- a/board/keymile/kmeter1/Makefile
+++ b/board/keymile/kmeter1/Makefile
@@ -22,12 +22,14 @@
 #

 include $(TOPDIR)/config.mk
+ifneq ($(OBJTREE),$(SRCTREE))
+$(shell mkdir -p $(obj)../common)
+endif

 LIB	= $(obj)lib$(BOARD).a

-COBJS-y += $(BOARD).o
+COBJS	+= $(BOARD).o ../common/common.o

-COBJS	:= $(COBJS-y)
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS))
 SOBJS	:= $(addprefix $(obj),$(SOBJS))
diff --git a/board/keymile/kmeter1/kmeter1.c b/board/keymile/kmeter1/kmeter1.c
index f9a59a6..03f5dda 100644
--- a/board/keymile/kmeter1/kmeter1.c
+++ b/board/keymile/kmeter1/kmeter1.c
@@ -141,7 +141,12 @@ phys_size_t initdram (int board_type)

 int checkboard (void)
 {
-	puts ("Board: Keymile kmeter1\n");
+	puts ("Board: Keymile kmeter1");
+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (0))
+		puts (" with PIGGY.");
+#endif
+	puts ("\n");
 	return 0;
 }

diff --git a/board/keymile/mgcoge/mgcoge.c b/board/keymile/mgcoge/mgcoge.c
index 3683417..98a4d43 100644
--- a/board/keymile/mgcoge/mgcoge.c
+++ b/board/keymile/mgcoge/mgcoge.c
@@ -25,6 +25,7 @@
 #include <mpc8260.h>
 #include <ioports.h>
 #include <malloc.h>
+#include <net.h>
 #include <asm/io.h>

 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
@@ -285,8 +286,12 @@ phys_size_t initdram (int board_type)

 int checkboard(void)
 {
-	puts ("Board: mgcoge\n");
-
+	puts ("Board: Keymile mgcoge");
+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (0))
+		puts (" with PIGGY.");
+#endif
+	puts ("\n");
 	return 0;
 }

diff --git a/board/keymile/mgsuvd/mgsuvd.c b/board/keymile/mgsuvd/mgsuvd.c
index 3726acf..eb4c285 100644
--- a/board/keymile/mgsuvd/mgsuvd.c
+++ b/board/keymile/mgsuvd/mgsuvd.c
@@ -22,6 +22,7 @@
  */
 #include <common.h>
 #include <mpc8xx.h>
+#include <net.h>
 #include <asm/io.h>

 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
@@ -60,7 +61,12 @@ const uint sdram_table[] =

 int checkboard (void)
 {
-	puts ("Board: Keymile mgsuvd\n");
+	puts ("Board: Keymile mgsuvd");
+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (0))
+		puts (" with PIGGY.");
+#endif
+	puts ("\n");
 	return (0);
 }

diff --git a/cpu/mpc8260/ether_scc.c b/cpu/mpc8260/ether_scc.c
index c65f0e0..a79d56c 100644
--- a/cpu/mpc8260/ether_scc.c
+++ b/cpu/mpc8260/ether_scc.c
@@ -191,6 +191,14 @@ int eth_init(bd_t *bis)
     scc_enet_t *pram_ptr;
     uint dpaddr;

+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (CONFIG_ETHER_INDEX) == 0) {
+		printf("Ethernet index: %d not present.\n",
+		       CONFIG_ETHER_INDEX);
+		return -1;
+	}
+#endif
+
     rxIdx = 0;
     txIdx = 0;

diff --git a/cpu/mpc8xx/scc.c b/cpu/mpc8xx/scc.c
index effb967..4be4d89 100644
--- a/cpu/mpc8xx/scc.c
+++ b/cpu/mpc8xx/scc.c
@@ -74,6 +74,12 @@ int scc_initialize(bd_t *bis)
 {
 	struct eth_device* dev;

+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (0) == 0) {
+		printf("Ethernet not present.\n");
+		return -1;
+	}
+#endif
 	dev = (struct eth_device*) malloc(sizeof *dev);
 	memset(dev, 0, sizeof *dev);

diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
index ed7ed65..8ae6d0f 100644
--- a/drivers/qe/uec.c
+++ b/drivers/qe/uec.c
@@ -1385,6 +1385,13 @@ int uec_initialize(int index)
 	uec_info_t		*uec_info;
 	int			err;

+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+	if (ethernet_present (index) == 0) {
+		printf("Ethernet index: %d not present.\n",
+		       index);
+		return -1;
+	}
+#endif
 	dev = (struct eth_device *)malloc(sizeof(struct eth_device));
 	if (!dev)
 		return 0;
diff --git a/include/configs/kmeter1.h b/include/configs/kmeter1.h
index d0fe6a3..e443086 100644
--- a/include/configs/kmeter1.h
+++ b/include/configs/kmeter1.h
@@ -314,6 +314,16 @@
 #define CONFIG_SYS_LOADS_BAUD_CHANGE	1	/* allow baudrate change */

 /*
+ * How to get access to the slot ID.  Put this here to make it easy
+ * to modify in a centralized location.  This is used in the HDLC
+ * driver to set the MAC.
+*/
+#define CONFIG_CHECK_ETHERNET_PRESENT	1
+#define CONFIG_SYS_SLOT_ID_BASE		CONFIG_SYS_PIGGY_BASE
+#define CONFIG_SYS_SLOT_ID_OFF		(0x07)	/* register offset */
+#define CONFIG_SYS_SLOT_ID_MASK		(0x3f)	/* mask for slot ID bits */
+
+/*
  * BOOTP options
  */
 #define CONFIG_BOOTP_BOOTFILESIZE
diff --git a/include/configs/mgcoge.h b/include/configs/mgcoge.h
index dc9b311..bfac3b0 100644
--- a/include/configs/mgcoge.h
+++ b/include/configs/mgcoge.h
@@ -400,4 +400,15 @@
 #define OF_TBCLK		(bd->bi_busfreq / 4)
 #define OF_STDOUT_PATH		"/soc/cpm/serial at 11a90"

+/*
+ * How to get access to the slot ID.  Put this here to make it easy
+ * to modify in a centralized location.  This is used in the HDLC
+ * driver to set the MAC.
+*/
+#define CONFIG_CHECK_ETHERNET_PRESENT	1
+#define CONFIG_SYS_SLOT_ID_BASE		CONFIG_SYS_PIGGY_BASE
+#define CONFIG_SYS_SLOT_ID_OFF		(0x07)	/* register offset */
+#define CONFIG_SYS_SLOT_ID_MASK		(0x3f)	/* mask for slot ID bits */
+
+
 #endif /* __CONFIG_H */
diff --git a/include/configs/mgsuvd.h b/include/configs/mgsuvd.h
index fca2e55..0bd2f40 100644
--- a/include/configs/mgsuvd.h
+++ b/include/configs/mgsuvd.h
@@ -211,6 +211,16 @@
 #define CONFIG_ENV_OFFSET_REDUND	(CONFIG_ENV_OFFSET+CONFIG_ENV_SECT_SIZE)
 #define CONFIG_ENV_SIZE_REDUND	(CONFIG_ENV_SIZE)

+/*
+ * How to get access to the slot ID.  Put this here to make it easy
+ * to modify in a centralized location.  This is used in the HDLC
+ * driver to set the MAC.
+*/
+#define CONFIG_CHECK_ETHERNET_PRESENT	1
+#define CONFIG_SYS_SLOT_ID_BASE		CONFIG_SYS_PIGGY_BASE
+#define CONFIG_SYS_SLOT_ID_OFF		(0x07)	/* register offset */
+#define CONFIG_SYS_SLOT_ID_MASK		(0x3f)	/* mask for slot ID bits */
+
 /*-----------------------------------------------------------------------
  * Cache Configuration
  */
diff --git a/include/net.h b/include/net.h
index a5a256b..112dd7b 100644
--- a/include/net.h
+++ b/include/net.h
@@ -489,6 +489,8 @@ extern ushort getenv_VLAN(char *);
 /* copy a filename (allow for "..." notation, limit length) */
 extern void	copy_filename (char *dst, char *src, int size);

-/**********************************************************************/
+#if defined(CONFIG_CHECK_ETHERNET_PRESENT)
+extern int ethernet_present (int index);
+#endif

 #endif /* __NET_H__ */
-- 
1.5.6.1

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Ben Warren]

Hi Heiko,

Heiko Schocher wrote:
> Check the presence of the PIGGY on the keymile boards mgcoge,
> mgsuvd and kmeter1. If the PIGGY is not present, dont register
> this Ethernet device.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
This looks like useful stuff to have, but I'd prefer that you put the 
check logic in board_eth_init() rather than adding to the individual 
device drivers.  I know the 8260 SCC driver is the older style, which 
precludes the use of board_eth_init, but I'll convert it if you're able 
to test.

regards,
Ben

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Heiko Schocher]

Hello Ben

Ben Warren wrote:
> Heiko Schocher wrote:
>   
>> Check the presence of the PIGGY on the keymile boards mgcoge,
>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
>> this Ethernet device.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>     
> This looks like useful stuff to have, but I'd prefer that you put the 
> check logic in board_eth_init() rather than adding to the individual 
> device drivers.  I know the 8260 SCC driver is the older style, which 
> precludes the use of board_eth_init, but I'll convert it if you're able 
> to test.
>   

Yes, I could test such a change for you, but hmm... I am not sure, if
board_eth_init () is the right place for my purpose.
I need for every Ethernet device a selection, if this device is present or
not.
Correct me if I am wrong, but it looks like board_eth_init ()
is not made for this purpose. (Ok, I can do a specific device init
in board_eth_init (), but then we must do something, that prevents
that the device is again initialized in eth_initialize () ...

Hmm... while writing this it comes a idea in my mind:
we could move all the *_initialize functions in eth_initialize () in a
seperate function, say eth_hardware_init() and maybe making this
function "weak", so a board writer can write his own
eth_hardware_init() ... in such a function, I could check which
device is present, and only initialize the present devices ...
what do you think?

bye
Heiko

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Ben Warren]

Heiko Schocher wrote:
> Hello Ben
>
> Ben Warren wrote:
>   
>> Heiko Schocher wrote:
>>   
>>     
>>> Check the presence of the PIGGY on the keymile boards mgcoge,
>>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
>>> this Ethernet device.
>>>
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>> ---
>>>     
>>>       
>> This looks like useful stuff to have, but I'd prefer that you put the 
>> check logic in board_eth_init() rather than adding to the individual 
>> device drivers.  I know the 8260 SCC driver is the older style, which 
>> precludes the use of board_eth_init, but I'll convert it if you're able 
>> to test.
>>   
>>     
>
> Yes, I could test such a change for you, but hmm... I am not sure, if
> board_eth_init () is the right place for my purpose.
> I need for every Ethernet device a selection, if this device is present or
> not.
> Correct me if I am wrong, but it looks like board_eth_init ()
> is not made for this purpose. (Ok, I can do a specific device init
> in board_eth_init (), but then we must do something, that prevents
> that the device is again initialized in eth_initialize () ...
>
>   
board_eth_init() was introduced for exactly this sort of thing.  Have a 
look at the net repo (I've sent a pull request to Wolfgang so the 
current changes will make it into the 12.2008 release).  There aren't 
any device initializations left in eth_initialize(), so there's no issue 
of a device being initialized twice.  The goal is for all devices to be 
started by cpu_eth_int() or board_eth_init().
> Hmm... while writing this it comes a idea in my mind:
> we could move all the *_initialize functions in eth_initialize () in a
> seperate function, say eth_hardware_init() and maybe making this
> function "weak", so a board writer can write his own
> eth_hardware_init() ... in such a function, I could check which
> device is present, and only initialize the present devices ...
> what do you think?
>   
That's what board_eth_init() and cpu_eth_init() are for.  In addition, I 
forgot to mention that a couple of days ago Gary Jennejohn submitted a 
patch for changing the 82xx SCC driver over to CONFIG_NET_MULTI style.

regards,
Ben

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Heiko Schocher]

Hello Ben,

Ben Warren wrote:
> Heiko Schocher wrote:
>   
>> Hello Ben
>>
>> Ben Warren wrote:
>>   
>>     
>>> Heiko Schocher wrote:
>>>   
>>>     
>>>       
>>>> Check the presence of the PIGGY on the keymile boards mgcoge,
>>>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
>>>> this Ethernet device.
>>>>
>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>> ---
>>>>     
>>>>       
>>>>         
>>> This looks like useful stuff to have, but I'd prefer that you put the 
>>> check logic in board_eth_init() rather than adding to the individual 
>>> device drivers.  I know the 8260 SCC driver is the older style, which 
>>> precludes the use of board_eth_init, but I'll convert it if you're able 
>>> to test.
>>>   
>>>     
>>>       
>> Yes, I could test such a change for you, but hmm... I am not sure, if
>> board_eth_init () is the right place for my purpose.
>> I need for every Ethernet device a selection, if this device is present or
>> not.
>> Correct me if I am wrong, but it looks like board_eth_init ()
>> is not made for this purpose. (Ok, I can do a specific device init
>> in board_eth_init (), but then we must do something, that prevents
>> that the device is again initialized in eth_initialize () ...
>>
>>   
>>     
> board_eth_init() was introduced for exactly this sort of thing.  Have a 
> look at the net repo (I've sent a pull request to Wolfgang so the 
> current changes will make it into the 12.2008 release).  There aren't 
> any device initializations left in eth_initialize(), so there's no issue 
> of a device being initialized twice.  The goal is for all devices to be 
> started by cpu_eth_int() or board_eth_init().
>   

Ahh... now I see it. I adjust my patch, thanks.

bye
Heiko

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Gary Jennejohn]

On Thu, 13 Nov 2008 09:30:51 -0800
Ben Warren <biggerbadderben@gmail.com> wrote:

> Heiko Schocher wrote:
> > Hello Ben
> >
> > Ben Warren wrote:
> >   
> >> Heiko Schocher wrote:
> >>   
> >>     
> >>> Check the presence of the PIGGY on the keymile boards mgcoge,
> >>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
> >>> this Ethernet device.
> >>>
> >>> Signed-off-by: Heiko Schocher <hs@denx.de>
> >>> ---
> >>>     
> >>>       
> >> This looks like useful stuff to have, but I'd prefer that you put the 
> >> check logic in board_eth_init() rather than adding to the individual 
> >> device drivers.  I know the 8260 SCC driver is the older style, which 
> >> precludes the use of board_eth_init, but I'll convert it if you're able 
> >> to test.
> >>   

Unfortunately, this approach won't work.  First of all, the 82xx SCC
driver is now initialized in cpu_eth_init(), which knows nothing about
board-specific peculiarities like the PIGGY.  Secondly, the HDLC
driver for Keymile has to be initialized in board_eth_init(), and it
has nothing to do with the PIGGY.  Putting the check in board_eth_init()
would break it completely.  I looked at Heiko's latest patch and couldn't
figure out a way to cleanly differentiate between initializing the HDLC
driver and checking whether the PIGGY was present fo the other ENET drivers.

> >>     
> >
> > Yes, I could test such a change for you, but hmm... I am not sure, if
> > board_eth_init () is the right place for my purpose.
> > I need for every Ethernet device a selection, if this device is present or
> > not.
> > Correct me if I am wrong, but it looks like board_eth_init ()
> > is not made for this purpose. (Ok, I can do a specific device init
> > in board_eth_init (), but then we must do something, that prevents
> > that the device is again initialized in eth_initialize () ...
> >
> >   
> board_eth_init() was introduced for exactly this sort of thing.  Have a 
> look at the net repo (I've sent a pull request to Wolfgang so the 
> current changes will make it into the 12.2008 release).  There aren't 
> any device initializations left in eth_initialize(), so there's no issue 
> of a device being initialized twice.  The goal is for all devices to be 
> started by cpu_eth_int() or board_eth_init().
> > Hmm... while writing this it comes a idea in my mind:
> > we could move all the *_initialize functions in eth_initialize () in a
> > seperate function, say eth_hardware_init() and maybe making this
> > function "weak", so a board writer can write his own
> > eth_hardware_init() ... in such a function, I could check which
> > device is present, and only initialize the present devices ...
> > what do you think?
> >   
> That's what board_eth_init() and cpu_eth_init() are for.  In addition, I 
> forgot to mention that a couple of days ago Gary Jennejohn submitted a 
> patch for changing the 82xx SCC driver over to CONFIG_NET_MULTI style.
> 

Correct, and it must explicitly call ethernet_present() - see above.

---
Gary Jennejohn
*********************************************************************
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
*********************************************************************

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Heiko Schocher]

Hello Gray,

Gary Jennejohn schrieb:
> On Thu, 13 Nov 2008 09:30:51 -0800
> Ben Warren <biggerbadderben@gmail.com> wrote:
> 
>> Heiko Schocher wrote:
>>> Hello Ben
>>>
>>> Ben Warren wrote:
>>>   
>>>> Heiko Schocher wrote:
>>>>   
>>>>     
>>>>> Check the presence of the PIGGY on the keymile boards mgcoge,
>>>>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
>>>>> this Ethernet device.
>>>>>
>>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>>> ---
>>>>>     
>>>>>       
>>>> This looks like useful stuff to have, but I'd prefer that you put the 
>>>> check logic in board_eth_init() rather than adding to the individual 
>>>> device drivers.  I know the 8260 SCC driver is the older style, which 
>>>> precludes the use of board_eth_init, but I'll convert it if you're able 
>>>> to test.
>>>>   
> 
> Unfortunately, this approach won't work.  First of all, the 82xx SCC
> driver is now initialized in cpu_eth_init(), which knows nothing about
> board-specific peculiarities like the PIGGY.  Secondly, the HDLC
> driver for Keymile has to be initialized in board_eth_init(), and it
> has nothing to do with the PIGGY.  Putting the check in board_eth_init()
> would break it completely.  I looked at Heiko's latest patch and couldn't
> figure out a way to cleanly differentiate between initializing the HDLC
> driver and checking whether the PIGGY was present fo the other ENET drivers.

Hmm.. you can now initialize in board_eth_init () in board/keymile/common/common.c
your HDLC specific things, or?

[...]
>> That's what board_eth_init() and cpu_eth_init() are for.  In addition, I 
>> forgot to mention that a couple of days ago Gary Jennejohn submitted a 
>> patch for changing the 82xx SCC driver over to CONFIG_NET_MULTI style.
>>
> 
> Correct, and it must explicitly call ethernet_present() - see above.

If I understand the actual code right, then _now_ (which means actual
top of tree+ patch 4/5) it gets first called board_eth_init (). There,
I check if ethernet_present (), and if so, I return -1, which will result
in calling cpu_eth_init(). If the ethernet is not present, it returns 0,
which will result in not calling cpu_eth_init() ... And your HDLC
initialization can be done in board_eth_init() ... thats what we need,
or?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Gary Jennejohn]

On Wed, 19 Nov 2008 14:03:44 +0100
Heiko Schocher <hs@denx.de> wrote:

> Gary Jennejohn schrieb:
> > On Thu, 13 Nov 2008 09:30:51 -0800
> > Ben Warren <biggerbadderben@gmail.com> wrote:
> > 
[snip]
> >> That's what board_eth_init() and cpu_eth_init() are for.  In addition, I 
> >> forgot to mention that a couple of days ago Gary Jennejohn submitted a 
> >> patch for changing the 82xx SCC driver over to CONFIG_NET_MULTI style.
> >>
> > 
> > Correct, and it must explicitly call ethernet_present() - see above.
> 
> If I understand the actual code right, then _now_ (which means actual
> top of tree+ patch 4/5) it gets first called board_eth_init (). There,
> I check if ethernet_present (), and if so, I return -1, which will result
> in calling cpu_eth_init(). If the ethernet is not present, it returns 0,
> which will result in not calling cpu_eth_init() ... And your HDLC
> initialization can be done in board_eth_init() ... thats what we need,
> or?
> 

Ah yes.  My face is red.  I read the logic backwards.  If the PIGGY is
present it returns -1 ==> call cpu_eth_init(), otherwise 0.

And keymile_hdlc_enet_initialize() always succeeds so it can always be
called.

OK.  Then all we need is to add a call to keymile_hdlc_enet_initialize() to
eth_board_init() before the PIGGY check.  Case closed.

And I have to remove the call to ethernet_present() from the 82xx SCC
driver.

Thanks Heiko.

---
Gary Jennejohn
*********************************************************************
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
*********************************************************************

[U-Boot] [PATCH 4/5] powerpc: keymile: Add a check for the PIGGY debug board

[Ben Warren]

Hi Gary,

Gary Jennejohn wrote:
> On Thu, 13 Nov 2008 09:30:51 -0800
> Ben Warren <biggerbadderben@gmail.com> wrote:
>
>   
>> Heiko Schocher wrote:
>>     
>>> Hello Ben
>>>
>>> Ben Warren wrote:
>>>   
>>>       
>>>> Heiko Schocher wrote:
>>>>   
>>>>     
>>>>         
>>>>> Check the presence of the PIGGY on the keymile boards mgcoge,
>>>>> mgsuvd and kmeter1. If the PIGGY is not present, dont register
>>>>> this Ethernet device.
>>>>>
>>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>>> ---
>>>>>     
>>>>>       
>>>>>           
>>>> This looks like useful stuff to have, but I'd prefer that you put the 
>>>> check logic in board_eth_init() rather than adding to the individual 
>>>> device drivers.  I know the 8260 SCC driver is the older style, which 
>>>> precludes the use of board_eth_init, but I'll convert it if you're able 
>>>> to test.
>>>>   
>>>>         
>
> Unfortunately, this approach won't work.  First of all, the 82xx SCC
> driver is now initialized in cpu_eth_init(), which knows nothing about
> board-specific peculiarities like the PIGGY.  Secondly, the HDLC
> driver for Keymile has to be initialized in board_eth_init(), and it
> has nothing to do with the PIGGY.  Putting the check in board_eth_init()
> would break it completely.  I looked at Heiko's latest patch and couldn't
> figure out a way to cleanly differentiate between initializing the HDLC
> driver and checking whether the PIGGY was present fo the other ENET drivers.
>
>   
I think you're missing something.  You can put whatever logic you want 
in board_eth_init(), *including* calling cpu_eth_init().  As long as  
board_eth_init() returns >= 0, cpu_eth_init() will never get called by 
eth_initialize().  When I designed this, my intention (although not well 
communicated) was that board_eth_init() would always return >= 0, and 
that the -1 return code would only be used by the weak function in 
net/eth.c  There's pretty serious flexibility here and I urge you to 
look again.

regards,
Ben