<div class="gmail_quote">Thanks for the review! I've attached an updated patch to address your comments.</div><div class="gmail_quote"><br></div><div class="gmail_quote">I'm still not sure what the best course of action is for dealing with the D16 versus Q16 evil twin scenario. Let's see if Carl-Daniel has further comments.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">New patch is:</div><div class="gmail_quote">Signed-off-by: David Hendricks <<a href="mailto:dhendrix@google.com" target="_blank">dhendrix@google.com</a>></div>
<div class="gmail_quote"><br></div><div class="gmail_quote">On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, 12 Apr 2011 12:58:35 -0700<br>
<div>David Hendricks <<a href="mailto:dhendrix@google.com" target="_blank">dhendrix@google.com</a>> wrote:<br>
<br>
</div><div>> Hello,<br>
> The attached patch adds support for the following Eon SPI flash chips:<br>
> EN25Q40, EN25Q80, EN25Q32A/B, EN25Q64, and EN25Q128<br>
><br>
> I was going to add support for the EN25Q16 as well, but found that it shares<br>
> the same ID as the EN25D16 but has different write protection capabilities.<br>
> So I added a comment instead.<br>
><br>
> I do not have real hardware to test with at the moment, so I left the status<br>
> as untested.<br>
><br>
> Datasheets available from Eon @ <a href="http://www.eonssi.com/products/products.aspx" target="_blank">http://www.eonssi.com/products/products.aspx</a><br>
><br>
<br>
</div>hello david and thanks for your patch.<br>
<br>
i can confirm that the EN25D16 shares the ID with EN25Q16.<br>
there is also a EN25QH16 but with another ID 7015h (and an upcoming<br>
EN25QH256 w/o data sheet yet).<br></blockquote><div><br></div><div>Thanks for pointing that out. I've added the EN25QH16 to the patch.</div><div><br></div><div>As we discussed on IRC, this particular chip supports "Serial Flash Discoverable Parameters" (SFDP). I tried to add support using the values listed in Table 9 of their datasheet, and left a TODO to update the probe routine later on.</div>
<div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> Index: flashchips.h<br>
> ===================================================================<br>
> --- flashchips.h (revision 1285)<br>
> +++ flashchips.h (working copy)<br>
> @@ -232,7 +232,12 @@<br>
> #define EON_EN25B64 0x2017 /* Same as P64 */<br>
> #define EON_EN25B64T 0x46<br>
> #define EON_EN25B64B 0x36<br>
> +#define EON_EN25Q40 0x3013<br>
ok<br>
> +#define EON_EN25Q80 0x3014<br>
i could only find references to a chip named EN25Q80A... either add a<br>
comment or rename to EON_EN25Q80A?<br></blockquote><div><br></div><div>Appended "A" to the name.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> #define EON_EN25D16 0x3015<br>
i think the twin's name should be noted here too.<br>
since the D is untested i would also suggest using Q as the variable<br>
name here, because there is no other D supported yet... so<br>
-#define EON_EN25D16 0x3015<br>
+#define EON_EN25Q16 0x3015 /* Same as D16 */<br>
or so..<br></blockquote><div><br></div><div>Added a comment to the line with the EON_EN25D16 #define.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> +#define EON_EN25Q32 0x3016<br>
correct; similar to the EON_EN25Q80 there seem to exist 3 versions: 'A',<br>
'B' and no suffix.<br></blockquote><div><br></div><div>Added comment next to the #define.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">somewhere below...<br>
+#define EON_EN25QH16 0x7015<br></blockquote><div><br></div><div>Done. I rearranged the EN25Q* #defines a bit so they're in alphabetical order with respect to the other EN25* chips.</div><div><br></div><div>
On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Index: flashchips.c<br>
> ===================================================================<br>
> --- flashchips.c (revision 1285)<br>
> +++ flashchips.c (working copy)<br>
> @@ -2807,6 +2807,8 @@<br>
> },<br>
><br>
> {<br>
> + /* Note: EN25Q16 is an evil twin which shares the model ID<br>
> + but has different write protection capabilities */<br>
> .vendor = "Eon",<br>
> .name = "EN25D16",<br>
change according to the macro name chosen (if applicable see above)<br></blockquote><div><br></div><div>The EN25D16 was there first... I'm not sure if we should change it. I imagine the D16 is out of production, but I assume *somebody* is using it.</div>
<div><br></div><div>WWCDD? (What Would Carl-Daniel Do?)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> .bustype = CHIP_BUSTYPE_SPI,<br>
> @@ -2814,6 +2816,7 @@<br>
> .model_id = EON_EN25D16,<br>
> .total_size = 2048,<br>
> .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
true for the Q, did not check D<br></blockquote><div><br></div><div>The EN25D16's datasheet no longer appears to be listed, but it is still available here: <a href="http://www.eonssi.com/upfile/p2008929181445.pdf" target="_blank">http://www.eonssi.com/upfile/p2008929181445.pdf</a> (found via google :-))</div>
<div><br></div><div>It The D16 manual states, "The Write Status Register (WRSR) instruction allows new values to be written to the Status Register. Before it can be accepted, a Write Enable (WREN) instruction must previously have been executed."</div>
<div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> .tested = TEST_UNTESTED,<br>
> .probe = probe_spi_rdid,<br>
> .probe_timing = TIMING_ZERO,<br>
.block_erase = spi_block_erase_52, is not supported by the Q version.<br>
add a comment please.<br>
other erasers seem to be ok with the Q version.<br></blockquote><div><br></div><div>Done. Nice catch!</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
.voltage = {2700, 3600}, (for the Q version)<br></blockquote><div><br></div><div>Same as the D version (the original EN25Q* patch came before the .voltage member was added)</div><div><br></div><div>
On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> @@ -3083,6 +3086,171 @@<br>
><br>
> {<br>
> .vendor = "Eon",<br>
> + .name = "EN25Q40",<br>
> + .bustype = CHIP_BUSTYPE_SPI,<br>
> + .manufacture_id = EON_ID_NOPREFIX,<br>
> + .model_id = EON_EN25Q40,<br>
> + .total_size = 512,<br>
> + .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
> + .tested = TEST_UNTESTED,<br>
> + .probe = probe_spi_rdid,<br>
> + .probe_timing = TIMING_ZERO,<br>
> + .block_erasers =<br>
> + {<br>
> + {<br>
> + .eraseblocks = { {4 * 1024, 128} },<br>
> + .block_erase = spi_block_erase_20,<br>
> + }, {<br>
> + .eraseblocks = { {64 * 1024, 8} },<br>
> + .block_erase = spi_block_erase_d8,<br>
> + }, {<br>
> + .eraseblocks = { {512 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_60,<br>
> + }, {<br>
> + .eraseblocks = { {512 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_c7,<br>
> + }<br>
> + },<br>
> + .unlock = spi_disable_blockprotect,<br>
> + .write = spi_chip_write_256,<br>
> + .read = spi_chip_read,<br>
.voltage = {2700, 3600},<br></blockquote><div><br></div><div>Added .voltage.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + },<br>
> +<br>
> + {<br>
> + .vendor = "Eon",<br>
> + .name = "EN25Q80(A)",<br>
> + .bustype = CHIP_BUSTYPE_SPI,<br>
> + .manufacture_id = EON_ID_NOPREFIX,<br>
> + .model_id = EON_EN25Q80,<br>
depends on macro name choice above<br></blockquote><div><br></div><div>Updated to EON_EN25Q80A.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + .total_size = 1024,<br>
> + .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
> + .tested = TEST_UNTESTED,<br>
> + .probe = probe_spi_rdid,<br>
> + .probe_timing = TIMING_ZERO,<br>
> + .block_erasers =<br>
> + {<br>
> + {<br>
> + .eraseblocks = { {4 * 1024, 256} },<br>
> + .block_erase = spi_block_erase_20,<br>
> + }, {<br>
> + .eraseblocks = { {64 * 1024, 16} },<br>
> + .block_erase = spi_block_erase_d8,<br>
> + }, {<br>
> + .eraseblocks = { {1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_60,<br>
> + }, {<br>
> + .eraseblocks = { {1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_c7,<br>
> + }<br>
> + },<br>
> + .unlock = spi_disable_blockprotect,<br>
> + .write = spi_chip_write_256,<br>
> + .read = spi_chip_read,<br>
.voltage = {2700, 3600},<br></blockquote><div><br></div><div>Added the .voltage member.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + },<br>
> +<br>
> + {<br>
> + .vendor = "Eon",<br>
> + .name = "EN25Q32(A)(B)",<br>
i think that should be (A/B)<br></blockquote><div><br></div><div>Fixed.</div><div><br></div><div>On Wed, Jun 22, 2011 at 9:15 AM, Stefan Tauner <span dir="ltr"><<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + .bustype = CHIP_BUSTYPE_SPI,<br>
> + .manufacture_id = EON_ID_NOPREFIX,<br>
> + .model_id = EON_EN25Q32,<br>
> + .total_size = 4096,<br>
> + .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
> + .tested = TEST_UNTESTED,<br>
> + .probe = probe_spi_rdid,<br>
> + .probe_timing = TIMING_ZERO,<br>
> + .block_erasers =<br>
> + {<br>
> + {<br>
> + .eraseblocks = { {4 * 1024, 1024} },<br>
> + .block_erase = spi_block_erase_20,<br>
> + }, {<br>
> + .eraseblocks = { {64 * 1024, 64} },<br>
> + .block_erase = spi_block_erase_d8,<br>
> + }, {<br>
> + .eraseblocks = { {4 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_60,<br>
> + }, {<br>
> + .eraseblocks = { {4 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_c7,<br>
> + }<br>
> + },<br>
> + .unlock = spi_disable_blockprotect,<br>
> + .write = spi_chip_write_256,<br>
> + .read = spi_chip_read,<br>
.voltage = {2700, 3600},<br>
> + },<br>
> +<br>
> + {<br>
> + .vendor = "Eon",<br>
> + .name = "EN25Q64",<br>
> + .bustype = CHIP_BUSTYPE_SPI,<br>
> + .manufacture_id = EON_ID_NOPREFIX,<br>
> + .model_id = EON_EN25Q64,<br>
> + .total_size = 8192,<br>
> + .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
> + .tested = TEST_UNTESTED,<br>
> + .probe = probe_spi_rdid,<br>
> + .probe_timing = TIMING_ZERO,<br>
> + .block_erasers =<br>
> + {<br>
> + {<br>
> + .eraseblocks = { {4 * 1024, 2048} },<br>
> + .block_erase = spi_block_erase_20,<br>
> + }, {<br>
> + .eraseblocks = { {64 * 1024, 128} },<br>
> + .block_erase = spi_block_erase_d8,<br>
> + }, {<br>
> + .eraseblocks = { {8 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_60,<br>
> + }, {<br>
> + .eraseblocks = { {8 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_c7,<br>
> + }<br>
> + },<br>
> + .unlock = spi_disable_blockprotect,<br>
> + .write = spi_chip_write_256,<br>
> + .read = spi_chip_read,<br>
.voltage = {2700, 3600},<br>
> + },<br>
> +<br>
> + {<br>
> + .vendor = "Eon",<br>
> + .name = "EN25Q128",<br>
> + .bustype = CHIP_BUSTYPE_SPI,<br>
> + .manufacture_id = EON_ID_NOPREFIX,<br>
> + .model_id = EON_EN25Q128,<br>
> + .total_size = 16384,<br>
> + .page_size = 256,<br>
> + .feature_bits = FEATURE_WRSR_WREN,<br>
> + .tested = TEST_UNTESTED,<br>
> + .probe = probe_spi_rdid,<br>
> + .probe_timing = TIMING_ZERO,<br>
> + .block_erasers =<br>
> + {<br>
> + {<br>
> + .eraseblocks = { {4 * 1024, 4096} },<br>
> + .block_erase = spi_block_erase_20,<br>
> + }, {<br>
> + .eraseblocks = { {64 * 1024, 256} },<br>
> + .block_erase = spi_block_erase_d8,<br>
> + }, {<br>
> + .eraseblocks = { {16 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_60,<br>
> + }, {<br>
> + .eraseblocks = { {16 * 1024 * 1024, 1} },<br>
> + .block_erase = spi_block_erase_c7,<br>
> + }<br>
> + },<br>
> + .unlock = spi_disable_blockprotect,<br>
> + .write = spi_chip_write_256,<br>
> + .read = spi_chip_read,<br>
.voltage = {2700, 3600},<br></blockquote><div><br></div><div>Added .voltage members.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + },<br>
> +<br>
> + {<br>
> + .vendor = "Eon",<br>
> .name = "EN29F010",<br>
> .bustype = CHIP_BUSTYPE_PARALLEL,<br>
> .manufacture_id = EON_ID,<br>
<br>
although the comment of the array indicates "alphabetically" sorted, it<br>
seems most of it is sorted by chipsize (if pushed). that means the<br>
entries above should be sorted like the IDs in flashchips.h<br>
<br>
also please add a comment to all chips with the respective OTP (=one<br>
time programmable) area size.<br>
we don't support that yet, nor do we warn the user (yet), but i am sure<br>
there is a patch to lure with such a comment... ;)<br>
512B:<br>
Q128<br>
Q64<br>
Q32B<br>
(QH16)<br>
<br>
256B:<br>
Q80A<br>
Q40<br>
<br>
128B:<br>
Q16 (but D16 has 512B, oh my...)<br></blockquote><div><br></div><div>Done -- Added the comments above the feature_bits. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
most of this are rather small problems. hence if everything i put into<br>
this review was at least briefly considered.. it is<br>
Acked-by: Stefan Tauner <<a href="mailto:stefan.tauner@student.tuwien.ac.at" target="_blank">stefan.tauner@student.tuwien.ac.at</a>><br>
<font color="#888888">--<br>
Kind regards/Mit freundlichen Grüßen, Stefan Tauner<br>
<br>
_______________________________________________<br>
flashrom mailing list<br>
<a href="mailto:flashrom@flashrom.org" target="_blank">flashrom@flashrom.org</a><br>
<a href="http://www.flashrom.org/mailman/listinfo/flashrom" target="_blank">http://www.flashrom.org/mailman/listinfo/flashrom</a><br>
</font></blockquote></div><br><br clear="all"><br>-- <br>David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.<br>