Attention is currently required from: Nikolai Artemiev.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> I fully support this change, chips shouldn't require a delay unless they're clearing the WIP/BUSY fl […]
alright, I dropped the "RFC" tag 😊
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 2
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 21:05:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Hsuan-ting Chen, Nikolai Artemiev, Stefan Reinauer.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email )
Change subject: udelay: clock_getres() does not provide clock_gettime() resolution
......................................................................
Patch Set 2:
(1 comment)
File udelay.c:
https://review.coreboot.org/c/flashrom/+/80806/comment/ab83709c_eec9b4b6 :
PS1, Line 120: {
> (This comment is copied from my comment in CHROMIUM repo) […]
I feel like part of that comment belongs closer to where we set `use_clock_gettime`, so I split it up. I hope this is to your liking.
--
To view, visit https://review.coreboot.org/c/flashrom/+/80806?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Gerrit-Change-Number: 80806
Gerrit-PatchSet: 2
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 21:04:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Nikolai Artemiev.
Hello Nikolai Artemiev, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/80807?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+2 by Nikolai Artemiev, Verified+1 by build bot (Jenkins)
The change is no longer submittable: Code-Review and Verified are unsatisfied now.
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
flashrom: Don't throw around "delay 1 second" so lightly
Waiting a full second is a very long time, especially when
default_delay() chooses to busy-loop. This code has been around for a
decade, with vague references to user reports:
https://review.coreboot.org/plugins/gitiles/flashrom/+/8ab49e72af8465d4527d…
Still, this logic does not belong in the the high-level library logic,
used by all programmers and all chips. If there is a timing issue, it
should either be encoded in the appropriate programmer or flashchip
timing.
Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc
Signed-off-by: Brian Norris <briannorris(a)chromium.org>
---
M flashrom.c
1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/80807/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 2
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Brian Norris, Nikolai Artemiev, Stefan Reinauer.
Hello Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/80806?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+2 by Nikolai Artemiev, Verified+1 by build bot (Jenkins)
Change subject: udelay: clock_getres() does not provide clock_gettime() resolution
......................................................................
udelay: clock_getres() does not provide clock_gettime() resolution
POSIX doesn't give a clear definition of how clock_getres() and
clock_gettime() relate, but Linux makes this clearer. See:
https://git.kernel.org/linus/01679b5db7172b898be325ff272e10aebd412911
posix-timers: Document sys_clock_getres() correctly
The deleted comments are quite clear:
"Clock resolution is used to round up timer and interval times, NOT to
report clock times"
The new comments instead tell how the resolution is only related to
clock_settime() and to the precision of *timers* -- not to the precision
of clock_gettime() itself.
This is particularly relevant depending on the Linux sysctl `sysctl
kernel.timer_highres`. When enabled, resolution is always 1 nanosecond;
when disabled, it follow the CONFIG_HZ of the kernel in question. But in
neither case does this result actually assist determining the quality of
the use case in question: busy-looping over clock_gettime().
In practice, any system with a typical CPU clock will have good enough
clock_gettime() resolution, and trusting clock_getres() for this is
actively misleading (and, wasteful on systems with
kernel.timer_highres==0). Any system that doesn't trust the precision of
clock_gettime() should simply be building with HAS_CLOCK_GETTIME=no.
Therefore, avoid ever looking at clock_getres(), and instead always use
clock_gettime() if we're allowed to. This saves a bunch of wasted time
in "calibration", which is itself not actually accurate in the presence
of hybrid CPUs or CPU DFS. Notably, calibration time-wastage has been
the subject of a few past stalled efforts:
https://review.coreboot.org/c/flashrom/+/39864https://review.coreboot.org/c/flashrom/+/44517
BUG=b:325539928
TEST=flashrom never resorts to "Calibrating delay loop" when
clock_gettime is available
TEST=flashrom works at the same speed (wall clock time) with
`sysctl kernel.timer_highres=1` and `sysctl kernel.timer_highres=0`
Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Signed-off-by: Brian Norris <briannorris(a)chromium.org>
---
M udelay.c
1 file changed, 15 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/80806/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/80806?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Gerrit-Change-Number: 80806
Gerrit-PatchSet: 2
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81106?usp=email )
Change subject: doc: Update arbitration team to be flashrom specific
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/81106?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I1043b9499ab22da5ec981592d7b4311f027c4b5f
Gerrit-Change-Number: 81106
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 07 Mar 2024 18:26:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Stefan Reinauer.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: udelay: clock_getres() does not provide clock_gettime() resolution
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
I'd like to note that I tested on several ChromeOS devices: 1. MTK (ARM) and 2. Intel Apollo Lake.
Our clock sources (arch_sys_counter on ARM and tsc on Intel) are reliable and do not require additional calibration in flashrom. Therefore, I believe removing the calibration will not cause issues on ChromeOS.
I'm also aware that not all clock sources are trustworthy. For example, using acpi_pm on Intel platforms can lead to significantly larger errors in clock_usec_delay. (But it is still tolerable)
So from ChromeOS's perspect, I would think this change should be good to go. But I have no sufficient knowledge for any other devices
File udelay.c:
https://review.coreboot.org/c/flashrom/+/80806/comment/a73a3c25_1fe1068e :
PS1, Line 120: {
(This comment is copied from my comment in CHROMIUM repo)
What about add a comment here:
// We assume clock_gettime() provides sufficient precision; avoid unnecessary calibration overhead.
--
To view, visit https://review.coreboot.org/c/flashrom/+/80806?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Gerrit-Change-Number: 80806
Gerrit-PatchSet: 1
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Thu, 07 Mar 2024 11:33:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81106?usp=email )
Change subject: doc: Update arbitration team to be flashrom specific
......................................................................
doc: Update arbitration team to be flashrom specific
Same approach as it was before: founder and current project lead.
Change-Id: I1043b9499ab22da5ec981592d7b4311f027c4b5f
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/about_flashrom/code_of_conduct.rst
1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/81106/1
diff --git a/doc/about_flashrom/code_of_conduct.rst b/doc/about_flashrom/code_of_conduct.rst
index da94e29..2967095 100644
--- a/doc/about_flashrom/code_of_conduct.rst
+++ b/doc/about_flashrom/code_of_conduct.rst
@@ -126,10 +126,8 @@
Our arbitration team consists of the following people
+* Anastasia Klimchuk <aklm(a)flashrom.org> (Australia)
* Stefan Reinauer <stefan.reinauer(a)coreboot.org> (USA)
-* Patrick Georgi <patrick(a)coreboot.org> (Germany)
-* Ronald Minnich <rminnich(a)coreboot.org> (USA)
-* Martin Roth <martin(a)coreboot.org> (USA)
License and attribution
=======================
--
To view, visit https://review.coreboot.org/c/flashrom/+/81106?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I1043b9499ab22da5ec981592d7b4311f027c4b5f
Gerrit-Change-Number: 81106
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newchange
Hsuan-ting Chen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81114?usp=email )
Change subject: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
......................................................................
classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
Update technical details for custom_rst of raiden_debug_spi to help
users better understand their configuration options.
BUG=b:161745002
BRANCH=none
TEST=none
Change-Id: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0
Signed-off-by: Hsuan Ting Chen <roccochen(a)google.com>
---
M doc/classic_cli_manpage.rst
1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/81114/1
diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst
index c46edf6..4b2c9a9 100644
--- a/doc/classic_cli_manpage.rst
+++ b/doc/classic_cli_manpage.rst
@@ -899,11 +899,14 @@
The servo device serial number can be found via ``lsusb``.
Raiden will poll the ``ap`` target waiting for the system power to settle on the AP and EC flash devices.
-The optional ``custom_rst=true`` parameter changes the timeout value from 3ms to 10ms::
+The optional ``custom_rst=true`` parameter alters the behavior of the reset process::
flashrom -p raiden_debug_spi:custom_rst=<true|false>
-syntax, where ``custom_rst=false`` is the implicit default timeout of 3ms. More information about the ChromiumOS servo
+syntax, where ``custom_rst=false`` is the implicit default timeout of 3ms and ``custom_rst=true`` set ``RAIDEN_DEBUG_SPI_REQ_ENABLE_AP_CUSTOM`` instead of ``RAIDEN_DEBUG_SPI_REQ_ENABLE_AP``.
+This custom reset modify the timeout from 3ms to 10ms and will not set ``EC_RST_L``, meaning neither the EC nor the AP will be reset. With this setting, it's the user's responsibility to manage the reset signal manually or by configuring the GPIO.
+Failure to handle the reset signal appropriately will likely result in flashing errors.
+More information about the ChromiumOS servo
hardware is available at `servos website <https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/HEAD/do…>`_.
--
To view, visit https://review.coreboot.org/c/flashrom/+/81114?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0
Gerrit-Change-Number: 81114
Gerrit-PatchSet: 1
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Brian Norris.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80808?usp=email )
Change subject: udelay: Lower the sleep vs delay threshold
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/80808?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ifd4821c66c5564f7c975c08769a6742f645e9be0
Gerrit-Change-Number: 80808
Gerrit-PatchSet: 1
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Comment-Date: Wed, 06 Mar 2024 23:51:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Stefan Reinauer.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email )
Change subject: udelay: clock_getres() does not provide clock_gettime() resolution
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/80806?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Gerrit-Change-Number: 80806
Gerrit-PatchSet: 1
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Wed, 06 Mar 2024 23:50:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment