Attention is currently required from: Alexandru Stan, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change by Alexandru Stan. ( https://review.coreboot.org/c/flashrom/+/84752?usp=email )
Change subject: flashchips: add Winbond W25R512NW / W74M51NW ......................................................................
Patch Set 2:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84752/comment/07e291c5_cc09a398?usp... : PS1, Line 8: : I don't actually have a datasheet for this (so feel free to reject this : CL)
I just got a datasheet myself
This is perfect, thanks so much for your effort!
I understand not everything is distributable and we don't have Winbond engineers collaborating with us yet, not yet. However if you have the datasheet and the chip itself, and you tested it, that's all what is needed.
You already checked the chip definition to the best of your knowledge, and tested the chip. If you have any doubts about anything, please ask! But I think you asked your questions already. So there are few open comments left and it should be all good.
Sadly, I won't have anything relevant on my gdrive anymore. Also sadly, my job situation has changed dramatically, my current job is completely irrelevant to flashrom. I am here as a hobby <3
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/84752/comment/69fcf0e8_4191ab55?usp... : PS1, Line 20271: 1650
Datasheet disagreed here.
What the datasheet says? I would say, voltage you better just copy from datasheet
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/84752/comment/0cc5e647_fcc3c82f?usp... : PS2, Line 20230: W25R512NW The situation you described in the following two lines, we do the following:
we mark with the name `.name = W25R512NW/W74M51NW` , and then you don't need the comment in the following two lines.
https://review.coreboot.org/c/flashrom/+/84752/comment/e3f8ed38_de5f2bbf?usp... : PS2, Line 20239: 1024B total, 256B reserved
Again, copied this comment from that Winbond chip. […]
I am often confused by how OTP size is described in the datasheet :( although I shouldn't be. Let's leave it. OTP is not implemented yet, these comments are for future. I will try to find similar chip model with similar OTP size with the datasheet I have, to compare.
You can leave it, or if you have any concerns, you can just remove this comment line.
https://review.coreboot.org/c/flashrom/+/84752/comment/bdb960de_6c116a1d?usp... : PS2, Line 20269: .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, : .unlock = SPI_DISABLE_BLOCKPROTECT, :
Given I haven't tested WP, I don't know if these are good. […]
Since you haven't tested write-protection and not marking it as tested, you can put here
.printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, .unlock = SPI_DISABLE_BLOCKPROTECT,
and then later when/if write-protection is added, these values might be updated.
-
Anastasia Klimchuk (Code Review)