[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Tolunay Orkun]

This patch (replaces patch submitted via DNX#2006110942000016):

   * Adds support for AMD command set Top Boot flash geometry reversal
   * Adds support for reading JEDEC Manufacturer ID and Device ID
   * Adds support for displaying command set, manufacturer id and
     device ids (flinfo)
   * Makes flinfo output to be consistent when CFG_FLASH_EMPTY_INFO defined
   * Removes outdated change history (refer to git log instead)

Signed-off-by: Tolunay Orkun <listmember@orkun.us>

---

drivers/cfi_flash.c |  207 +++++++++++++++++++++++++++++++++++++--------------
include/flash.h     |    8 ++
2 files changed, 159 insertions(+), 56 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_driver.patch
Type: text/x-patch
Size: 13011 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061109/c17689fc/attachment.bin 

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Stefan Roese]

Hi Tolunay,

On Friday 10 November 2006 00:46, Tolunay Orkun wrote:
> This patch (replaces patch submitted via DNX#2006110942000016):
>
>    * Adds support for AMD command set Top Boot flash geometry reversal
>    * Adds support for reading JEDEC Manufacturer ID and Device ID
>    * Adds support for displaying command set, manufacturer id and
>      device ids (flinfo)
>    * Makes flinfo output to be consistent when CFG_FLASH_EMPTY_INFO defined
>    * Removes outdated change history (refer to git log instead)

I tied you patch one a board with a ST M29W160ET FLASH, that couldn't be 
handled by the "old" CFI driver, because of the geometry reversal. I had 
hoped your patch would have solved this issue. Unfortunately not:

=> fli

Bank # 1: CFI conformant FLASH (16 x 16)  Size: 2 MB in 35 Sectors
  AMD Standard command set, Manufacturer ID: 0x05, Device ID: 0x56
  Erase timeout: 8192 ms, write timeout: 1 ms

  Sector Start Addresses:
  FFE00000        FFE04000        FFE06000        FFE08000        FFE10000
  FFE20000 E      FFE30000 E      FFE40000 E      FFE50000 E      FFE60000 E
  FFE70000 E      FFE80000 E      FFE90000 E      FFEA0000 E      FFEB0000 E
  FFEC0000 E      FFED0000 E      FFEE0000 E      FFEF0000 E      FFF00000 E
  FFF10000 E      FFF20000 E      FFF30000 E      FFF40000 E      FFF50000 E
  FFF60000 E      FFF70000 E      FFF80000 E      FFF90000 E      FFFA0000   
RO
  FFFB0000   RO   FFFC0000   RO   FFFD0000   RO   FFFE0000   RO   FFFF0000   
RO


As you may notice, even the ID's are not correct (0020 and 22c4 are correct) 
and the geometry is not correct (bottom instead of top).

I didn't look into this deeply and since you have the insight right now (;-)), 
perhaps you have an idea what is going wrong here.

Thanks a lot.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Tolunay Orkun]

Stefan,

