[U-Boot] [PATCH] 8xx: prevent a machine check in scc_init().

[U-Boot] [PATCH] 8xx: prevent a machine check in scc_init().

[Gary Jennejohn]

Signed-off-by: Gary Jennejohn <garyj@denx.de>
---
 cpu/mpc8xx/scc.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/cpu/mpc8xx/scc.c b/cpu/mpc8xx/scc.c
index 09a3db1..9ffeb11 100644
--- a/cpu/mpc8xx/scc.c
+++ b/cpu/mpc8xx/scc.c
@@ -70,6 +70,9 @@ static int scc_recv(struct eth_device* dev);
 static int scc_init (struct eth_device* dev, bd_t * bd);
 static void scc_halt(struct eth_device* dev);
 
+/* avoid unnecessary reinitialization of the SCC */
+static int scc_init_completed;
+
 int scc_initialize(bd_t *bis)
 {
 	struct eth_device* dev;
@@ -193,6 +196,19 @@ static int scc_init (struct eth_device *dev, bd_t * bis)
 
 	volatile immap_t *immr = (immap_t *) CFG_IMMR;
 
+	/*
+	 * This routine is called again and again from eth_init(),
+	 * especially when CONFIG_NETCONSOLE is defined and
+	 * stdout=nc.
+	 * Avoid unneccessary flailing, otherwise we can get a panic here.
+	 */
+	if (scc_init_completed) {
+		immr->im_ioport.iop_pcso |= (PC_ENET_CLSN | PC_ENET_RENA);
+		immr->im_cpm.cp_scc[SCC_ENET].scc_gsmrl |=
+			(SCC_GSMRL_ENR | SCC_GSMRL_ENT);
+		return 1;
+	}
+
 #if defined(CONFIG_LWMON)
 	reset_phy();
 #endif
@@ -544,6 +560,7 @@ static int scc_init (struct eth_device *dev, bd_t * bis)
 	udelay (100000);	/* wait 100 ms */
 #endif
 
+	scc_init_completed = 1;
 	return 1;
 }
 
-- 
1.5.4.3

---
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] 8xx: prevent a machine check in scc_init().

[Wolfgang Denk]

Dear Gary Jennejohn,

In message <20080914114924.0090d95c@peedub.jennejohn.org> you wrote:
> 
> Signed-off-by: Gary Jennejohn <garyj@denx.de>

Sorry, but I don't understand what you're doing here, or why.

Why would there be any machine checks in  scc_init()?  Such  problems
have never been repoorted for any systems. Actuallym this part of the
code  is  one  of  the oldest in U-Boot - if there were machine check
issues in any configruation, these should be known.

