[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Timur Tabi]

On the P1022, the pins which drive the video display (DIU) are muxed with the
local bus controller (LBC), so if the DIU is active, the pins need to be
temporarily muxed to LBC whenever accessing NOR flash.

The code which handled this transition is checking and changing the wrong
bits in PMUXCR.

Also add a follow-up read after a write to NOR flash if we're going to
mux back to DIU after the write, as described in the P1022 RM.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

I have no idea how this ever worked before.  It's a complete mystery to me.

 board/freescale/p1022ds/diu.c |   45 +++++++++++++++++++++++++++++++++-------
 1 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/board/freescale/p1022ds/diu.c b/board/freescale/p1022ds/diu.c
index 12b40a0..2259384 100644
--- a/board/freescale/p1022ds/diu.c
+++ b/board/freescale/p1022ds/diu.c
@@ -32,6 +32,7 @@
 
 #define PMUXCR_ELBCDIU_MASK	0xc0000000
 #define PMUXCR_ELBCDIU_NOR16	0x80000000
+#define PMUXCR_ELBCDIU_DIU	0x40000000
 
 /*
  * DIU Area Descriptor
@@ -131,9 +132,8 @@ int platform_diu_init(unsigned int *xres, unsigned int *yres)
 	px_brdcfg0 = in_8(lbc_lcs1_ba);
 	out_8(lbc_lcs1_ba, px_brdcfg0 | PX_BRDCFG0_ELBC_DIU);
 
-	/* Setting PMUXCR to switch to DVI from ELBC */
-	clrsetbits_be32(&gur->pmuxcr,
-		PMUXCR_ELBCDIU_MASK, PMUXCR_ELBCDIU_NOR16);
+	/* Set PMUXCR to switch the muxed pins from the LBC to the DIU */
+	clrsetbits_be32(&gur->pmuxcr, PMUXCR_ELBCDIU_MASK, PMUXCR_ELBCDIU_DIU);
 	pmuxcr = in_be32(&gur->pmuxcr);
 
 	return fsl_diu_init(*xres, pixel_format, 0);
@@ -161,7 +161,7 @@ static int set_mux_to_lbc(void)
 	ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
 
 	/* Switch the muxes only if they're currently set to DIU mode */
-	if ((in_be32(&gur->pmuxcr) & PMUXCR_ELBCDIU_MASK) ==
+	if ((in_be32(&gur->pmuxcr) & PMUXCR_ELBCDIU_MASK) !=
 	    PMUXCR_ELBCDIU_NOR16) {
 		/*
 		 * In DIU mode, the PIXIS can only be accessed indirectly
@@ -216,8 +216,16 @@ void flash_write8(u8 value, void *addr)
 	int sw = set_mux_to_lbc();
 
 	__raw_writeb(value, addr);
-	if (sw)
+
+	if (sw) {
+		/*
+		 * To ensure the post-write is completed to eLBC, software must
+		 * perform a dummy read from one valid address from eLBC space
+		 * before changing the eLBC_DIU from NOR mode to DIU mode.
+		 */
+		__raw_readb(addr);
 		set_mux_to_diu();
+	}
 }
 
 void flash_write16(u16 value, void *addr)
@@ -225,8 +233,15 @@ void flash_write16(u16 value, void *addr)
 	int sw = set_mux_to_lbc();
 
 	__raw_writew(value, addr);
-	if (sw)
+	if (sw) {
+		/*
+		 * To ensure the post-write is completed to eLBC, software must
+		 * perform a dummy read from one valid address from eLBC space
+		 * before changing the eLBC_DIU from NOR mode to DIU mode.
+		 */
+		__raw_readb(addr);
 		set_mux_to_diu();
+	}
 }
 
 void flash_write32(u32 value, void *addr)
@@ -234,8 +249,15 @@ void flash_write32(u32 value, void *addr)
 	int sw = set_mux_to_lbc();
 
 	__raw_writel(value, addr);
-	if (sw)
+	if (sw) {
+		/*
+		 * To ensure the post-write is completed to eLBC, software must
+		 * perform a dummy read from one valid address from eLBC space
+		 * before changing the eLBC_DIU from NOR mode to DIU mode.
+		 */
+		__raw_readb(addr);
 		set_mux_to_diu();
+	}
 }
 
 void flash_write64(u64 value, void *addr)
@@ -244,8 +266,15 @@ void flash_write64(u64 value, void *addr)
 
 	/* There is no __raw_writeq(), so do the write manually */
 	*(volatile u64 *)addr = value;
-	if (sw)
+	if (sw) {
+		/*
+		 * To ensure the post-write is completed to eLBC, software must
+		 * perform a dummy read from one valid address from eLBC space
+		 * before changing the eLBC_DIU from NOR mode to DIU mode.
+		 */
+		__raw_readb(addr);
 		set_mux_to_diu();
+	}
 }
 
 u8 flash_read8(void *addr)
