[M] Change in flashrom[master]: flashchips: Add support for MXIC MX2525643G It is similar to the MX25...

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Name of user not set #1005084.

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/76853?usp=email )

Change subject: flashchips: Add support for MXIC MX2525643G It is similar to the MX2525635F and shares its RDID.
......................................................................

Patch Set 1:

(10 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/76853/comment/5b7e47b2_fa06df8f :
PS1, Line 7: MX2525643G
Let's use exact name MX25U25643G , can you please update?

https://review.coreboot.org/c/flashrom/+/76853/comment/c63ecb56_34d773ce :
PS1, Line 8: MX2525635F
Same here, probably MX25U25635F

https://review.coreboot.org/c/flashrom/+/76853/comment/978d2742_ecc2afdf :
PS1, Line 10: Tested: read, write and erase.
Could you please add info which programmer you ran?

https://review.coreboot.org/c/flashrom/+/76853/comment/82c36f2b_9b710bbd :
PS1, Line 12: Datasheet is available at the following URL:
            : https://www.mxic.com.tw/Lists/Datasheet/Attachments/8766/MX2525643G,%201.8V....
You need to update the link (the new one you gave me in the comment works)

Patchset:

PS1:
> Dear Anastasia Klimchuk […]

Thank you, new link works, I opened datasheet successfully.
I added some comments, you will need to make changes to your patch and then upload new version and respond to comments.
If you have questions, feel free to ask!

File flashchips.c:

https://review.coreboot.org/c/flashrom/+/76853/comment/1788f041_bcb07613 :
PS1, Line 10094: MACRONIX_MX25U25643G
After you fix my other comment about id, you will change this to MACRONIX_MX25U25635F

https://review.coreboot.org/c/flashrom/+/76853/comment/56f7cf37_7ba56c55 :
PS1, Line 10104: {
               : 				.eraseblocks = { {4 * 1024, 8192} },
               : 				.block_erase = SPI_BLOCK_ERASE_21,
               : 			},
Maybe I am missing something, I don't see this in the datasheet. I can see 20h for sector erase but not 21?

https://review.coreboot.org/c/flashrom/+/76853/comment/ee93c4dd_8d63d827 :
PS1, Line 10113: {
               : 				.eraseblocks = { {32 * 1024, 1024} },
               : 				.block_erase = SPI_BLOCK_ERASE_52,
               : 			},
same

https://review.coreboot.org/c/flashrom/+/76853/comment/aff5d7a2_e211f672 :
PS1, Line 10117: 	.eraseblocks = { {64 * 1024, 512} },
               : 				.block_erase = SPI_BLOCK_ERASE_DC,
               : 			}, {
same

File include/flashchips.h:

https://review.coreboot.org/c/flashrom/+/76853/comment/15ae0d87_479973b2 :
PS1, Line 530: #define MACRONIX_MX25U25643G	0x2539
Don't add the same id, add a comment on existing id (MACRONIX_MX25U25635F), like that:

/* Same as MACRONIX_MX25U25635F */

You can look at examples of the same situation with same ids just few lines above in this file.