> --- a/cpu/mpc8xx/scc.c
> +++ b/cpu/mpc8xx/scc.c
> @@ -70,6 +70,9 @@ static int scc_recv(struct eth_device* dev);
>  static int scc_init (struct eth_device* dev, bd_t * bd);
>  static void scc_halt(struct eth_device* dev);
>  
> +/* avoid unnecessary reinitialization of the SCC */
> +static int scc_init_completed;
> +
>  int scc_initialize(bd_t *bis)
>  {
>  	struct eth_device* dev;
> @@ -193,6 +196,19 @@ static int scc_init (struct eth_device *dev, bd_t * bis)
>  
>  	volatile immap_t *immr = (immap_t *) CFG_IMMR;
>  
> +	/*
> +	 * This routine is called again and again from eth_init(),
> +	 * especially when CONFIG_NETCONSOLE is defined and
> +	 * stdout=nc.
> +	 * Avoid unneccessary flailing, otherwise we can get a panic here.
> +	 */
> +	if (scc_init_completed) {
> +		immr->im_ioport.iop_pcso |= (PC_ENET_CLSN | PC_ENET_RENA);
> +		immr->im_cpm.cp_scc[SCC_ENET].scc_gsmrl |=
> +			(SCC_GSMRL_ENR | SCC_GSMRL_ENT);

If the initialization has already been done, why would we repeat these
two steps here?

And why exactle these two, and not the others that are performed for a
regular init sequence?


> +		return 1;
> +	}
> +
>  #if defined(CONFIG_LWMON)
>  	reset_phy();
>  #endif

I'm not sure if your early return here is actually valid. As you  can
see,  other  actions follow - for example here on the LWMON baord the
PHY reset; I suspect that your change might eventually work  on  your
current system, but break many others.


Also, I'm not sure if you are4 aware that the eth_init() part may  be
called  from  a  interface  switching  sequence,  i.  e. when cycling
between several ethenret interfaces. These will have been  shut  down
before  by  eth_halt()  -  skipping  the init sequence here seems the
wrong thing to me.


Maybe you could explain what problem you are actually truing to fix,
and especially under which situations you observe machine checks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The reasonable man adapts himself to the world; the unreasonable  one
persists  in  trying  to  adapt  the  world to himself. Therefore all
progress depends on the unreasonable man."      - George Bernard Shaw

[U-Boot] [PATCH] 8xx: prevent a machine check in scc_init().

[Gary Jennejohn]

On Sun, 14 Sep 2008 19:07:52 +0200
Wolfgang Denk <wd@denx.de> wrote:

> In message <20080914114924.0090d95c@peedub.jennejohn.org> you wrote:
> > 
> > Signed-off-by: Gary Jennejohn <garyj@denx.de>
> 
> Sorry, but I don't understand what you're doing here, or why.
> 
> Why would there be any machine checks in  scc_init()?  Such  problems
> have never been repoorted for any systems. Actuallym this part of the
> code  is  one  of  the oldest in U-Boot - if there were machine check
> issues in any configruation, these should be known.
> 
> > --- a/cpu/mpc8xx/scc.c
> > +++ b/cpu/mpc8xx/scc.c
> > @@ -70,6 +70,9 @@ static int scc_recv(struct eth_device* dev);
> >  static int scc_init (struct eth_device* dev, bd_t * bd);
> >  static void scc_halt(struct eth_device* dev);
> >  
> > +/* avoid unnecessary reinitialization of the SCC */
> > +static int scc_init_completed;
> > +
> >  int scc_initialize(bd_t *bis)
> >  {
> >  	struct eth_device* dev;
> > @@ -193,6 +196,19 @@ static int scc_init (struct eth_device *dev, bd_t * bis)
> >  
> >  	volatile immap_t *immr = (immap_t *) CFG_IMMR;
> >  
> > +	/*
> > +	 * This routine is called again and again from eth_init(),
> > +	 * especially when CONFIG_NETCONSOLE is defined and
> > +	 * stdout=nc.
> > +	 * Avoid unneccessary flailing, otherwise we can get a panic here.
> > +	 */
> > +	if (scc_init_completed) {
> > +		immr->im_ioport.iop_pcso |= (PC_ENET_CLSN | PC_ENET_RENA);
> > +		immr->im_cpm.cp_scc[SCC_ENET].scc_gsmrl |=
> > +			(SCC_GSMRL_ENR | SCC_GSMRL_ENT);
> 
> If the initialization has already been done, why would we repeat these
> two steps here?
> 
> And why exactle these two, and not the others that are performed for a
> regular init sequence?
> 
> 
> > +		return 1;
> > +	}
> > +
> >  #if defined(CONFIG_LWMON)
> >  	reset_phy();
> >  #endif
> 
> I'm not sure if your early return here is actually valid. As you  can
> see,  other  actions follow - for example here on the LWMON baord the
> PHY reset; I suspect that your change might eventually work  on  your
> current system, but break many others.
> 
> 
> Also, I'm not sure if you are4 aware that the eth_init() part may  be
> called  from  a  interface  switching  sequence,  i.  e. when cycling
> between several ethenret interfaces. These will have been  shut  down
> before  by  eth_halt()  -  skipping  the init sequence here seems the
> wrong thing to me.
> 
> 
> Maybe you could explain what problem you are actually truing to fix,
> and especially under which situations you observe machine checks.
> 

The first call to scc_init() executes all the code.  Only subsequent
calls are short-circuited.

OK, just think about it a little.

When nc is active as a console it is often invoked for every byte.

The network stack code calls eth_init() prior to sending the byte
and eth_halt() after sending the byte.

scc_init() happily tries to reallocate dpram at every call when
CFG_ALLOC_DPRAM is defined, as it was in my case.  Eventually dpram
is exhausted which evidently results in a machine check.

Also note that scc_halt() merely clears some bits in scc_gsmrl and
iop_pcso.  It really doesn't seem necessary to reinitialize every
register at every call to scc_init().

Most network drivers behave this way, which works in the general
use case, but not when nc is used.

I doubt that nc is used very often, otherwise this problem would
have reared its ugly head a lot sooner.  The general use case is
also to merely invoke tftp to load the kernel and maybe fdt.
This succeeds because it takes numerous (in this case about 14)
calls to the init routine before the machine check happens.

---
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] 8xx: prevent a machine check in scc_init().

[Wolfgang Denk]

Dear Gary Jennejohn,

In message <20080915105924.3249d5db@peedub.jennejohn.org> you wrote:
>
> The first call to scc_init() executes all the code.  Only subsequent
> calls are short-circuited.

I am aware of that. But after the first call, eth_halt()  might  have
been  run,  deactivating  the  interface; then for example interfaces
might have been switched to another port, and finally  switched  back
to the old port. In this case, a full initialization sequence will be
necessary.

> OK, just think about it a little.

Indeed, please do.

> When nc is active as a console it is often invoked for every byte.

Ah! That's the origin of your concern. But I think you're  trying  to
attack  it  at  the wrong place. Your modification will most probably
break some (or many) boards.

> The network stack code calls eth_init() prior to sending the byte
> and eth_halt() after sending the byte.
> 
> scc_init() happily tries to reallocate dpram at every call when
> CFG_ALLOC_DPRAM is defined, as it was in my case.  Eventually dpram
> is exhausted which evidently results in a machine check.

-> grep -l CONFIG_NETCONSOLE include/configs/* | xargs egrep CFG_ALLOC_DPRAM | wc -l
0
->

You are right. None of the boards that use  netconsole  so  far  (and
that  have  been  tested  with  that  feature,  obviously)  is  using
CFG_ALLOC_DPRAM. So a fix is necessary not to re-allocate DPRAM here.

It would be great if you could submit a patch that fixes this problem.

> I doubt that nc is used very often, otherwise this problem would
> have reared its ugly head a lot sooner.  The general use case is
> also to merely invoke tftp to load the kernel and maybe fdt.
> This succeeds because it takes numerous (in this case about 14)
> calls to the init routine before the machine check happens.

In fact I think the problem never showed before because nodody used
CFG_ALLOC_DPRAM with netconsole. I agree that this should be fixed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
                  Nail here --X-- for new monitor.

[U-Boot] [PATCH V2] 8xx: prevent a machine check in scc_init().

[Gary Jennejohn]

Without this change DPRAM can be exhausted when CFG_ALLOC_DPRAM is
defined, which eventually leads to a machine check.  This change
assures that DPRAM is allocated only once in that case.

Signed-off-by: Gary Jennejohn <garyj@denx.de>
---
 cpu/mpc8xx/scc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/cpu/mpc8xx/scc.c b/cpu/mpc8xx/scc.c
index 09a3db1..d91289a 100644
--- a/cpu/mpc8xx/scc.c
+++ b/cpu/mpc8xx/scc.c
@@ -216,7 +216,9 @@ static int scc_init (struct eth_device *dev, bd_t * bis)
 	txIdx = 0;
 
 #ifdef CFG_ALLOC_DPRAM
-	rtx = (RTXBD *) (immr->im_cpm.cp_dpmem +
+	/* Avoid exhausting DPRAM. */
+	if (rtx == NULL)
+		rtx = (RTXBD *) (immr->im_cpm.cp_dpmem +
 			 dpram_alloc_align (sizeof (RTXBD), 8));
 #else
 	rtx = (RTXBD *) (immr->im_cpm.cp_dpmem + CPM_SCC_BASE);
-- 
1.5.4.3

---
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 V2] 8xx: prevent a machine check in scc_init().

[Wolfgang Denk]

Dear Gary Jennejohn,

In message <20080918135224.3ad21fd6@peedub.jennejohn.org> you wrote:
> 
> Without this change DPRAM can be exhausted when CFG_ALLOC_DPRAM is
> defined, which eventually leads to a machine check.  This change
> assures that DPRAM is allocated only once in that case.
> 
> Signed-off-by: Gary Jennejohn <garyj@denx.de>
> ---
>  cpu/mpc8xx/scc.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

Applied, with minor re-editing to make the code look similar to what
we have in cpu/mpc8xx/fec.c

Thanks.


Bein, I hope it is OK with you that I pulled this directly.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It would be illogical to kill without reason
	-- Spock, "Journey to Babel", stardate 3842.4

[U-Boot] [PATCH V2] 8xx: prevent a machine check in scc_init().

[Ben Warren]

Wolfgang Denk wrote:
> Dear Gary Jennejohn,
>
> In message <20080918135224.3ad21fd6@peedub.jennejohn.org> you wrote:
>   
>> Without this change DPRAM can be exhausted when CFG_ALLOC_DPRAM is
>> defined, which eventually leads to a machine check.  This change
>> assures that DPRAM is allocated only once in that case.
>>
>> Signed-off-by: Gary Jennejohn <garyj@denx.de>
>> ---
>>  cpu/mpc8xx/scc.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>     
>
> Applied, with minor re-editing to make the code look similar to what
> we have in cpu/mpc8xx/fec.c
>
> Thanks.
>
>
> Bein, I hope it is OK with you that I pulled this directly.
>
>   
Totally fine.  I've been off-grid for a while and am finally starting to 
catch up.  Thanks for taking care of this.
> Best regards,
>
> Wolfgang Denk
>
>