Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62658 )
Change subject: amd/*/Makefile.inc: Put common words into common Makefile.inc
......................................................................
Patch Set 13: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/62658
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644
Gerrit-Change-Number: 62658
Gerrit-PatchSet: 13
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 01 Apr 2022 01:43:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held.
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Elyes Haouas, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/62658
to look at the new patch set (#13).
Change subject: amd/*/Makefile.inc: Put common words into common Makefile.inc
......................................................................
amd/*/Makefile.inc: Put common words into common Makefile.inc
Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644
Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com>
---
M src/soc/amd/cezanne/Makefile.inc
M src/soc/amd/common/Makefile.inc
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/sabrina/Makefile.inc
M src/soc/amd/stoneyridge/Makefile.inc
M src/southbridge/amd/pi/hudson/Makefile.inc
6 files changed, 11 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/62658/13
--
To view, visit https://review.coreboot.org/c/coreboot/+/62658
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644
Gerrit-Change-Number: 62658
Gerrit-PatchSet: 13
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62658 )
Change subject: amd/*/Makefile.inc: Put common words into common Makefile.inc
......................................................................
Patch Set 12: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/62658
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644
Gerrit-Change-Number: 62658
Gerrit-PatchSet: 12
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 01 Apr 2022 01:40:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Shelley Chen has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/63289 )
Change subject: soc/qualcomm/common: Add strict flag to clock_configure()
......................................................................
soc/qualcomm/common: Add strict flag to clock_configure()
When strict parameter is set to true, clock_configure() will try to
find an exact match to the clock frequency given. If exact match
cannot be found, then will throw an assert error.
BUG=b:198627043
BRANCH=None
TEST=build herobrine image and try to set SPI frequency to number not
supported. Ensure assert is seen.
Change-Id: I9cfad7236241f4d03ff1a56683654649658b68fc
Signed-off-by: Shelley Chen <shchen(a)google.com>
---
M src/soc/qualcomm/common/clock.c
M src/soc/qualcomm/common/include/soc/clock_common.h
M src/soc/qualcomm/sc7180/clock.c
M src/soc/qualcomm/sc7280/clock.c
4 files changed, 30 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/63289/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/63289
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9cfad7236241f4d03ff1a56683654649658b68fc
Gerrit-Change-Number: 63289
Gerrit-PatchSet: 2
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63289 )
Change subject: soc/qualcomm/common: Add strict_check flags to clock_configure()
......................................................................
Patch Set 1:
(4 comments)
File src/soc/qualcomm/sc7180/clock.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145192):
https://review.coreboot.org/c/coreboot/+/63289/comment/320a196c_c80573c8
PS1, Line 218: &mdss_clk_cfg, 0, 0, false);
code indent should use tabs where possible
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145192):
https://review.coreboot.org/c/coreboot/+/63289/comment/218cf625_7e7f7512
PS1, Line 218: &mdss_clk_cfg, 0, 0, false);
please, no spaces at the start of a line
File src/soc/qualcomm/sc7280/clock.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145192):
https://review.coreboot.org/c/coreboot/+/63289/comment/50f54c07_3c7ff175
PS1, Line 270: true);
code indent should use tabs where possible
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145192):
https://review.coreboot.org/c/coreboot/+/63289/comment/27b42a87_bb3bdf19
PS1, Line 270: true);
please, no spaces at the start of a line
--
To view, visit https://review.coreboot.org/c/coreboot/+/63289
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9cfad7236241f4d03ff1a56683654649658b68fc
Gerrit-Change-Number: 63289
Gerrit-PatchSet: 1
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 01 Apr 2022 01:22:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Nico Huber, Xiang W, Philipp Hug, Jonathan Neuschäfer, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 )
Change subject: lib: Add a mutex
......................................................................
Patch Set 15:
(1 comment)
Patchset:
PS15:
> All this review is happening in a no-op change, while CB:59321 is where spinlock/mutex change really takes place. It has not been rebased for couple months.
I was just trying to keep the discussion in one place, I'm aware there is more to this patch series and I assume Raul is going to start cleaning the later parts up again once the fundamental question of direction here is resolved.
> I have provided pointers in my previous commentary (CB:60253 and CB:60280) that show it's easy to use compiler atomic builtins to provide a spinlock, and easy to build mutex with such a spinlock.
Right, I've seen your patches, but they go in a different direction than what I'm trying to argue here so I haven't commented on them yet while we're still hashing out the fundamental question. I understand that what you're proposing is possible, it's not a question about proving that we can do it that way. I just think Raul's direction is better.
> This is known to not be SMP safe, as it turns out it completely lacks a spinlock currently, but why is mutex() ever better than spinlock() around a index/data IO access like these:
>
> src/arch/x86/include/arch/pci_io_cfg.h
I don't think the question is "why is mutex better in this case", the question is "would a spinlock in this case really be so superior, or would a mutex be so harmful, that it outweighs the cost of extra complexity of keeping both concepts separate". Yes, you don't need thread-safety for those cases. But how much does the thread-safety cost? One extra function call that's not inlined, or something like that? Are we really running this code in tight enough loops that that makes any noticeable impact?
Requiring the programmer to think about whether a certain point of mutual exclusion needs to be thread-safe or "only" SMP-safe is a real maintenance cost, and it creates a real risk for making mistakes that create dangerous bugs (especially if threading is a niche feature that many people who add code that needs mutual exclusion may not care about). So I would tend towards preventing that issue, unless it creates a real problem elsewhere, and I don't think the miniscule amount of overhead from having this code over pure spinlock code does that (unless you know an example where it would be called often enough to really amplify that to notable levels?).
--
To view, visit https://review.coreboot.org/c/coreboot/+/59320
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I41e02a54a17b1f6513b36a0274e43fc715472d78
Gerrit-Change-Number: 59320
Gerrit-PatchSet: 15
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx>
Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Xiang W <wxjstz(a)126.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Xiang W <wxjstz(a)126.com>
Gerrit-Attention: Philipp Hug <philipp(a)hug.cx>
Gerrit-Attention: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Rob Barnes <robbarnes(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 01 Apr 2022 00:20:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Karthik Ramasubramanian, Felix Held.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63286 )
Change subject: mb/google/skyrim: Fix ESPI communication issues
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/63286
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I951afdada8ee4f917cdeba8e287e5a2ae77c97ee
Gerrit-Change-Number: 63286
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 01 Apr 2022 00:00:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment