Attention is currently required from: Bao Zheng, Jason Glenesk, Zheng Bao, Felix Held.
Fred Reitberger has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70314 )
Change subject: mb/amd/birman: Add EC fw to flash
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> BIRMAN_EC_POSITION […]
If you want, I can handle updating the birman EC placement.
The main EC blob does not need to be in a CBFS region, so I had been planning on using an FMAP region to hold it (and renaming EC to ECSIG like you did here).
--
To view, visit https://review.coreboot.org/c/coreboot/+/70314
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2e20f399c4bfe32ec43242619628f7775b4e659e
Gerrit-Change-Number: 70314
Gerrit-PatchSet: 1
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
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: Zheng Bao
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 05 Dec 2022 12:53:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Sergii Dmytruk.
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70106 )
Change subject: commonlib/helpers.h: use unsigned literals in definitions
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/70106/comment/14a4484e_d0a0da6c
PS1, Line 21: so if result of
: multiplication is negative
> If result of multiplying two positive integers is negative, further integer promotions don't matter […]
Overflow reporting sounds nice, but negative frequency doesn't make sense, so I've changed it to `ULL`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/70106
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie60a6ee82db80328e44639175272cc8097f36c3b
Gerrit-Change-Number: 70106
Gerrit-PatchSet: 3
Gerrit-Owner: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 12:25:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Krystian Hebel.
Hello build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/70106
to look at the new patch set (#3).
Change subject: commonlib/helpers.h: use unsigned literals in definitions
......................................................................
commonlib/helpers.h: use unsigned literals in definitions
This protects against some of unintentional integer promotions, e.g.:
uint16_t freq_mhz = 2148; // or bigger
uint64_t freq = freq_mhz * MHz;
In this example, MHz used to be treated as int32_t, and because int32_t
can represent every value that uin16_t can, multiplication was being
performed with both arguments treated as int32_t. During assignment,
result of multiplication is promoted to int64_t (because it can
represent each value that int32_t can), and finally implicitly
converted to uint64_t.
Promotions preserve the value, including the sign, so if result of
multiplication is negative, the same negative number (but extended to
64 bits) is converted to unsigned number.
Signed-off-by: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Change-Id: Ie60a6ee82db80328e44639175272cc8097f36c3b
---
M src/commonlib/bsd/include/commonlib/bsd/helpers.h
1 file changed, 32 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/70106/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/70106
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie60a6ee82db80328e44639175272cc8097f36c3b
Gerrit-Change-Number: 70106
Gerrit-PatchSet: 3
Gerrit-Owner: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Krystian Hebel.
Hello build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/70106
to look at the new patch set (#2).
Change subject: commonlib/helpers.h: use unsigned literals in definitions
......................................................................
commonlib/helpers.h: use unsigned literals in definitions
This protects against some of unintentional integer promotions, e.g.:
uint16_t freq_mhz = 2148; // or bigger
uint64_t freq = freq_mhz * MHz;
In this example, MHz used to be treated as int32_t, and because int32_t
can represent every value that uin16_t can, multiplication was being
performed with both arguments treated as int32_t. During assignment,
result of multiplication is promoted to int64_t (because it can
represent each value that int32_t can), and finally implicitly
converted to uint64_t.
Promotions preserve the value, including the sign, so if result of
multiplication is negative, the same negative number (but extended to
64 bits) is converted to unsigned number.
Signed-off-by: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Change-Id: Ie60a6ee82db80328e44639175272cc8097f36c3b
Signed-off-by: Krystian Hebel <krystian.hebel(a)3mdeb.com>
---
M src/commonlib/bsd/include/commonlib/bsd/helpers.h
1 file changed, 33 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/70106/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/70106
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie60a6ee82db80328e44639175272cc8097f36c3b
Gerrit-Change-Number: 70106
Gerrit-PatchSet: 2
Gerrit-Owner: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Tarun Tuli, Michał Kopeć.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69953 )
Change subject: soc/intel/alderlake: make SOC_INTEL_CSE_SEND_EOP_EARLY per-board configurable
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/69953
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7febf7c029e7eac94052cc3a8142949d6813c1bc
Gerrit-Change-Number: 69953
Gerrit-PatchSet: 2
Gerrit-Owner: Michał Kopeć <michal.kopec(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:22:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Bora Guvendik, Subrata Banik, Name of user not set #1004406, Michal Zygowski.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69798 )
Change subject: Add basic support for Raptor Lake S CPUs on Alder Lake PCH (#640555 Rev. 2.2)
......................................................................
Patch Set 4:
(8 comments)
File src/include/cpu/intel/cpu_ids.h:
https://review.coreboot.org/c/coreboot/+/69798/comment/74e390c8_611d09f6
PS3, Line 67: #define CPUID_RAPTORLAKE_S_S0 0xb0671
> CPUID_RAPTORLAKE_S_B0 it is B0 stepping according to the datasheet. And if so, I would also add: […]
Ack
File src/soc/intel/alderlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/69798/comment/f8562f72_31e026ac
PS3, Line 37: { CPUID_RAPTORLAKE_S_S0, "Raptorlake-S S0 Platform" },
> { CPUID_RAPTORLAKE_S_B0, "Raptorlake-S B0 Platform" },
Ack
https://review.coreboot.org/c/coreboot/+/69798/comment/75ca1d29_f01e2575
PS3, Line 207: { PCI_DID_INTEL_RPL_S_GT1, "Raptorlake S GT1(32EU)" },
: { PCI_DID_INTEL_RPL_S_GT2, "Raptorlake S GT2(24EU)" },
: { PCI_DID_INTEL_RPL_S_GT3, "Raptorlake S GT3(16EU)" },
> Let's stick with previous convention and omit execution units.
Ack
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/69798/comment/4cfd96d1_0c7635cd
PS3, Line 544: return ICC_MAX_ADL_S;
> Isn't it 36A for RPL-S?
Ack
File src/soc/intel/alderlake/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/69798/comment/cf4d391a_0fb41070
PS3, Line 28: RPL_S,
> Please add it as a last item in the enum
Ack
File src/soc/intel/alderlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/69798/comment/bb2ce447_8d9c3dd3
PS3, Line 174: { PCI_DID_INTEL_RPL_S_ID_1, 125, VR_CFG_ALL_DOMAINS_ICC(280, 30) },
: { PCI_DID_INTEL_RPL_S_ID_2, 125, VR_CFG_ALL_DOMAINS_ICC(280, 30) },
> RPL-S 8+16 and 8+8 have 307A ICCmax for IA domain.
Ack
https://review.coreboot.org/c/coreboot/+/69798/comment/36313269_16e8eb8a
PS3, Line 176: { PCI_DID_INTEL_RPL_S_ID_3, 125, VR_CFG_ALL_DOMAINS_ICC(280, 30) },
> RPL-S 6+8 has 200A ICCmax for IA domain
Ack
File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/69798/comment/2ff1ce4a_577ba1a2
PS3, Line 84: { X86_VENDOR_INTEL, CPUID_RAPTORLAKE_S_S0 },
> { X86_VENDOR_INTEL, CPUID_RAPTORLAKE_S_B0 },
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/69798
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I767dd08a169a6af59188d9ecd73520b916f69155
Gerrit-Change-Number: 69798
Gerrit-PatchSet: 4
Gerrit-Owner: Name of user not set #1004406
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Michal Zygowski <miczyg94(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.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: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Name of user not set #1004406
Gerrit-Attention: Michal Zygowski <miczyg94(a)gmail.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:17:42 +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: Lance Zhao, Christian Walter, Tim Wawrzynczak, Julius Werner, Krystian Hebel, Yu-Ping Wu, Sergii Dmytruk.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69161 )
Change subject: security/tpm: replace CONFIG(TPMx) checks with runtime check
......................................................................
Patch Set 10:
(1 comment)
File src/drivers/crb/tis.c:
https://review.coreboot.org/c/coreboot/+/69161/comment/2165ad1e_b40fec9e
PS6, Line 144: if (tlcl_get_family() == TPM_1)
: return get_smbios_data(dev, handle, current);
> It was done this way, because the function is invoked like: […]
CRB is always TPM 2.0 AFAIK, thus TPM_1 is not a valid family to run this function. Thus we should return 0 and leave handle and current unmodfied.
--
To view, visit https://review.coreboot.org/c/coreboot/+/69161
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id9cc25aad8d1d7bfad12b7a92059b1b3641bbfa9
Gerrit-Change-Number: 69161
Gerrit-PatchSet: 10
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Lance Zhao
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Lance Zhao
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:14:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Jason Nien, Matt DeVillier, Arthur Heymans, Martin Roth, Fred Reitberger, Marvin Drees, Karthik Ramasubramanian, Felix Held.
Christopher Meis has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69852 )
Change subject: util/amdfwtool: Deal with psp position in flash offset directly
......................................................................
Patch Set 4:
(1 comment)
File src/soc/amd/common/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/69852/comment/07d626a7_e428dc82
PS4, Line 25: $(call int-subtract, 0xffffffff $(CONFIG_ROM_SIZE)) 1 $(CONFIG_AMD_FWM_POSITION))
> This also is not exactly correct.
>
> When looking for the EFS location, the PSP only cares about flash offsets.
> In EFT, again, addresses to PSP and BIOS directories are set in flash offsets.
>
> PSP and BIOS directory headers have a indicator to provide information about address mode to expect from each entry. Entries inherit this information if not overwritten by entry configuration from the outside/user.
Addition:
This is also true, when placing the EFT and/or binaries in upper/lower flash if ROM is larger than 16Mb.
Note: I observed serveral aspects of the documentation as invalid, untrue, unspecific and/or mutual exclusive. Stuff which might work for servers do not work for client platform and vice versa.
Basically everything can be right or wrong at the same time, depending on the platform we're talking about.
--
To view, visit https://review.coreboot.org/c/coreboot/+/69852
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I89c9e73e7db748379c97e3c0ad69af3faedc8d66
Gerrit-Change-Number: 69852
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com>
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: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Marvin Drees <marvin.drees(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-CC: Christopher Meis <christopher.meis(a)9elements.com>
Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Marvin Drees <marvin.drees(a)9elements.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:12:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Christopher Meis <christopher.meis(a)9elements.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Comment-In-Reply-To: Marvin Drees <marvin.drees(a)9elements.com>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment