Change in ...flashrom[master]: flashchips.h: merge definitions from Chromium fork
Alan Green has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/33997 Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... flashchips.h: merge definitions from Chromium fork This change merges flashchips.h definitions from the chromium fork of flashrom. It is a precursor to merging flashchips.c. The corresponding downstream change, where coreboot's flashchips.h was merged into Chromium's version is: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... After this change and the Chromium change are submitted, the two versions of flashchips.h will be identical. Signed-off-by: Alan Green <avg@google.com> Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 --- M flashchips.c M flashchips.h 2 files changed, 38 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/33997/1 diff --git a/flashchips.c b/flashchips.c index 2a9ebb7..1b4b3ce 100644 --- a/flashchips.c +++ b/flashchips.c @@ -5889,7 +5889,7 @@ .name = "GD25LQ128", .bustype = BUS_SPI, .manufacture_id = GIGADEVICE_ID, - .model_id = GIGADEVICE_GD25LQ128, + .model_id = GIGADEVICE_GD25LQ128CD, .total_size = 16384, .page_size = 256, /* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44 */ diff --git a/flashchips.h b/flashchips.h index a1b38e0..0d816c8 100644 --- a/flashchips.h +++ b/flashchips.h @@ -36,6 +36,9 @@ #define PROGMANUF_ID 0xFFFE /* dummy ID for opaque chips behind a programmer */ #define PROGDEV_ID 0x01 /* dummy ID for opaque chips behind a programmer */ +#define VARIABLE_SIZE_MANUF_ID 0x3eaf +#define VARIABLE_SIZE_DEVICE_ID 0x10af + #define ALLIANCE_ID 0x52 /* Alliance Semiconductor */ #define ALLIANCE_AS29F002B 0x34 #define ALLIANCE_AS29F002T 0xB0 @@ -151,6 +154,7 @@ #define ATMEL_AT25SF081 0x8501 #define ATMEL_AT25SF161 0x8601 #define ATMEL_AT25SL128A 0x4218 +#define ATMEL_AT25SF128A 0x8901 /* Adesto AT25SF128A */ #define ATMEL_AT26DF041 0x4400 #define ATMEL_AT26DF081 0x4500 /* guessed, no datasheet available */ #define ATMEL_AT26DF081A 0x4501 @@ -196,7 +200,9 @@ #define ATMEL_AT49BV512 0x03 /* Same as AT49F512 */ #define ATMEL_AT49F001N 0x05 /* Same as AT49F001 */ #define ATMEL_AT49F001NT 0x04 /* Same as AT49F001T */ +#define ATMEL_AT49F020 0x0B #define ATMEL_AT49F002N 0x07 /* for AT49F002(N) */ +#define ATMEL_AT49F002NT 0x08 /* for AT49F002(N)T */ #define ATMEL_AT49LH002 0xE9 #define ATMEL_AT49LH00B4 0xED #define ATMEL_AT49LH004 0xEE @@ -270,6 +276,13 @@ #define EON_EN25P64 0x16 #define EON_EN25B64T 0x46 #define EON_EN25B64B 0x36 +#define EON_EN25D16 0x3015 +#define EON_EN25Q40 0x3013 +#define EON_EN25Q80 0x3014 +#define EON_EN25D16 0x3015 /* Same as Q16 */ +#define EON_EN25Q32 0x3016 +#define EON_EN25Q64 0x3017 +#define EON_EN25Q128 0x3018 #define EON_EN25F05 0x3110 #define EON_EN25F10 0x3111 #define EON_EN25F20 0x3112 @@ -302,6 +315,7 @@ #define EON_EN29F010 0x20 #define EON_EN29F040A 0x7F04 #define EON_EN29LV010 0x7F6E +#define EON_EN29LV040A 0x7F4F /* EN29LV040(A) */ #define EON_EN29LV040 0x4F /* Same as EN29LV040A */ #define EON_EN29LV640B 0xCB #define EON_EN29LV640T 0xC9 @@ -373,6 +387,7 @@ #define GIGADEVICE_GD25Q32 0x4016 /* Same as GD25Q32B */ #define GIGADEVICE_GD25Q64 0x4017 /* Same as GD25Q64B */ #define GIGADEVICE_GD25Q128 0x4018 /* GD25Q128B and GD25Q128C only, can be distinguished by SFDP */ +#define GIGADEVICE_GD25Q256D 0x4019 #define GIGADEVICE_GD25VQ21B 0x4212 #define GIGADEVICE_GD25VQ41B 0x4213 /* Same as GD25VQ40C, can be distinguished by SFDP */ #define GIGADEVICE_GD25VQ80C 0x4214 @@ -382,7 +397,7 @@ #define GIGADEVICE_GD25LQ16 0x6015 #define GIGADEVICE_GD25LQ32 0x6016 #define GIGADEVICE_GD25LQ64 0x6017 /* Same as GD25LQ64B (which is faster) */ -#define GIGADEVICE_GD25LQ128 0x6018 +#define GIGADEVICE_GD25LQ128CD 0x6018 #define GIGADEVICE_GD29GL064CAB 0x7E0601 #define HYUNDAI_ID 0xAD /* Hyundai */ @@ -452,6 +467,8 @@ #define SHARP_LH28F008SA 0xA2 /* Sharp chip, Intel Vendor ID */ #define SHARP_LH28F008SC 0xA6 /* Sharp chip, Intel Vendor ID */ +#define INTEL_HWSEQ 0xFFFE /* dummy ID for hardware sequencing */ + #define ISSI_ID 0xD5 /* ISSI Integrated Silicon Solutions, see also PMC. */ #define ISSI_ID_SPI 0x9D /* ISSI ID used for SPI flash, see also PMC_ID_NOPREFIX */ #define ISSI_IS25LP064 0x6017 @@ -487,6 +504,7 @@ #define MACRONIX_MX25L1605 0x2015 /* MX25L1605 (64k 0x20); MX25L1605A/MX25L1606E/MX25L1608E (4k 0x20, 64k 0x52); MX25L1605D/MX25L1608D/MX25L1673E (4k 0x20) */ #define MACRONIX_MX25L3205 0x2016 /* MX25L3205, MX25L3205A (64k 0x20); MX25L3205D/MX25L3208D (4k 0x20); MX25L3206E/MX25L3208E (4k 0x20, 64k 0x52); MX25L3273E (4k 0x20, 32k 0x52) */ #define MACRONIX_MX25L6405 0x2017 /* MX25L6405, MX25L6405D (64k 0x20); MX25L6406E/MX25L6408E (4k 0x20); MX25L6436E/MX25L6445E/MX25L6465E/MX25L6473E (4k 0x20, 32k 0x52) */ +#define MACRONIX_MX25L12805 0x2018 /* MX25L12805 */ #define MACRONIX_MX25L12805D 0x2018 /* MX25L12805D (no 32k); MX25L12865E, MX25L12835F, MX25L12845E (32k 0x52) */ #define MACRONIX_MX25L25635F 0x2019 /* Same as MX25L25639F, but the latter seems to not support REMS */ #define MACRONIX_MX25L1635D 0x2415 @@ -644,8 +662,17 @@ #define SPANSION_S25FL204 0x4013 #define SPANSION_S25FL208 0x4014 #define SPANSION_S25FL216 0x4015 /* Same as S25FL216K, but the latter supports OTP, 3 status regs, quad I/O, SFDP etc. */ +#define SPANSION_S25FL116K 0x4015 #define SPANSION_S25FL132K 0x4016 #define SPANSION_S25FL164K 0x4017 +#define SPANSION_S25FS128S_L 0x20180081 /* Large sectors. */ +#define SPANSION_S25FS128S_S 0x20180181 /* Small sectors. */ +#define SPANSION_S25FS256S_L 0x02190081 /* Large sectors. */ +#define SPANSION_S25FS256S_S 0x02190181 /* Small sectors. */ +#define SPANSION_S25FL128S_UL 0x20180080 /* Uniform Large (128kB) sectors */ +#define SPANSION_S25FL128S_US 0x20180180 /* Uniform Small (64kB) sectors */ +#define SPANSION_S25FL256S_UL 0x02190080 /* Uniform Large (128kB) sectors */ +#define SPANSION_S25FL256S_US 0x02190180 /* Uniform Small (64kB) sectors */ /* Spansion 29GL families got a suffix indicating the process technology but share the same 3-Byte IDs. They can * however be differentiated by CFI byte 45h. Some versions exist which have special top or bottom boot sectors @@ -920,7 +947,12 @@ #define WINBOND_NEX_W25Q16_V 0x4015 /* W25Q16CV; W25Q16DV */ #define WINBOND_NEX_W25Q32_V 0x4016 /* W25Q32BV; W25Q32FV in SPI mode (default) */ #define WINBOND_NEX_W25Q64_V 0x4017 /* W25Q64BV, W25Q64CV; W25Q64FV in SPI mode (default) */ -#define WINBOND_NEX_W25Q128_V 0x4018 /* W25Q128BV; W25Q128FV in SPI mode (default) */ + +/* + * W25Q128 has several variants. Currently all are 3.3V except for the W25Q128FW + * which is 1.8V. Otherwise they should behave the same... + */ +#define WINBOND_NEX_W25Q128_V 0x4018 /* W25Q128BV, W25Q128FV (SPI mode), W25Q128JV */ #define WINBOND_NEX_W25Q256_V 0x4019 /* W25Q256FV or W25Q256JV_Q (QE=1) */ #define WINBOND_NEX_W25Q20_W 0x5012 /* W25Q20BW */ #define WINBOND_NEX_W25Q40BW 0x5013 /* W25Q40BW */ @@ -930,9 +962,11 @@ #define WINBOND_NEX_W25Q16_W 0x6015 /* W25Q16DW */ #define WINBOND_NEX_W25Q32_W 0x6016 /* W25Q32DW; W25Q32FV in QPI mode */ #define WINBOND_NEX_W25Q64_W 0x6017 /* W25Q64DW; W25Q64FV in QPI mode */ -#define WINBOND_NEX_W25Q128_W 0x6018 /* W25Q128FW; W25Q128FV in QPI mode */ +#define WINBOND_NEX_W25Q128_W 0x6018 /* Same as W25Q128FV (QPI mode), W25R128FV */ +#define WINBOND_NEX_W25Q128FW 0x6018 /* Same as W25Q128FV (QPI mode), W25R128FV */ #define WINBOND_NEX_W25Q128_V_M 0x7018 /* W25Q128JVSM */ #define WINBOND_NEX_W25Q256JV_M 0x7019 /* W25Q256JV_M (QE=0) */ +#define WINBOND_NEX_W25Q32JW 0x8016 #define WINBOND_ID 0xDA /* Winbond */ #define WINBOND_W19B160BB 0x49 -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 1 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-MessageType: newchange
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 2: Code-Review+1 (2 comments) https://review.coreboot.org/#/c/33997/2/flashchips.h File flashchips.h: https://review.coreboot.org/#/c/33997/2/flashchips.h@400 PS2, Line 400: GIGADEVICE_GD25LQ128CD Is there no user of this in flashchips.c? https://review.coreboot.org/#/c/33997/2/flashchips.h@470 PS2, Line 470: #define INTEL_HWSEQ 0xFFFE /* dummy ID for hardware sequencing */ Here in upstream we don't have intel hwseq support so better to leave this out for now and ship it with the rest of the support once we extract it out and send it in for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 2 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 03 Jul 2019 07:03:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/33997/2/flashchips.h File flashchips.h: https://review.coreboot.org/#/c/33997/2/flashchips.h@470 PS2, Line 470: #define INTEL_HWSEQ 0xFFFE /* dummy ID for hardware sequencing */
Here in upstream we don't have intel hwseq support so better to leave this out for now and ship it w […] There is some hwseq support here in upstream, but I don't know much about it.
-- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 2 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-Comment-Date: Wed, 03 Jul 2019 09:05:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
Hello Edward O'Callaghan, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33997 to look at the new patch set (#5). Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... flashchips.h: merge definitions from Chromium fork This change merges flashchips.h definitions from the chromium fork of flashrom. It is a precursor to merging flashchips.c. The corresponding downstream change, where coreboot's flashchips.h was merged into Chromium's version is SHA: d8c683b2e8104e48b0ee73279e0a55f76a1281b8 https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... After this change is submitted, the two versions of flashchips.h will be identical, with the exception of the identifier INTEL_HWSEQ, which is in the ChromiumOS tree only. Adding it will be part of the hwseq changes. Signed-off-by: Alan Green <avg@google.com> Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 --- M flashchips.c M flashchips.h 2 files changed, 36 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/33997/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/flashrom/+/33997/2/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/2/flashchips.h@400 PS2, Line 400: GIGADEVICE_GD25LQ128CD
Is there no user of this in flashchips. […] There is. I have updated flashchips.c to match.
https://review.coreboot.org/c/flashrom/+/33997/2/flashchips.h@470 PS2, Line 470: #define INTEL_HWSEQ 0xFFFE /* dummy ID for hardware sequencing */
There is some hwseq support here in upstream, but I don't know much about it. I see. Yes, it makes sense to add this ID in the change that uses it. Will remove for now. This will be the one remaining difference in flashchips.h between upstream and downstream, so should be easy enough to track.
Have also updated the commit message accordingly. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 04 Jul 2019 06:31:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 5: Code-Review+1 (3 comments) Looks rather good https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@204 PS5, Line 204: Minor: One tab too much? https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@949 PS5, Line 949: W25Q128 I think this also applies to W25Q64 and friends https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c File flashchips.c: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c@5892 PS5, Line 5892: CD Out of curiosity, any specific reason for this rename? -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 04 Jul 2019 09:42:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */ This comment doesn't make much sense. All W25Q...W are 1.8V, all W25Q...V are 3.3V. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 04 Jul 2019 18:54:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Edward O'Callaghan, Angel Pons, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33997 to look at the new patch set (#6). Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... flashchips.h: merge definitions from Chromium fork This change merges flashchips.h definitions from the chromium fork of flashrom. It is a precursor to merging flashchips.c. The corresponding downstream change, where coreboot's flashchips.h was merged into Chromium's version is SHA: d8c683b2e8104e48b0ee73279e0a55f76a1281b8 https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... After this change is submitted, the two versions of flashchips.h will be identical, with the exception of the identifier INTEL_HWSEQ, which is in the ChromiumOS tree only. Adding it will be part of the hwseq changes. Signed-off-by: Alan Green <avg@google.com> Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 --- M flashchips.c M flashchips.h 2 files changed, 36 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/33997/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 6: (4 comments) https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@204 PS5, Line 204:
Minor: One tab too much? Done
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@949 PS5, Line 949: W25Q128
I think this also applies to W25Q64 and friends I'm unfamiliar with these parts. If you'd like me to update the wording to also include W25Q64, please let me know. Otherwise, I will have some time to look up datasheets next week.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */
This comment doesn't make much sense. All W25Q...W are 1.8V, […] Do you have a suggestion for alternate wording?
For context, this wording was introduced in downstream change 0715328 - https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c File flashchips.c: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c@5892 PS5, Line 5892: CD
Out of curiosity, any specific reason for this rename? The downstream names were more specific and therefore seemed more helpful.
Originally, the part was added with the name GIGADEVICE_GD25LQ128C: SHA:62cd8106f30a1232464e6818f8736db76c64446e aka https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... Later, this was updated to GIGADEVICE_GD25LQ128CD: SHA:6c957d745f5d3dcadd1035734a5cf1b804bd0f2f aka https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... I'm happy to retain the upstream name (GIGADEVICE_GD25LQ128) if you'd prefer it. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 05 Jul 2019 06:52:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 6: (6 comments) https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@40 PS6, Line 40: #define VARIABLE_SIZE_DEVICE_ID 0x10af What are these for? https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@278 PS6, Line 278: #define EON_EN25D16 0x3015 Spurious double entry? https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@504 PS6, Line 504: #define MACRONIX_MX25L12805 0x2018 /* MX25L12805 */ Does it really exist? https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@962 PS6, Line 962: #define WINBOND_NEX_W25Q128_W 0x6018 /* Same as W25Q128FV (QPI mode), W25R128FV */ Spurious, double entry? https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */
Do you have a suggestion for alternate wording? […] I would prefer to just drop the comment. It's merely a random detail that is already better documented in `flashchips.c`.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c File flashchips.c: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c@5892 PS5, Line 5892: CD
The downstream names were more specific and therefore seemed more helpful. […] I think it's common to use the name of the chip that introduced the id. So that would be `GD25LQ128C`? It doesn't matter much, IMHO.
What we do care about, though, is the `.name` presented to the ui. It should mention all supported chips e.g. "GD25LQ128C/D" or "GD25LQ128C/GD25LQ128D". But that's for a future patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 05 Jul 2019 22:22:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Alan Green <avg@google.com> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 6: Code-Review+1 (7 comments) Looks good, save for a few nits https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@40 PS6, Line 40: #define VARIABLE_SIZE_DEVICE_ID 0x10af
What are these for? If unused, please drop.
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@278 PS6, Line 278: #define EON_EN25D16 0x3015
Spurious double entry? Definitely. I'd keep the bottom one, and optionally rename it to Q16 for consistency (doesn't need to be in this patch)
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@504 PS6, Line 504: #define MACRONIX_MX25L12805 0x2018 /* MX25L12805 */
Does it really exist? Looks like it's the same as the line below, but following the style of the lines above, so it seems to be duplicated.
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@962 PS6, Line 962: #define WINBOND_NEX_W25Q128_W 0x6018 /* Same as W25Q128FV (QPI mode), W25R128FV */
Spurious, double entry? Looks like it, please keep the "W25Q128_W" entry
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@949 PS5, Line 949: W25Q128
I'm unfamiliar with these parts. […] I don't think this comment needs to be updated (see the other comment thread), but I wanted to note that this is not specific to the W25Q128.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */
I would prefer to just drop the comment. It's merely a random detail […] Agreed.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c File flashchips.c: https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c@5892 PS5, Line 5892: CD
I think it's common to use the name of the chip that introduced […] Agreed. I wouldn't change the model_id define, but rather the string name (on a separate patch).
-- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 12 Jul 2019 14:56:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Alan Green <avg@google.com> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Patch Set 6:
Patch Set 6: Code-Review+1
(7 comments)
Looks good, save for a few nits
Hi Angel, Nico, After thinking about this, probably the right action is to drop this patch, then update this flashchips.h a piece at a time as we bring definitions upstream from Chromium. I had been hoping to have identical flashchips.h files in both upstream and downstream, but (a) it requires more thought than I've put into it so far, and (b) having unused definitions in flashchips.h is confusing and unhelpful, particularly here in upstream. Thanks, Alan. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 16 Jul 2019 01:53:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Alan Green has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/33997 ) Change subject: flashchips.h: merge definitions from Chromium fork ...................................................................... Abandoned Will update in conjunction with flashchips.c rather than independently. -- To view, visit https://review.coreboot.org/c/flashrom/+/33997 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie280f351045bdcbc454d6f829fc071af820382b1 Gerrit-Change-Number: 33997 Gerrit-PatchSet: 6 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: abandon
participants (4)
-
Alan Green (Code Review) -
Angel Pons (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)