Stefan Roese wrote:
> Hi Tolunay,
>
> On Friday 10 November 2006 00:46, Tolunay Orkun wrote:
>   
>> This patch (replaces patch submitted via DNX#2006110942000016):
>>
>>    * Adds support for AMD command set Top Boot flash geometry reversal
>>    * Adds support for reading JEDEC Manufacturer ID and Device ID
>>    * Adds support for displaying command set, manufacturer id and
>>      device ids (flinfo)
>>    * Makes flinfo output to be consistent when CFG_FLASH_EMPTY_INFO defined
>>    * Removes outdated change history (refer to git log instead)
>>     
>
> I tied you patch one a board with a ST M29W160ET FLASH, that couldn't be 
> handled by the "old" CFI driver, because of the geometry reversal. I had 
> hoped your patch would have solved this issue. Unfortunately not:
>
> => fli
>
> Bank # 1: CFI conformant FLASH (16 x 16)  Size: 2 MB in 35 Sectors
>   AMD Standard command set, Manufacturer ID: 0x05, Device ID: 0x56
>   Erase timeout: 8192 ms, write timeout: 1 ms
>
>   Sector Start Addresses:
>   FFE00000        FFE04000        FFE06000        FFE08000        FFE10000
>   FFE20000 E      FFE30000 E      FFE40000 E      FFE50000 E      FFE60000 E
>   FFE70000 E      FFE80000 E      FFE90000 E      FFEA0000 E      FFEB0000 E
>   FFEC0000 E      FFED0000 E      FFEE0000 E      FFEF0000 E      FFF00000 E
>   FFF10000 E      FFF20000 E      FFF30000 E      FFF40000 E      FFF50000 E
>   FFF60000 E      FFF70000 E      FFF80000 E      FFF90000 E      FFFA0000   
> RO
>   FFFB0000   RO   FFFC0000   RO   FFFD0000   RO   FFFE0000   RO   FFFF0000   
> RO
>
>
> As you may notice, even the ID's are not correct (0020 and 22c4 are correct) 
> and the geometry is not correct (bottom instead of top).
>   
The DeviceID detection seems still broken for AMD at this moment. I 
might be reading the actual data or possibly wrong offsets. The 
Manufacturer ID should be 0x20 and Device Id should be displayed as 0xC4 
(I might add code to get that 0x22 but if a word/byte device is in byte 
mode 0x22 part is not available)

I need your help. Can you compile cfi_flash.c with DEBUG and capture the 
output as U-Boot boots. Also, the output of following commands please.

mw.w ffe00555 00aa
mw.w ffe002aa 0055
mw.w ffe00555 0090
md ffe00000
mw.w ffe00000 00f0

Even if detection is right we might fail sometimes with CFI version 1.0 
which your ST part is according to datasheet. But in your case if we can 
identify the device id as 0xC4 it would be designated as top boot 
(because msb is set) and it would work. The bottom boot version of the 
device would have device id 0x49 [By the way device ids have odd parity 
according to MTD code]

We might need to add specific device IDs to the driver as exceptions for 
some CFI 1.0 cases. Linux driver did not have a good solution for this 
either. But for your case as soon as device id algorithm is fixed it 
should work fine.

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Timur Tabi]

Stefan Roese wrote:

> As you may notice, even the ID's are not correct (0020 and 22c4 are correct) 
> and the geometry is not correct (bottom instead of top).

It looks like the problem with the IDs (I have them too) is that 
flash_read_jedec_ids() is broken.  After sending the commands, the function 
just reads the regular data instead of the command reply.

I'm reading the CFI spec, and I see this:

"The software must write a 90h to the first location in the device. If the
device returns a Manufacturer?s ID and Component ID, the flash device may be 
accessed as it has been in the past, based on the Manufacturer and Component 
ID. If the device does not return a Manufacturer and Component ID, then the 
device is not a flash memory and other routines are necessary to determine 
what type of device is installed."

The address that flash_read_jedec_ids() uses is fe000000, which is definitely 
the first byte of flash, so I don't know how it can claim that it isn't.  It 
gets even more screwy.  According to the flowchart in appendix A1, after 
writing 90h to fe000000, you're supposed to read back a 90h.  Well, I get a 
4h, which is what's normally there.  I presume these reads and writes only 
work if the memory hasn't been erased?  Because then, if I write anything to 
fe000000, I better get back the same value.

But isn't it possible to write to unerased flash anyway?  Erasing just changes 
all the zeros to ones, and writing can only change a 1 to a 0, so if I write a 
90h to a flash location, I should get back, 0, 90h, 80h, or 10h, right?

The flowchart says that if you don't get back 90h, you're supposed to restore 
the previous value.  But how can you restore it without erasing the entire sector?

Is it possible that my AMD flash chips just don't support the JEDEC interface 
at all, and only CFI?  Does the even make sense?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Timur Tabi]

Timur Tabi wrote:
> Stefan Roese wrote:
> 
>> As you may notice, even the ID's are not correct (0020 and 22c4 are correct) 
>> and the geometry is not correct (bottom instead of top).
> 
> It looks like the problem with the IDs (I have them too) is that 
> flash_read_jedec_ids() is broken.  After sending the commands, the function 
> just reads the regular data instead of the command reply.

Well, I think I fixed it.

In flash_read_jedec_ids(), make this change:

-               flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
+               flash_write_cmd(info, 0, AMD_ADDR_START, FLASH_CMD_READ_ID);

I got the value of AMD_ADDR_START from the MX29LV640BT/BB reference manual, 
which says that the third bus cycle should be a write of 90h to 555 or AAA, 
depending on the width.

Now when I run flinfo, I get this:

Bank # 1: CFI conformant FLASH (16 x 16)  Size: 8 MB in 135 Sectors
   AMD Standard command set, Manufacturer ID: 0xC2, Device ID: 0xC9
   Erase timeout: 16384 ms, write timeout: 1 ms

If this fix is real, then it means that flash_read_jedec_ids() never worked 
for any AMD part.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Tolunay Orkun]

Timur Tabi wrote:
> Timur Tabi wrote:
>> Stefan Roese wrote:
>>
>>> As you may notice, even the ID's are not correct (0020 and 22c4 are 
>>> correct) and the geometry is not correct (bottom instead of top).
>>
>> It looks like the problem with the IDs (I have them too) is that 
>> flash_read_jedec_ids() is broken.  After sending the commands, the 
>> function just reads the regular data instead of the command reply.
>
> Well, I think I fixed it.

Great :)
>
> In flash_read_jedec_ids(), make this change:
>
> -               flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
> +               flash_write_cmd(info, 0, AMD_ADDR_START, 
> FLASH_CMD_READ_ID);

I think this change is what was missing which I overlooked. Intel does 
not need a specific address to write the command nor unlock sequence.
>
> I got the value of AMD_ADDR_START from the MX29LV640BT/BB reference 
> manual, which says that the third bus cycle should be a write of 90h 
> to 555 or AAA, depending on the width.
The documentation on this matter is a bit scattered. Bits and pieces are 
here and there. It was educational to me as well.

>
> Now when I run flinfo, I get this:
>
> Bank # 1: CFI conformant FLASH (16 x 16)  Size: 8 MB in 135 Sectors
>   AMD Standard command set, Manufacturer ID: 0xC2, Device ID: 0xC9
>   Erase timeout: 16384 ms, write timeout: 1 ms
>
> If this fix is real, then it means that flash_read_jedec_ids() never 
> worked for any AMD part.
>
There was no flash_read_jedec_ids() before I sent the last patch. I 
wrote it from scratch and the bug there belongs to me :) Since, I did 
not have AMD style flash myself was not able to test that path and that 
is why I was asking you to collect debug data . It is hard to prepare 
patches for an issue that you do not have in house :) Anyway, I guess we 
will not need the debug data now.

I will update the patch tonight and resend it yet again.

Tolunay

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Stefan Roese]

Hi Tolunay,

On Sunday 12 November 2006 05:42, Tolunay Orkun wrote:
> > In flash_read_jedec_ids(), make this change:
> >
> > -               flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
> > +               flash_write_cmd(info, 0, AMD_ADDR_START,
> > FLASH_CMD_READ_ID);
>
> I think this change is what was missing which I overlooked. Intel does
> not need a specific address to write the command nor unlock sequence.

And that's the reason I don't like those Intel devices. I got some devices 
accidentally screwed in a project once.

> > Now when I run flinfo, I get this:
> >
> > Bank # 1: CFI conformant FLASH (16 x 16)  Size: 8 MB in 135 Sectors
> >   AMD Standard command set, Manufacturer ID: 0xC2, Device ID: 0xC9
> >   Erase timeout: 16384 ms, write timeout: 1 ms

And I get:

=> fli

Bank # 1: CFI conformant FLASH (16 x 16)  Size: 2 MB in 35 Sectors
  AMD Standard command set, Manufacturer ID: 0x20, Device ID: 0xC4
  Erase timeout: 8192 ms, write timeout: 1 ms

  Sector Start Addresses:
  FFE00000        FFE10000        FFE20000 E      FFE30000 E      FFE40000 E
  FFE50000 E      FFE60000 E      FFE70000 E      FFE80000 E      FFE90000 E
  FFEA0000 E      FFEB0000 E      FFEC0000 E      FFED0000 E      FFEE0000 E
  FFEF0000 E      FFF00000 E      FFF10000 E      FFF20000 E      FFF30000 E
  FFF40000 E      FFF50000 E      FFF60000 E      FFF70000 E      FFF80000 E
  FFF90000 E      FFFA0000   RO   FFFB0000   RO   FFFC0000   RO   FFFD0000   
RO
  FFFE0000   RO   FFFF0000   RO   FFFF8000 E      FFFFA000 E      FFFFC000

So FLASH ID's are now read back correctly.

> There was no flash_read_jedec_ids() before I sent the last patch. I
> wrote it from scratch and the bug there belongs to me :)

;-)

> Since, I did 
> not have AMD style flash myself was not able to test that path and that
> is why I was asking you to collect debug data . It is hard to prepare
> patches for an issue that you do not have in house :)

As usual the rule applies: "never tested, never running". Thanks a lot for 
even trying to fix a patch for this without having the hardware. :)

> I will update the patch tonight and resend it yet again.

Not necessary. I'll forward this patch with the fix to Wolfgang. Thanks again.

And when it's included we can continue to extend the code with additional 
features, like fixing this ST FLASH geometry reversal (or others) by 
identifying it's device ID.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Tolunay Orkun]

Stefan Roese wrote:

>>> -               flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
>>> +               flash_write_cmd(info, 0, AMD_ADDR_START,
>>> FLASH_CMD_READ_ID);
>> I think this change is what was missing which I overlooked. Intel does
>> not need a specific address to write the command nor unlock sequence.
> 
> And that's the reason I don't like those Intel devices. I got some devices 
> accidentally screwed in a project once.

I hear you. I was not talking about merits.

> So FLASH ID's are now read back correctly.

Great. Your flash device should validate CFI = 1.0 code path for geometry 
reversal detection as well.

>> I will update the patch tonight and resend it yet again.
> 
> Not necessary. I'll forward this patch with the fix to Wolfgang. Thanks again.

I did it anyway [DNX#2006111242000019]. Plus a few more minor changes: I've 
added my copyright line. Added a few more comments and removed some other 
change history comments included with previous copyright lines. Copyrights 
are still there as it should be.

I prefer if you can use this patch if it is not too late.

> And when it's included we can continue to extend the code with additional 
> features, like fixing this ST FLASH geometry reversal (or others) by 
> identifying it's device ID.

It looks the sector addresses you included (below) is now that of top boot. 
Is it not working for you? When I enabled DEBUG the flash driver would have 
trouble with writes even for Intel  (possibily clearing the status register 
prematurely). Make sure you test with DEBUG disabled for cp command.

> Sector Start Addresses:
> FFE00000        FFE10000        FFE20000 E      FFE30000 E      FFE40000 E
> FFE50000 E      FFE60000 E      FFE70000 E      FFE80000 E      FFE90000 E
> FFEA0000 E      FFEB0000 E      FFEC0000 E      FFED0000 E      FFEE0000 E
> FFEF0000 E      FFF00000 E      FFF10000 E      FFF20000 E      FFF30000 E
> FFF40000 E      FFF50000 E      FFF60000 E      FFF70000 E      FFF80000 E
> FFF90000 E      FFFA0000   RO   FFFB0000   RO   FFFC0000   RO   FFFD0000   RO
> FFFE0000   RO   FFFF0000   RO   FFFF8000 E      FFFFA000 E      FFFFC000

I think should probably re-org the code sometime to be more table driven. We 
can avoid testing for vendor everywhere and make code much easier to read 
and extend. If a vendor is sticking to AMD style chips we could also have 
option to exclude the Intel support (or vice versa) completely to save some 
space that way.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Stefan Roese]

Hi Tolunay,

On Sunday 12 November 2006 23:04, Tolunay Orkun wrote:
> I did it anyway [DNX#2006111242000019]. Plus a few more minor changes: I've
> added my copyright line. Added a few more comments and removed some other
> change history comments included with previous copyright lines. Copyrights
> are still there as it should be.
>
> I prefer if you can use this patch if it is not too late.

No problem. The updated patch is on it's way...

> > And when it's included we can continue to extend the code with additional
> > features, like fixing this ST FLASH geometry reversal (or others) by
> > identifying it's device ID.
>
> It looks the sector addresses you included (below) is now that of top boot.
> Is it not working for you?

Yes, you're right of course. I didn't notice it. Sorry for the noise. It's 
working now (also tested with the new patch). Thanks again.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI driver AMD Command Set Top boot geometry reversal, etc. [Updated]

[Timur Tabi]

Tolunay Orkun wrote:

> There was no flash_read_jedec_ids() before I sent the last patch.

Ah, that function wasn't in your first patch, and for some reason, I didn't 
think to check your most recent patch.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale