Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81958?usp=email )
Change subject: soc/intel/xeon_sp: Add soc_add_dram_resources
......................................................................
Patch Set 2:
(2 comments)
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/11ece306_3025ebfe :
PS1, Line 234: * @param mc_values List of system memory map variables.
> Personally, I think to have the soc memory map codes to get a full picture of system memory map sett […]
This would mean that you'd move the entire enum into the API only to move
it back in the next commit? I don't think this makes sense, it would create
noise in the git history.
If you want to split the two steps (making it TOHM only, and moving the code
to another file), you could first create a static function in `uncore.c` with
the new interface, and then move the code in a second commit.
You seem to have missed my question above:
> After this patch all the array entries but TOHM are handled by common code. If
> an implementation of soc_add_dram_resources() would try to handle other entries
> as well, wouldn't that cause conflicts?
IMO, this is very important for the API design: "[...], wouldn't that cause
conflicts?". If it would, we should never create such an API that encourages
error-prone code.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/d603ae7c_036d3e61 :
PS1, Line 306: } else {
> It's hard to review this change without knowing what the implementation of `soc_add_dram_resources` […]
Sorry, I guess this was misunderstood. Your gen1 soc_add_dram_resources()
has all its code in two branches of an `if` that is decided at compile time.
This already makes it two different implementations of the same function.
With splitting I meant to split it into to separate soc_add_dram_resources()
functions one for gen1 w/o CXL (`chip_gen1.c` seems to be a good place for
that) and one for SPR which has CXL (if it's not shared between platforms,
I guess it should reside somewhere in `spr/`).
--
To view, visit https://review.coreboot.org/c/coreboot/+/81958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie15b11e1f4cdc861324286b1397f9c5e431b74ab
Gerrit-Change-Number: 81958
Gerrit-PatchSet: 2
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.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: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 21:25:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Karthik Ramasubramanian, Nick Vaccaro, Pavan Holla, Shelley Chen.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81967?usp=email )
Change subject: ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled
......................................................................
Patch Set 2:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81967/comment/5a7e9123_0f448292 :
PS2, Line 7: Hide typec ACPI device if UCSI is supported
Please add prefix:
> ec/google/chromeec
or
> google/chromeec
https://review.coreboot.org/c/coreboot/+/81967/comment/806c4d01_0b90b14e :
PS2, Line 19: https://crrev.com/c/5416841 is the change for adding the feature flag
Please add a dot/period at the end of sentences.
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/81967/comment/929ede80_763620ce :
PS2, Line 435: int
Why not bool?
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/311f00cc_392354de :
PS2, Line 735: printk(BIOS_INFO, "Cannot check whether EC_FEATURE_UCSI_PPM is available.\n");
Log the return value?
File src/ec/google/chromeec/ec_acpi.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/fd287061_d146bbf3 :
PS2, Line 161: /* UCSI implementations do not require an ACPI device
: * with mux info since the linux kernel doesn't set
: * the muxes. */
Please use the recommended commenting styles.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81967?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67dff6445aa7ba3ba48a04d1df3541f880d09d0a
Gerrit-Change-Number: 81967
Gerrit-PatchSet: 2
Gerrit-Owner: Pavan Holla <pholla(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daisuke Nojiri <dnojiri(a)chromium.org>
Gerrit-CC: Forest Mittelberg <bmbm(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Pavan Holla <pholla(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 21:22:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Karthik Ramasubramanian, Nick Vaccaro, Pavan Holla, Shelley Chen.
Hello Caveh Jalali, Karthik Ramasubramanian, Nick Vaccaro, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81967?usp=email
to look at the new patch set (#8).
The following approvals got outdated and were removed:
Code-Review+2 by Karthik Ramasubramanian, Code-Review-1 by Caveh Jalali, Verified+1 by build bot (Jenkins)
Change subject: ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled
......................................................................
ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled
Do not fill the ACPI table entry associated with the cros_ec_typec
driver once we switch to the UCSI kernel driver. Skip the ACPI entry if
EC implements the UCSI_PPM feature, and the CBI flag to enable UCSI is
set.
BUG=b:333078787
TEST=emerge-brox coreboot chromeos-bootimage
Cq-Depend: chromium:5416841
Change-Id: I67dff6445aa7ba3ba48a04d1df3541f880d09d0a
Signed-off-by: Pavan Holla <pholla(a)google.com>
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/ec/google/chromeec/ec_acpi.c
3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/81967/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/81967?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67dff6445aa7ba3ba48a04d1df3541f880d09d0a
Gerrit-Change-Number: 81967
Gerrit-PatchSet: 8
Gerrit-Owner: Pavan Holla <pholla(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daisuke Nojiri <dnojiri(a)chromium.org>
Gerrit-CC: Forest Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Pavan Holla <pholla(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81957?usp=email )
Change subject: device_util: Handle domain device in dev_get_domain
......................................................................
Patch Set 1:
(1 comment)
File src/device/device_util.c:
https://review.coreboot.org/c/coreboot/+/81957/comment/d1f9e43f_61702822 :
PS1, Line 255: dev
> I suppose the NULL-deref should not be a problem in the previous codes? Since it is blocked by the w […]
The original code checked `dev` and `dev->upstream`:
```
while (dev && dev->upstream && ...
```
But then `dev` got overwritten:
```
dev = dev->upstream->dev;
```
and the new `dev` was dereferenced unchecked:
```
if (dev->path...
```
I don't know if it matters. A valid device tree should never have a NULL
there. But the same applies to `dev->upstream` and we check it too. It
was just inconsistent to check one but not the other.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81957?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I3a278a8f573de95406ee256fba17767def4ad75d
Gerrit-Change-Number: 81957
Gerrit-PatchSet: 1
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-CC: 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: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 21:11:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Arthur Heymans, Maximilian Brune, Nico Huber.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64184?usp=email )
Change subject: haswell NRI: Initialise MPLL
......................................................................
Patch Set 5:
(1 comment)
File src/northbridge/intel/haswell/native_raminit/init_mpll.c:
https://review.coreboot.org/c/coreboot/+/64184/comment/60833669_71404c9d :
PS5, Line 155: if (!ctrl->qclkps) {
> Ah, I remember: this is for the fast boot path. […]
ah, ok. adding a comment about that would make the code easier to understand
--
To view, visit https://review.coreboot.org/c/coreboot/+/64184?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I978c352de68f6d8cecc76f4ae3c12daaf4be9ed6
Gerrit-Change-Number: 64184
Gerrit-PatchSet: 5
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 21:03:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Nick Vaccaro, Pavan Holla, Shelley Chen.
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81967?usp=email )
Change subject: ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled
......................................................................
Patch Set 7:
(1 comment)
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/af2bd12f_ef2be6e9 :
PS7, Line 729: uint32_t cc
Do you want to define cc as union ec_common_control? Then, you can call cbi_get_uint32:
rv = cbi_get_uint32(&cc.raw_value, CBI_TAG_COMMON_CONTROL);
and change line 746 to:
*ucsi_enabled = cc.ucsi_enabled;
--
To view, visit https://review.coreboot.org/c/coreboot/+/81967?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67dff6445aa7ba3ba48a04d1df3541f880d09d0a
Gerrit-Change-Number: 81967
Gerrit-PatchSet: 7
Gerrit-Owner: Pavan Holla <pholla(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daisuke Nojiri <dnojiri(a)chromium.org>
Gerrit-CC: Forest Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Pavan Holla <pholla(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 21:03:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nick Vaccaro, Pavan Holla, Shelley Chen.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81967?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled
......................................................................
Patch Set 7: Code-Review-1
(1 comment)
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/81967/comment/3f5618fd_f79f6180 :
PS7, Line 427: CBI_COMMON_CONTROL_UCSI_ENABLED
this bit position needs to be derived from ec_commands.h
otherwise we have to way to correlate the coreboot vs EC
implementation.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81967?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67dff6445aa7ba3ba48a04d1df3541f880d09d0a
Gerrit-Change-Number: 81967
Gerrit-PatchSet: 7
Gerrit-Owner: Pavan Holla <pholla(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Forest Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Pavan Holla <pholla(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 20:03:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment