[U-Boot] [PATCH] mpc83xx: Make it boot again

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

After the removal of COLD/WARM start flags my mpc8321
board didn't boot anymore.
Trial and error suggests that map_flash_by_law1 needs
an isync(padding with 4 nop's also did the trick)
after updating LBLAWAR1 to make sure the the change has
reached the HW before continuing with the code that depends on it.
Add an isync to remap_flash_by_law0 for good measure too.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 arch/powerpc/cpu/mpc83xx/start.S |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
index e8b1ebc..d3ec580 100644
--- a/arch/powerpc/cpu/mpc83xx/start.S
+++ b/arch/powerpc/cpu/mpc83xx/start.S
@@ -1191,6 +1191,7 @@ map_flash_by_law1:
 	bne 1b
 
 	stw r4, LBLAWAR1(r3) /* LBLAWAR1 <= 8MB Flash Size */
+	isync /* Wait for HW to catch up */
 	blr
 
 	/* Though all the LBIU Local Access Windows and LBC Banks will be
@@ -1229,5 +1230,6 @@ remap_flash_by_law0:
 	xor r4, r4, r4
 	stw r4, LBLAWBAR1(r3)
 	stw r4, LBLAWAR1(r3) /* Off LBIU LAW1 */
+	isync /* Wait for HW to catch up */
 	blr
 #endif /* CONFIG_SYS_FLASHBOOT */
-- 
1.7.2.2

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Wood Scott-B07421]

Joakim Tjernlund wrote:
> Trial and error suggests that map_flash_by_law1 needs
> an isync(padding with 4 nop's also did the trick)
> after updating LBLAWAR1 to make sure the the change has
> reached the HW before continuing with the code that depends on it.
> Add an isync to remap_flash_by_law0 for good measure too.

To be totally safe, we probably want to do a readback plus twi (to turn a data dependency into a flow dependency) before the isync.

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Timur Tabi]

Wood Scott-B07421 wrote:
> To be totally safe, we probably want to do a readback plus twi (to turn
> a data dependency into a flow dependency) before the isync.

twi == trap word immediate?

If so, I don't see how that will turn a data dependency into a flow dependency.
 Is that some sort of side effect of twi?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Wood Scott-B07421 <B07421@freescale.com> wrote on 2010/11/04 17:47:41:
>
> Joakim Tjernlund wrote:
> > Trial and error suggests that map_flash_by_law1 needs
> > an isync(padding with 4 nop's also did the trick)
> > after updating LBLAWAR1 to make sure the the change has
> > reached the HW before continuing with the code that depends on it.
> > Add an isync to remap_flash_by_law0 for good measure too.
>
> To be totally safe, we probably want to do a readback plus twi (to turn a data dependency into a flow dependency) before the isync.

Feel free but there is no data cache here and IMMR should be GUARDED, still need that anyway?

   Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Wood Scott-B07421]

Timur Tabi wrote:
> Wood Scott-B07421 wrote:
> > To be totally safe, we probably want to do a readback plus twi (to turn
> > a data dependency into a flow dependency) before the isync.
> 
> twi == trap word immediate?

Yes.

> If so, I don't see how that will turn a data dependency into a flow dependency.
> Is that some sort of side effect of twi?

It decides based on the input register (data dependency) whether to cause a trap (flow dependency).  We never want it to actually trap, so we set a condition that says never trap, but the dependency is still there -- the hardware doesn't decode it as a no-op.

See arch/powerpc/include/asm/io.h, it's used in the I/O read accessors.

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Andre Schwarz]

On 11/04/2010 08:49 PM, Wood Scott-B07421 wrote:
>
> Timur Tabi wrote:
> > Wood Scott-B07421 wrote:
> > > To be totally safe, we probably want to do a readback plus twi (to 
> turn
> > > a data dependency into a flow dependency) before the isync.
> >
> > twi == trap word immediate?
>
> Yes.
>
> > If so, I don't see how that will turn a data dependency into a flow 
> dependency.
> > Is that some sort of side effect of twi?
>
> It decides based on the input register (data dependency) whether to 
> cause a trap (flow dependency).  We never want it to actually trap, so 
> we set a condition that says never trap, but the dependency is still 
> there -- the hardware doesn't decode it as a no-op.
>

Might there be any other location this could be used for ?

Actually my 8377 is running fine up to 533MHz core and csb up to 400MHz.
As soon as core frequency rises to 600MHz+ there'll be frequent hangs 
during flush_dcache.

core voltage is clean at 1.05V without any glitches or significant load 
steps.

Can anybody confirm that current code runs stable on MPC837x at 600MHz+ ?


Regards,
Andr?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

>
> Timur Tabi wrote:
> > Wood Scott-B07421 wrote:
> > > To be totally safe, we probably want to do a readback plus twi (to turn
> > > a data dependency into a flow dependency) before the isync.
> >
> > twi == trap word immediate?
>
> Yes.
>
> > If so, I don't see how that will turn a data dependency into a flow dependency.
> > Is that some sort of side effect of twi?
>
> It decides based on the input register (data dependency) whether to cause a trap (flow dependency).  We never want it to actually trap, so we set a condition that says never trap, but the dependency is still there -- the hardware doesn't decode it as a no-op.
>
> See arch/powerpc/include/asm/io.h, it's used in the I/O read accessors.

Guys, unless you will do the twi stuff now I suggest you ACK this
patch so 83xx works again.

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

>
> >
> > Timur Tabi wrote:
> > > Wood Scott-B07421 wrote:
> > > > To be totally safe, we probably want to do a readback plus twi (to turn
> > > > a data dependency into a flow dependency) before the isync.
> > >
> > > twi == trap word immediate?
> >
> > Yes.
> >
> > > If so, I don't see how that will turn a data dependency into a flow dependency.
> > > Is that some sort of side effect of twi?
> >
> > It decides based on the input register (data dependency) whether to cause a trap (flow dependency).  We never want it to actually trap, so we set a condition that says never trap, but the dependency is still there -- the hardware doesn't decode it as a no-op.
> >
> > See arch/powerpc/include/asm/io.h, it's used in the I/O read accessors.
>
> Guys, unless you will do the twi stuff now I suggest you ACK this
> patch so 83xx works again.

Ping?

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Kumar Gala]

On Nov 4, 2010, at 9:45 AM, Joakim Tjernlund wrote:

> After the removal of COLD/WARM start flags my mpc8321
> board didn't boot anymore.
> Trial and error suggests that map_flash_by_law1 needs
> an isync(padding with 4 nop's also did the trick)
> after updating LBLAWAR1 to make sure the the change has
> reached the HW before continuing with the code that depends on it.
> Add an isync to remap_flash_by_law0 for good measure too.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
> arch/powerpc/cpu/mpc83xx/start.S |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
> index e8b1ebc..d3ec580 100644
> --- a/arch/powerpc/cpu/mpc83xx/start.S
> +++ b/arch/powerpc/cpu/mpc83xx/start.S
> @@ -1191,6 +1191,7 @@ map_flash_by_law1:
> 	bne 1b
> 
> 	stw r4, LBLAWAR1(r3) /* LBLAWAR1 <= 8MB Flash Size */

I think what you want is:
	lwz	r4, LBLAWAR1(r3)

> +	isync /* Wait for HW to catch up */

I dont think this is needed (unless the manual says otherwise about updates to *LAWAR*

> 	blr
> 
> 	/* Though all the LBIU Local Access Windows and LBC Banks will be
> @@ -1229,5 +1230,6 @@ remap_flash_by_law0:
> 	xor r4, r4, r4
> 	stw r4, LBLAWBAR1(r3)
> 	stw r4, LBLAWAR1(r3) /* Off LBIU LAW1 */
> +	isync /* Wait for HW to catch up */

same here.

> 	blr
> #endif /* CONFIG_SYS_FLASHBOOT */
> -- 
> 1.7.2.2
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Kumar Gala]

On Nov 4, 2010, at 11:47 AM, Wood Scott-B07421 wrote:

> Joakim Tjernlund wrote:
>> Trial and error suggests that map_flash_by_law1 needs
>> an isync(padding with 4 nop's also did the trick)
>> after updating LBLAWAR1 to make sure the the change has
>> reached the HW before continuing with the code that depends on it.
>> Add an isync to remap_flash_by_law0 for good measure too.
> 
> To be totally safe, we probably want to do a readback plus twi (to turn a data dependency into a flow dependency) before the isync.

We only do the 'twi' for loads/in_beX not stores/out_beX.

- k

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Kumar Gala <galak@kernel.crashing.org> wrote on 2010/11/12 16:58:48:
>
>
> On Nov 4, 2010, at 9:45 AM, Joakim Tjernlund wrote:
>
> > After the removal of COLD/WARM start flags my mpc8321
> > board didn't boot anymore.
> > Trial and error suggests that map_flash_by_law1 needs
> > an isync(padding with 4 nop's also did the trick)
> > after updating LBLAWAR1 to make sure the the change has
> > reached the HW before continuing with the code that depends on it.
> > Add an isync to remap_flash_by_law0 for good measure too.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> > arch/powerpc/cpu/mpc83xx/start.S |    2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
> > index e8b1ebc..d3ec580 100644
> > --- a/arch/powerpc/cpu/mpc83xx/start.S
> > +++ b/arch/powerpc/cpu/mpc83xx/start.S
> > @@ -1191,6 +1191,7 @@ map_flash_by_law1:
> >    bne 1b
> >
> >    stw r4, LBLAWAR1(r3) /* LBLAWAR1 <= 8MB Flash Size */
>
> I think what you want is:
>    lwz   r4, LBLAWAR1(r3)
>
> > +   isync /* Wait for HW to catch up */
>
> I dont think this is needed (unless the manual says otherwise about updates to *LAWAR*

I can confirm that replacing isync with lwz   r4, LBLAWAR1(r3) also
works. I really don't know this stuff well so I hope you can sort this out.

    Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

On Fri, 12 Nov 2010 09:58:53 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Nov 4, 2010, at 11:47 AM, Wood Scott-B07421 wrote:
> 
> > Joakim Tjernlund wrote:
> >> Trial and error suggests that map_flash_by_law1 needs
> >> an isync(padding with 4 nop's also did the trick)
> >> after updating LBLAWAR1 to make sure the the change has
> >> reached the HW before continuing with the code that depends on it.
> >> Add an isync to remap_flash_by_law0 for good measure too.
> > 
> > To be totally safe, we probably want to do a readback plus twi (to turn a data dependency into a flow dependency) before the isync.
> 
> We only do the 'twi' for loads/in_beX not stores/out_beX.

Yes, and the readback is a load.

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Kumar Gala]

On Nov 12, 2010, at 11:22 AM, Scott Wood wrote:

> On Fri, 12 Nov 2010 09:58:53 -0600
> Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>> 
>> On Nov 4, 2010, at 11:47 AM, Wood Scott-B07421 wrote:
>> 
>>> Joakim Tjernlund wrote:
>>>> Trial and error suggests that map_flash_by_law1 needs
>>>> an isync(padding with 4 nop's also did the trick)
>>>> after updating LBLAWAR1 to make sure the the change has
>>>> reached the HW before continuing with the code that depends on it.
>>>> Add an isync to remap_flash_by_law0 for good measure too.
>>> 
>>> To be totally safe, we probably want to do a readback plus twi (to turn a data dependency into a flow dependency) before the isync.
>> 
>> We only do the 'twi' for loads/in_beX not stores/out_beX.
> 
> Yes, and the readback is a load.

following the store with a load should be sufficient I dont think we need the twi in this case.

- k

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

On Fri, 12 Nov 2010 13:26:17 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Nov 12, 2010, at 11:22 AM, Scott Wood wrote:
> 
> > On Fri, 12 Nov 2010 09:58:53 -0600
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >> We only do the 'twi' for loads/in_beX not stores/out_beX.
> > 
> > Yes, and the readback is a load.
> 
> following the store with a load should be sufficient I dont think we need the twi in this case.

Do we ever need it, or is guarded+cache inhibited sufficient to
completely hold up execution of anything beyond the load?

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Andre Schwarz]

On 11/12/2010 08:31 PM, Scott Wood wrote:
> On Fri, 12 Nov 2010 13:26:17 -0600
> Kumar Gala<galak@kernel.crashing.org>  wrote:
>
>    
>> On Nov 12, 2010, at 11:22 AM, Scott Wood wrote:
>>
>>      
>>> On Fri, 12 Nov 2010 09:58:53 -0600
>>> Kumar Gala<galak@kernel.crashing.org>  wrote:
>>>        
>>>> We only do the 'twi' for loads/in_beX not stores/out_beX.
>>>>          
>>> Yes, and the readback is a load.
>>>        
>> following the store with a load should be sufficient I dont think we need the twi in this case.
>>      
> Do we ever need it, or is guarded+cache inhibited sufficient to
> completely hold up execution of anything beyond the load?
>    

Sorry for being insistent :
Is there any explanation why this issue only came up on Jocke's MPC8321 
(?) and my MPC8377 ?

As mentioned here 
(http://lists.denx.de/pipermail/u-boot/2010-November/081459.html) 
there's still a mysterious issue which might have the same root cause.
Again : USB is working fine under Linux - it's an U-Boot only issue.

Is anybody willing to have a closer look - I'm completely stuck here.


Regards,
Andr?


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Andre Schwarz <andre.schwarz@matrix-vision.de> wrote on 2010/11/15 10:57:30:
>
> On 11/12/2010 08:31 PM, Scott Wood wrote:
> > On Fri, 12 Nov 2010 13:26:17 -0600
> > Kumar Gala<galak@kernel.crashing.org>  wrote:
> >
> >
> >> On Nov 12, 2010, at 11:22 AM, Scott Wood wrote:
> >>
> >>
> >>> On Fri, 12 Nov 2010 09:58:53 -0600
> >>> Kumar Gala<galak@kernel.crashing.org>  wrote:
> >>>
> >>>> We only do the 'twi' for loads/in_beX not stores/out_beX.
> >>>>
> >>> Yes, and the readback is a load.
> >>>
> >> following the store with a load should be sufficient I dont think we need the twi in this case.
> >>
> > Do we ever need it, or is guarded+cache inhibited sufficient to
> > completely hold up execution of anything beyond the load?
> >
>
> Sorry for being insistent :
> Is there any explanation why this issue only came up on Jocke's MPC8321
> (?) and my MPC8377 ?

Has anyone else tested 83xx on NOR?
My guess is that cache line crossing shifted so that now the CPU
doesn't need to read in a new cache at the critical part.

>
> As mentioned here
> (http://lists.denx.de/pipermail/u-boot/2010-November/081459.html)
> there's still a mysterious issue which might have the same root cause.
> Again : USB is working fine under Linux - it's an U-Boot only issue.
>
> Is anybody willing to have a closer look - I'm completely stuck here.

Nope, don't use USB.

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Liu Dave-R63238]

> Has anyone else tested 83xx on NOR?
> My guess is that cache line crossing shifted so that now the CPU
> doesn't need to read in a new cache at the critical part.

I notice this is hot thread for 83xx in these days.
Anybody can share more background for the issue?
I would like have a look the issue.

Thanks,
 Dave

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Andre Schwarz]

Dave,
> I notice this is hot thread for 83xx in these days.
> Anybody can share more background for the issue?
> I would like have a look the issue.
>    

during MPC8377 board bring up we couldn't get U-Boot up and running - 
the serial line has been dead.

Using a bdi2000 debugger told us the CPU, RAM and Flash was basically 
working fine.

Jocke had the same problems on a MPC8321 and found a solution/workaround 
(adding 4 nops in Start.S right after _start).
I followed his proposal and it worked out fine.

The experts found an issue within init code and it looks like a proper 
patch will be added to mainline shortly.
The discussion of the proper fix is right in this thread ...

This leaves the question why this only happens on Jocke's MPC8321 and my 
MPC8377.
Looks like nobody else observed this. Our MPC8343 based boards never had 
this issue.


Regards,
Andr?


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Liu Dave-R63238]

> The experts found an issue within init code and it looks like a proper
> patch will be added to mainline shortly.
> The discussion of the proper fix is right in this thread ...

It should be timing issue in the SoC, software did not have a proper
process to handle
IMMR registers accessing.

I agree Kumar on this.
Adding the read back with load is needing for the LAW window changing.
And something like sync/eieio instruction also need to be added between
stw and lwz...
to have a proper order accessing.

Thanks,
 Dave

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

>
> > The experts found an issue within init code and it looks like a proper
> > patch will be added to mainline shortly.
> > The discussion of the proper fix is right in this thread ...
>
> It should be timing issue in the SoC, software did not have a proper
> process to handle
> IMMR registers accessing.
>
> I agree Kumar on this.
> Adding the read back with load is needing for the LAW window changing.
> And something like sync/eieio instruction also need to be added between
> stw and lwz...
> to have a proper order accessing.

Then please drum up a patch. ATM there is a lot of comments on how it
should be done by various freescale people but no patch. I am not
entering this rat nest again.

   Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

[Responding here rather than directly to Dave since his e-mail showed
up blank here for some reason]

On Mon, 15 Nov 2010 18:30:46 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> >
> > > The experts found an issue within init code and it looks like a proper
> > > patch will be added to mainline shortly.
> > > The discussion of the proper fix is right in this thread ...
> >
> > It should be timing issue in the SoC, software did not have a proper
> > process to handle
> > IMMR registers accessing.
> >
> > I agree Kumar on this.
> > Adding the read back with load is needing for the LAW window changing.
> > And something like sync/eieio instruction also need to be added between
> > stw and lwz...
> > to have a proper order accessing.

Wouldn't the fact that you're accessing the same address -- and
that it's cache inhibited -- eliminate the need for a sync instruction
between the stw and lwz?

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Liu Dave-R63238]

> Wouldn't the fact that you're accessing the same address -- and
> that it's cache inhibited -- eliminate the need for a sync instruction
> between the stw and lwz?

You are right. If st and ld the same address, e300 core have a address
collision inside.
It will make sure the order. Here we don't need the sync.

I meant we should access the IMMR register with something like I/O
accessors
to have a correct access order.

Thanks,
 Dave

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

> From: Liu Dave-R63238 <r63238@freescale.com>
> To: Andre Schwarz <andre.schwarz@matrix-vision.de>
> Cc: Wood Scott-B07421 <B07421@freescale.com>, Kumar at theia.denx.de, Tabi Timur-B04825 <B04825@freescale.com>, Phillips Kim-R1AAHA <R1AAHA@freescale.com>, Gala <galak@kernel.crashing.org>, U-Boot List <u-boot@lists.denx.de>
> Date: 2010/11/15 17:58
> Subject: Re: [U-Boot] [PATCH] mpc83xx: Make it boot again
> Sent by: u-boot-bounces at lists.denx.de
>
> > The experts found an issue within init code and it looks like a proper
> > patch will be added to mainline shortly.
> > The discussion of the proper fix is right in this thread ...
>
> It should be timing issue in the SoC, software did not have a proper
> process to handle
> IMMR registers accessing.
>
> I agree Kumar on this.
> Adding the read back with load is needing for the LAW window changing.
> And something like sync/eieio instruction also need to be added between
> stw and lwz...
> to have a proper order accessing.

After adding some more stuff in start.S I find that a lwz isn't
enough. An extra isync fixes this though

 lwz r4, LBLAWAR1(r3)
 isync

So something is missing but what? I guess isync isn't it either but
it works for now.

 Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Liu Dave-R63238]

>After adding some more stuff in start.S I find that a lwz isn't
> enough. An extra isync fixes this though
> lwz r4, LBLAWAR1(r3)
>  isync

> So something is missing but what? I guess isync isn't it either but
> it works for now.

Joakim,
Please post more code to the list to have a better understanding your situation.

Thanks,
 Dave

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

On Wed, 17 Nov 2010 17:57:53 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> > From: Liu Dave-R63238 <r63238@freescale.com>
> > To: Andre Schwarz <andre.schwarz@matrix-vision.de>
> > Cc: Wood Scott-B07421 <B07421@freescale.com>, Kumar at theia.denx.de, Tabi Timur-B04825 <B04825@freescale.com>, Phillips Kim-R1AAHA <R1AAHA@freescale.com>, Gala <galak@kernel.crashing.org>, U-Boot List <u-boot@lists.denx.de>
> > Date: 2010/11/15 17:58
> > Subject: Re: [U-Boot] [PATCH] mpc83xx: Make it boot again
> > Sent by: u-boot-bounces at lists.denx.de
> >
> > > The experts found an issue within init code and it looks like a proper
> > > patch will be added to mainline shortly.
> > > The discussion of the proper fix is right in this thread ...
> >
> > It should be timing issue in the SoC, software did not have a proper
> > process to handle
> > IMMR registers accessing.
> >
> > I agree Kumar on this.
> > Adding the read back with load is needing for the LAW window changing.
> > And something like sync/eieio instruction also need to be added between
> > stw and lwz...
> > to have a proper order accessing.
> 
> After adding some more stuff in start.S I find that a lwz isn't
> enough. An extra isync fixes this though
> 
>  lwz r4, LBLAWAR1(r3)
>  isync
> 
> So something is missing but what? I guess isync isn't it either but
> it works for now.

As I said before, the sequence we follow in the normal I/O accessors is:

lwz	r4, LBLAWAR1(r3)
twi	0, r4, 0
isync

Unless we have a good reason to deviate from that -- or a good reason
why that sequence is not necessary in general -- I think we should
use it here as well.

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 18:05:37:
>
> On Wed, 17 Nov 2010 17:57:53 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > > From: Liu Dave-R63238 <r63238@freescale.com>
> > > To: Andre Schwarz <andre.schwarz@matrix-vision.de>
> > > Cc: Wood Scott-B07421 <B07421@freescale.com>, Kumar at theia.denx.de, Tabi Timur-B04825 <B04825@freescale.com>, Phillips Kim-R1AAHA <R1AAHA@freescale.com>, Gala <galak@kernel.crashing.org>, U-Boot List <u-boot@lists.denx.de>
> > > Date: 2010/11/15 17:58
> > > Subject: Re: [U-Boot] [PATCH] mpc83xx: Make it boot again
> > > Sent by: u-boot-bounces at lists.denx.de
> > >
> > > > The experts found an issue within init code and it looks like a proper
> > > > patch will be added to mainline shortly.
> > > > The discussion of the proper fix is right in this thread ...
> > >
> > > It should be timing issue in the SoC, software did not have a proper
> > > process to handle
> > > IMMR registers accessing.
> > >
> > > I agree Kumar on this.
> > > Adding the read back with load is needing for the LAW window changing.
> > > And something like sync/eieio instruction also need to be added between
> > > stw and lwz...
> > > to have a proper order accessing.
> >
> > After adding some more stuff in start.S I find that a lwz isn't
> > enough. An extra isync fixes this though
> >
> >  lwz r4, LBLAWAR1(r3)
> >  isync
> >
> > So something is missing but what? I guess isync isn't it either but
> > it works for now.
>
> As I said before, the sequence we follow in the normal I/O accessors is:
>
> lwz   r4, LBLAWAR1(r3)
> twi   0, r4, 0
> isync

That works too. I do wonder if twi ..,r4,.. is correct though?
Even a
 lwz r4, LBLAWAR1(r3)
 nop
works.
>
> Unless we have a good reason to deviate from that -- or a good reason
> why that sequence is not necessary in general -- I think we should
> use it here as well.

Seems so. My start.S is a mess but the code added when this happened is
unrelated. It just pushes the map_xxx/remap_xxx funs further down.

      Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

On Wed, 17 Nov 2010 18:26:02 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 18:05:37:
> >
> > On Wed, 17 Nov 2010 17:57:53 +0100
> > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >
> > > After adding some more stuff in start.S I find that a lwz isn't
> > > enough. An extra isync fixes this though
> > >
> > >  lwz r4, LBLAWAR1(r3)
> > >  isync
> > >
> > > So something is missing but what? I guess isync isn't it either but
> > > it works for now.
> >
> > As I said before, the sequence we follow in the normal I/O accessors is:
> >
> > lwz   r4, LBLAWAR1(r3)
> > twi   0, r4, 0
> > isync
> 
> That works too. I do wonder if twi ..,r4,.. is correct though?
> Even a
>  lwz r4, LBLAWAR1(r3)
>  nop
> works.

Just because you can get away with something doesn't mean that it's
theoretically sufficient.  We got away with nothing at all until
recently.

The "load, conditional branch, isync" sequence is documented in the
architecture manual (1.7.1), "even if the effects of the 'dependency'
are independent of the value loaded".

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 20:03:25:
>
> On Wed, 17 Nov 2010 18:26:02 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 18:05:37:
> > >
> > > On Wed, 17 Nov 2010 17:57:53 +0100
> > > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > >
> > > > After adding some more stuff in start.S I find that a lwz isn't
> > > > enough. An extra isync fixes this though
> > > >
> > > >  lwz r4, LBLAWAR1(r3)
> > > >  isync
> > > >
> > > > So something is missing but what? I guess isync isn't it either but
> > > > it works for now.
> > >
> > > As I said before, the sequence we follow in the normal I/O accessors is:
> > >
> > > lwz   r4, LBLAWAR1(r3)
> > > twi   0, r4, 0
> > > isync
> >
> > That works too. I do wonder if twi ..,r4,.. is correct though?
> > Even a
> >  lwz r4, LBLAWAR1(r3)
> >  nop
> > works.
>
> Just because you can get away with something doesn't mean that it's
> theoretically sufficient.  We got away with nothing at all until
> recently.

Right, was just saying ..

>
> The "load, conditional branch, isync" sequence is documented in the
> architecture manual (1.7.1), "even if the effects of the 'dependency'
> are independent of the value loaded".

So it doesn't matter what address you do twi on?
Still, it would make a little more sense if
it read twi 0,r3,0

   Jocke

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Scott Wood]

On Wed, 17 Nov 2010 20:15:01 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 20:03:25:
> > The "load, conditional branch, isync" sequence is documented in the
> > architecture manual (1.7.1), "even if the effects of the 'dependency'
> > are independent of the value loaded".
> 
> So it doesn't matter what address you do twi on?
> Still, it would make a little more sense if
> it read twi 0,r3,0

It's not an address, it's data that is being compared with zero.  And it
has to be r4, since that's what you want to create the data dependency
on.

The architecture suggests a similar but slightly longer sequence (but
I don't think the wording rules out using twi), which may make the
intent clearer:

lwz	r4, ...
cmpw	r4, r4
beq	1f
1: isync

-Scott

[U-Boot] [PATCH] mpc83xx: Make it boot again

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 20:27:01:
>
> On Wed, 17 Nov 2010 20:15:01 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > Scott Wood <scottwood@freescale.com> wrote on 2010/11/17 20:03:25:
> > > The "load, conditional branch, isync" sequence is documented in the
> > > architecture manual (1.7.1), "even if the effects of the 'dependency'
> > > are independent of the value loaded".
> >
> > So it doesn't matter what address you do twi on?
> > Still, it would make a little more sense if
> > it read twi 0,r3,0
>
> It's not an address, it's data that is being compared with zero.  And it
> has to be r4, since that's what you want to create the data dependency
> on.

aha, should have looked closer on twi.

>
> The architecture suggests a similar but slightly longer sequence (but
> I don't think the wording rules out using twi), which may make the
> intent clearer:
>
> lwz   r4, ...
> cmpw   r4, r4
> beq   1f
> 1: isync

I hope so because this is a lot of fuzz for something so
simple, especially as the space is already GUARDED and no cache.

 Jocke