Attention is currently required from: Brian Norris, Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75991?usp=email )
Change subject: flashrom: only unlock for write/erase operations
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I checked the unlock functions, it turns out one of them actually does unlock for reads (last bullet […]
Tricky, what about doing the `R|W` specialisation within the `unlock_flash_wp()` preamble? If the chip has `SPI_DISABLE_BLOCKPROTECT_SST26_GLOBAL_UNPROTECT` set `return 1` to let the fallback to the old `bp_func` mechanism for these special chips?
--
To view, visit https://review.coreboot.org/c/flashrom/+/75991?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5dc66474a0b7969b51b86ac9f5daa2c95ae968f1
Gerrit-Change-Number: 75991
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 27 Jun 2023 00:02:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Joseph Goh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68278?usp=email )
Change subject: flashchips: add support for MX25V16066/KH25V16066
......................................................................
Patch Set 3:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/68278/comment/db0dd93c_82d2faf8 :
PS2, Line 8772: MACRONIX_MX25L1605
> Thanks for explaining! I agree. […]
Done! I've also added the erase commands in brackets although I'm not completely sure why the erase commands were included in brackets specifically for these chips.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68278?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5f0548f023fcd09a970148586497e00414ad1ae
Gerrit-Change-Number: 68278
Gerrit-PatchSet: 3
Gerrit-Owner: Joseph Goh <josephgoh7(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 26 Jun 2023 13:42:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joseph Goh <josephgoh7(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Joseph Goh.
Hello Anastasia Klimchuk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68278?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Code-Review+1 by Anastasia Klimchuk, Verified+1 by build bot (Jenkins)
Change subject: flashchips: add support for MX25V16066/KH25V16066
......................................................................
flashchips: add support for MX25V16066/KH25V16066
Change-Id: Ic5f0548f023fcd09a970148586497e00414ad1ae
Signed-off-by: Joseph Goh <josephgoh7(a)gmail.com>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M flashchips.c
M include/flashchips.h
2 files changed, 39 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/68278/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/68278?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5f0548f023fcd09a970148586497e00414ad1ae
Gerrit-Change-Number: 68278
Gerrit-PatchSet: 3
Gerrit-Owner: Joseph Goh <josephgoh7(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Joseph Goh <josephgoh7(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Angel Pons, Artur Kowalski, ServError.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68557?usp=email )
Change subject: flashchips: add support for MX77L25650F chip
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> The datasheet specifies the set in the ML patch […]
@ServError, since your patch is more complete, do you want to update this one? You would do that by using "DOWNLOAD" button to pull it locally, rebasing/amending the patch and then pushing it back via `git push {URL} HEAD:refs/for/master`. If not, I can combine patches myself and will put `Signed-off-by:` to give credit to you.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68557?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iaea5485f8b59b8538dc47beada2c308376ea027c
Gerrit-Change-Number: 68557
Gerrit-PatchSet: 4
Gerrit-Owner: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Artur Kowalski <arturkow2000(a)gmail.com>
Gerrit-CC: ServError <admin(a)serverror.com>
Gerrit-Attention: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: ServError <admin(a)serverror.com>
Gerrit-Comment-Date: Mon, 26 Jun 2023 13:26:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: ServError <admin(a)serverror.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Artur Kowalski, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68557?usp=email )
Change subject: flashchips: add support for MX77L25650F chip
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I asked about it on Friday, waiting for a response after the work week begins. […]
Thank you for help! Will wait for your response then.
With the patches, all patches need to go through Gerrit. Even if patch is sent by ML, someone needs to push it to Gerrit.
This one already created, so someone needs to rebase and push a new patchset to finish it. If that new patchset will be done by another contributor, they add a second Signed-off-by line.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68557?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iaea5485f8b59b8538dc47beada2c308376ea027c
Gerrit-Change-Number: 68557
Gerrit-PatchSet: 4
Gerrit-Owner: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Artur Kowalski <arturkow2000(a)gmail.com>
Gerrit-CC: ServError <admin(a)serverror.com>
Gerrit-Attention: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 26 Jun 2023 12:48:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Angel Pons, Artur Kowalski.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68557?usp=email )
Change subject: flashchips: add support for MX77L25650F chip
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> Sergii, do you know if anyone on your side (maybe you) would finish the patch (fix remaining comment […]
I asked about it on Friday, waiting for a response after the work week begins. The ML patch looks more complete to me so far.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68557?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iaea5485f8b59b8538dc47beada2c308376ea027c
Gerrit-Change-Number: 68557
Gerrit-PatchSet: 4
Gerrit-Owner: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Artur Kowalski <arturkow2000(a)gmail.com>
Gerrit-CC: ServError <admin(a)serverror.com>
Gerrit-Attention: Artur Kowalski <artur.kowalski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 26 Jun 2023 12:19:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Edward O'Callaghan.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75991?usp=email )
Change subject: flashrom: only unlock for write/erase operations
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I couldn't figure out whether this got here for a good reason. […]
I checked the unlock functions, it turns out one of them actually does unlock for reads (last bullet).
Most unlock functions call `spi_disable_blockprotect_generic()`, which only tries to clear bits in SR1. I've never seen a chip with SR1-controlled read protection, chips that do have read protection usually control it via special registers or command sequences. So these are probably safe.
There are four unlock functions that don't call `spi_disable_blockprotect_generic()`:
- `UNPROTECT_28SF040` - used by SST28SF040A - I don't have datasheet, it looks like a very old parallel flash. I would guess it doesn't have read protection.
- `UNLOCK_SST_FWHUB` - used by several chips - the SST49LF0008A datasheet doesn't indicate it has read protection.
- `SPI_DISABLE_BLOCKPROTECT_AT45DB` - used by several chips - the AT45DB321E
datasheet doesn't indicate it has read protection.
- `SPI_DISABLE_BLOCKPROTECT_SST26_GLOBAL_UNPROTECT` - used by several chips - according to the SST26VF032B datasheet, there is sector based read and write protection that can be globally cleared with a ULBPR (98h) instruciton, which is what the unlock function does.
Maybe we can work it around with something like a `FEATURE_UNLOCK_FOR_READ` chip flag?
--
To view, visit https://review.coreboot.org/c/flashrom/+/75991?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5dc66474a0b7969b51b86ac9f5daa2c95ae968f1
Gerrit-Change-Number: 75991
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 26 Jun 2023 12:14:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Joseph Goh.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68278?usp=email )
Change subject: flashchips: add support for MX25V16066/KH25V16066
......................................................................
Patch Set 2: Code-Review+1
(3 comments)
Patchset:
PS2:
Thanks Joseph! You need to add a comment about model id (see my comment below) and that should be it.
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/68278/comment/cf3534cd_d620473b :
PS2, Line 8772: MACRONIX_MX25L1605
> Nope, it is 0x2015 since the RDID command is used (probe_spi_rdid, see page 20).
Thanks for explaining! I agree.
Why I was initially confused is that ID MACRONIX_MX25L1605 from another chip is used. You need to add a comment to include/flashchips.h that this id also serves MX25V16066 (there are lots comments like this as examples).
https://review.coreboot.org/c/flashrom/+/68278/comment/637da7c1_d2333141 :
PS2, Line 8802: 2700
> yep, 2.3V-2.7V works, but at a slower speed. […]
I see, thanks!
--
To view, visit https://review.coreboot.org/c/flashrom/+/68278?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5f0548f023fcd09a970148586497e00414ad1ae
Gerrit-Change-Number: 68278
Gerrit-PatchSet: 2
Gerrit-Owner: Joseph Goh <josephgoh7(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Joseph Goh <josephgoh7(a)gmail.com>
Gerrit-Comment-Date: Mon, 26 Jun 2023 11:35:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Joseph Goh <josephgoh7(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Ao Zhong.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75830?usp=email )
Change subject: flashchips.c: Adding support for ISSI IS25WP020/40/80
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/75830/comment/d97d7eb6_888cead1 :
PS4, Line 9: Hello,
: I added
Thank you, it is very nice of you to say hello :)
And now since everyone saw this already, probably better to re-word the commit message slightly, let's say:
"This patch adds support for ... <the rest of message>"
Patchset:
PS4:
Thank you Ao, I checked the patch, all good! I have just one comment about commit message and that would be it.
--
To view, visit https://review.coreboot.org/c/flashrom/+/75830?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8a786de5cf9ffefb2d57f89bbab71e289b5c2b28
Gerrit-Change-Number: 75830
Gerrit-PatchSet: 4
Gerrit-Owner: Ao Zhong <hacc1225(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Ao Zhong <hacc1225(a)gmail.com>
Gerrit-Comment-Date: Mon, 26 Jun 2023 11:12:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Ao Zhong has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75830?usp=email )
Change subject: flashchips.c: Adding support for ISSI IS25WP020/40/80
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS1:
> Hello Ao, thank you for the patch! […]
Done :-)
--
To view, visit https://review.coreboot.org/c/flashrom/+/75830?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8a786de5cf9ffefb2d57f89bbab71e289b5c2b28
Gerrit-Change-Number: 75830
Gerrit-PatchSet: 4
Gerrit-Owner: Ao Zhong <hacc1225(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 25 Jun 2023 01:50:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment