Hi steven,

2013/5/27 Steven Zakulec <spzakulec@gmail.com>

Before I get started on the review, just a general comment or two.

If you plan to contribute in the future (which I certainly hope you will), please break submissions down into blocks of 5-10 chips.  This makes it so much easier for me, and any other reviewers, to quickly check the patch and provide feedback in a timely manner.

Sorry, this is my first patch. My next patch i wrote with you wish. (http://patchwork.coreboot.org/patch/3896/) 8)

 

In the last review, I mentioned the Numonyx M45PE series (10, 20, 40, 80, 16), should only have
+        .feature_bits    = FEATURE_WRSR_WREN,

set when the command is there.  It appears I may have been unclear there, because instead of removing just the WREN flag, you removed the whole line (the chips do have a Write Status Register- WRSR).

Continuing the review from Numonyx M45PE16.

Sorry i don't know which flag must be used. You right these chips have a only WREN instruction, and i think if these chips doesn't have a feature that i exclude feature_bits. Maybe we will be able to specify the details.

Numonyx M25PX80-
+        .voltage    = {2700, 3600},

The datasheets I've seen have it as 2.3-3.6V (rev 4 Dec 2008 and Rev B 3/2013)

+        .tested        = TEST_OK_PREW,

Unless I'm reading it wrong, it looks like you're adding this chip as a fully tested one- would you mind supplying some logs showing that it successfully does these operations? (write logs shouldn't contain all :S (for skipped blocks) next to the addresses).

You right this is my typos, and i fixed them.

 

Numonyx N25Q00AA13-

I'll admit I don't really know how this chip works, but it seems there should be a 3rd eraseblock from the description of the memory organization (datasheet is Rev E, 2/12):

Memory Configuration and Block Diagram
The memory is a stacked device comprised of four 256Mb chips. Each chip is internally
partitioned into two 128Mb segments. Each page of memory can be individually pro-
grammed. Bits are programmed from one through zero. The device is subsector, sector,
or single 256Mb chip erasable, but not page-erasable. Bits are erased from zero through
one. The memory is configured as 134,217,728 bytes (8 bits each); 2048 sectors (64KB
each); 32,768 subsectors (4KB each); and 524,288 pages (256 bytes each); and 64 OTP
bytes are located outside the main memory array.

You've got:
+                .eraseblocks = { {4 * 1024, 32768 } },
+               .block_erase = spi_block_erase_20,
+            }, {
+                .eraseblocks = { {64 * 1024, 2048 } },
+               .block_erase = spi_block_erase_d8,

There's a die erase opcode, which I am unfamiliar with- maybe it handles erasing a single 256Mb chip at a time?

 

You right this chip used a four 256 Mb chips. And 256 Mb chip has a three instruction subsector 20(hex) sector D8(hex) bulk C7(hex) erase commands. But chip N25Q00AA13 has a other op-code to initiate die erase command C4 (hex)

Sanyo LF25FW203A / LE25FW203A-

Thanks so much for getting to the bottom of this chip, and figuring out which chip it actually is.

         .probe        = probe_spi_rdid,

Maybe I'm reading the patch wrong, but you appear to be missing the .probe_timing line for this chip.

You reading patch wrong.  The probe_timing line for this chip is present below in source code.

LE25FW418A, LE25FW808  and LE25FW806 -

For the LE25FW806A you included the small sector erase, but left it out for the rest of the chips- any particular reason for this?

Because only LE25FW806A has a small sector erase op-code instruction, other chips didn't have it.

I'll cover from Spansion S25FL128S to the end tomorrow.

Excellent  

how to delete all versions of the patch and leave only comments in one thread?

I send a new version patch

--

With best regards Nikolay Nikolaev

С Уважением Николаев Николай