Attention is currently required from: Wonkyu Kim.
Hello Subrata Banik, Wonkyu Kim, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83948?usp=email
to look at the new patch set (#9).
Change subject: soc/intel/common/block/cpu: Fix ways count computation regression
......................................................................
soc/intel/common/block/cpu: Fix ways count computation regression
Commit 16ab9bdcd578612bb3822373547f939eb90afd82 ("soc/intel/common:
Calculate and configure SF Mask 2") breaks the computation of the
number of ways and as result, all the derived masks. It results in MSR
such as `IA32_L3_MASK_1' to be improperly programmed yielding
unpredictable NEM issues such as hangs.
Indeed, this commit has introduced a backup of 0x1 into %edx before
comparing the requested cache-as-RAM size against the way size. When
the requested cache-as-RAM is larger, it reaches the second part of
the algorithm which computes the necessary number of ways to fit the
requested cache-as-RAM.
This algorithm uses the `div' instruction. Per specification, the div
instruction divides the 64 bits combination of %edx and %eax register.
Since 0x1 got backed up in %edx and assuming a
`CONFIG_DCACHE_RAM_SIZE' of 0x200000, we end up dividing 0x100200000
by the way size instead of 0x200000 which result in a necessary number
of ways of 4098 for a way size of 0x100000.
This commit clears the %edx register before calling the `div'
instruction.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/83948/9
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 9
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Attention is currently required from: Jérémy Compostella.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83982?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: soc/intel/common/block/cpu: Fix number of way computation
......................................................................
soc/intel/common/block/cpu: Fix number of way computation
`CONFIG_DCACHE_RAM_SIZE' is not necessarily a multiple of way size. As
a result, when the `div' instruction is called to compute the needed
number of way, there could be a remainder. When there is, one extra
way should be added to cover `CONFIG_DCACHE_RAM_SIZE'.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521e
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/83982/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/83982?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521e
Gerrit-Change-Number: 83982
Gerrit-PatchSet: 4
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Bora Guvendik, Jamie Ryu, Jérémy Compostella, Saurabh Mishra, Subrata Banik, Wonkyu Kim.
Cliff Huang has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83981?usp=email )
Change subject: soc/intel/common/gpio: support 16-bit CPU Port ID and vw mapping fix
......................................................................
Patch Set 4:
(2 comments)
File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/c/coreboot/+/83981/comment/6cd1bd6f_721c26e6?us… :
PS4, Line 1063: if (comm->vw_map) {
> ``` […]
okay. great. Let me try and test.
https://review.coreboot.org/c/coreboot/+/83981/comment/1574da0a_e77da607?us… :
PS4, Line 1071: }
> what will happen when the `if` clause is not meeting? […]
This condition is to find the the pad from a range & it's offset from the first pad and returns false if not found.
start_pos will take care of PTL community 5 group B:
GPP_B_00: vw_base = 10h start pos = 3
Also, vw_map[i].base will take care of the jumped base and start_pos such as group D in community 5:
...
GPP_B_25: 13h 1
GPP_D_00: 14h 0 <- not continue to use vw_base 13h start = 2.
...
In PTL SOC, this is added to pint to comm5 .vw_map:
static const struct vw_map ptl_community5_vw_map[] = {
{0x10, 3},
{0x14, 0},
};
Also, in community 3:
...
GPP_A_17: 12h 1
GPP_H_00: 13h 0 <- not continue with vw_base 12h start_pos = 2
...
This is added to point to comm5 .vw_map
static const struct vw_map ptl_community3_vw_map[] = {
{0x10, 0},
{0x13, 0},
};
--
To view, visit https://review.coreboot.org/c/coreboot/+/83981?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8c1a48d587bd41178b0c6bb0144fda93e292423d
Gerrit-Change-Number: 83981
Gerrit-PatchSet: 4
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 16:28:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Wonkyu Kim.
Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83948?usp=email )
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
Patch Set 8:
(2 comments)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/83948/comment/b37cc3d1_c48e1470?us… :
PS5, Line 526: xor %edx, %edx
> I added a comment.
Done
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/83948/comment/44a0cd27_de66937a?us… :
PS8, Line 513: /* Clear the upper 32-bit of dividend */
> this is what I was looking for that add a comment saying. you are clearing the upper 32-bit.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 8
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 16:13:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Wonkyu Kim <wonkyu.kim(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Bora Guvendik, Cliff Huang, Jamie Ryu, Jérémy Compostella, Saurabh Mishra, Wonkyu Kim.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83981?usp=email )
Change subject: soc/intel/common/gpio: support 16-bit CPU Port ID and vw mapping fix
......................................................................
Patch Set 4:
(3 comments)
File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/c/coreboot/+/83981/comment/44a99ec4_82da113a?us… :
PS4, Line 1063: if (comm->vw_map) {
```
/* Get the community (potentially updating 'comm' if vw_map exists) */
comm = gpio_get_community(pad);
/* Find the VW entry containing 'pad' */
for (i = 0; i < comm->num_vw_entries; i++) {
if (pad >= comm->vw_entries[i].first_pad && pad <= comm->vw_entries[i].last_pad)
break;
offset += 1 + comm->vw_entries[i].last_pad - comm->vw_entries[i].first_pad;
}
/* Check if we found a valid entry */
if (i == comm->num_vw_entries)
return false;
/* Adjust offset and calculate vw_index based on the mapping type */
if (comm->vw_map) {
offset = pad - comm->vw_entries[i].first_pad;
offset += comm->vw_map[i].start_pos;
*vw_index = comm->vw_map[i].base + offset / 8;
} else {
offset += pad - comm->vw_entries[i].first_pad;
offset += comm->vw_base;
*vw_index = offset / 8;
}
/* Calculate vw_bit */
*vw_bit = offset % 8;
```
https://review.coreboot.org/c/coreboot/+/83981/comment/631ed785_1e0d8c82?us… :
PS4, Line 1064: comm = gpio_get_community(pad);
this is redundant? (line #1060 added the same)
https://review.coreboot.org/c/coreboot/+/83981/comment/b5cded38_d73a2399?us… :
PS4, Line 1071: }
what will happen when the `if` clause is not meeting?
```
if (pad >= comm->vw_entries[i].first_pad && pad <= comm->vw_entries[i].last_pad)
```
This if statement checks if a given value `pad` falls within the range defined by the `first_pad` and `last_pad` within the GPIO community, current VW entry.
If a pad is within this range, the break statement immediately exits the loop (after adjusting the offset)
If not found, we are not doing anything with offset. I assume this `start_pos` is now added to mitigate the problem where VM offsets are not incremental ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/83981?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8c1a48d587bd41178b0c6bb0144fda93e292423d
Gerrit-Change-Number: 83981
Gerrit-PatchSet: 4
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 15:43:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel, Yu-Ping Wu.
Yidi Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/83922?usp=email )
Change subject: soc/mediatek/mt8196: Add GPIO driver
......................................................................
Patch Set 7:
(1 comment)
File src/soc/mediatek/mt8196/gpio.c:
https://review.coreboot.org/c/coreboot/+/83922/comment/64abac3d_f9443a7e?us… :
PS7, Line 8: #include <console/console.h>
no need.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83922?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I6d1e6ef17660308c8de908697ffba6b5f17ff9ae
Gerrit-Change-Number: 83922
Gerrit-PatchSet: 7
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 15:18:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Hung-Te Lin, Jarried Lin, Xi Chen, Yu-Ping Wu.
Yidi Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/83988?usp=email )
Change subject: soc/mediatek: Move GPIO definations to MediaTek common directory
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83988/comment/d35a2cda_a9d5bdf6?us… :
PS1, Line 7: soc/mediatek: Move GPIO definations to MediaTek common directory
soc/mediatek/common: Move GPIO definition to the common folder
--
To view, visit https://review.coreboot.org/c/coreboot/+/83988?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I815df8a3083cf04b821165ec834ca98ee71a0c78
Gerrit-Change-Number: 83988
Gerrit-PatchSet: 1
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 15:17:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Dinesh Gehlot, Eric Lai, Jayvik Desai, Kapil Porwal, Matt DeVillier, Nick Vaccaro, Rishika Raj.
Subrata Banik has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/coreboot/+/83996?usp=email )
Change subject: mb/google/byra/var/kinox: Add/update VBT files
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83996?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I01c19222628fee3874ef592ec40b40d9bd679dce
Gerrit-Change-Number: 83996
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Tue, 20 Aug 2024 15:14:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes