Attention is currently required from: Nico Huber, Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Raj Astekar, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61389 )
Change subject: soc/intel/graphics: Create Kconfig for mapping graphic memory base
......................................................................
Patch Set 19: -Code-Review
(1 comment)
File src/soc/intel/common/block/graphics/Kconfig:
https://review.coreboot.org/c/coreboot/+/61389/comment/a115c718_74ec004a
PS19, Line 26: to
: reach at DSM
> Given that graphics_get_memory_base() is only used to guess the
> framebuffer address atm., we could also just rename it to make
> that clear and then won't have a function that looks like it is
> about the hardware and wouldn't have to argue about hardware
> changes :) e.g. graphics_guess_framebuffer_address().
graphics_guess_framebuffer_address() is more scalable IMO as we just need a framebuffer address which may have different mechanism between current generation and next generation SoC.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61389
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6b1e34ada9b895dabcdc8116d2470e8831ed0a9e
Gerrit-Change-Number: 61389
Gerrit-PatchSet: 19
Gerrit-Owner: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 10 Feb 2022 14:05:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Ethan Tsao <ethan.tsao(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Wonkyu Kim, Michał Żygowski, Benjamin Doron, Ravishankar Sarawadi, Stefan Reinauer, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56171 )
Change subject: util/inteltool: Add support for Tiger Lake chips detection and GPIOs
......................................................................
Patch Set 7:
(11 comments)
Patchset:
PS7:
last batch from me 😊
File util/inteltool/gpio_names/tigerlake.h:
https://review.coreboot.org/c/coreboot/+/56171/comment/9665c776_a042e5fe
PS7, Line 30: n/a
BKLTEN_SEC
https://review.coreboot.org/c/coreboot/+/56171/comment/c388cead_4528ba8f
PS7, Line 31: n/a
BKLTCTL_SEC
https://review.coreboot.org/c/coreboot/+/56171/comment/fb910d86_16a61d73
PS7, Line 179: _
I buy a `_group_` for consistency ;)
https://review.coreboot.org/c/coreboot/+/56171/comment/88a10eda_7097a7f8
PS7, Line 183: "
missing comma
https://review.coreboot.org/c/coreboot/+/56171/comment/611b83f2_7299c400
PS7, Line 188: _
`_group_`
https://review.coreboot.org/c/coreboot/+/56171/comment/7827e4d2_2f51531c
PS7, Line 411: B
nit: 'B' means the same as '#', so maybe use '#' everywhere?
https://review.coreboot.org/c/coreboot/+/56171/comment/b46e31db_0ab66309
PS7, Line 544: vpgio3
p<->g
https://review.coreboot.org/c/coreboot/+/56171/comment/900f805c_6f17ca41
PS7, Line 576: "SPI0_TPM_CSB", "SPI0_TPM_CSB",
: "SPI0_FLASH_0_CSB", "SPI0_FLASH_0_CSB",
: "SPI0_FLASH_1_CSB", "SPI0_FLASH_1_CSB",
:
nit: I know, this comes from refcode, but at least for me the names are totally confusing.... what about SPI0_TPM_CS2#, SPI0_FLASH_CS0#, SPI0_FLASH_CS1#?
https://review.coreboot.org/c/coreboot/+/56171/comment/00ff080a_9693f3a5
PS7, Line 630: _vpgio3
p<->g
https://review.coreboot.org/c/coreboot/+/56171/comment/e1b956c8_f94ea872
PS7, Line 679: /*
: * These pads start at offset 0x680, but according to EDS the PADBAR is 0x700.
: * This would cause the tool to parse the GPIOs incorrectly.
: * For informational purposes only.
: */
:
well, this would just be another group at index 0 of tigerlake_pch_h_community_0_groups and tigerlake_pch_h_community_0 would need another field `.padbar`
--
To view, visit https://review.coreboot.org/c/coreboot/+/56171
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6071a999be9e8a372997db0369218f297e579d08
Gerrit-Change-Number: 56171
Gerrit-PatchSet: 7
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Wonkyu Kim
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Wonkyu Kim
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 14:02:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Martin Roth, Igor Bagnucki, Stefan Reinauer, Krystian Hebel, Timothy Pearson.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58656 )
Change subject: payloads/external: add skiboot (for QEMU/Power9)
......................................................................
Patch Set 14:
(2 comments)
File payloads/external/skiboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/58656/comment/5a690724_421dd8e6
PS14, Line 10: default "https://gitlab.com/qemu-project/skiboot.git" if BOARD_EMULATION_QEMU_POWER9
> Does it have to be a special QEMU repo? I would rather want to avoid specifying repo address.
The one at https://git.raptorcs.com/git/talos-skiboot doesn't work with QEMU and it's specified on our development branch, which is why I made URL configurable. The GitLab repo is in sync with https://github.com/open-power/skiboot, by the way.
File src/mainboard/emulation/qemu-power9/Kconfig:
https://review.coreboot.org/c/coreboot/+/58656/comment/d4f3d01b_065fbedc
PS14, Line 11: select BOARD_ROMSIZE_KB_1024
> Is it a sane value when skiboot is included?
Yes, compressed skiboot fits in 1 MiB along with coreboot. If you want to make it more future proof, can make it 2 MiB.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58656
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0b49ea7464c97cc2ff0d5030629deed549851372
Gerrit-Change-Number: 58656
Gerrit-PatchSet: 14
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Igor Bagnucki <igor.bagnucki(a)3mdeb.com>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Timothy Pearson <tpearson(a)raptorengineering.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Igor Bagnucki <igor.bagnucki(a)3mdeb.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Timothy Pearson <tpearson(a)raptorengineering.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 14:01:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Raj Astekar, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61389 )
Change subject: soc/intel/graphics: Create Kconfig for mapping graphic memory base
......................................................................
Patch Set 19:
(1 comment)
File src/soc/intel/common/block/graphics/Kconfig:
https://review.coreboot.org/c/coreboot/+/61389/comment/19d546b8_17833931
PS19, Line 26: to
: reach at DSM
> > In future platform, GMADR is replaced by LMEMBAR which is fixed map to DSM/GSM. […]
Given that graphics_get_memory_base() is only used to guess the
framebuffer address atm., we could also just rename it to make
that clear and then won't have a function that looks like it is
about the hardware and wouldn't have to argue about hardware
changes :) e.g. graphics_guess_framebuffer_address().
--
To view, visit https://review.coreboot.org/c/coreboot/+/61389
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6b1e34ada9b895dabcdc8116d2470e8831ed0a9e
Gerrit-Change-Number: 61389
Gerrit-PatchSet: 19
Gerrit-Owner: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 10 Feb 2022 13:28:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Ethan Tsao <ethan.tsao(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61776 )
Change subject: Revert "cpu/x86/lapic: Unconditionally use CPUID leaf 0xb if available"
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/61776/comment/8a7ef137_aaadd791
PS1, Line 16: valid APIC ID.
> !cpu_is_amd() wouldn't be the right check there, since cezanne does support x2apic and cpuid_ext(0xb […]
the picasso/rv1 ppr #55570 Rev 3.16 which i looked at when reviewing the original patch has the lower byte of cpuid_ext(0xb, 0).edx defined as APIC ID:
CPUID_Fn0000000B_EDX [Extended Topology Enumeration] (Core::X86::Cpuid::ExtTopEnumEdx)
Bits 7:0: ApicId: APIC ID. Read-only. Reset: XXh. APIC ID.
in revision 3.14 of the same document the whole CPUID_Fn0000000B_EDX is however defined as reserved.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61776
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If1e3c55ce2d048b14c08e06bb79810179a87993d
Gerrit-Change-Number: 61776
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 13:20:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Raj Astekar, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61389 )
Change subject: soc/intel/graphics: Create Kconfig for mapping graphic memory base
......................................................................
Patch Set 19:
(1 comment)
File src/soc/intel/common/block/graphics/Kconfig:
https://review.coreboot.org/c/coreboot/+/61389/comment/44dcff38_af00050b
PS19, Line 26: to
: reach at DSM
> In future platform, GMADR is replaced by LMEMBAR which is fixed map to DSM/GSM.
> There is document 643504(FAS) mention it.
Alas, I can't access that document, might be too early. However, the LMEMBAR
is a concept from Intel's discrete graphics projects. So it seems likely
true that they replaced GMADR for future integrated graphics as well.
But if that's the case, why alter functions for GMADR instead of adding
functions for LMEMBAR? They are different concepts. LMEMBAR + offset is
not the same as GMADR (the former is about physical memory with an offset
and the latter about virtual memory). It just happens to provide the same
results wrt. the pre-OS framebuffer.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61389
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6b1e34ada9b895dabcdc8116d2470e8831ed0a9e
Gerrit-Change-Number: 61389
Gerrit-PatchSet: 19
Gerrit-Owner: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 10 Feb 2022 13:17:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Ethan Tsao <ethan.tsao(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment