Attention is currently required from: Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73367 )
Change subject: memory_info: Bump to 64 DIMMs
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File src/include/memory_info.h:
https://review.coreboot.org/c/coreboot/+/73367/comment/ca443ea5_1981ba17
PS1, Line 11: #define DIMM_INFO_TOTAL 64
Since this is a coreboot internal structure, maybe we can simply use `DIMM_MAX` instead. It would require checking all platforms, though.
--
To view, visit https://review.coreboot.org/c/coreboot/+/73367
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I52d77c4e9bff96adba6d265a272e0e425dbdb791
Gerrit-Change-Number: 73367
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Evolve <naresh.solanki.2011(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 12:17:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73366 )
Change subject: xeon_sp: Setup x2apic in SRAT
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/73366
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib8b4cebefe81f7b5514524dba2fa364eee4bb157
Gerrit-Change-Number: 73366
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
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: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 12:15:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73364 )
Change subject: soc/intel/xeon_sp: Fix CBMEM corruption
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/73364
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifbd79e84097231b41f900425a2e8750dce71a25a
Gerrit-Change-Number: 73364
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
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: Evolve <naresh.solanki.2011(a)gmail.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 12:14:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Angel Pons, Arthur Heymans, Keith Hui, Elyes Haouas.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61540 )
Change subject: mb/asus/p8x7x-series: Refactor mainboard_get_spd()
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS4:
> As far as precedents go, it has already been set.
Alas, that's a general problem in coreboot. Many developers can patch/merge what
they like and there is no coordination. And then patterns spread no matter if they
are good, bad, have been discussed or not. __weak spread before, even across project
boundaries into one payload at least where it caused a 4-$-digit bug (not fun to
learn that its inherently incompatible to symbols in static libraries like lib-
payload). IMO, every instance of any __attribute__ should be treated with maximum
delicacy.
TBH, this case here is actually one of the better if not best uses of __weak.
However, there are many alternatives, and hence I don't think a __weak should
be introduced as a footnote without discussion.
> 1. ...
Seems a bit overkill.
> 2. Conditionals in the function by board variant or # of RAM slots (which still needs to be defined somewhere). Compiler and the next programmer looking will have to sort through the inevitable rats nest.
Such definitions are done in Kconfig and that is actually the cannonical way to
do things in coreboot, I believe. I don't understand what the rats nest is?
> 3. Leave out the __weak and leave it to the guy porting a P8x7x with less than 4 RAM slots (eg. P8H77-I, ITX with 2 slots) to patch the override. This could just be kicking the ball down the road, and his patch could be more ugly.
It could be more ugly, could also be less ugly. I'm not able to predict the
future and assume the same is true for others. A discussion with an actual case
at hand usually leads better results than a preemptive one based on assumptions.
Hence, I see nothing wrong with postponing the discussion.
> 4. Leave out the __weak and force all current and future P8x7x variants to try reading 4 sets of SPD data. Current variants are not the problem (otherwise this patch would not exist) but future <4 slots boards would have useless work done in runtime.
I don't think we can enforce anything for the future (at least not with the current,
unstructured coreboot development).
Let me throw another alternative into the ring:
5. Add an array with the SMBus addresses to the devicetree. The devicetree already
has an override mechanism.
The thought actually reminded me that there might be patches for this already...
Also reminds me that I had another discussion about maintaining boilerplate-code
vs. getting rid of it (by making more use of the devicetree for instance) less
than two weeks ago. Would probably be better have such discussions on the ML
where everybody can see them (I tried to encourage more ML discussions in the
past but currently lack the strength for it (feels too much like nobody else wants
to use the ML)).
--
To view, visit https://review.coreboot.org/c/coreboot/+/61540
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3b9c616a6c722e1f0fc124ced225efdcadb46b25
Gerrit-Change-Number: 61540
Gerrit-PatchSet: 6
Gerrit-Owner: Keith Hui <buurin(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Keith Hui <buurin(a)gmail.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Thu, 02 Mar 2023 11:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Keith Hui <buurin(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph.
Evolve has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73367 )
Change subject: memory_info: Bump to 64 DIMMs
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/73367
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I52d77c4e9bff96adba6d265a272e0e425dbdb791
Gerrit-Change-Number: 73367
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Evolve <naresh.solanki.2011(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 11:25:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu.
Evolve has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73364 )
Change subject: soc/intel/xeon_sp: Fix CBMEM corruption
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/73364
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifbd79e84097231b41f900425a2e8750dce71a25a
Gerrit-Change-Number: 73364
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Evolve <naresh.solanki.2011(a)gmail.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 11:24:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment