Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/72418?usp=email )
Change subject: drivers/intel/gma: Dump output setting only if DEBUG_ADA_CODE is set
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Why was this changed? We don't hide the output of other components in coreboot
by default either. Without this, it's impossible to see if libgfxinit did any-
thing at all in the log of a release build. Is this really what we want?
Please disclose all the information from the Google bug tracker.
--
To view, visit https://review.coreboot.org/c/coreboot/+/72418?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iadd6c9552b184f7d6ec8df9d0d392634864ba50c
Gerrit-Change-Number: 72418
Gerrit-PatchSet: 4
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-CC: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Zhixing Ma <zhixing.ma(a)intel.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:39:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Dolan Liu, Elyes Haouas, Jianeng Ceng, Paul Menzel, Subrata Banik.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81773?usp=email )
Change subject: drivers/i2c/rt5645: Add RT5645 amp driver
......................................................................
Patch Set 24:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81773/comment/1ccecd23_e457c325 :
PS24, Line 9: In order to be different from rt5663, the driver of RT5645 is
: specially declared.
> Sorry, just reading this, I do not understand. Please list the differences.
while the wording here is slightly awkward, I don't see enumerating the differences between the two drivers/chips in the commit msg being useful or necessary. Simply stating that the RT5645/5650 are different enough from the RT5663 to warrant a separate implementation is sufficient IMO
https://review.coreboot.org/c/coreboot/+/81773/comment/6700f071_3b9f9a2c :
PS24, Line 17:
> Please also mention, that you hide the device because of Microsoft Windows.
or stated another way, the ACPI device is being created specifically for Linux
--
To view, visit https://review.coreboot.org/c/coreboot/+/81773?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: I602fcc4dd8576043943f6e20884edc4703350320
Gerrit-Change-Number: 81773
Gerrit-PatchSet: 24
Gerrit-Owner: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:35:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81597?usp=email )
Change subject: cpu/intel/model_206ax: Allow to configure VR settings
......................................................................
cpu/intel/model_206ax: Allow to configure VR settings
Allow to set board specific CPU voltage regulator settings.
The VR12 compatible voltage regulator for the CPU can be configured
by two MSRs. Currently a default value is applied, which mimics the
Intel reference code and is what the BWG suggest. However most board
vendors fill in the actual VR parameters to support OC or ULV board
variants.
When the mainboard design is too different from the Intel reference
design, not updating the VR settings might result in:
- unstable system behaviour
- limited turbo performance
- excessive battery drain
- no over-clocking capability
This patch adds support to set the board specific current limit for
Icc and Igfx.
It also allows to adjust PSI1, PSI2 and PSI3, which are powerstates
used by the VR, that consume less energy when the system is idle.
Test on Lenovo X220 with full CPU load after 1 minute, compared to
previous code with default settings:
- Limiting PP0 max current below Iccmax results in less CPU performance.
RAPL readings show that less power is drawn over time.
- Limiting PP0 max current to Iccmax results in equal CPU performance.
RAPL readings show that the same power is drawn over time.
- Setting the PP0 max current to a value >> Iccmax results in equal CPU
performance. RAPL readings show that the same power is drawn over
time.
- Updating the MSR at runtime has no effect.
Change-Id: I59edab47fc4fbe0240e1dd7d25647f7549b4def2
Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81597
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
---
M src/cpu/intel/model_206ax/chip.h
M src/cpu/intel/model_206ax/model_206ax_init.c
M src/mainboard/lenovo/x220/devicetree.cb
3 files changed, 102 insertions(+), 9 deletions(-)
Approvals:
Angel Pons: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/src/cpu/intel/model_206ax/chip.h b/src/cpu/intel/model_206ax/chip.h
index 57e145e..9080e2f 100644
--- a/src/cpu/intel/model_206ax/chip.h
+++ b/src/cpu/intel/model_206ax/chip.h
@@ -14,12 +14,40 @@
CPU_ACPI_C7S,
};
+/* VR12 PSI codes */
+enum vr12_phases {
+ VR12_KEEP_DEFAULT = 0, /* For device-trees missing the setting */
+ VR12_ALL_PHASES,
+ VR12_2_PHASES,
+ VR12_1_PHASE,
+ VR12_LIGHT_LOAD,
+};
+
+/* VR12 power state listing */
+enum vr12_psi {
+ VR12_PSI1 = 0,
+ VR12_PSI2,
+ VR12_PSI3,
+ VR12_PSI_MAX,
+};
+
+struct psi_state {
+ enum vr12_phases phases;
+ int current; /* In Amps */
+};
+
struct cpu_intel_model_206ax_config {
enum cpu_acpi_level acpi_c1;
enum cpu_acpi_level acpi_c2;
enum cpu_acpi_level acpi_c3;
int tcc_offset; /* TCC Activation Offset */
+ int pp0_current_limit; /* Primary Plane Current Limit (Icc) in Amps */
+ int pp1_current_limit; /* Secondary Plane Current Limit (IAXG) in Amps */
+
+ /* PSI states only have an effect when in Package C3 or higher */
+ struct psi_state pp0_psi[3]; /* Power states for Primary Plane (Icc) */
+ struct psi_state pp1_psi[3]; /* Power states for Secondary Plane (IAXG) */
};
#endif /* __CPU_INTEL_MODEL_206AX_CHIP_H__ */
diff --git a/src/cpu/intel/model_206ax/model_206ax_init.c b/src/cpu/intel/model_206ax/model_206ax_init.c
index d9340ff..cdec3a6 100644
--- a/src/cpu/intel/model_206ax/model_206ax_init.c
+++ b/src/cpu/intel/model_206ax/model_206ax_init.c
@@ -156,8 +156,9 @@
}
}
-static void configure_c_states(void)
+static void configure_c_states(struct device *dev)
{
+ struct cpu_intel_model_206ax_config *conf = dev->upstream->dev->chip_info;
msr_t msr;
msr = rdmsr(MSR_PKG_CST_CONFIG_CONTROL);
@@ -202,20 +203,70 @@
msr.lo = IRTL_VALID | IRTL_1024_NS | 0x6D;
wrmsr(MSR_PKGC7_IRTL, msr);
- /* Primary Plane Current Limit */
+ /* Primary Plane Current Limit (Icc) */
msr = rdmsr(MSR_PP0_CURRENT_CONFIG);
msr.lo &= ~0x1fff;
- msr.lo |= PP0_CURRENT_LIMIT;
+ if (conf->pp0_current_limit) {
+ /* Fill in board specific maximum current supported by VR */
+ msr.lo |= conf->pp0_current_limit * 8;
+ } else {
+ printk(BIOS_INFO, "%s: PP0 current limit not set in devicetree\n", dev_path(dev));
+ /*
+ * The default value might over-stress the voltage regulator or
+ * prevent OC on boards with regulators that can handle currents
+ * above the Intel recommendation.
+ */
+ msr.lo |= PP0_CURRENT_LIMIT;
+ }
+ for (int i = 0; i < VR12_PSI_MAX; i++) {
+ /*
+ * Light load optimization. Depending on the VR output filter the
+ * number of phases can be reduced at light load. This is a board
+ * specific setting.
+ */
+ if (conf->pp0_psi[i].phases != VR12_KEEP_DEFAULT) {
+ msr.hi &= ~(0x3ff << (i * 10));
+ msr.hi |= (conf->pp0_psi[i].phases - 1) << (i * 10 + 7);
+ msr.hi |= conf->pp0_psi[i].current << (i * 10);
+ } else {
+ printk(BIOS_INFO, "%s: PP0 PSI%d not set in devicetree\n", dev_path(dev), i);
+ }
+ }
msr.lo |= PP0_CURRENT_LIMIT_LOCK;
wrmsr(MSR_PP0_CURRENT_CONFIG, msr);
- /* Secondary Plane Current Limit */
+ /* Secondary Plane Current Limit (IAXG) */
msr = rdmsr(MSR_PP1_CURRENT_CONFIG);
msr.lo &= ~0x1fff;
- if (IS_IVY_CPU(cpu_get_cpuid()))
- msr.lo |= PP1_CURRENT_LIMIT_IVB;
- else
- msr.lo |= PP1_CURRENT_LIMIT_SNB;
+ if (conf->pp1_current_limit) {
+ /* Fill in board specific maximum current supported by VR */
+ msr.lo |= conf->pp1_current_limit * 8;
+ } else {
+ printk(BIOS_INFO, "%s: PP1 current limit not set in devicetree\n", dev_path(dev));
+ /*
+ * The default value might over-stress the voltage regulator or
+ * prevent OC on boards with regulators that can handle currents
+ * above the Intel recommendation.
+ */
+ if (IS_IVY_CPU(cpu_get_cpuid()))
+ msr.lo |= PP1_CURRENT_LIMIT_IVB;
+ else
+ msr.lo |= PP1_CURRENT_LIMIT_SNB;
+ }
+ for (int i = 0; i < VR12_PSI_MAX; i++) {
+ /*
+ * Light load optimization. Depending on the VR output filter the
+ * number of phases can be reduced at light load. This is a board
+ * specific setting.
+ */
+ if (conf->pp1_psi[i].phases != VR12_KEEP_DEFAULT) {
+ msr.hi &= ~(0x3ff << (i * 10));
+ msr.hi |= (conf->pp1_psi[i].phases - 1) << (i * 10 + 7);
+ msr.hi |= conf->pp1_psi[i].current << (i * 10);
+ } else {
+ printk(BIOS_INFO, "%s: PP1 PSI%d not set in devicetree\n", dev_path(dev), i);
+ }
+ }
msr.lo |= PP1_CURRENT_LIMIT_LOCK;
wrmsr(MSR_PP1_CURRENT_CONFIG, msr);
}
@@ -354,7 +405,7 @@
set_vmx_and_lock();
/* Configure C States */
- configure_c_states();
+ configure_c_states(cpu);
/* Configure Enhanced SpeedStep and Thermal Sensors */
configure_misc();
diff --git a/src/mainboard/lenovo/x220/devicetree.cb b/src/mainboard/lenovo/x220/devicetree.cb
index 233f690..dc2f4a5 100644
--- a/src/mainboard/lenovo/x220/devicetree.cb
+++ b/src/mainboard/lenovo/x220/devicetree.cb
@@ -35,6 +35,20 @@
{ 1, 7, 0x0080 },
{ 1, 6, 0x0080 },}"
+ chip cpu/intel/model_206ax
+ # Values obtained from vendor BIOS v1.46
+ # schematics say 33Amps for 17W TDP, 53Amps for 35W TDP
+ register "pp0_current_limit" = "98"
+ # schematics say 33Amps for GFX
+ register "pp1_current_limit" = "33"
+ register "pp0_psi[VR12_PSI1]" = "{VR12_2_PHASES, 20}"
+ register "pp0_psi[VR12_PSI2]" = "{VR12_ALL_PHASES, 5}"
+ register "pp0_psi[VR12_PSI3]" = "{VR12_ALL_PHASES, 1}"
+ register "pp1_psi[VR12_PSI1]" = "{VR12_2_PHASES, 20}"
+ register "pp1_psi[VR12_PSI2]" = "{VR12_ALL_PHASES, 5}"
+ register "pp1_psi[VR12_PSI3]" = "{VR12_ALL_PHASES, 1}"
+ device cpu_cluster 0 on end
+ end
device domain 0 on
subsystemid 0x17aa 0x21db inherit
--
To view, visit https://review.coreboot.org/c/coreboot/+/81597?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: I59edab47fc4fbe0240e1dd7d25647f7549b4def2
Gerrit-Change-Number: 81597
Gerrit-PatchSet: 3
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Felix Held, Máté Kukri, Paul Menzel, Varshit Pandya.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization
......................................................................
Patch Set 9:
(1 comment)
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/065f9d44_b1d5ba0e :
PS9, Line 8: ec_read
> this function name is already used by the generic ec access code, so would be good to rename the two […]
How about `sch555x_mbox_read`?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81529?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: Ibdccd3fc7364e03e84ca606592928410624eed43
Gerrit-Change-Number: 81529
Gerrit-PatchSet: 9
Gerrit-Owner: Máté Kukri <kukri.mate(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Leah Rowe <leahleahrowerowe(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Máté Kukri <kukri.mate(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:24:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Christian Walter, Filip Lewiński, Michał Kopeć.
Hello Christian Walter, Krystian Hebel, Michał Kopeć, Michał Żygowski, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/82037?usp=email
to look at the new patch set (#5).
Change subject: security/tpm: Add TPM2 NV_ReadPublic command support
......................................................................
security/tpm: Add TPM2 NV_ReadPublic command support
Change-Id: I3c032b4f88d445372beebbe354f458a061a63bb9
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-2.0/tss.c
M src/security/tpm/tss/tcg-2.0/tss_marshaling.c
M src/security/tpm/tss/tcg-2.0/tss_structures.h
4 files changed, 153 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/82037/5
--
To view, visit https://review.coreboot.org/c/coreboot/+/82037?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: I3c032b4f88d445372beebbe354f458a061a63bb9
Gerrit-Change-Number: 82037
Gerrit-PatchSet: 5
Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Held, Martin L Roth, Naveen Iyer, Nico Huber.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81296?usp=email )
Change subject: Docs/tutorial: Do not install Ada compiler by default
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Thanks for your response. […]
Sounds good to me. Martin, Nico, Felix, any comments?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81296?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: I57f421f7ee1bda4063c13d372fe9c32b95cfd2bc
Gerrit-Change-Number: 81296
Gerrit-PatchSet: 2
Gerrit-Owner: Naveen Iyer
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Naveen Iyer
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:19:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com>
Comment-In-Reply-To: Naveen Iyer
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Dinesh Gehlot, Jérémy Compostella, Nick Vaccaro, Tarun Tuli, Tim Wawrzynczak.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/67494?usp=email )
Change subject: gma pipe_setup: Update for TGL
......................................................................
Patch Set 32: Code-Review+1
(10 comments)
File common/hw-gfx-gma-config.ads.template:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/15a6b2d6_84f9ee75 :
PS32, Line 217: Need_Pipe_Arb_Slots : <tglbool> := Alderlake_On;
For reference, this is `Wa_22012358565:adl-p`
https://github.com/torvalds/linux/commit/0b86952d15ceae275f685f9bb571fea309…https://review.coreboot.org/c/libgfxinit/+/67494/comment/ac1b0b0f_6b89ea0d :
PS32, Line 222: Has_TGL_Plane_Control : <genbool> := Tigerlake_On;
Looks like the `PLANE_COLOR_CTL` registers were first introduced with GLK (N.B. `DISPLAY_VER(i915)` is 10 for GLK), and a few bits from `PLANE_CTL` got moved into the new register. Maybe `Has_Plane_Color_Control` would be a more precise name (this flag indicates whether the register is supported).
https://github.com/torvalds/linux/commit/47f9ea8b9143890c9b98ad9e16e8ad793b…https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65…
File common/hw-gfx-gma-pipe_setup.adb:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/9c009339_af59e087 :
PS32, Line 46: PLANE_COLOR_CTL_GAMMA_DISABLE : constant := 1 * 2 ** 13;
nit: Apparently this bit is *plane* gamma disable. There's a *pipe* gamma **enable** bit (30) in the same register (even though it's deprecated), which might lead to confusion. How about renaming this to `PLANE_COLOR_PLANE_GAMMA_DISABLE` (same name as i915)?
https://review.coreboot.org/c/libgfxinit/+/67494/comment/5187ab92_28c9fae4 :
PS32, Line 228: function PLANE_CTL_ARB_SLOTS (N : Word32) return Word32
: is (Shift_Left (N, 28));
nit: format function expressions so that the first line ends with `is` and the second line contains the expression value indented one level:
```
function PLANE_CTL_ARB_SLOTS (N : Word32) return Word32 is
(Shift_Left (N, 28));
```
https://review.coreboot.org/c/libgfxinit/+/67494/comment/5df62fbe_60450553 :
PS32, Line 254: then PLANE_CTL_ARB_SLOTS (1)
Looks like the value used here is dependent on plane state:
https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65…https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65…
Is it OK to hardcode `1` here? If so, why?
https://review.coreboot.org/c/libgfxinit/+/67494/comment/49cdf2b3_0ee23aa1 :
PS32, Line 326: type BW_Credit is new Natural range 0 .. 3;
: function MBUS_DBOX_BW_CREDIT (C : BW_Credit) return Word32
: is (Shift_Left (Word32 (C), 14));
:
: type B_Credit is new Natural range 0 .. 31;
: function MBUS_DBOX_B_CREDIT (C : B_Credit) return Word32
: is (Shift_Left (Word32 (C), 8));
:
: type A_Credit is new Natural range 0 .. 15;
: function MBUS_DBOX_A_CREDIT (C : A_Credit) return Word32
: is (Word32 (C));
:
: type B2B_Trans_Max is new Natural range 0 .. 31;
: function MBUS_DBOX_B2B_TRANSACTIONS_MAX (B : B2B_Trans_Max) return Word32
: is (Shift_Left (Word32 (B), 20));
:
: type B2B_Trans_Delay is new Natural range 0 .. 7;
: function MBUS_DBOX_B2B_TRANSACTIONS_DELAY (B : B2B_Trans_Delay) return Word32
: is (Shift_Left (Word32 (B), 17));
: MBUS_DBOX_REGULATE_B2B_TRANSACTIONS_EN : constant := 1 * 2 ** 16;
:
nit: format for function expressions (see previous comment)
https://review.coreboot.org/c/libgfxinit/+/67494/comment/7ae070d5_e67fda80 :
PS32, Line 354: if Config.Has_New_Mbus_Dbox_Credits then
Where is this procedure described? The only procedure in i915 that seems to use these registers is this: https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65…
Looks like these steps are only applied when some conditions are met. I guess it doesn't matter much for us; we can set these bits once and call it a day.
Given the WA name (`Wa_22010947358:adl-p`), one may think this is only for ADL-P. But i915 only distinguishes between ADL-P and ADL-S, so I believe `Has_New_Mbus_Dbox_Credits` has the right value.
What I don't know is whether we can forget about `joined_mbus`.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/517561ae_c2641f98 :
PS32, Line 380: -- ADL_P requires that we disable underrun recovery when
: -- downscaling (or using the scaler for YUV420 pipe output),
: -- using DSC, or using PSR2.
From i915 sources:
> Underrun recovery must always be disabled on display 13+.
> DG2 chicken bit meaning is inverted compared to other platforms.
Looks like this got updated in this i915 commit: https://github.com/torvalds/linux/commit/4fe7907f3775034140a518d1582580926d…https://review.coreboot.org/c/libgfxinit/+/67494/comment/a5f28f08_10a836d5 :
PS32, Line 387: if Config.Has_Type_C_Ports then
Note to self: equivalent to `if (DISPLAY_VER(dev_priv) >= 11)`
https://review.coreboot.org/c/libgfxinit/+/67494/comment/848adc5a_36bd5eeb :
PS32, Line 441: Has_TGL_Plane_Control
AIUI, this is TGL+ (technically `PIPE_MISC_HDR_MODE_PRECISION` is ICL+ and only when `is_hdr_mode(crtc_state)`). But I feel using `Has_TGL_Plane_Control` here is not accurate (the `PIPE_MISC` register is not about *plane* control).
--
To view, visit https://review.coreboot.org/c/libgfxinit/+/67494?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: libgfxinit
Gerrit-Branch: main
Gerrit-Change-Id: I93329c0a012da83abc379d6782fabe257dc180f3
Gerrit-Change-Number: 67494
Gerrit-PatchSet: 32
Gerrit-Owner: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com>
Gerrit-CC: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:18:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Alicja Michalska.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81611?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: Documentation: Add Erying Polestar G613 Pro
......................................................................
Documentation: Add Erying Polestar G613 Pro
Documentation entry has to be submitted with board tree that's currently
being reviewed upstream.
Change-Id: I5d60508dbde10373b0da2fb4ece0992760d3121c
Signed-off-by: Alicja Michalska <ahplka19(a)gmail.com>
---
A Documentation/mainboard/erying/tgl_matx.md
A Documentation/mainboard/erying/tgl_matx_board.jpg
M Documentation/mainboard/index.md
3 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/81611/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/81611?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: I5d60508dbde10373b0da2fb4ece0992760d3121c
Gerrit-Change-Number: 81611
Gerrit-PatchSet: 4
Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Alicja Michalska, Felix Singer, Maxim, Michał Żygowski, Nicholas Chin, Paul Menzel.
Hello Felix Singer, Maxim, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/80853?usp=email
to look at the new patch set (#8).
The following approvals got outdated and were removed:
Code-Review+1 by Maxim, Verified+1 by build bot (Jenkins)
Change subject: mb/erying: Add Erying Polestar G613 Pro (TGL-H)
......................................................................
mb/erying: Add Erying Polestar G613 Pro (TGL-H)
Erying is a Chinese manufacturer selling desktop motherboards with
laptop SoCs and custom shim to mount desktop coolers.
Working:
- Serial port (IT8613E RS232)
- All rear USB ports (3.0, 2.0)
- Both HDMI ports
- Realtek GbE NIC
- Internal audio (ALC897/ TGL-H HDMI)
- Environment Controller (SuperIO fan control)
- All SATA ports
- All PCI-E/M.2 ports (somewhat)
- M.2 NGFF WiFi
- PCI-E Resizable BAR (ReBAR)
- VT-x (PCI-E passtrough, broken on stock)
WIP/Broken:
- PCI-E ASPM (even though I force-disabled it, I'm still getting AERs)
- S3/s0ix (also broken on stock, setting 3VSB register didn't help -
system goes to sleep, but RAM loses power)
- DisplayPort on I/O panel (simple fix, need to re-configure GPIOs)
- One of USB2 FP connectors, as well as NGFF USB isn't mapped (yet)
- Automatic fan control (IT8613E can't read CPU_TIN at the moment)
Can be flashed using `flashrom -p internal -w build/coreboot.rom`, as
vendor hasn't enabled any protections on SPI chip.
I'd like to get ASPM working as it makes big difference in idle power
consumtion (25 vs 60W measured from the wall at 230V).
Likewise, I can't wrap my head around PCI-E AERs I'm getting if I boot
the machine without `pcie_aspm=off` parameter:
- BadTLP
- BadDLLP
- Timeout
- Rollover
Adjusting LaneEq's didn't change anything, all settings are configured
in (mostly) the same way as they were on stock firmware.
Starting to suspect Intel's FSP might be buggy, as I haven't had those
issues when I initially started working on this project when 4.20 tree
was current.
TEST=Flash coreboot build onto the motherboard, install following PCI-E
cards: Radeon RX 7800XT, Kingston KC3000, Optane 900P, Audigy X-Fi.
Power the system up and boot into Windows 10 to check ACPI sanity, then
reboot into Fedora Linux (kernel 6.7.4) and launch 3D application, disk
benchmark, compilation at the same time to check system's stability.
Change-Id: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e
Signed-off-by: Alicja Michalska <ahplka19(a)gmail.com>
---
A src/mainboard/erying/Kconfig
A src/mainboard/erying/Kconfig.name
A src/mainboard/erying/tgl/Kconfig
A src/mainboard/erying/tgl/Kconfig.name
A src/mainboard/erying/tgl/Makefile.mk
A src/mainboard/erying/tgl/board_info.txt
A src/mainboard/erying/tgl/bootblock.c
A src/mainboard/erying/tgl/cmos.layout
A src/mainboard/erying/tgl/data.vbt
A src/mainboard/erying/tgl/devicetree.cb
A src/mainboard/erying/tgl/dsdt.asl
A src/mainboard/erying/tgl/gpio.h
A src/mainboard/erying/tgl/hda_verb.c
A src/mainboard/erying/tgl/ramstage.c
A src/mainboard/erying/tgl/romstage_fsp_params.c
15 files changed, 856 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/80853/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/80853?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: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e
Gerrit-Change-Number: 80853
Gerrit-PatchSet: 8
Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Maxim <max.senia.poliak(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Maxim <max.senia.poliak(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Dolan Liu, Elyes Haouas, Jianeng Ceng, Subrata Banik.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81773?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: drivers/i2c/rt5645: Add RT5645 amp driver
......................................................................
Patch Set 24:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81773/comment/fad78662_2d0b5819 :
PS24, Line 9: In order to be different from rt5663, the driver of RT5645 is
: specially declared.
Sorry, just reading this, I do not understand. Please list the differences.
https://review.coreboot.org/c/coreboot/+/81773/comment/5c7749fa_8e5459a6 :
PS24, Line 17:
Please also mention, that you hide the device because of Microsoft Windows.
File src/drivers/i2c/rt5645/rt5645.c:
https://review.coreboot.org/c/coreboot/+/81773/comment/96073253_a4f2e11a :
PS24, Line 48: acpigen_write_STA(ACPI_STATUS_DEVICE_HIDDEN_ON);
Maybe add a comment here, that it is for Microsoft Windows.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81773?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: I602fcc4dd8576043943f6e20884edc4703350320
Gerrit-Change-Number: 81773
Gerrit-PatchSet: 24
Gerrit-Owner: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Mon, 22 Apr 2024 15:12:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment