[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Haiying Wang]

Add sync at the end of flash_write_cmd for MPC86xx
to ensure write command are really finished before reading data

Signed-off-by: Haiying Wang <haiying.wang@freescale.com>
---
According to Tolunay's first suggestion, this patch for cfi_flash
driver fixed the flash operation error on MPC8641HPCN board. Since there
is not such flash error reported on 85xx, 83xx platform and as Tolunay
said, there is not a PowerPC macro to define, I just use CONFIG_MPC86xx
for now. I also tested his second suggestion(adding udelay directly
after reset command), but flash still failed to work. 

Based on my tests before (I added udelay after the flash_write_cmd()
which switched flash to CFI read mode, and it worked as well) and the
test with this patch, I think that most possible fix of this issue should
be that we should make sure command has been fully executed before
return to data read mode(as in his first suggestion). So please review
this patch below:

 drivers/cfi_flash.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index 696f9a4..a93e94d 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -971,6 +971,9 @@ static void flash_write_cmd (flash_info_t * info, flash_sect_t sect, uint offset
 #endif
 		break;
 	}
+#ifdef CONFIG_MPC86xx
+	asm("sync;");
+#endif
 }
 
 static void flash_unlock_seq (flash_info_t * info, flash_sect_t sect)

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Andrew Dyer]

On 2/5/07, Haiying Wang <r54964@freescale.com> wrote:
> Add sync at the end of flash_write_cmd for MPC86xx
> to ensure write command are really finished before reading data
>
> Signed-off-by: Haiying Wang <haiying.wang@freescale.com>
> ---
> According to Tolunay's first suggestion, this patch for cfi_flash
> driver fixed the flash operation error on MPC8641HPCN board. Since there
> is not such flash error reported on 85xx, 83xx platform and as Tolunay
> said, there is not a PowerPC macro to define, I just use CONFIG_MPC86xx
> for now. I also tested his second suggestion(adding udelay directly
> after reset command), but flash still failed to work.

Given that this is already looks to have been an issue on blackfin and
now PPC (and probably some MIPS also), I would suggest it's better to
define a macro in the CPU definition for your arch and blackfin and do
a generic test like so:

#ifdef SYNC
    SYNC;
#endif

Maybe you can come up with a more descriptive name than SYNC :-)

I had e-mailed Tolunay about this and he concurred.

-- 
Hardware, n.:
        The parts of a computer system that can be kicked.

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Haavard Skinnemoen]

On 2/5/07, Andrew Dyer <amdyer@gmail.com> wrote:
> Given that this is already looks to have been an issue on blackfin and
> now PPC (and probably some MIPS also), I would suggest it's better to
> define a macro in the CPU definition for your arch and blackfin and do
> a generic test like so:
>
> #ifdef SYNC
>     SYNC;
> #endif

Better to drop the #ifdef and just add empty stubs for the arches that
don't need it.

Haavard

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Stefan Roese]

On Monday 05 February 2007 22:11, Haavard Skinnemoen wrote:
> > #ifdef SYNC
> >     SYNC;
> > #endif
>
> Better to drop the #ifdef and just add empty stubs for the arches that
> don't need it.

Yes, please do it this way.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
=====================================================================

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Tolunay Orkun]

Stefan Roese wrote:
> On Monday 05 February 2007 22:11, Haavard Skinnemoen wrote:
>>> #ifdef SYNC
>>>     SYNC;
>>> #endif
>> Better to drop the #ifdef and just add empty stubs for the arches that
>> don't need it.
> 
> Yes, please do it this way.

I think defining the SYNC macro in architecture files is probably better 
but #ifdef wrapper would avoid touching architectures that does not have 
SYNC concept.

I guess at this moment we need a real (not a stub) SYNC for all PowerPC 
variants and Blackfin. I do not know if other architectures supported by 
U-Boot have such instructions in instruction set. So, it is probably the 
best time for those architecture people to speak up.

I think this patch should be independent than the cfi_flash.c patch. 
cfi_flash.c patch will depend on this patch applied first.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Haavard Skinnemoen]

On 2/6/07, Tolunay Orkun <listmember@orkun.us> wrote:
> I guess at this moment we need a real (not a stub) SYNC for all PowerPC
> variants and Blackfin. I do not know if other architectures supported by
> U-Boot have such instructions in instruction set. So, it is probably the
> best time for those architecture people to speak up.

AVR32 does have a sync instruction, but I don't think it's needed
here. Please just add a stub and I'll patch in a real implementation
later if it turns out to be necessary.

Haavard

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Haiying Wang]

> Given that this is already looks to have been an issue on blackfin and
> now PPC (and probably some MIPS also), I would suggest it's better to
> define a macro in the CPU definition for your arch and blackfin and do
> a generic test like so:
> 
> #ifdef SYNC
>     SYNC;
> #endif
> 
> Maybe you can come up with a more descriptive name than SYNC :-)
> 
> I had e-mailed Tolunay about this and he concurred.
> 
It sounds good to me:-). So, should I patch my arch after your suggested
patch is applied?

Haiying

[U-Boot-Users] [PATCH] Add sync at the end of flash_write_cmd for MPC86xx

[Tolunay Orkun]

Haiying Wang wrote:

> diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
> index 696f9a4..a93e94d 100644
> --- a/drivers/cfi_flash.c
> +++ b/drivers/cfi_flash.c
> @@ -971,6 +971,9 @@ static void flash_write_cmd (flash_info_t * info, flash_sect_t sect, uint offset
>  #endif
>  		break;
>  	}
> +#ifdef CONFIG_MPC86xx
> +	asm("sync;");
> +#endif
>  }
>  
>  static void flash_unlock_seq (flash_info_t * info, flash_sect_t sect)

I think for consistency of the code, you should either relocate blackfin 
sync instruction to the end or add MPC86xx sync instructions to each 
case just like blackfin does.

Having addressed that, I think this patch would be OK for me except that 
as you have probably seen the discussion in the list, it will be better 
to define SYNC macros in cpu architecture files and call SYNC; instead 
fo asm("sync;");

So, please create an independent patch that adds SYNC to all CPU 
architectures (dummy stub if architecture does not have such feature) 
and submit that patch first. Refer to other email for details. Right now 
all PowerPC variants and Blackfin needs this but more might come.

Having submitted that patch, please modify this patch about to use SYNC; 
and submit as well.

I know it is probably a bit more work than you wished for but we should 
do this the right way.

Tolunay