Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47115 )
Change subject: ec/purism/librem/ec.asl: End comment
......................................................................
Patch Set 2:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/47115
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id192d665a22a8885d7cec56cd6b8ea207fb54402
Gerrit-Change-Number: 47115
Gerrit-PatchSet: 2
Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 04 Nov 2020 16:49:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46485 )
Change subject: mb/intel/adlrvp: Add support for DDR5 memory
......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46485/7/src/mainboard/intel/adlrvp…
File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:
https://review.coreboot.org/c/coreboot/+/46485/7/src/mainboard/intel/adlrvp…
PS7, Line 62: .rcomp_targets = {50, 30, 30, 30, 27},
> No, I meant to have something like: […]
Sounds good Angel, will wait for your observation, you can modify this later as well
--
To view, visit https://review.coreboot.org/c/coreboot/+/46485
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4711d66c7b4b7b09e15a4d06e28c876ec35bc192
Gerrit-Change-Number: 46485
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.bmg(a)gmail.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 04 Nov 2020 15:39:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46485 )
Change subject: mb/intel/adlrvp: Add support for DDR5 memory
......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46485/7/src/mainboard/intel/adlrvp…
File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:
https://review.coreboot.org/c/coreboot/+/46485/7/src/mainboard/intel/adlrvp…
PS7, Line 62: .rcomp_targets = {50, 30, 30, 30, 27},
> Sure Angel. Nice feedback. […]
No, I meant to have something like:
const uint16_t rcomp_targets_ddr5_p[5] = {50, 30, 30, 30, 27};
Then, one should be able to simply use the array as initializer:
.rcomp_targets = rcomp_targets_ddr5_p,
I haven't tested this, though. I would also like to ensure these Rcomp targets are reasonable defaults. I can try to do this for SKL and CFL as an example, since I know the platforms better. I mean, I've desoldered measured the Rcomp resistors on a SKL-S CPU 😄
--
To view, visit https://review.coreboot.org/c/coreboot/+/46485
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4711d66c7b4b7b09e15a4d06e28c876ec35bc192
Gerrit-Change-Number: 46485
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.bmg(a)gmail.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 04 Nov 2020 15:30:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47054 )
Change subject: mb/intel/adlrvp: Replace if-else-if ladder with switch construct
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47054/2/src/mainboard/intel/adlrvp…
File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:
https://review.coreboot.org/c/coreboot/+/47054/2/src/mainboard/intel/adlrvp…
PS2, Line 76: case ADL_P_LP4_1:
> Because missing a 'break' (or return, etc.) in switch statements is a common mistake, I like to add […]
I would agree in other cases, but I'd say this case is pretty self-explanatory already. Generally, I only add a comment about falling through when one case executes something and then falls through another case. Here's an example:
/* Ensure magic is not power gated before reading the status */
pci_and_config8(dev, MAGIC_CONTROL_REG, ~POWER_GATE_MAGIC);
enum magic_power_level magic_power = pci_read_config8(dev, MAGIC_STATUS_REG);
switch (magic_power) {
case NO_MAGIC:
printk(BIOS_DEBUG, "Magic is disabled\n");
/* Power off magic to save power */
pci_or_config8(dev, MAGIC_CONTROL_REG, POWER_GATE_MAGIC);
/* Intentional fallthrough */
case MINIMUM_MAGIC:
/* Ensure advanced magic is off */
pci_and_config8(dev, MAGIC_CONTROL_REG, ~ADVANCED_MAGIC_ENABLE);
break;
case STANDARD_MAGIC:
case MORE_MAGIC:
pci_write_config8(dev, MAGIC_POWER_SELECT_REG, magic_power);
udelay(400); /* Magic power changes need some time to complete */
/* Only enable advanced magic after power has been configured */
pci_or_config8(dev, MAGIC_CONTROL_REG, ADVANCED_MAGIC_ENABLE);
break;
default:
/* Cannot safely handle too much magic */
die("Requested magic power %u is unsafe\n", magic_power);
}
Yes, it might not make much sense, but this magic hardware is programmed using magic sequences.
--
To view, visit https://review.coreboot.org/c/coreboot/+/47054
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I268db8bc63aaf64d4a91c9a44ef5282154b20a53
Gerrit-Change-Number: 47054
Gerrit-PatchSet: 2
Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Comment-Date: Wed, 04 Nov 2020 13:39:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment