February 2022 - flashrom-gerrit

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, 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 2: (1 comment) Patchset: PS2: > > > > I guess a simple experiment could give a hint if it does: The hardware is also > > > > able to start cycles on its own for memory-mapped flash access. You could test > > > > if this toggles the SCIP bit in the host master interface. For instance, poll > > > > SCIP and then read from the memory-mapped BIOS area. If polling too fast is an > > > > issue, I'd measure the time of a short cycle, then take half of that as the > polling > > > delay. > > > > > > I believe this scenario is valid while we are booting from SPI memory mapped memory (till we have resources are memory mapped) and not in the case of while we are running the flashrom from OS. There should not be any code which is getting fetched from XIP/SPI mapped memory while BIOS is done. > > > > Doesn't matter what is valid. If it toggles the host master SCIP bit, than there > > is definitely something to do in flashrom. If it doesn't, that says nothing, but > > we could continue to investigate and see if CSE activity toggles the host SCIP bit. > > CSE access the SPI for read hence, it will for sure flip the SCIP. No doubt about that. Have you tested that (and with that I mean that the *host* SCIP bit flips when the CSE uses its own interface)? (sorry to ask dump questions, but so far you haven't told us much beside the background story) -- 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: 2 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: 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> 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-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: Fri, 11 Feb 2022 15:57:23 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > > > > > While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. > > > > > > > > Was upstream flashrom used? > > > > > > Yup. > > So is the issue easy to reproduce on the affected systems? It's not easy for sure but we are able to replicate. > > > > > > > > Why do you assume that the CSE is involved? > > > > > > Because Chrome OS needs few features that involve CSE to start it's services post booting to OS. Those services might further need to fetch the CSE active partition residing in SPI at regular intervals. We suspect flashrom operation is conflicting with such access as flashrom doesn't sync based on the SCIP bit. > > Please describe the actual symptoms, error messages? data corruption? > Something must have led to this suspicion, right? > > > > > > > > > I would expect the hardware to coordinate between the different masters. > > > > > > Unfortunately, it seems doesn't support multimaster protocol. > > I don't know any such thing. What I am referring to is that Intel calls the > hardware blocks with SPI access (CSE, host, GbE etc.) masters. Somehow these > must be coordinated. It's possible that one master can toggle the SCIP bits > of all masters at once. It's possible that this is all there is. I doubt it, > but it's possible. That there is no hardware synchronization at all would be > very odd. We are waiting to understand how HW arbitration actually works between those agents. So, far we don't have much answer. > > > > > > > > At least, > > > > the SCIP bit can never be enough to synchronize masters. Even with the added > > > > waiting loop, there is still much room for race conditions (e.g. second master > > > > reading SCIP = 0 before the first posted its write to start a cycle). > > > > > > I believe this sync might work like this. > > > When first master is creating the command and haven't set the FGO bit. There is no operation that is taking place so, 2nd master can read SCIP = 0 and if 2nd master wish to start a new command, it has still room for that. But immediately after FGO is set, the HW logic will flip the SCIP bit so, 2nd master if about to start the new command, it has to in wait loop till 1st master finishes the operation. And I have created the blocking logic. At based on the initial code analysis, CSE FW will ensure not to intrude into host cpu operation but host CPU (using flashrom) might run into issue because CSE is using SPI bus for read operation hence flashrom operation might fail. > > It seems you ignore hardware concurrency. What do you think happens if > 1st master reads SCIP = 0, 2nd master reads SCIP = 0, and both write > FGO = 1. There is no magic that would make any of the masters return > to the waiting loop. Yes, this is valid concern. And even with this implementation, we still might have few % chance that we may see a failure. What I have suggested is to have a retry implementation inside flashrom for giving you an insurance. The example that you have given, if we assume that CSE attempt to get the bus and Flashrom fail, but it can retry again. On the next attempt, it might see the SPI cycle is busy and wait there. isn't it? > > > > > > I guess a simple experiment could give a hint if it does: The hardware is also > > > able to start cycles on its own for memory-mapped flash access. You could test > > > if this toggles the SCIP bit in the host master interface. For instance, poll > > > SCIP and then read from the memory-mapped BIOS area. If polling too fast is an > > > issue, I'd measure the time of a short cycle, then take half of that as the > polling > > delay. > > > > I believe this scenario is valid while we are booting from SPI memory mapped memory (till we have resources are memory mapped) and not in the case of while we are running the flashrom from OS. There should not be any code which is getting fetched from XIP/SPI mapped memory while BIOS is done. > > Doesn't matter what is valid. If it toggles the host master SCIP bit, than there > is definitely something to do in flashrom. If it doesn't, that says nothing, but > we could continue to investigate and see if CSE activity toggles the host SCIP bit. CSE access the SPI for read hence, it will for sure flip the SCIP. No doubt about that. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 14:50:19 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > > > > While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. > > > > > > Was upstream flashrom used? > > > > Yup. So is the issue easy to reproduce on the affected systems? > > > > > Why do you assume that the CSE is involved? > > > > Because Chrome OS needs few features that involve CSE to start it's services post booting to OS. Those services might further need to fetch the CSE active partition residing in SPI at regular intervals. We suspect flashrom operation is conflicting with such access as flashrom doesn't sync based on the SCIP bit. Please describe the actual symptoms, error messages? data corruption? Something must have led to this suspicion, right? > > > > > > I would expect the hardware to coordinate between the different masters. > > > > Unfortunately, it seems doesn't support multimaster protocol. I don't know any such thing. What I am referring to is that Intel calls the hardware blocks with SPI access (CSE, host, GbE etc.) masters. Somehow these must be coordinated. It's possible that one master can toggle the SCIP bits of all masters at once. It's possible that this is all there is. I doubt it, but it's possible. That there is no hardware synchronization at all would be very odd. > > > > > At least, > > > the SCIP bit can never be enough to synchronize masters. Even with the added > > > waiting loop, there is still much room for race conditions (e.g. second master > > > reading SCIP = 0 before the first posted its write to start a cycle). > > > > I believe this sync might work like this. > > When first master is creating the command and haven't set the FGO bit. There is no operation that is taking place so, 2nd master can read SCIP = 0 and if 2nd master wish to start a new command, it has still room for that. But immediately after FGO is set, the HW logic will flip the SCIP bit so, 2nd master if about to start the new command, it has to in wait loop till 1st master finishes the operation. And I have created the blocking logic. At based on the initial code analysis, CSE FW will ensure not to intrude into host cpu operation but host CPU (using flashrom) might run into issue because CSE is using SPI bus for read operation hence flashrom operation might fail. It seems you ignore hardware concurrency. What do you think happens if 1st master reads SCIP = 0, 2nd master reads SCIP = 0, and both write FGO = 1. There is no magic that would make any of the masters return to the waiting loop. > > > I guess a simple experiment could give a hint if it does: The hardware is also > > able to start cycles on its own for memory-mapped flash access. You could test > > if this toggles the SCIP bit in the host master interface. For instance, poll > > SCIP and then read from the memory-mapped BIOS area. If polling too fast is an > > issue, I'd measure the time of a short cycle, then take half of that as the > polling > delay. > > I believe this scenario is valid while we are booting from SPI memory mapped memory (till we have resources are memory mapped) and not in the case of while we are running the flashrom from OS. There should not be any code which is getting fetched from XIP/SPI mapped memory while BIOS is done. Doesn't matter what is valid. If it toggles the host master SCIP bit, than there is definitely something to do in flashrom. If it doesn't, that says nothing, but we could continue to investigate and see if CSE activity toggles the host SCIP bit. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> 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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 14:40:59 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > > > While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. > > > > Was upstream flashrom used? > > Yup. > > > Why do you assume that the CSE is involved? > > Because Chrome OS needs few features that involve CSE to start it's services post booting to OS. Those services might further need to fetch the CSE active partition residing in SPI at regular intervals. We suspect flashrom operation is conflicting with such access as flashrom doesn't sync based on the SCIP bit. > > > > I would expect the hardware to coordinate between the different masters. > > Unfortunately, it seems doesn't support multimaster protocol. > > > At least, > > the SCIP bit can never be enough to synchronize masters. Even with the added > > waiting loop, there is still much room for race conditions (e.g. second master > > reading SCIP = 0 before the first posted its write to start a cycle). > > I believe this sync might work like this. > When first master is creating the command and haven't set the FGO bit. There is no operation that is taking place so, 2nd master can read SCIP = 0 and if 2nd master wish to start a new command, it has still room for that. But immediately after FGO is set, the HW logic will flip the SCIP bit so, 2nd master if about to start the new command, it has to in wait loop till 1st master finishes the operation. And I have created the blocking logic. At based on the initial code analysis, CSE FW will ensure not to intrude into host cpu operation but host CPU (using flashrom) might run into issue because CSE is using SPI bus for read operation hence flashrom operation might fail. > I guess a simple experiment could give a hint if it does: The hardware is also > able to start cycles on its own for memory-mapped flash access. You could test > if this toggles the SCIP bit in the host master interface. For instance, poll > SCIP and then read from the memory-mapped BIOS area. If polling too fast is an > issue, I'd measure the time of a short cycle, then take half of that as the > polling delay. I believe this scenario is valid while we are booting from SPI memory mapped memory (till we have resources are memory mapped) and not in the case of while we are running the flashrom from OS. There should not be any code which is getting fetched from XIP/SPI mapped memory while BIOS is done. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 14:15:23 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > > While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. > > Was upstream flashrom used? Yup. > Why do you assume that the CSE is involved? Because Chrome OS needs few features that involve CSE to start it's services post booting to OS. Those services might further need to fetch the CSE active partition residing in SPI at regular intervals. We suspect flashrom operation is conflicting with such access as flashrom doesn't sync based on the SCIP bit. > > I would expect the hardware to coordinate between the different masters. Unfortunately, it seems doesn't support multimaster protocol. > At least, > the SCIP bit can never be enough to synchronize masters. Even with the added > waiting loop, there is still much room for race conditions (e.g. second master > reading SCIP = 0 before the first posted its write to start a cycle). I believe this sync might work like this. When first master is creating the command and haven't set the FGO bit. There is no operation that is taking place so, 2nd master can read SCIP = 0 and if 2nd master wish to start a new command, it has still room for that. But immediately after FGO is set, the HW logic will flip the SCIP bit so, 2nd master if about to start the new command, it has to in wait loop till 1st master finishes the operation. And I have created the blocking logic. At based on the initial code analysis, CSE FW will ensure not to intrude into host cpu operation but host CPU (using flashrom) might run into issue because CSE is using SPI bus for read operation hence flashrom operation might fail. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 14:10:58 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > I would expect the hardware to coordinate between the different masters. I guess a simple experiment could give a hint if it does: The hardware is also able to start cycles on its own for memory-mapped flash access. You could test if this toggles the SCIP bit in the host master interface. For instance, poll SCIP and then read from the memory-mapped BIOS area. If polling too fast is an issue, I'd measure the time of a short cycle, then take half of that as the polling delay. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> 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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 13:56:38 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (1 comment) Patchset: PS2: > While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. Was upstream flashrom used? Why do you assume that the CSE is involved? I would expect the hardware to coordinate between the different masters. At least, the SCIP bit can never be enough to synchronize masters. Even with the added waiting loop, there is still much room for race conditions (e.g. second master reading SCIP = 0 before the first posted its write to start a cycle). -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> 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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 13:37:45 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/a15b4b76_3cb054e8 PS2, Line 23: BUG=b:215255210 > Please describe the issue you encountered. I have added the issue description below @Nico. So far we don't know if this fix will able to fix the original issue hence tried to capture at the high level what SW expects us to implement and what we missed. Patchset: PS2: > The whole driver works synchronously, i.e. waits for any cycle to finish > that it started itself. So unless I miss something, checking SCIP would > only serve us in case of hardware failures or another program trying to > use the SPI controller at the same time (e.g. broken SMM). In both cases, > I'd say all bets are off anyway and we should definitely bail out. > > Datasheets say to check the SCIP bit; we could just do so once (without > waiting) and if the bit is set bail out. @Nico, there are multiple HW agents who also has access to the SPI controller, example: CSE. While you are flashing the SPI using flashrom on DUT at S0, there are cases when we are seeing some flashrom failure issue and debug data that we have so far is not very conclusive to say that CSE is doing anything wrong. There might be case where both CSE and Host CPU attempt to initiate the transaction without any sync mechanism. Apparently on multiple master SPI access case, SW programming says, don't initiate next command unless SCIP is set to zero. We have verified CSE FW use this implementation to avoid race scenarios but none of HW seq implementation that we have in coreboot/depthcharge/flashrom adheres to the same recommendation so far. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 13:00:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Rizwan Qureshi, Edward O'Callaghan, Angel Pons, Nick Vaccaro, Alex Levin. 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 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/284f9c57_ac895439 PS2, Line 23: BUG=b:215255210 Please describe the issue you encountered. Patchset: PS2: The whole driver works synchronously, i.e. waits for any cycle to finish that it started itself. So unless I miss something, checking SCIP would only serve us in case of hardware failures or another program trying to use the SPI controller at the same time (e.g. broken SMM). In both cases, I'd say all bets are off anyway and we should definitely bail out. Datasheets say to check the SCIP bit; we could just do so once (without waiting) and if the bit is set bail out. -- 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: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> 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: 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-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Feb 2022 12:51:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michał Żygowski (Code Review) 11 Feb '22

11 Feb '22

Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Michael Niewöhner. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 27: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55715/comment/88e6fde8_d48c0c29 PS27, Line 1: acpi_ec > nit: ite_ecfw Actually no. apic_ec is on purpose. The patch implements the interface described in ACPI specification: https://uefi.org/specs/ACPI/6.4/12_ACPI_Embedded_Controller_Interface_Speci… ITE EC firmware is compliant with APCI specification to the extent of the registers and functions described in APCI spec and the source file added in this patch, -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 27 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Fri, 11 Feb 2022 12:43:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2022