coreboot-gerrit January 2023

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

[M] Change in coreboot[master]: soc/intel/alderlake: Inform user of memory training
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Bora Guvendik, Anil Kumar K, Hannah Williams, Cliff Huang, Paul Menzel, Angel Pons, Arthur Heymans, Andrey Petrov, Zhixing Ma, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Nick Vaccaro. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70300 ) Change subject: soc/intel/alderlake: Inform user of memory training ...................................................................... Patch Set 39: Code-Review+1 (5 comments) File src/drivers/intel/fsp2_0/Kconfig: https://review.coreboot.org/c/coreboot/+/70300/comment/660e7ac6_f75bf0e4 PS26, Line 42: config INFORM_USER_OF_MEMORY_TRAINING_ON_SCREEN : bool : default n : depends on EARLY_GFX_GMA : help : Display a message on-screen to inform the end-user that : memory training is in progress. : : config MEMORY_TRAINING_TEXT : string "Text to display during memory training" : default "Memory training in progress, please wait..." : depends on INFORM_USER_OF_MEMORY_TRAINING_ON_SCREEN : help : Message to be displayed while memory training is in : progress. It can be a multi-line string by using the '\' : character as a line break. The text is rendered centered : vertically and horizontally on the screen and due to VGA : text mode constraints it is limited to 80 columns and 25 : lines. > Same question applied here. I implemented Subrata suggestion and as a result got rid of these configuration flags. File src/drivers/intel/fsp2_0/intel_early_graphics.h: https://review.coreboot.org/c/coreboot/+/70300/comment/0e34566d_8e46487d PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */ > Where do you suggest to put this code precisely ? `drivers/gfx/early_graphics` seems odd as this is […] As requested by Subrata I moved this "generic code" and put it in a SoC specific code tree. As a result the question of the host directory is now irrelevant. File src/drivers/intel/fsp2_0/intel_early_graphics.c: https://review.coreboot.org/c/coreboot/+/70300/comment/d884d15d_6676e1bd PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */ > > > Where do you suggest to put this code precisely ? […] As requested I moved this "generic code" and put it in a SoC specific code tree. As also requested, I also hardcoded the text string. File src/drivers/intel/fsp2_0/intel_early_graphics.c: https://review.coreboot.org/c/coreboot/+/70300/comment/5a4b152d_0068957f PS36, Line 78: > > Thinking about this, I too suggest dropping the Kconfig in favor of a const char[] somewhere. […] As requested I moved this "generic code" and put it in a SoC specific code tree. As also requested, I also hardcoded the text string. File src/drivers/intel/fsp2_0/memory_init.c: https://review.coreboot.org/c/coreboot/+/70300/comment/d2cc0a6f_b7727d6b PS27, Line 308: > > The current implementation is using the same PCI BAR 0 that is being used at ramstage and as such […] Regarding the use of your temporary MMIO region instead of the use of the use of a range in the memory hole we need to re-visit in the light of Angel interesting suggestion (https://review.coreboot.org/c/coreboot/+/70276/15/src/drivers/intel/gma/Kco…) Does it make sense to re-use a SPI map region and to have to release it if we consider `CONFIG_GFX_GMA_DEFAULT_MMIO` reserved for graphics MMIO operation and that we make it clear to the coreboot resource allocator that this is reserved for this purpose ? -- To view, visit https://review.coreboot.org/c/coreboot/+/70300 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4ea15123eed1a4355c5ff7d815925032d4151de0 Gerrit-Change-Number: 70300 Gerrit-PatchSet: 39 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:14:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com> Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/alderlake: Inform user of memory training
by build bot (Jenkins) (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Bora Guvendik, Anil Kumar K, Hannah Williams, Cliff Huang, Jérémy Compostella, Paul Menzel, Angel Pons, Arthur Heymans, Andrey Petrov, Zhixing Ma, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Nick Vaccaro. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70300 ) Change subject: soc/intel/alderlake: Inform user of memory training ...................................................................... Patch Set 39: Verified-1 (1 comment) File src/soc/intel/alderlake/romstage/romstage.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-167651): https://review.coreboot.org/c/coreboot/+/70300/comment/8e2145be_02682ced PS39, Line 173: * RUN_FSP_GOP is selected code indent should use tabs where possible -- To view, visit https://review.coreboot.org/c/coreboot/+/70300 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4ea15123eed1a4355c5ff7d815925032d4151de0 Gerrit-Change-Number: 70300 Gerrit-PatchSet: 39 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:14:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/alderlake: Add romstage early graphics support
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Bora Guvendik, Anil Kumar K, Hannah Williams, Cliff Huang, Paul Menzel, Angel Pons, Arthur Heymans, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Swift Geek (Sebastian Grzywna), Elyes Haouas. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70303 ) Change subject: soc/intel/alderlake: Add romstage early graphics support ...................................................................... Patch Set 44: Code-Review+1 (6 comments) File src/soc/intel/alderlake/Kconfig: https://review.coreboot.org/c/coreboot/+/70303/comment/f9b1c95d_2248bfc3 PS33, Line 209: if EARLY_GFX_GMA : : config GFX_GMA_DEFAULT_MMIO : default 0x81000000 : : endif # EARLY_GFX_GMA > > Would this work? […] Done File src/soc/intel/alderlake/Kconfig: https://review.coreboot.org/c/coreboot/+/70303/comment/f0fbebb1_7fdd3d62 PS42, Line 211: 0xfa000000 > 1) It isn't directly straightforward - I agree. […] Done https://review.coreboot.org/c/coreboot/+/70303/comment/3ed4f82d_7868c449 PS42, Line 211: 0xfa000000 > this is from the logs we had dumped in romstage sometime back […] Done File src/soc/intel/alderlake/romstage/graphics.c: https://review.coreboot.org/c/coreboot/+/70303/comment/437a54bd_f0f55853 PS33, Line 21: /* Attempt to unlock memory */ : wrmsr(MSR_UNLOCK_MEMORY, msrval); > > Probably but outside this patch. […] Done https://review.coreboot.org/c/coreboot/+/70303/comment/ec7f6b3e_6ce219b4 PS33, Line 37: unlock_memory(); > Also, have you thought of a scenario where user still would like to keep TXT support enabled and don't wish to disable it (like what the code is doing now) but would like to show a video msg. Seems like both are exclusive to each other. The Alder Lake / Raptor Lake FSP-M seems to unconditionally unlock memory. Looks like this use-case is already going to be conflicting with FSP-M. > What is the value of reading TPM base address and unlock the LT memory on the SKUs where TXT itself is fuse disable. Shouldn't we save some cycle there by adding a check? Good question: I don't know which one of the extra cpuid or the blank MSR unlock is going to take more cycles. I am under the impression this should not make a major difference though. File src/soc/intel/alderlake/romstage/graphics.c: https://review.coreboot.org/c/coreboot/+/70303/comment/8e4e8fd7_4cf1f27c PS39, Line 20: read8p(TPM_BASE_ADDRESS); > @Subrata, I also went through the FSP code and I experimented around it to arrive to the point we ar […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/70303 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I727b28bbe180edc2574e09bf03f1534d6282bdb2 Gerrit-Change-Number: 70303 Gerrit-PatchSet: 44 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:14:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com> Comment-In-Reply-To: Hannah Williams <hannah.williams(a)intel.com> Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/common: Add common function to disable LT memory lock
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71655 ) Change subject: soc/intel/common: Add common function to disable LT memory lock ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/71655 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic666919ed51f683c9a1ef0b7ba061cd2949faf1e Gerrit-Change-Number: 71655 Gerrit-PatchSet: 2 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:14:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/common/block: Add Intel VGA early graphics support
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Bora Guvendik, Zhixing Ma, Anil Kumar K, Hannah Williams, Cliff Huang, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Paul Menzel, Arthur Heymans, Swift Geek (Sebastian Grzywna). Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70299 ) Change subject: soc/intel/common/block: Add Intel VGA early graphics support ...................................................................... Patch Set 26: (4 comments) File src/soc/intel/common/block/graphics/early_graphics.c: https://review.coreboot.org/c/coreboot/+/70299/comment/5558e9b7_dfcc20f4 PS23, Line 6: static bool initialized; > don't need drop please Required with your suggested implementation in the SoC specific code tree. https://review.coreboot.org/c/coreboot/+/70299/comment/3b2e3c06_776a133e PS23, Line 21: initialized = !!ret; : : return initialized; > return !!ret Ack https://review.coreboot.org/c/coreboot/+/70299/comment/22da93ad_0a30b1e1 PS23, Line 29: : if (!initialized) : return; > Got it. Thanks for the explanation. […] I kept as it is required with the implementation you have recommended in the SoC specific code tree. https://review.coreboot.org/c/coreboot/+/70299/comment/4156c301_a1dc9c2f PS23, Line 32: : /* : * Graphics MMIO device configuration may have been corrupted since it : * has been initialized. The execution of a binary block such as FSP may : * indeed have reset the PCI BAR0 address of the graphic device. : */ : early_graphics_soc_device_init(); > > > > Use suggestion here: https://review.coreboot. […] I re-used your quite interesting `m_cfg->GttMmAdr = CONFIG_GFX_GMA_DEFAULT_MMIO` suggestion in the SoC specific code tree. Regarding the use of your temporary MMIO region instead of the use of the use of a range in the memory hole we need to re-visit in the light of Angel interesting suggestion (https://review.coreboot.org/c/coreboot/+/70276/15/src/drivers/intel/gma/Kco…) Does it make sense to re-use a SPI map region and to have to release it if we consider `CONFIG_GFX_GMA_DEFAULT_MMIO` reserved for graphics MMIO operation and that we make it clear to the coreboot resource allocator that this is reserved for this purpose ? -- To view, visit https://review.coreboot.org/c/coreboot/+/70299 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie4ad1215e5fadd0adc1271b6bd6ddb0ea258cb5b Gerrit-Change-Number: 70299 Gerrit-PatchSet: 26 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:13:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com> Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: Revert "mb/google/brya: Define a more suitable MRC training text mess...
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Anil Kumar K, Tarun Tuli, Jakub Czapiga, Angel Pons, Nick Vaccaro. Jérémy Compostella has uploaded a new patch set (#3) to the change originally created by Jakub Czapiga. ( https://review.coreboot.org/c/coreboot/+/71279 ) Change subject: Revert "mb/google/brya: Define a more suitable MRC training text message" ...................................................................... Revert "mb/google/brya: Define a more suitable MRC training text message" This reverts commit e45f70423e5da8509bae83aba84b08f8fc0f624e. Reason for revert: Merged out of order, broke tree Signed-off-by: Jakub Czapiga <jacz(a)semihalf.com> Change-Id: I38a7be6b94199d3a23e78114fb6708c535f241cc --- M src/mainboard/google/brya/Kconfig 1 file changed, 14 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/71279/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/71279 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38a7be6b94199d3a23e78114fb6708c535f241cc Gerrit-Change-Number: 71279 Gerrit-PatchSet: 3 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: Revert "mb/google/brya: Define a more suitable MRC training text mess...
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Anil Kumar K, Tarun Tuli, Jakub Czapiga, Angel Pons, Nick Vaccaro. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71279 ) Change subject: Revert "mb/google/brya: Define a more suitable MRC training text message" ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/71279 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38a7be6b94199d3a23e78114fb6708c535f241cc Gerrit-Change-Number: 71279 Gerrit-PatchSet: 2 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:10:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: drivers/intel/gma: Hook up libgfxinit in romstage
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Attention is currently required from: Bora Guvendik, Zhixing Ma, Anil Kumar K, Hannah Williams, Cliff Huang, Tarun Tuli, Nico Huber, Michał Żygowski, Paul Menzel, Angel Pons, Arthur Heymans. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70276 ) Change subject: drivers/intel/gma: Hook up libgfxinit in romstage ...................................................................... Patch Set 20: (3 comments) File src/drivers/intel/gma/Kconfig: https://review.coreboot.org/c/coreboot/+/70276/comment/58c4b620_5751518e PS13, Line 95: SOC_INTEL_ALDERLAKE > Let's keep it like this for now. Ack https://review.coreboot.org/c/coreboot/+/70276/comment/49dff4d1_d92d9830 PS13, Line 103: Graphic MMIO address Thanks Arthur and Subrata for the information and pointers. This is quite interesting. I never realized that these two are actually "mixed-up". > Note: here in VGA text mode, we are using 0xb8000 address for video FB in VGA mode. The only purpose of GTTMMADR is to program the panel specific registers (within 2MB range). The list of registers touched goes beyond this subset of five register of course 😊. Thanks again. File src/drivers/intel/gma/Kconfig: https://review.coreboot.org/c/coreboot/+/70276/comment/e85d0bfb_97a57b51 PS15, Line 105: If : libgfxinit is used for both romstage and ramstage this : address should be the same than the one allocated by the PCI : resource allocator in ramstage. > For now, you can tell coreboot to report BAR0 as a fixed resource. […] Thanks for this great suggestion. I did not realize how flexible the PCI device/driver framework is in coreboot 😊. Unfortunately, it does not work as-is because: 1. The PCI BAR0 is not necessarily set at this point in time. 2. `IORESOURCE_ASSIGNED` amd `IORESOURCE_FIXED` are mandatory but make the resource allocator to skip setting this resource. I had to rework your suggestion as follow (and it does not look as good anymore): ``` if (CONFIG(HWBASE_STATIC_MMIO)) { struct resource *res_bar0 = find_resource(dev, PCI_BASE_ADDRESS_0); res_bar0->base = CONFIG_GFX_GMA_DEFAULT_MMIO; res_bar0->flags |= IORESOURCE_ASSIGNED; pci_dev_set_resources(dev); res_bar0->flags |= IORESOURCE_FIXED; } ``` Once I am done reworking all these patches, I'll try to identify when is the configuration lost. I bet this is during FSP-S. I still think having global initialized variable would be the best the solution. I experimented a little bit on Friday. I have promising results but this is going to take a little while to have something merge-able. -- To view, visit https://review.coreboot.org/c/coreboot/+/70276 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c2101de10dc5df54fe873e43bbe0f1c4dccff44 Gerrit-Change-Number: 70276 Gerrit-PatchSet: 20 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:06:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/common/block/early_graphics: Introduce a 200 ms delay
by build bot (Jenkins) (Code Review) 04 Jan '23

04 Jan '23

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71656 ) Change subject: soc/intel/common/block/early_graphics: Introduce a 200 ms delay ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/common/block/graphics/early_graphics.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-167641): https://review.coreboot.org/c/coreboot/+/71656/comment/6ec5dc1e_7169b42b PS1, Line 48: udelay(200000); long udelay - prefer mdelay; see arch/arm/include/asm/delay.h -- To view, visit https://review.coreboot.org/c/coreboot/+/71656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4ea15123eed1a4355c5ff7d815925032d4151de1 Gerrit-Change-Number: 71656 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 04 Jan 2023 18:06:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/common/block/early_graphics: Introduce a 200 ms delay
by Jérémy Compostella (Code Review) 04 Jan '23

04 Jan '23

Jérémy Compostella has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/71656 ) Change subject: soc/intel/common/block/early_graphics: Introduce a 200 ms delay ...................................................................... soc/intel/common/block/early_graphics: Introduce a 200 ms delay It has been reported that the PEIM graphics driver may temporarily fail communication with the display if the time between libgfxinit turning off the displays and the PEIM driver initialization is too short. 200 ms has been identified as a safe delay. This is a temporary workaround and an investigation is in progress to come up with a better and long term solution. BUG=b:252792591 BRANCH=firmware-brya-14505.B TEST=Developer screen is systematically seen Change-Id: I4ea15123eed1a4355c5ff7d815925032d4151de1 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/soc/intel/common/block/graphics/early_graphics.c 1 file changed, 37 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/71656/1 diff --git a/src/soc/intel/common/block/graphics/early_graphics.c b/src/soc/intel/common/block/graphics/early_graphics.c index a4b0a9e..fdffda3 100644 --- a/src/soc/intel/common/block/graphics/early_graphics.c +++ b/src/soc/intel/common/block/graphics/early_graphics.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#include <delay.h> #include <drivers/intel/gma/libgfxinit.h> #include <intelblocks/early_graphics.h> @@ -31,4 +32,18 @@ return; gma_gfxstop(&ret); + + /* + * Temporary workaround + * + * It has been reported that the PEIM graphics driver may temporarily + * fail communication with the display if the time between libgfxinit + * turning off the displays and the PEIM driver initialization is too + * short. 200 ms has been identified as a safe delay. + * + * An investigation is in progress to come up with a better and long + * term solution. + */ + if (CONFIG(RUN_FSP_GOP)) + udelay(200000); } -- To view, visit https://review.coreboot.org/c/coreboot/+/71656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4ea15123eed1a4355c5ff7d815925032d4151de1 Gerrit-Change-Number: 71656 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2023