Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78348?usp=email )
Change subject: flashchips: Add GD25Q128E name to the GD25Q127C/GD25Q128C entry ......................................................................
Patch Set 2:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/78348/comment/3734bd01_fb32eaa1 : PS1, Line 9: Q128E and Q127C/Q128C have compatible main functions, their differences : are
Do you have a link to datasheet? Thank you!
https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00... (added to the comment)
https://review.coreboot.org/c/flashrom/+/78348/comment/7340a796_43437544 : PS1, Line 31: flashrom_tester with flashrom binary could pass with Q128E
Which specifically flashrom commands did that run? Probe/read/write/erase/write-protect ?
Yes, all of them are tested. Updated.
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/78348/comment/eb066b2c_3f4f3b96 : PS1, Line 6802: /* FIXME: 128E's OTP: 3072B total and doesn't support QPI */
QPI part of this might be an issue. […]
I have two questions about these:
1. According to the [GD25Q128C datasheet](https://www.endrich.com/sixcms/media.php/2/GD25Q128C-Rev2.pdf) page 8, GD25Q128C has QPI, [GD25Q127C datasheet](https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00...) page 4, GD25Q127C doesn't has QPI. So the best solution might be: separate to "GD25Q128C" and "GD25Q127C/GD25Q128E" instead?
(And I think there was already some discussion about it: https://review.coreboot.org/c/flashrom/+/52883/2/flashchips.c#6274)
2. Could you help me understand what will happen if there are 3 different chip entries? Will we always get the very first match with `flashrom --flash-name`?
https://review.coreboot.org/c/flashrom/+/78348/comment/f2cad5c8_c6f3c61d : PS1, Line 6833: .reg_bits = : { : .srp = {STATUS1, 7, RW}, : .srl = {STATUS2, 0, RW}, : .bp = {{STATUS1, 2, RW}, {STATUS1, 3, RW}, {STATUS1, 4, RW}}, : .tb = {STATUS1, 5, RW}, /* Called BP3 in datasheet, acts like TB */ : .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */ : .cmp = {STATUS2, 6, RW}, : }
Are all these registers the same for new chip you are adding
Yes, they are exactly the same, based on the datasheet.
and have you tested write-protect operations? (current chip definition marks write-protect as tested).
Yes, we've tested.
File include/flashchips.h:
https://review.coreboot.org/c/flashrom/+/78348/comment/09fd21c0_3e337dc0 : PS1, Line 393: Same as GD25Q128B, GD25B128B, GD25Q127C, and GD25Q128E, can be distinguished by SFDP
With this patch, the same model id GIGADEVICE_GD25Q128 will be used for 5 different chips, used to b […]
Done