Attention is currently required from: Arthur Heymans, Martin Roth, Maulik V Vaghela, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, Felix Held.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE
......................................................................
Patch Set 58:
(13 comments)
Patchset:
PS58:
Sorry for the long silence. I've spotted a few minor things that I
think should be addressed.
Irritating comments and everything looking like vendorcode seems to
be accepted in soc/intel/elkhartlake so I marked these as resolved
(should be fixed but probably with a time machine to not avoid review
of the initial bring up).
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55367/comment/57a00837_daff0602
PS58, Line 477: hidden from coreboot
NB. This is only true if you consider the PSE firmware foreign and not
part of coreboot. Which is the current kludge and should be the exception
IMHO.
https://review.coreboot.org/c/coreboot/+/55367/comment/305fb7a5_18dadb9f
PS58, Line 497: sideband
What's sideband in this context?
https://review.coreboot.org/c/coreboot/+/55367/comment/cfd387a2_3785bea1
PS58, Line 497: Disable (0) / Enable (1)
disable (false) / enable (true)
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/38bead9f_c64fbee9
PS58, Line 86: static char psefwbuf[(CONFIG_PSE_FW_FILE_SIZE +
line break looks ugly and unnecessary
https://review.coreboot.org/c/coreboot/+/55367/comment/cfe3d926_f426c520
PS58, Line 87: KiB
If these Kconfig values are scaled to KiB, it would be nice to have that
in their names to avoid mistakes, e.g. PSE_CONFIG_BUFFER_SIZE_KIB.
https://review.coreboot.org/c/coreboot/+/55367/comment/9a16da08_dec88af5
PS58, Line 91: uint32_t
Pointers should always be casted to `uintptr_t` first. The compiler could
warn about different sizes otherwise.
This also shows that there is a requirement (by FSP? by the PSE?) to have
the firmware in 32-bit space. I guess our CBFS API has no notion for that.
So I'd leave a comment at the cbfs_load() call that we have this requirement.
https://review.coreboot.org/c/coreboot/+/55367/comment/169cb7fb_e7205dac
PS58, Line 126: * as the settings from devicetree are overwritten here.
I still don't understand why we offer the devicetree settings in the
first place. This TODO wouldn't be necessary if there was less config
clutter.
https://review.coreboot.org/c/coreboot/+/55367/comment/79e3e81e_39726d2e
PS58, Line 130: * much better.
So it's definitely a requirement by the current PSE FW? If so, why isn't that
stated? If so, why does the Kconfig help text not mentioned that a very specific
binary is expected?
https://review.coreboot.org/c/coreboot/+/55367/comment/ba6d587a_28c48605
PS58, Line 143: die("PSE enabled but PSE FW not available!\n");
Is it really impossible to go on?
https://review.coreboot.org/c/coreboot/+/55367/comment/b638ca17_f815ce9f
PS58, Line 436: && cbfs_file_exists("pse.bin")
Isn't this mixed up with the else path inside fill_fsps_pse_params()? The current,
hard-to-read flow seems to be this:
if file exists and can be loaded, configure
if file exists but can't be loaded, die saying the fw is not available
if file doesn't exist, go silently on
Is that the case? Is that intentional?
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/9925aff6_03d1c15b
PS49, Line 127: m_cfg->PchPseEnable = CONFIG(PSE_ENABLE);
> Hi Nico, added the handler cbfs_file_exists() per your advice. […]
Thanks!
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/795a9fb8_6dbeee79
PS58, Line 127: !!(
What's the !! for? The result of && is already boolean.
--
To view, visit https://review.coreboot.org/c/coreboot/+/55367
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifea08fb82fea18ef66bab04b3ce378e79a0afbf7
Gerrit-Change-Number: 55367
Gerrit-PatchSet: 58
Gerrit-Owner: Lean Sheng Tan
Gerrit-Reviewer: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Attention: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Lean Sheng Tan
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:10:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Lean Sheng Tan
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59761 )
Change subject: src/console: Print architecture
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
Hmm no power env variables here?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59761
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I34eec64ac32254c270dcbb97e20a7e6be0f478fc
Gerrit-Change-Number: 59761
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:08:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59760 )
Change subject: src/soc/intel/skylake/Kconfig: Enable x86_64 support
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Great! A buildtest config to make sure this does not regress would be a nice followup.
I'd prefer it to be part of this commit.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59760
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3605853e3e15957ea56bd8a1ce7d8a9b87a1a9e1
Gerrit-Change-Number: 59760
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:06:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59760 )
Change subject: src/soc/intel/skylake/Kconfig: Enable x86_64 support
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
Great! A buildtest config to make sure this does not regress would be a nice followup.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59760
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3605853e3e15957ea56bd8a1ce7d8a9b87a1a9e1
Gerrit-Change-Number: 59760
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:06:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59760 )
Change subject: src/soc/intel/skylake/Kconfig: Enable x86_64 support
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59760/comment/925779fe_12ce602f
PS1, Line 9: Tested on Supermicro X11SSH-TF.
Can you please add a config file that build-tests this combination of settings (X11SSH-TF and x86_64)?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59760
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3605853e3e15957ea56bd8a1ce7d8a9b87a1a9e1
Gerrit-Change-Number: 59760
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:05:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Frans Hendriks, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59755 )
Change subject: configs/config.facebook_fbg1701: Remove CONFIG_ONBOARD_SAMSUNG_MEM
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59755/comment/700e6f65_bdb1fa73
PS1, Line 11: CONFIG_ONBOARD_SAMSUNG_MEM
> Done
Hmmm, now that I think of it, maybe Jenkins isn't happy with the two separate commits. If so, I'd squash them.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59755
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I60626552f2e2338cf5cbaaf4dca1b1eb2756d8df
Gerrit-Change-Number: 59755
Gerrit-PatchSet: 2
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:04:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Frans Hendriks <fhendriks(a)eltan.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Frans Hendriks, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59755 )
Change subject: configs/config.facebook_fbg1701: Remove CONFIG_ONBOARD_SAMSUNG_MEM
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/59755
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I60626552f2e2338cf5cbaaf4dca1b1eb2756d8df
Gerrit-Change-Number: 59755
Gerrit-PatchSet: 2
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:03:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Angel Pons, Arthur Heymans.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59755 )
Change subject: configs/config.facebook_fbg1701: Remove CONFIG_ONBOARD_SAMSUNG_MEM
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59755/comment/e27a009d_5633a0b4
PS1, Line 11: CONFIG_ONBOARD_SAMSUNG_MEM
> OK, please rebase this change atop CB:59754 then
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/59755
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I60626552f2e2338cf5cbaaf4dca1b1eb2756d8df
Gerrit-Change-Number: 59755
Gerrit-PatchSet: 2
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 30 Nov 2021 13:02:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Frans Hendriks <fhendriks(a)eltan.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment