Attention is currently required from: Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, 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 4:
(1 comment)
Patchset:
PS2:
> > > > > This leaves some questions. Is something broken wrt. arbitration? How does
> > > > arbitration work if two masters set FGO at the same time? is that accounted
> > > > for in the hardware?
> > > >
> > > > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served.
> > >
> > > > Thank you Rizwan. That's how I always understood it. However, Subrata's test
> > > result suggest otherwise:
> > >
> > > My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction.
> > >
> > > Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue.
> >
> > As I said there is no way to synchronize this with the SCIP bit alone.
>
> Can you please share your thoughts about how the sync should have been taken care without a HW sync bit at each master operation? Additionally, what is your thought behind running more than one application that might potentially use flashrom concurrently being same context master? In such case, how those processes can avoid entering into SPI access move using same flashrom utility?
> Don't we need simple semaphore logic to prohibit more than instance to trigger the SPI access using flashrom.
>
> Here is a scenario and please suggest how would you implement such sync: an application example: set_gbb.sh (which might internally use flashrom) would know if some flash operations are in progress using flashrom (example: futility) at the same time? Eventually multiple flashrom utilities would start executing in parallel and without any lock mechanism, we are meant to run into such a timeout error.
>
> The recommendation is to check H_SCIP bit before starting a transaction. This indicates a transaction by the same master is in progress.
>
> > Trying to
> > do so makes the code look rather ill-considered. Running two programs using these
> > registers at once has many more implications, e.g. one process could clear status
> > bits that another is waiting for.
>
> This is what is happening now. You could argue saying, we could use the file locking mechanism to avoid multiple program to use the same flashrom while one operation is in progress. Isn't having HW register (SCIP) is better than SW file locking logic.
>
> > It seems mind-boggling to continue the program
> > when it detects that another one is accessing the registers.
>
> This is the point, how do you even *detect* this today that *another* one is accessing register without knowing the busy bit register. And SCIP polling is the way to prevent such access and wait till SPI bus is freely available before initiate newer command with FGO. (This is my bad that I overlooked SCIP polling at first place in SKL days during HW sequencing implemented in coreboot and later flashrom just ported coreboot fast spi driver hence, missed this too).
>
> >
> > Please describe your use-case including this other tool in detail and bring it
> > to the mailing list <flashrom(a)flashrom.org>.
>
> I have added Edward to help on this regard.
>
> > We can try to help finding a solution
> > to your problems but not if the problem description stays vague, changes at your
> > convenience and earlier test results that suggest that your hardware does something
> > wrong are ignored.
>
> Sorry I didn't get your point.
Can you please try this at your side
- Run `flashrom -p host -r read.bin` in a loop while running the flashrom write test.
we are seeing below error:
Transaction error between offset 0x003f5680 and 0x003f56bf (= 0x003f5680 + 63)!
Read operation failed!
--
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: 4
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 18:35:42 +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: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, 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 4:
(1 comment)
Patchset:
PS2:
> > > > This leaves some questions. Is something broken wrt. arbitration? How does
> > > arbitration work if two masters set FGO at the same time? is that accounted
> > > for in the hardware?
> > >
> > > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served.
> >
> > > Thank you Rizwan. That's how I always understood it. However, Subrata's test
> > result suggest otherwise:
> >
> > My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction.
> >
> > Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue.
>
> As I said there is no way to synchronize this with the SCIP bit alone.
Can you please share your thoughts about how the sync should have been taken care without a HW sync bit at each master operation? Additionally, what is your thought behind running more than one application that might potentially use flashrom concurrently being same context master? In such case, how those processes can avoid entering into SPI access move using same flashrom utility?
Don't we need simple semaphore logic to prohibit more than instance to trigger the SPI access using flashrom.
Here is a scenario and please suggest how would you implement such sync: an application example: set_gbb.sh (which might internally use flashrom) would know if some flash operations are in progress using flashrom (example: futility) at the same time? Eventually multiple flashrom utilities would start executing in parallel and without any lock mechanism, we are meant to run into such a timeout error.
The recommendation is to check H_SCIP bit before starting a transaction. This indicates a transaction by the same master is in progress.
> Trying to
> do so makes the code look rather ill-considered. Running two programs using these
> registers at once has many more implications, e.g. one process could clear status
> bits that another is waiting for.
This is what is happening now. You could argue saying, we could use the file locking mechanism to avoid multiple program to use the same flashrom while one operation is in progress. Isn't having HW register (SCIP) is better than SW file locking logic.
> It seems mind-boggling to continue the program
> when it detects that another one is accessing the registers.
This is the point, how do you even *detect* this today that *another* one is accessing register without knowing the busy bit register. And SCIP polling is the way to prevent such access and wait till SPI bus is freely available before initiate newer command with FGO. (This is my bad that I overlooked SCIP polling at first place in SKL days during HW sequencing implemented in coreboot and later flashrom just ported coreboot fast spi driver hence, missed this too).
>
> Please describe your use-case including this other tool in detail and bring it
> to the mailing list <flashrom(a)flashrom.org>.
I have added Edward to help on this regard.
> We can try to help finding a solution
> to your problems but not if the problem description stays vague, changes at your
> convenience and earlier test results that suggest that your hardware does something
> wrong are ignored.
Sorry I didn't get your point.
--
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: 4
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 17:55:51 +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: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg.
Hello build bot (Jenkins), Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Edward O'Callaghan, Sridhar Siricilla, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/61854
to look at the new patch set (#4).
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
As per EDS, SPI controller sets the HSFSTS.bit5 (SCIP) when software
sets the Flash Cycle Go (FGO) bit in the Hardware Sequencing Flash
Control register.
This bit remains set until the cycle completes on the SPI interface.
Hardware automatically sets and clears this bit so that software can
determine when read data is valid and/or when it is safe to begin
programming the next command.
Software must initiate the next SPI transaction when this bit is 0.
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.
Added blocking mechanism without timeout to ensure previous SPI
transaction is complete before initiating newer command.
Current debug data suggests that concurrent access to flashrom by user
space utility might run into issues where some operations are getting
timed out without synchronisation.
BUG=b:215255210
TEST=Concurrent flashrom access is not throwing timeout.
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
---
M ichspi.c
1 file changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/61854/4
--
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: 4
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Subrata Banik, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg.
Nico Huber 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 3: Code-Review-1
(1 comment)
Patchset:
PS2:
> > > This leaves some questions. Is something broken wrt. arbitration? How does
> > arbitration work if two masters set FGO at the same time? is that accounted
> > for in the hardware?
> >
> > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served.
>
> > Thank you Rizwan. That's how I always understood it. However, Subrata's test
> result suggest otherwise:
>
> My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction.
>
> Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue.
As I said there is no way to synchronize this with the SCIP bit alone. Trying to
do so makes the code look rather ill-considered. Running two programs using these
registers at once has many more implications, e.g. one process could clear status
bits that another is waiting for. It seems mind-boggling to continue the program
when it detects that another one is accessing the registers.
Please describe your use-case including this other tool in detail and bring it
to the mailing list <flashrom(a)flashrom.org>. We can try to help finding a solution
to your problems but not if the problem description stays vague, changes at your
convenience and earlier test results that suggest that your hardware does something
wrong are ignored.
--
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: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 12:41:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Caveh Jalali, Tim Wawrzynczak, Edward O'Callaghan, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, 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 3:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/61854/comment/4dfdbbba_5cadfb3f
PS3, Line 1392: while
> @Angel pons already pointed this! I'm closing the comment here.
@Sridhar, Rizwan suggested me this is what CSE also does, kind of blocking.
if you see my CLs in coreboot and DC, I have used the non-blocking call but with flashrom, don't want to take the risk because it will get consumed in multithreaded env
https://review.coreboot.org/c/coreboot/+/61849/4/src/soc/intel/common/block…
--
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: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 04:34:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg.
Sridhar Siricilla 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 3:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/61854/comment/296f0dc9_2c37ec4d
PS3, Line 1392: while
I think we need to connect this loop with some timeout(may be 15sec considering higher sider). Worst case scenario, tool may stuck here. Agree?
--
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: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 04:20:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Edward O'Callaghan, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, 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 3:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61854/comment/2e18981e_edabd184
PS2, Line 23: BUG=b:215255210
> I have added the issue description below @Nico. […]
Ack
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/61854/comment/87be0750_23c1aa86
PS1, Line 1390: REGREAD32
> > Isn't this register 16-bit? […]
Ack
https://review.coreboot.org/c/flashrom/+/61854/comment/e7ce3e1a_33238220
PS1, Line 1392: } while ((hsfsts & HSFS_SCIP) == HSFS_SCIP);
> > This loop could potentially never exit.
>
> It depends on HW operation and here is what EDS says, so, I believe we might not run into such issue. Also, *Software must only program the next command when this bit is 0.*
>
> SPI Cycle In Progress (H_SCIP): Hardware sets this bit when software sets the
> Flash Cycle Go (FGO) bit in the Hardware Sequencing Flash Control register.
> This bit remains set until the cycle completes on the SPI interface. Hardware
> automatically sets and clears this bit so that software can determine when read data
> is valid and/or when it is safe to begin programming the next command.
> Software must only program the next command when this bit is 0.
This will ensure synchronisation in operations while using flashrom concurrently.
--
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: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sat, 12 Feb 2022 04:07:26 +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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment