IOMUX: Add console multiplexing support.

Since this patch touches net/eth.c it is being sent separately.

When CONFIG_IO_MUX, CONFIG_NETCONSOLE and CFG_CONSOLE_IS_IN_ENV are all defined together it is possible that nc (netconsole) is defined as an output device. In this case it is necessary to set GD_FLG_DEVINIT after the network devices have been initialized, otherwise u-boot might try to send output to a device before it is ready, which leads to various errors.

Signed-off-by Gary Jennejohn <[email protected]>

---
 net/eth.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

net/eth.c +14 -0

index 432dd60..94b6e3a 100644
@@ -26,6 +26,11 @@
 #include <net.h>
 #include <miiphy.h>
 
+#if defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) && \
+	defined(CFG_CONSOLE_IS_IN_ENV)
+DECLARE_GLOBAL_DATA_PTR;
+#endif
+
 #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
 
 /*

@@ -256,6 +261,15 @@ int eth_initialize(bd_t *bis)
 		putc ('\n');
 	}
 
+#if defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) && \
+	defined(CFG_CONSOLE_IS_IN_ENV)
+	/*
+	 * Must do this very late because a network device may be set as a
+	 * console at boot time.
+	 */
+	gd->flags |= GD_FLG_DEVINIT;	/* device initialization completed */
+#endif
+
 	return eth_number;
 }
 

@@ -532,6 +546,16 @@ int eth_initialize(bd_t *bis)
 #if defined(CONFIG_DRIVER_TI_EMAC)
 	davinci_eth_miiphy_initialize(bis);
 #endif

Quoted 9 lines · In message <[email protected]> you wrote:

Since this patch touches net/eth.c it is being sent separately.

When CONFIG_IO_MUX, CONFIG_NETCONSOLE and CFG_CONSOLE_IS_IN_ENV are all defined together it is possible that nc (netconsole) is defined as an output device. In this case it is necessary to set GD_FLG_DEVINIT after the network devices have been initialized, otherwise u-boot might try to send output to a device before it is ready, which leads to various errors.

I don't understand this patch for two reasons:

1) What has CONFIG_IO_MUX to do with that? We can have a netconsole defined with or without that new feature, so why would it make any difference?

Quoted 41 lines

Signed-off-by Gary Jennejohn <[email protected]>

---
 net/eth.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

net/eth.c +14 -0

index 432dd60..94b6e3a 100644
@@ -26,6 +26,11 @@
 #include <net.h>
 #include <miiphy.h>
 
+#if defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) && \
+	defined(CFG_CONSOLE_IS_IN_ENV)
+DECLARE_GLOBAL_DATA_PTR;
+#endif
+
 #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
 
 /*

@@ -256,6 +261,15 @@ int eth_initialize(bd_t *bis)
 		putc ('\n');
 	}
 
+#if defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) && \
+	defined(CFG_CONSOLE_IS_IN_ENV)
+	/*
+	 * Must do this very late because a network device may be set as a
+	 * console at boot time.
+	 */
+	gd->flags |= GD_FLG_DEVINIT;	/* device initialization completed */
+#endif
+
 	return eth_number;
 }
 

@@ -532,6 +546,16 @@ int eth_initialize(bd_t *bis)
 #if defined(CONFIG_DRIVER_TI_EMAC)
 	davinci_eth_miiphy_initialize(bis);
 #endif

net/eth.c int eth_initialize(bd_t *bis) · line 558

Quoted 10 lines

net/eth.c +10 -0

+
+#if defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) && \
+	defined(CFG_CONSOLE_IS_IN_ENV)
+	/*
+	 * Must do this very late because a network device may be set as a
+	 * console at boot time.
+	 */
+	gd->flags |= GD_FLG_DEVINIT;	/* device initialization completed */
+#endif
+

2) You only add new points where the GD_FLG_DEVINIT bit gets set in gd->flags. That means there are two possibilities when your newly added code is run: either, this bit is already set by other parts of the codem than the operation was redundant and couldbe omitted; or, the flasg was not set yet, then you set it now, which means you set it EARLIE than it would have been set before.

But your comment suggests that that would be done LATER now.

So what exactly is the purpose of this patch?

Quoted 11 lines

2) You only add new points where the GD_FLG_DEVINIT bit gets set in gd->flags. That means there are two possibilities when your newly added code is run: either, this bit is already set by other parts of the codem than the operation was redundant and couldbe omitted; or, the flasg was not set yet, then you set it now, which means you set it EARLIE than it would have been set before.

But your comment suggests that that would be done LATER now.

Quoted 1 line

So what exactly is the purpose of this patch?

This bit is normally ser in console_init_r, which is called very early.

You have to look at this patch in conjunction with the first patch and not as a separate entity, which it most definitely is not.

Quoted 11 lines · In message <[email protected]> you wrote:

Quoted 11 lines

2) You only add new points where the GD_FLG_DEVINIT bit gets set in gd->flags. That means there are two possibilities when your newly added code is run: either, this bit is already set by other parts of the codem than the operation was redundant and couldbe omitted; or, the flasg was not set yet, then you set it now, which means you set it EARLIE than it would have been set before.

But your comment suggests that that would be done LATER now.

Quoted 6 lines

Quoted 1 line

So what exactly is the purpose of this patch?

This bit is normally ser in console_init_r, which is called very early.

You have to look at this patch in conjunction with the first patch and not as a separate entity, which it most definitely is not.

Sorry, but this doesn't work. If you split patches, you have to do it in an orthogoanl way, such that each patch on it's own makes sense. This patch doesn't make any sense as is. Maybe theree are parts missing that may be buried somewhere in some other patch, but please do not expect that we will try to find them.

Please re-split patches such that they are independent of each other (except maybe that one has to be applied first), and make sure that each patch is complete in itself.

Quoted 3 lines

Sorry, but this doesn't work. If you split patches, you have to do it in an orthogoanl way, such that each patch on it's own makes sense.

Quoted 3 lines

This patch doesn't make any sense as is. Maybe theree are parts missing that may be buried somewhere in some other patch, but please do not expect that we will try to find them.

Quoted 3 lines

Please re-split patches such that they are independent of each other (except maybe that one has to be applied first), and make sure that each patch is complete in itself.

I did it this way because I didn't want to send the net custodian an unnecessary patch. I though that was the way patches were supposed to be handled. I know I've had complaints from custodians in the past about this.

A consistent policy certainly would be nice.

When both CONFIG_SYS_CONSOLE_IS_IN_ENV and CONFIG_NETCONSOLE are defined the user can have stdout set to nc (netconsole).

This causes problems because u-boot will try to write to nc as soon as GD_FLG_DEVINIT is set in gd->flags, which happens before the network devices are initialized in net/eth.c. This results in error messages being spewed out.

To prevent this problem set GD_FLG_DEVINIT in net/eth.c:eth_initialize(), after the network devices have been initialized, instead of in common/console.c:console_init_r().

Is there another part of the patch, part 1/2, too?

Quoted 4 lines · In message <[email protected]> you wrote:

When both CONFIG_SYS_CONSOLE_IS_IN_ENV and CONFIG_NETCONSOLE are defined the user can have stdout set to nc (netconsole).

Quoted 4 lines

This causes problems because u-boot will try to write to nc as soon as GD_FLG_DEVINIT is set in gd->flags, which happens before the network devices are initialized in net/eth.c. This results in error messages being spewed out.

It seems this can happen only if CONFIG_SYS_CONSOLE_IS_IN_ENV is defined, right?

Quoted 3 lines

To prevent this problem set GD_FLG_DEVINIT in net/eth.c:eth_initialize(), after the network devices have been initialized, instead of in common/console.c:console_init_r().

I have to admit that I don't like the idea of splitting the GD_FLG_DEVINIT into several, unrelated parts of the code.

Would it not make more sense to have the netconsole part wait with output until it's been initialized? And/or move the netweork init to an earlier point, when netconsole is enabled?

Quoted 3 lines · On Mon, 20 Oct 2008 15:24:53 +0200 Wolfgang Denk <[email protected]> wrote:

I have to admit that I don't like the idea of splitting the GD_FLG_DEVINIT into several, unrelated parts of the code.

I don't like it too much myself, but it seemed like the logical approach to me at the time I made this modification.

Quoted 4 lines

Would it not make more sense to have the netconsole part wait with output until it's been initialized? And/or move the netweork init to an earlier point, when netconsole is enabled?

Not a bad idea. I think it would be most logical to do it in the netconsole code, rather than moving up the network initialization.

I'll take a look at that.

I looked at this some more. eth_initialize() is called in every architecture-specific library which means changing 8 files to move it up to before the initialization of netconsole.

netconsole is intialized in devices_init() which is called even before console_init_r() and long before eth_initialize().

One alrernative which occurs to me would be to introduce a new flag GD_FLG_ETHINIT, but this is even worse because it requires modifying 12 header files and 3 or more C files.

Dear Gary Jennejohn,

Quoted 7 lines · In message <[email protected]> you wrote:

I looked at this some more. eth_initialize() is called in every architecture-specific library which means changing 8 files to move it up to before the initialization of netconsole.

netconsole is intialized in devices_init() which is called even before console_init_r() and long before eth_initialize().

Well, maybe there is a less intrusive approach to solve the problem.

Quoted 4 lines

One alrernative which occurs to me would be to introduce a new flag GD_FLG_ETHINIT, but this is even worse because it requires modifying 12 header files and 3 or more C files.

Hm... let's sum up what exactly the problem is; you wrote:

| This causes problems because u-boot will try to write to nc as soon
| as GD_FLG_DEVINIT is set in gd->flags, which happens before the
| network devices are initialized in net/eth.c. This results in error
| messages being spewed out.

I read this that what we actually want to do is stopping NC to transmit too early. Correct?

Well, nc_send_packet() (see "drivers/net/netconsole.c") can be easily shortcut if we find a way to make eth_get_dev() return NULL.

And eth_get_dev() (see "net/eth.c") just returns "eth_current".

So maybe there is a way to make sure "eth_current" is set to NULL until it's OK for netconsole to transmit?

Maybe, maybe eventually the real cause of your problems is that "eth_current" is not read as NULL while running from flash? Maybe changing the declaration in "net/eth.c" into something like this would help?

static struct eth_device *eth_current __attribute__ ((section(".data"))) = NULL;

What do you think?

Best regards,

Wolfgang Denk

Quoted 43 lines · Wolfgang Denk

Dear Gary Jennejohn,

Quoted 7 lines · In message <[email protected]> you wrote:

I looked at this some more. eth_initialize() is called in every architecture-specific library which means changing 8 files to move it up to before the initialization of netconsole.

netconsole is intialized in devices_init() which is called even before console_init_r() and long before eth_initialize().

Well, maybe there is a less intrusive approach to solve the problem.

Quoted 4 lines

One alrernative which occurs to me would be to introduce a new flag GD_FLG_ETHINIT, but this is even worse because it requires modifying 12 header files and 3 or more C files.

Hm... let's sum up what exactly the problem is; you wrote:

| This causes problems because u-boot will try to write to nc as soon
| as GD_FLG_DEVINIT is set in gd->flags, which happens before the
| network devices are initialized in net/eth.c. This results in error
| messages being spewed out.

I read this that what we actually want to do is stopping NC to transmit too early. Correct?

Well, nc_send_packet() (see "drivers/net/netconsole.c") can be easily shortcut if we find a way to make eth_get_dev() return NULL.

And eth_get_dev() (see "net/eth.c") just returns "eth_current".

So maybe there is a way to make sure "eth_current" is set to NULL until it's OK for netconsole to transmit?

Maybe, maybe eventually the real cause of your problems is that "eth_current" is not read as NULL while running from flash? Maybe changing the declaration in "net/eth.c" into something like this would help?

Quoted 5 lines

static struct eth_device *eth_current __attribute__ ((section(".data"))) = NULL;

What do you think?

This looks like a good idea. eth_current is a variable that should definitely be in a known state.

By the time the problem arises we're already running out of RAM.

I think the real problem is that console_init_r() is called too early. If it were called *after* eth_initialize() then there would be no difficulties.

AFAICT moving console_init_r() shouldn't present any problems, since until its invocation U-Boot will simply use the serial interface.

I can't imagine that waiting a few milliseconds longer to switch to whatever console the user has set in the environment would present a problem.

This has the following advantages
a) nc has been initialized and can be used as a console, if so desired
b) any hardware initializations, such as SPI or I2C, which may be required to initialize an ethernet device have already happened
c) the ethernet device is already initialized and ready to go
d) no need to split up setting gd->flags

It isn't clear to me whether the architecture-specific libs have custodians. Does anyone know?

Quoted 2 lines · In message <[email protected]> you wrote:

By the time the problem arises we're already running out of RAM.

So maybe you can describe exactly what happens, and when?

Quoted 4 lines

I think the real problem is that console_init_r() is called too early. If it were called *after* eth_initialize() then there would be no difficulties.

Quoted 2 lines

AFAICT moving console_init_r() shouldn't present any problems, since until its invocation U-Boot will simply use the serial interface.

I am not so sure. Moving things around not as trivial as it may seem, as we have all kinds of configurations with LCD display, modem support etc.

Quoted 2 lines

It isn't clear to me whether the architecture-specific libs have custodians. Does anyone know?

What is not clear about that? Do we have custodians for architecture specific files? Yes, we do. So whay do you doubt that the libs fall into their responsibility, too?

Yes, but I did this part first because it's small and easily generated. Since it also affects net I wanted to get it to the custodian. The other part adds the console multiplexing and isn't directly related to this.

Not a bad idea. I think it would be most logical to do it in the netconsole code, rather than moving up the network initialization.

I'll take a look at that.