[U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.

[Haiying Wang <r54964@freescale.com>]

> Except for "vendor" everything looks OK here. I should have asked you to 
> print vendor right before flash_read_jedec_ids() call. It was not yet 
> initialized when printed. It must be correctly set as well since 
> flash_read_jedec_ids() uses it would not have returned correct data 
> otherwise.
My fault, vendor is 2 when I put the printf to the correct place.:-)

> You have a 16-bit flash accessible on 16-bit bus. The flash manufacturer 
> is "AMD/Spansion" or compatible and device id is 7E1301 which is correct 
> for your part:
> 
> http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/25538a1.pdf
> 
> Please note that flash_read_jedec_ids() is used for manufacturer id and 
> device_ids and this function in turn is using flash_read_uchar() which 
> worked just fine for you.
Thanks.

> > 124 erase regions found, only 4 used
> 
> This is not correct. This is why you are blaming the flash_read_uchar() 
> function. But since it worked correctly before the failure to read the 
> number of erase regions is probably not due to the function.
I agree with you that the failure is not due to the function
(flash_read_uchar()). We thought before that it was better to modify
this function so that flash with different port-width could be accessed
safer.

> You demonstrated that this did not affect you because the output was the 
> same. The problem with this line was that info->cmd_reset was not yet 
> initialized when flash_detect_cfi() was called. This is a minor bug 
> since the flash was not in query mode yet. I think we can safely remove 
> this call as well or replace it both amd anf intel reset commands back 
> to back. But anyway, back to your issue.
Understood.:)

> It looks like the rest of the commands worked as well :) All without 
> changing flash_read_uchar() functionality!
Yes. But in our previous tests, without adding code in between
flash_write_cmd() and flash_read_uchar()(for num_erase_region), just
adding some code elsewhere, the flash worked sometimes and failed
sometimes. Another case is that, one of our customer pointed that on
their test, flash could work with ELDK 1.3.0 and failed at ELDK 1.4.0. I
just suspect that the different compiler may have difference on
optimization thus affected the instruction execution sequence.

> At this point, all data indicates that flash_read_uchar() is just fine.
> 
> I am suspecting two things:
> 
> 1) Your CPU might be trying to re-order writes and reads to optimize 
> performance. Since the command write and data read are from different 
> addresses it could do this but the write command should really finish 
> before we access the table data. So, we might need to add powerpc "sync" 
> instructions in flash_write_cmd() function. Apparently CONFIG_BLACKFIN 
> has a similar need as well. I would add a generic "sync" for the whole 
> PowerPC family but there is not a conveninent CONFIG_POWERPC macro as 
> far as I can see. Anyway, try adding:
> 
> asm("sync;");
> 
> statements in that function. Use Blackfin as an example.
> 
> 2) Because you have a rather fast CPU it is possible that we are not 
> allowing enough time after reset command is executed before array is in 
> read mode. According to the datasheet of your flash part if external 
> reset signal was asserted you could need up to 20usec before flash 
> returns to read mode. I am not sure if this delay is needed for issued 
> reset commands as well. We might need to add some delay after flash 
> reset commands. Try adding udelay(100); right after flash reset command 
> is sent. You can locate flash reset commands by searching 
> flash_write_cmd() statements that passes FLASH_CMD_RESET, AMD_CMD_RESET 
> or cfi->cmd_reset as the last argument.
Your suggestions are very valuable. I did think about the flash response
time for reset command, but did not figure out where and why was
reasonable.I am very appreciated for your kind help on this. We will try
both suggestions and do enough tests before sending out a new patch for
review.:-)
 
> I hope this helps.
Many thanks again.

Haiying