Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth - Personal, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 12:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61854/comment/208fba67_bb2c08ea
PS7, Line 17: Without this synchronisation being implemented, flashrom is running
: into below error:
:
: Erasing and writing flash chip... Timeout error between offset
: 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED!
: Uh oh. Erase/write failed. Checking if anything has changed.
> > Now I'm feeling trolled. How often have I told you that the SCIP bit isn't related
> > to sync'ing multiple processes? I've lost count.
> >
> > I guess after all, it's best if you'd write a commit message about SCPI only and
> > leave the confusion out of it. Also, please don't add a reference to the bug
> > tracker. I think that would be the best way to avoid further confusion. If you
> > still have questions, please ask your colleagues. No need to bother independent
> > open-source projects with Google-internal confusion.
>
> if removing the bug id makes you happy and let this CL see the light, I'm happy to move in that way.
Please let me know if anything we need to help for moving this CL?
https://review.coreboot.org/c/flashrom/+/61854/comment/e9e51a4c_68d0797d
PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout.
> > > > how since then so many years AU worked without being bothered about checking this SCIP bit in HW SEQ on older platform, I don't have that answer either with me. But for sure SW SEQ platform do use this SCIP bit checking.
> >
> > cros-flashrom used software sequencing for a long time because some implementations of hardware sequencing did not provide a way to write all of the status registers on recent flash chips. This meant that write protection could not be set up using hwseq.
>
> Yes David, you are spot on, I know what you mean here, SPI status register 2/3 is not supported using hw seq, hence a hybrid model is the only way. But starting from ADL, chipset doesn't support the sw seq, so sw seq is the only way out here. (I remember reading this is some ADL spec but can double confirm)
>
> >
> > As you pointed out, software sequencing has checked the SCIP bit since commit 01d05914 when Carl-Daniel added it over a decade ago.
>
> Yes.
>
> > Obviously because futility moved from calling cros flashrom to using lib-
> flashrom lately. Why do I know that?
>
> Don't want to argue but there is no evidence about other chipsets adopted libflashrom lately shows the same failure during AU except ADL. So, we are covering all possible to angle to fix this issue and adopt Intel's recommendation.
Please let me know if anything we need to help for moving this CL?
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 12
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 28 Feb 2022 06:03:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Nico Huber, Rizwan Qureshi, Edward O'Callaghan, Angel Pons.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62251 )
Change subject: ichspi: Add Alder Lake support
......................................................................
Patch Set 11:
(1 comment)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/62251/comment/a286c447_14b6e108
PS11, Line 1063: CHIPSET_600_SERIES_ALDER_POINT
> `guess_ich_chipset_from_content()` needs to be updated accordingly.
Agree, we need to add at https://review.coreboot.org/c/flashrom/+/62251/11/ich_descriptors.c#1040 to identify the chipset proper atleast using new CPU Soft Strap Length (CSSL) field. As per SPI programming guide, the value for CSSL is 0x14 on ADL.
if (content->CSSL == 0x14)
return CHIPSET_600_SERIES_ALDER_POINT;
Also you can consider changing the default to 600 series now as latest
msg_pwarn("Unknown flash descriptor, assuming 600 series compatibility.\n");
return CHIPSET_600_SERIES_ALDER_POINT;
--
To view, visit https://review.coreboot.org/c/flashrom/+/62251
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie66cf519df13f3391c41f5016b16a81ef3dfd4bf
Gerrit-Change-Number: 62251
Gerrit-PatchSet: 11
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 28 Feb 2022 05:39:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 )
Change subject: pcidev: Move pci_get_dev() logic into canonical place
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/59276/comment/daf41a2b_12732784
PS4, Line 1097: pcidev_getdev1
Looks like you have named this function based on it's last argument which is `func`.
Possible to create one helper function as pcidev_getdevfn() and then pcidev_getdev1() calls into this with passing function number?
struct pci_dev *pcidev_getdevfn(struct pci_dev *dev, const int func)
{
#if !defined(OLD_PCI_GET_DEV)
return pci_get_dev(pacc, dev->domain, dev->bus, dev->dev, func);
#else
/* pciutils/libpci before version 2.2 is too old to support
* PCI domains. Such old machines usually don't have domains
* besides domain 0, so this is not a problem.
*/
return pci_get_dev(pacc, dev->bus, dev->dev, func);
#endif
}
struct pci_dev *pcidev_getdev1(struct pci_dev *dev)
{
return pcidev_getdevfn(dev, 1);
}
Then you can actually replace call pci_get_dev() function to pcidev_getdevfn() or pcidev_getdev1() as applicable?
$ grep -rsn "pci_get_dev" .
./pcidev.c:314: undo_pci_write_data->dev = pci_get_dev(pacc, \
./chipset_enable.c:932: struct pci_dev *const spi_dev = pci_get_dev(pci_acc, dev->domain, dev->bus, slot, func);
./Makefile.d/pci_old_get_dev_test.c:15: dev = pci_get_dev(pacc, dev->bus, dev->dev, 1);
--
To view, visit https://review.coreboot.org/c/flashrom/+/59276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7
Gerrit-Change-Number: 59276
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 28 Feb 2022 05:25:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58570
to look at the new patch set (#31).
Change subject: spi25_statusreg,flashchips: add SR2 read/write support
......................................................................
spi25_statusreg,flashchips: add SR2 read/write support
This patch adds support for reading and writing the second status
register and enables it on a limited set of flash chips.
Chip support for RDSR2/WRSR2/extended WRSR is represented using feature
flags to be consistent with how other SPI capabilities are represented.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom -{r,w,E}
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series
TEST=logged SR2 read/write values during wp commands
Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M flash.h
M flashchips.c
M spi.h
M spi25_statusreg.c
4 files changed, 53 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/58570/31
--
To view, visit https://review.coreboot.org/c/flashrom/+/58570
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db
Gerrit-Change-Number: 58570
Gerrit-PatchSet: 31
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58477
to look at the new patch set (#38).
Change subject: flash.h,flashchips.c: add writeprotect bit layout map to chips
......................................................................
flash.h,flashchips.c: add writeprotect bit layout map to chips
This patch adds a register bit map `struct reg_bit_info`, with fields
for storing the register, bit index, and writability of each bit that
affects the chip's write protection. This allows writeprotect code to be
independent of the register layout of any specific chip. The new fields
have been filled out for example chips.
The representation is centered around describing how bits can be
accessed and modified, rather than the layout of registers. This is
generally easier to work with in code that needs to access specific bits
and typically requires specifying the locations of fewer bits overall.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series
Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M flash.h
M flashchips.c
M writeprotect.h
3 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/58477/38
--
To view, visit https://review.coreboot.org/c/flashrom/+/58477
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284
Gerrit-Change-Number: 58477
Gerrit-PatchSet: 38
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58478
to look at the new patch set (#40).
Change subject: writeprotect.h: add structure to represent chip wp configuration bits
......................................................................
writeprotect.h: add structure to represent chip wp configuration bits
Add `struct wp_bits` for representing values of all WP bits in a chip's
status/config register(s).
It allows most WP code to store and manipulate a chip's configuration
without knowing the exact layout of bits in the chip's status registers.
Supporting other chips may require additional fields to be added to the
structure.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series
Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M writeprotect.h
1 file changed, 36 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/58478/40
--
To view, visit https://review.coreboot.org/c/flashrom/+/58478
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809
Gerrit-Change-Number: 58478
Gerrit-PatchSet: 40
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58479
to look at the new patch set (#45).
Change subject: libflashrom,writeprotect: add functions for reading/writing WP configs
......................................................................
libflashrom,writeprotect: add functions for reading/writing WP configs
New functions are exposed through the libflashrom API for
reading/writing chip's WP settins: `flashrom_wp_{read,write}_cfg()`.
They read/write an opaque `struct flashrom_wp_cfg` instance, which
includes the flash protection range and status register protection mode.
This commit also adds `{read,write}_wp_bits()` helper functions that
read/write chip-specific WP configuration bits.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series
Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M libflashrom.c
M libflashrom.h
M writeprotect.c
M writeprotect.h
4 files changed, 363 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/58479/45
--
To view, visit https://review.coreboot.org/c/flashrom/+/58479
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad
Gerrit-Change-Number: 58479
Gerrit-PatchSet: 45
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/58475 )
Change subject: spi25_statusreg: make register read/write functions generic
......................................................................
spi25_statusreg: make register read/write functions generic
This patch adds new spi_{read,write}_register() functions that take the
source/destination register as an argument. Currently they can only
access SR1, support for other registers will be added in another patch.
Since we're refactoring things, this commit also makes
spi_read_register() return an error code, making it possible to identify
error conditions that spi_read_status_register() concealed.
This also removes the initial 100ms delay between writing a register and
the first attempt to check the chip's status. An initial delay was added
to avoid needing to read the status register multiple times, but that is
unlikely to cause problems on modern flash chips.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom -{r,w,E}
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series
Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/58475
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M chipdrivers.h
M flash.h
M spi25_statusreg.c
3 files changed, 99 insertions(+), 43 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Edward O'Callaghan: Looks good to me, approved
Anastasia Klimchuk: Looks good to me, approved
diff --git a/chipdrivers.h b/chipdrivers.h
index e1d6aa9..ea8d480 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -62,8 +62,10 @@
/* spi25_statusreg.c */
+/* FIXME: replace spi_read_status_register() calls with spi_read_register() */
uint8_t spi_read_status_register(const struct flashctx *flash);
-int spi_write_status_register(const struct flashctx *flash, int status);
+int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value);
+int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value);
void spi_prettyprint_status_register_bit(uint8_t status, int bit);
int spi_prettyprint_status_register_plain(struct flashctx *flash);
int spi_prettyprint_status_register_default_welwip(struct flashctx *flash);
diff --git a/flash.h b/flash.h
index 654cdee..f1a8b34 100644
--- a/flash.h
+++ b/flash.h
@@ -166,6 +166,13 @@
#define flashctx flashrom_flashctx /* TODO: Agree on a name and convert all occurences. */
typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
+enum flash_reg {
+ INVALID_REG = 0,
+ STATUS1,
+ STATUS2,
+ MAX_REGISTERS
+};
+
struct flashchip {
const char *vendor;
const char *name;
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index a0b0fcf..0d7bc25 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -22,24 +22,23 @@
#include "spi.h"
/* === Generic functions === */
-static int spi_write_status_register_flag(const struct flashctx *flash, int status, const unsigned char enable_opcode)
+static int spi_write_register_flag(const struct flashctx *flash, uint8_t enable_opcode, uint8_t *write_cmd, size_t write_cmd_len, enum flash_reg reg)
{
- int result;
- int i = 0;
/*
- * WRSR requires either EWSR or WREN depending on chip type.
- * The code below relies on the fact hat EWSR and WREN have the same
- * INSIZE and OUTSIZE.
+ * Enabling register writes requires either EWSR or WREN depending on
+ * chip type. The code below relies on the fact hat EWSR and WREN have
+ * the same INSIZE and OUTSIZE.
*/
+
struct spi_command cmds[] = {
{
.writecnt = JEDEC_WREN_OUTSIZE,
- .writearr = (const unsigned char[]){ enable_opcode },
+ .writearr = &enable_opcode,
.readcnt = 0,
.readarr = NULL,
}, {
- .writecnt = JEDEC_WRSR_OUTSIZE,
- .writearr = (const unsigned char[]){ JEDEC_WRSR, (unsigned char) status },
+ .writecnt = write_cmd_len,
+ .writearr = write_cmd,
.readcnt = 0,
.readarr = NULL,
}, {
@@ -49,69 +48,117 @@
.readarr = NULL,
}};
- result = spi_send_multicommand(flash, cmds);
+ int result = spi_send_multicommand(flash, cmds);
if (result) {
msg_cerr("%s failed during command execution\n", __func__);
- /* No point in waiting for the command to complete if execution
+ /*
+ * No point in waiting for the command to complete if execution
* failed.
*/
return result;
}
- /* WRSR performs a self-timed erase before the changes take effect.
+
+ /*
+ * WRSR performs a self-timed erase before the changes take effect.
* This may take 50-85 ms in most cases, and some chips apparently
* allow running RDSR only once. Therefore pick an initial delay of
* 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+ *
+ * Newer chips with multiple status registers (SR2 etc.) are unlikely
+ * to have problems with multiple RDSR commands, so only wait for the
+ * initial 100 ms if the register we wrote to was SR1.
*/
- programmer_delay(100 * 1000);
- while (spi_read_status_register(flash) & SPI_SR_WIP) {
- if (++i > 490) {
- msg_cerr("Error: WIP bit after WRSR never cleared\n");
- return TIMEOUT_ERROR;
- }
+ int delay_ms = 5000;
+ if (reg == STATUS1) {
+ programmer_delay(100 * 1000);
+ delay_ms -= 100;
+ }
+
+ for (; delay_ms > 0; delay_ms -= 10) {
+ if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0)
+ return 0;
programmer_delay(10 * 1000);
}
- return 0;
+
+ msg_cerr("Error: WIP bit after WRSR never cleared\n");
+ return TIMEOUT_ERROR;
}
-int spi_write_status_register(const struct flashctx *flash, int status)
+int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value)
{
int feature_bits = flash->chip->feature_bits;
- int ret = 1;
+
+ uint8_t write_cmd[3];
+ size_t write_cmd_len = 0;
+
+ /*
+ * Create SPI write command sequence based on the destination register
+ * and the chip's supported command set.
+ */
+ switch (reg) {
+ case STATUS1:
+ write_cmd[0] = JEDEC_WRSR;
+ write_cmd[1] = value;
+ write_cmd_len = JEDEC_WRSR_OUTSIZE;
+ break;
+ default:
+ msg_cerr("Cannot write register: unknown register\n");
+ return 1;
+ }
if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
msg_cdbg("Missing status register write definition, assuming "
"EWSR is needed\n");
feature_bits |= FEATURE_WRSR_EWSR;
}
+
+ int ret = 1;
if (feature_bits & FEATURE_WRSR_WREN)
- ret = spi_write_status_register_flag(flash, status, JEDEC_WREN);
+ ret = spi_write_register_flag(flash, JEDEC_WREN, write_cmd, write_cmd_len, reg);
if (ret && (feature_bits & FEATURE_WRSR_EWSR))
- ret = spi_write_status_register_flag(flash, status, JEDEC_EWSR);
+ ret = spi_write_register_flag(flash, JEDEC_EWSR, write_cmd, write_cmd_len, reg);
return ret;
}
+int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value)
+{
+ uint8_t read_cmd;
+
+ switch (reg) {
+ case STATUS1:
+ read_cmd = JEDEC_RDSR;
+ break;
+ default:
+ msg_cerr("Cannot read register: unknown register\n");
+ return 1;
+ }
+
+ /* FIXME: No workarounds for driver/hardware bugs in generic code. */
+ /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
+ uint8_t readarr[2];
+
+ int ret = spi_send_command(flash, sizeof(read_cmd), sizeof(readarr), &read_cmd, readarr);
+ if (ret) {
+ msg_cerr("Register read failed!\n");
+ return ret;
+ }
+
+ *value = readarr[0];
+ return 0;
+}
+
uint8_t spi_read_status_register(const struct flashctx *flash)
{
- static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
- /* FIXME: No workarounds for driver/hardware bugs in generic code. */
- unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
- int ret;
-
- /* Read Status Register */
- ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd, readarr);
- if (ret) {
- msg_cerr("RDSR failed!\n");
- /* FIXME: We should propagate the error. */
- return 0;
- }
-
- return readarr[0];
+ uint8_t status = 0;
+ /* FIXME: We should propagate the error. */
+ spi_read_register(flash, STATUS1, &status);
+ return status;
}
static int spi_restore_status(struct flashctx *flash, uint8_t status)
{
msg_cdbg("restoring chip status (0x%02x)\n", status);
- return spi_write_status_register(flash, status);
+ return spi_write_register(flash, STATUS1, status);
}
/* A generic block protection disable.
@@ -156,9 +203,9 @@
return 1;
}
/* All bits except the register lock bit (often called SPRL, SRWD, WPEN) are readonly. */
- result = spi_write_status_register(flash, status & ~lock_mask);
+ result = spi_write_register(flash, STATUS1, status & ~lock_mask);
if (result) {
- msg_cerr("spi_write_status_register failed.\n");
+ msg_cerr("Could not write status register 1.\n");
return result;
}
status = spi_read_status_register(flash);
@@ -169,9 +216,9 @@
msg_cdbg("done.\n");
}
/* Global unprotect. Make sure to mask the register lock bit as well. */
- result = spi_write_status_register(flash, status & ~(bp_mask | lock_mask) & unprotect_mask);
+ result = spi_write_register(flash, STATUS1, status & ~(bp_mask | lock_mask) & unprotect_mask);
if (result) {
- msg_cerr("spi_write_status_register failed.\n");
+ msg_cerr("Could not write status register 1.\n");
return result;
}
status = spi_read_status_register(flash);
24 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58475
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7
Gerrit-Change-Number: 58475
Gerrit-PatchSet: 27
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: merged