| Message ID | 1291163785-24443-1-git-send-email-timur@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
>> /* 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
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? >
> > 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
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.
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
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?
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
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()?
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)
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(-)