-- 
1.7.2.3

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Scott Wood]

On Tue, 30 Nov 2010 18:36:25 -0600
Timur Tabi <timur@freescale.com> wrote:

> @@ -244,8 +266,15 @@ void flash_write64(u64 value, void *addr)
>  
>  	/* There is no __raw_writeq(), so do the write manually */
>  	*(volatile u64 *)addr = value;
> -	if (sw)
> +	if (sw) {
> +		/*
> +		 * To ensure the post-write is completed to eLBC, software must
> +		 * perform a dummy read from one valid address from eLBC space
> +		 * before changing the eLBC_DIU from NOR mode to DIU mode.
> +		 */
> +		__raw_readb(addr);
>  		set_mux_to_diu();
> +	}

Careful with the barriers.

You've got a raw readback, which means it's not going to wait for
completion with the twi/isync hack.

Ordinarily that would be OK, since you only need ordering between the
readb and the first access in set_mux_to_diu().  Unfortunately, that
first access is an 8-bit access, which for some strange reason does
sync differently than 16/32-bit accesses.  The latter do sync+write,
but 8-bit does write+eieio.  So there's no barrier between the read
and the write.

-Scott

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Kumar Gala]

On Nov 30, 2010, at 7:50 PM, Scott Wood wrote:

> On Tue, 30 Nov 2010 18:36:25 -0600
> Timur Tabi <timur@freescale.com> wrote:
> 
>> @@ -244,8 +266,15 @@ void flash_write64(u64 value, void *addr)
>> 
>> 	/* There is no __raw_writeq(), so do the write manually */
>> 	*(volatile u64 *)addr = value;
>> -	if (sw)
>> +	if (sw) {
>> +		/*
>> +		 * To ensure the post-write is completed to eLBC, software must
>> +		 * perform a dummy read from one valid address from eLBC space
>> +		 * before changing the eLBC_DIU from NOR mode to DIU mode.
>> +		 */
>> +		__raw_readb(addr);
>> 		set_mux_to_diu();
>> +	}
> 
> Careful with the barriers.
> 
> You've got a raw readback, which means it's not going to wait for
> completion with the twi/isync hack.
> 
> Ordinarily that would be OK, since you only need ordering between the
> readb and the first access in set_mux_to_diu().  Unfortunately, that
> first access is an 8-bit access, which for some strange reason does
> sync differently than 16/32-bit accesses.  The latter do sync+write,
> but 8-bit does write+eieio.  So there's no barrier between the read
> and the write.

Any reason not to use in_8 here?

- k

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Liu Dave-R63238]

>>      /* There is no __raw_writeq(), so do the write manually */
>>      *(volatile u64 *)addr = value;
>> -    if (sw)
>> +    if (sw) {
>> +            /*
>> +             * To ensure the post-write is completed to eLBC, software must
>> +             * perform a dummy read from one valid address from eLBC space
>> +             * before changing the eLBC_DIU from NOR mode to DIU mode.
>> +             */
>> +            __raw_readb(addr);
>>              set_mux_to_diu();
>> +    }
>
> Careful with the barriers.
>
> You've got a raw readback, which means it's not going to wait for
> completion with the twi/isync hack.
>
> Ordinarily that would be OK, since you only need ordering between the
> readb and the first access in set_mux_to_diu().  Unfortunately, that
> first access is an 8-bit access, which for some strange reason does
> sync differently than 16/32-bit accesses.  The latter do sync+write,
> but 8-bit does write+eieio.  So there's no barrier between the read
> and the write.

Any reason not to use in_8 here?

Timur,
Any reason not to use in_be8?
The first version code is finished by me some months ago. at that time I used the in_be8.

Thanks,
 Dave

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Tabi Timur-B04825]

On Dec 1, 2010, at 7:51 AM, "Liu Dave-R63238" <r63238@freescale.com> wrote

> 
> Timur,
> Any reason not to use in_be8?
> The first version code is finished by me some months ago. at that time I used the in_be8.

What first version code?  Did you implement this feature, too?
> 

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Liu Dave-R63238]

> > Timur,
> > Any reason not to use in_be8?
> > The first version code is finished by me some months ago. at that
time I used
> the in_be8.
> 
> What first version code?  Did you implement this feature, too?

http://git.ap.freescale.net/gitweb/?p=u-boot-p1022.git;a=commitdiff;h=45
ddca32e5f288e81a444cf781c1f1a64cb1bfba;hp=e002776d6cbd647e0f410706d9aa8f
3bb96e7f8a

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Timur Tabi]

Wood Scott-B07421 wrote:
> 
> Careful with the barriers.
> 
> You've got a raw readback, which means it's not going to wait for
> completion with the twi/isync hack.

You told me that since I'm doing a read following a write to uncached memory,
that I don't need a sync.

And what twi/isync hack are you talking about?  The one in in_8?

> 
> Ordinarily that would be OK, since you only need ordering between the
> readb and the first access in set_mux_to_diu().  Unfortunately, that
> first access is an 8-bit access, which for some strange reason does
> sync differently than 16/32-bit accesses.  The latter do sync+write,
> but 8-bit does write+eieio.  So there's no barrier between the read
> and the write.

Wait, I don't understand.  Where are you getting this from?  What do you mean by
16-bit accesses does sync+write vs. write+eieio?  Where is the sync/eieio coming
from?

As for why I don't use in_8, etc, it's because I'm trying to optimize this code.
 Unlike Dave's code, this stuff needs to be as fast as possible.  Although, I
wonder if an extra sync will make a difference considering the overhead of
switching the muxes.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Scott Wood]

On Wed, 01 Dec 2010 16:45:57 -0600
Timur Tabi <timur@freescale.com> wrote:

> Wood Scott-B07421 wrote:
> > 
> > Careful with the barriers.
> > 
> > You've got a raw readback, which means it's not going to wait for
> > completion with the twi/isync hack.
> 
> You told me that since I'm doing a read following a write to uncached memory,
> that I don't need a sync.

No, I was talking about a write followed by a read, to the same location.

> And what twi/isync hack are you talking about?  The one in in_8?

Yes.

> > Ordinarily that would be OK, since you only need ordering between the
> > readb and the first access in set_mux_to_diu().  Unfortunately, that
> > first access is an 8-bit access, which for some strange reason does
> > sync differently than 16/32-bit accesses.  The latter do sync+write,
> > but 8-bit does write+eieio.  So there's no barrier between the read
> > and the write.
> 
> Wait, I don't understand.  Where are you getting this from?  What do you mean by
> 16-bit accesses does sync+write vs. write+eieio?  Where is the sync/eieio coming
> from?

Look in arch/powerpc/include/asm/io.h:

extern inline void out_8(volatile unsigned char __iomem *addr, int val)
{
	__asm__ __volatile__("stb%U0%X0 %1,%0; eieio" : "=m" (*addr) : "r" (val));
}

versus:

extern inline void out_be32(volatile unsigned __iomem *addr, int val)
{
	__asm__ __volatile__("sync; stw%U0%X0 %1,%0" : "=m" (*addr) : "r" (val));
}

> As for why I don't use in_8, etc, it's because I'm trying to optimize this code.
>  Unlike Dave's code, this stuff needs to be as fast as possible. 

It still needs to be correct.

out_8 should be fixed to behave like the other accessors.

-Scott

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Timur Tabi]

Scott Wood wrote:
>> You told me that since I'm doing a read following a write to uncached memory,
>> > that I don't need a sync.
> No, I was talking about a write followed by a read, to the same location.

That's what I said. Read following write == write followed by read.

> out_8 should be fixed to behave like the other accessors.

Ok, but I'm not using any of the I/O accessors, so this doesn't affect me.

I just need to make sure that the read is executed after the write, and that the
read completes before I continue.  So I should I put an isync between the write
and the read, and a sync after the read?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Scott Wood]

On Wed, 01 Dec 2010 17:28:28 -0600
Timur Tabi <timur@freescale.com> wrote:

> Scott Wood wrote:
> >> You told me that since I'm doing a read following a write to uncached memory,
> >> > that I don't need a sync.
> > No, I was talking about a write followed by a read, to the same location.
> 
> That's what I said. Read following write == write followed by read.

Sorry, misread.

The sync I was concerned about wasn't between that write and the
following read, but between the read and whatever comes after the read.

> > out_8 should be fixed to behave like the other accessors.
> 
> Ok, but I'm not using any of the I/O accessors, so this doesn't affect me.

Yes you are, in set_mux_to_diu().  But it's actually setbits_8(), which
will do an in_8() first, which has synchronization, so it should be OK.

> I just need to make sure that the read is executed after the write, and that the
> read completes before I continue.

Right.  It was that last bit I was talking about.

> So I should I put an isync between the write and the read, 

Not necessary since they're the same address, and wouldn't help if they
weren't (you'd want sync or mbar 1 in that case, not isync).

> and a sync after the read?

If you were to immediately follow it with out_8 as currently defined,
yes.  But setbits_8 should be OK.

-Scott

[U-Boot] [PATCH] p1022ds: fix switching of DIU/LBC signals

[Timur Tabi]

Scott Wood wrote:
> If you were to immediately follow it with out_8 as currently defined,
> yes.  But setbits_8 should be OK.

I don't want my flash_writeX functions to make any assumptions about what
set_mux_to_diu() does.  Can I just replace the __raw_readb() with an in_8()?

-- 
Timur Tabi
Linux kernel developer at Freescale