Attention is currently required from: Jason Glenesk, Raul Rangel, EricKY Cheng, Matt DeVillier, Caveh Jalali, Paul Menzel, Tim Wawrzynczak, Eric Peers, Fred Reitberger, Karthikeyan Ramasubramanian, Boris Mittelberg, Felix Held.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68471 )
Change subject: common/acpi/dptc: Implement DTTS Proposal
......................................................................
Patch Set 25:
(1 comment)
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/60c7e972_5b69a587
PS12, Line 44: TIN4
> As your and Raul suggestions, you mean move all these change out of dptc.asl? […]
Please don't resolve comments that are still incomplete, since it makes it difficult to see there are still unanswered questions.
Yes, all of this logic should be moved into a variant-specific `.asl` file, since this tuning is not common to all Mendocino SOCs.
My idea would be to do something like:
`src/soc/amd/common/acpi/dptc.asl`
Update `_SB.DPTC()` to call the variant-specific DPTC method, if it exists. That function will need to be called after the low/no battery check, so all of the Skyrim variants still support that feature. There are no convertible Whiterun devices, so in this case, we can simply do:
```
/* If _SB.DTHL is not present, then DPTC Tablet Mode is not enabled.
* Throttle the SOC if the battery is not present (BTEX), the battery level is critical
* (BFCR), or the battery is cutoff (BFCT). */
If (CondRefOf (\_SB.DTHL) &&
(!\_SB.PCI0.LPCB.EC0.BTEX || \_SB.PCI0.LPCB.EC0.BFCR || \_SB.PCI0.LPCB.EC0.BFCT))
{
\_SB.DTHL()
Return (Zero)
}
[[[ new code start ]]]
/* If _SB.DVAR is not present, then variant-specific DPTC is not enabled. */
If (CondRefOf (\_SB.DVAR))
{
\_SB.DVAR()
Return (Zero)
}
[[[ new code end ]]]
/* If _SB.DTAB is not present, then DPTC Tablet Mode is not enabled. */
If (CondRefOf (\_SB.DTAB) && (\_SB.PCI0.LPCB.EC0.TBMD == One))
{
\_SB.DTAB()
Return (Zero)
}
```
- NOTE: `_SB.DVAR()` is just my example name for the variant-specific DPTC method. Feel free to pick whatever you like that makes sense.
Add a new `dptc_override.asl` to `src/mainboard/google/skyrim/variants/winterhold` (or some name that makes sense). Then, `src/mainboard/google/skyrim/dsdt.asl` can `#include` that file before `soc.asl` so the override function is visible and callable by the common `_SB.DPTC()`.
I think this limits the variant-specific `asl` changes in `src/soc/amd/common/acpi/dptc.asl` to checking if the variant-specific method exists and calling it, just like any other mode/feature (e.g., low/no battery, tablet mode).
@Raul - Thoughts on this kind of approach?
--
To view, visit https://review.coreboot.org/c/coreboot/+/68471
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I866e5e497e2936984e713029b5f0b6d54cbc9622
Gerrit-Change-Number: 68471
Gerrit-PatchSet: 25
Gerrit-Owner: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Eric Peers <epeers(a)google.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: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Eric Peers <epeers(a)google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 03 Nov 2022 23:20:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Tarun Tuli, Kangheui Won, Ren Kuo, Tim Wawrzynczak, Tyler Wang, Vidya Gopalakrishnan, Peter Ou.
Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68523 )
Change subject: mb/google/nissa/var/craask: Modify DPTF related settings
......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/google/brya/variants/craask/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/68523/comment/3d6714e5_e300c513
PS3, Line 154: .time_window_min = 28 * MSECS_PER_SEC,
> Done, thanks!
Should this really be 0? That seems strange. I thought it would make more sense to round to 1 ms?
Also does pl2.time_window_max need to be changed too?
--
To view, visit https://review.coreboot.org/c/coreboot/+/68523
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I88c8c4e6798ec5bc2930dd713e8c8b2c543cfaf9
Gerrit-Change-Number: 68523
Gerrit-PatchSet: 5
Gerrit-Owner: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Peter Ou <peter.ou(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Cathy Chen <cathy_chen(a)quanta.corp-partner.google.com>
Gerrit-CC: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-CC: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-CC: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.corp-partner.google.com>
Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Attention: Kangheui Won <khwon(a)chromium.org>
Gerrit-Attention: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Attention: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.corp-partner.google.com>
Gerrit-Attention: Peter Ou <peter.ou(a)quanta.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 23:01:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Comment-In-Reply-To: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.corp-partner.google.com>
Comment-In-Reply-To: Peter Ou <peter.ou(a)quanta.corp-partner.google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jason Nien, EricKY Cheng, Paul Menzel, Jon Murphy, Nelson Ye, Chris Wang, Martin Roth, Karthik Ramasubramanian.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68742 )
Change subject: mb/google/skyrim/var/winterhold: Update DPTC setting for EVT-SMT
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/68742
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4aa90304e1e7bda7d580de2582129191e9eb0e76
Gerrit-Change-Number: 68742
Gerrit-PatchSet: 4
Gerrit-Owner: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Chris Wang <chris.wang(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Isaac Lee <isaaclee(a)google.com>
Gerrit-CC: Nelson Ye <nelson_ye(a)compal.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Nelson Ye <nelson_ye(a)compal.corp-partner.google.com>
Gerrit-Attention: Chris Wang <chris.wang(a)amd.corp-partner.google.com>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:48:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jason Nien, EricKY Cheng, Chris Wang, Martin Roth, Eric Peers, Karthikeyan Ramasubramanian.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68472 )
Change subject: mb/google/skyrim/var/winterhold: update thermal config
......................................................................
Patch Set 18: Code-Review+1
(1 comment)
File src/mainboard/google/skyrim/variants/winterhold/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/68472/comment/7a969151_9594abce
PS7, Line 6: register "system_configuration" = "4"
> Thanks for the doc numbers. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/68472
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie0740cb5bb16cd53c2ee6937e32a974346012823
Gerrit-Change-Number: 68472
Gerrit-PatchSet: 18
Gerrit-Owner: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Chris Wang <chris.wang(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Eric Peers <epeers(a)google.com>
Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nelson Ye <nelson_ye(a)compal.corp-partner.google.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Gerrit-Attention: Chris Wang <chris.wang(a)amd.corp-partner.google.com>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Eric Peers <epeers(a)google.com>
Gerrit-Attention: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:47:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com>
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Julius Werner.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68953 )
Change subject: security/vboot: Update build rules using x86 SHA extension
......................................................................
Patch Set 3:
(1 comment)
File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/68953/comment/8447e36c_bc28e728
PS3, Line 31:
> Actually splitting long lines in makefile introduces space automatically even if there is no space i […]
Another odd behavior that I observed is for bootblock, this condition evaluates to nothing, not even an empty string.
bootblock
romstage "y"
ramstage "y"
postcar "y"
verstage ""
--
To view, visit https://review.coreboot.org/c/coreboot/+/68953
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4f388e963eb82990cda41d3880e66ad937334908
Gerrit-Change-Number: 68953
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:44:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Julius Werner.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68953 )
Change subject: security/vboot: Update build rules using x86 SHA extension
......................................................................
Patch Set 3:
(1 comment)
File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/68953/comment/84f38153_2eaa3b21
PS3, Line 31:
> Just double-checking, are you sure this whitespace will be removed by make? We wouldn't want it to e […]
Actually splitting long lines in makefile introduces space automatically even if there is no space in the true part. So I have to replace the space with $ i.e. " \" with "$\" - https://www.gnu.org/software/make/manual/make.html#Splitting-Lines
--
To view, visit https://review.coreboot.org/c/coreboot/+/68953
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4f388e963eb82990cda41d3880e66ad937334908
Gerrit-Change-Number: 68953
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:21:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Jonathan Zhang, Johnny Lin, Paul Menzel, Jingle Hsu.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68779 )
Change subject: drivers/ipmi/ocp: add functions to get board configuration
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/68779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I85ec7ba68d580c13e368e7d656dba47ea043d33e
Gerrit-Change-Number: 68779
Gerrit-PatchSet: 7
Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Jingle Hsu <jingle_hsu(a)wiwynn.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jingle Hsu <jingle_hsu(a)wiwynn.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:03:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, Rocky Phagura, Paul Menzel, Shuming Chu (Shuming), Tim Chu.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68758 )
Change subject: drivers/ipmi/ocp: add PCIe SEL support
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
File src/drivers/ipmi/ocp/ipmi_sel.c:
https://review.coreboot.org/c/coreboot/+/68758/comment/e59900ff_79c38da1
PS7, Line 11: if (CONFIG(IPMI_KCS))
Why?
ipmi_add_sel_entry is only called in ipmi_send_to_bmc and that is already guarded by if(CONFIG(IPMI_KCS)) - there should be no need to guard it twice.
--
To view, visit https://review.coreboot.org/c/coreboot/+/68758
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1ee46c8da7dbccbe1e2cc00bfe62e5df2f072d65
Gerrit-Change-Number: 68758
Gerrit-PatchSet: 7
Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Rocky Phagura <rphagura(a)fb.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Rocky Phagura <rphagura(a)fb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 22:03:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment