March 2024 - flashrom-gerrit - mail.coreboot.org

[XS] Change in flashrom[main]: linux_mtd: Provide no-op delay implementation
by Anastasia Klimchuk (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Brian Norris. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80771?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: linux_mtd: Provide no-op delay implementation ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Do you have any testing info that you can add to commit message? I strongly suspect that there is in the bug which id you linked, but this bug is not publicly visible. Testing info is what operations you run on the patch. You are removing the delay, so presumably you want some operations to run faster, and they probably do run faster now. If you just mention in commit message what you tested that would be great! thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/80771?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: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Gerrit-Change-Number: 80771 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> 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: Fri, 08 Mar 2024 06:45:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Hsuan-ting Chen (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Brian Norris, Nikolai Artemiev, Stefan Reinauer. Hsuan-ting Chen 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: 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: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> 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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 03:44:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: flashchips: Add Macronix Flash EPN support
by Stefan Reinauer (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Anastasia Klimchuk, DZ, Nikolai Artemiev. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81096?usp=email ) Change subject: flashchips: Add Macronix Flash EPN support ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/81096?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: Ia07032c1a3168bd595cea6b24e4875843e20190c Gerrit-Change-Number: 81096 Gerrit-PatchSet: 1 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> Gerrit-Reviewer: Anastasia Klimchuk <aklm(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: DZ <danielzhang(a)mxic.com.cn> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 02:56:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) 07 Mar '24

07 Mar '24

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

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Brian Norris (Code Review) 07 Mar '24

07 Mar '24

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

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) 07 Mar '24

07 Mar '24

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

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Brian Norris (Code Review) 07 Mar '24

07 Mar '24

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/+/39864 https://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

1 0

[XS] Change in flashrom[main]: doc: Update arbitration team to be flashrom specific
by Stefan Reinauer (Code Review) 07 Mar '24

07 Mar '24

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

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Hsuan-ting Chen (Code Review) 07 Mar '24

07 Mar '24

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

1 0

[XS] Change in flashrom[main]: doc: Update arbitration team to be flashrom specific
by Anastasia Klimchuk (Code Review) 07 Mar '24

07 Mar '24

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

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2024