coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5875 discussions

Change in coreboot[master]: soc/qualcomm/common: Add strict_check flags to clock_configure()
by build bot (Jenkins) (Code Review) 01 Apr '22

01 Apr '22

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

1 0

Change in coreboot[master]: soc/qualcomm/common: Add strict_check flags to clock_configure()
by Shelley Chen (Code Review) 01 Apr '22

01 Apr '22

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63289 ) Change subject: soc/qualcomm/common: Add strict_check flags to clock_configure() ...................................................................... soc/qualcomm/common: Add strict_check flags to clock_configure() Add strict parameter to clock_configure(). When this 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/1 diff --git a/src/soc/qualcomm/common/clock.c b/src/soc/qualcomm/common/clock.c index e06a954..4a2b0a8 100644 --- a/src/soc/qualcomm/common/clock.c +++ b/src/soc/qualcomm/common/clock.c @@ -90,8 +90,8 @@ /* Clock Root clock Generator Operations */ enum cb_err clock_configure(struct clock_rcg *clk, - struct clock_freq_config *clk_cfg, uint32_t hz, - uint32_t num_perfs) + struct clock_freq_config *clk_cfg, uint32_t hz, + uint32_t num_perfs, bool strict) { uint32_t reg_val, idx; @@ -99,6 +99,10 @@ if (hz <= clk_cfg[idx].hz) break; + /* If strict flag is set, check if found exact match */ + if (strict && hz != clk_cfg[idx].hz) + assert("ERROR: No exact match found for QSPI clk frequency!\n"); + reg_val = (clk_cfg[idx].src << CLK_CTL_CFG_SRC_SEL_SHFT) | (clk_cfg[idx].div << CLK_CTL_CFG_SRC_DIV_SHFT); diff --git a/src/soc/qualcomm/common/include/soc/clock_common.h b/src/soc/qualcomm/common/include/soc/clock_common.h index 0911827..82464cf 100644 --- a/src/soc/qualcomm/common/include/soc/clock_common.h +++ b/src/soc/qualcomm/common/include/soc/clock_common.h @@ -145,8 +145,22 @@ void clock_reset_bcr(void *bcr_addr, bool assert); +/* + * clock_configure(): Configure the clock at the given clock speed (hz). If hz + * does not match any entries in the clk_cfg array, will round up to the next + * highest clock frequency. If strict=1, will throw an assert error if there is + * no frequency match in clk_cfg. + * + * @param clk struct clock_rcg pointer (root clock generator) + * @param clk_cfg Array with possible clock configurations + * @param hz frequency of clock to set + * @param num_perfs size of clock array + * @param strict Flag to enable strict checking. If the given frequency + * in hz does not match any entry in clk_cfg array, will + * throw an assert error. + */ enum cb_err clock_configure(struct clock_rcg *clk, struct clock_freq_config *clk_cfg, - uint32_t hz, uint32_t num_perfs); + uint32_t hz, uint32_t num_perfs, bool strict); void clock_configure_dfsr_table(int qup, struct clock_freq_config *clk_cfg, uint32_t num_perfs); diff --git a/src/soc/qualcomm/sc7180/clock.c b/src/soc/qualcomm/sc7180/clock.c index 5741c54..ae5f43d 100644 --- a/src/soc/qualcomm/sc7180/clock.c +++ b/src/soc/qualcomm/sc7180/clock.c @@ -119,7 +119,7 @@ { clock_configure(&gcc->qspi_core, qspi_core_cfg, hz, - ARRAY_SIZE(qspi_core_cfg)); + ARRAY_SIZE(qspi_core_cfg), true); clock_enable(&gcc->qspi_cnoc_ahb_cbcr); clock_enable(&gcc->qspi_core_cbcr); } @@ -215,7 +215,7 @@ mdss_clk_cfg.d_2 = d_2; return clock_configure((struct clock_rcg *)mdss_clock[clk_type], - &mdss_clk_cfg, 0, 0); + &mdss_clk_cfg, 0, 0, false); } int mdss_clock_enable(enum mdss_clock clk_type) diff --git a/src/soc/qualcomm/sc7280/clock.c b/src/soc/qualcomm/sc7280/clock.c index 746035c..68b5909 100644 --- a/src/soc/qualcomm/sc7280/clock.c +++ b/src/soc/qualcomm/sc7280/clock.c @@ -266,7 +266,8 @@ { clock_configure(&gcc->qspi_core, qspi_core_cfg, hz, - ARRAY_SIZE(qspi_core_cfg)); + ARRAY_SIZE(qspi_core_cfg), + true); clock_enable(&gcc->qspi_cnoc_ahb_cbcr); clock_enable(&gcc->qspi_core_cbcr); } @@ -307,7 +308,7 @@ clock_configure_enable_gpll(&gpll10_cfg, true, 9); } clock_configure((struct clock_rcg *)&gcc->sdcc1, sdcc1_core_cfg, - hz, ARRAY_SIZE(sdcc1_core_cfg)); + hz, ARRAY_SIZE(sdcc1_core_cfg), false); clock_enable(&gcc->sdcc1_ahb_cbcr); clock_enable(&gcc->sdcc1_apps_cbcr); } else if (sdcc == SDCC2_CLK) { @@ -326,7 +327,7 @@ clock_configure_enable_gpll(&gpll9_cfg, true, 8); } clock_configure((struct clock_rcg *)&gcc->sdcc2, sdcc2_core_cfg, - hz, ARRAY_SIZE(sdcc2_core_cfg)); + hz, ARRAY_SIZE(sdcc2_core_cfg), false); clock_enable(&gcc->sdcc2_ahb_cbcr); clock_enable(&gcc->sdcc2_apps_cbcr); } @@ -422,11 +423,11 @@ case MDSS_CLK_EDP_PIXEL: case MDSS_CLK_PCLK0: return clock_configure((struct clock_rcg *) - mdss_clock_mnd[clk_type], - &mdss_clk_cfg, hz, 0); + mdss_clock_mnd[clk_type], + &mdss_clk_cfg, hz, 0, false); default: return clock_configure(mdss_clock[clk_type], - &mdss_clk_cfg, hz, 0); + &mdss_clk_cfg, hz, 0, false); } } -- 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-MessageType: newchange

1 0

Change in coreboot[master]: soc/qualcomm/common: Increase SPI gpios drive strength to 8mA
by Shelley Chen (Code Review) 01 Apr '22

01 Apr '22

Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63192 ) Change subject: soc/qualcomm/common: Increase SPI gpios drive strength to 8mA ...................................................................... soc/qualcomm/common: Increase SPI gpios drive strength to 8mA EE requested that we increase the drive strength for the SPI lines to 8mA. BUG=b:198627043 BRANCH=None TEST=EE help verify Change-Id: Ic887a7eef74f1063f7284db042c5fbd2e1d5bd4c Signed-off-by: Shelley Chen <shchen(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/63192 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Julius Werner <jwerner(a)chromium.org> --- M src/soc/qualcomm/common/qspi.c 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved diff --git a/src/soc/qualcomm/common/qspi.c b/src/soc/qualcomm/common/qspi.c index b6d2b2d..448fb86 100644 --- a/src/soc/qualcomm/common/qspi.c +++ b/src/soc/qualcomm/common/qspi.c @@ -146,10 +146,10 @@ gpio_output(QSPI_CS, 1); gpio_configure(QSPI_DATA_0, GPIO_FUNC_QSPI_DATA_0, - GPIO_NO_PULL, GPIO_2MA, GPIO_OUTPUT); + GPIO_NO_PULL, GPIO_8MA, GPIO_OUTPUT); gpio_configure(QSPI_DATA_1, GPIO_FUNC_QSPI_DATA_1, - GPIO_NO_PULL, GPIO_2MA, GPIO_OUTPUT); + GPIO_NO_PULL, GPIO_8MA, GPIO_OUTPUT); gpio_configure(QSPI_CLK, GPIO_FUNC_QSPI_CLK, GPIO_NO_PULL, GPIO_8MA, GPIO_OUTPUT); -- To view, visit https://review.coreboot.org/c/coreboot/+/63192 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic887a7eef74f1063f7284db042c5fbd2e1d5bd4c Gerrit-Change-Number: 63192 Gerrit-PatchSet: 4 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: lib: Add a mutex
by Julius Werner (Code Review) 01 Apr '22

01 Apr '22

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

1 0

Change in coreboot[master]: mb/google/skyrim: Fix ESPI communication issues
by Jon Murphy (Code Review) 01 Apr '22

01 Apr '22

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

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2022