coreboot-gerrit July 2021

coreboot-gerrit@coreboot.org

1 participants
5824 discussions

Change in coreboot[master]: acpi: Add function to simplify If (CondRefOf (..)) sequences
by Tim Wawrzynczak (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Cliff Huang, Furquan Shaikh, Tim Wawrzynczak, Michael Niewöhner, Karthik Ramasubramanian. Hello Lance Zhao, build bot (Jenkins), Cliff Huang, Furquan Shaikh, Angel Pons, Michael Niewöhner, Karthik Ramasubramanian, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56003 to look at the new patch set (#2). Change subject: acpi: Add function to simplify If (CondRefOf (..)) sequences ...................................................................... acpi: Add function to simplify If (CondRefOf (..)) sequences The new function is called acpigen_write_if_cond_refof(), and it must be paired with a following acpigen_write_if_end() call. Change-Id: I6e192a569f550ecb77ad264275d52f219eacaca1 Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> --- M src/acpi/acpigen.c M src/include/acpi/acpigen.h 2 files changed, 18 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/56003/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6e192a569f550ecb77ad264275d52f219eacaca1 Gerrit-Change-Number: 56003 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common: Add cpu_fill_code_cache() to test eNEM
by Arthur Heymans (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Patrick Rudolph, EricR Lai. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55653 ) Change subject: soc/intel/common: Add cpu_fill_code_cache() to test eNEM ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: How exactly does eNEM work? Do you set up a CONFIG_DCACHE_RAM_SIZE region that never gets evicted and all the rest can be evicted if the cache is full? Or is specifically RO cached memory that gets evicted? If not you could set up a second WB region (just like the CAR one) large enough (larger than the cache) to ensure eviction and write to it. The address needs to be carefully selected though to not be a real address range. I'm not sure what happens when cache gets evicted to address that nothing is decoding. Anyway if that works, you could simplify this code a lot. -- To view, visit https://review.coreboot.org/c/coreboot/+/55653 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb1e644f154b8f8d8da41ba8084e99827e7eff23 Gerrit-Change-Number: 55653 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 16:04:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload: Continue in ahci_cmdslot_exec() despite unset HBA_PxCMD_CR
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Nico Huber, Michał Żygowski, Marshall Dawson, Paul Menzel, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56062 ) Change subject: libpayload: Continue in ahci_cmdslot_exec() despite unset HBA_PxCMD_CR ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Those folks working with AMD devices, have you seen this on your systems? Or are you only using NVMe […] We don't use SATA. Only NVMe or eMMC. -- To view, visit https://review.coreboot.org/c/coreboot/+/56062 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b94cf4aa40dce587f52350ff4624946aeaafcc0 Gerrit-Change-Number: 56062 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:56:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86/mtrr: Add check_var_mtrr_range_enabled() helper function
by Arthur Heymans (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56151 ) Change subject: cpu/x86/mtrr: Add check_var_mtrr_range_enabled() helper function ...................................................................... Patch Set 1: (1 comment) File src/cpu/x86/mtrr/earlymtrr.c: https://review.coreboot.org/c/coreboot/+/56151/comment/5d32231a_ee39183a PS1, Line 168: bool check_var_mtrr_range_enabled(uintptr_t base, size_t size) > This function relies on the caller to align base and size of the address range you want to check. That's not a good API. > I'd prefer a function that can check if a certain arbitrary range is cached a certain way: UC/RW/RO/WC... > e.g.: checking if [0xff700000, 0xffffffff] is covered by RO caching type MTRR. > You also don't want to make assumptions on the MTRR default caching type: read it out first. It can also be the case that your input range spans multiple MTRRs. -- To view, visit https://review.coreboot.org/c/coreboot/+/56151 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0df698c1501ad678ec0d58e133d7e437b658f4ca Gerrit-Change-Number: 56151 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:55:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86/mtrr: Add check_var_mtrr_range_enabled() helper function
by Arthur Heymans (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56151 ) Change subject: cpu/x86/mtrr: Add check_var_mtrr_range_enabled() helper function ...................................................................... Patch Set 1: (2 comments) File src/cpu/x86/mtrr/earlymtrr.c: https://review.coreboot.org/c/coreboot/+/56151/comment/f2f455c7_b620e728 PS1, Line 136: static bool get_var_mtrr_range(int index, uintptr_t *base_address, size_t *length) : { : bool ret = false; : uint32_t address_bits; : uint64_t address_mask; : const uint32_t msr_reg = MTRR_PHYS_BASE(index); : uint64_t mask; : union { : uint64_t u64; : msr_t s; : } msr_a; : union { : uint64_t u64; : msr_t s; : } msr_m; : : address_bits = cpu_phys_address_size(); : address_mask = (1ULL << address_bits) - 1; : : msr_a.s = rdmsr(msr_reg); : msr_m.s = rdmsr(msr_reg + 1); : if (msr_m.u64 & MTRR_PHYS_MASK_VALID) { : *base_address = (msr_a.u64 & 0xfffffffffffff000ULL) : & address_mask; : : mask = (msr_m.u64 & 0xfffffffffffff000ULL) & address_mask; : *length = (~mask & address_mask) + 1; : ret = true; : } : return ret; : } maybe use struct region? https://review.coreboot.org/c/coreboot/+/56151/comment/f2a9b5f0_431473b8 PS1, Line 168: bool check_var_mtrr_range_enabled(uintptr_t base, size_t size) This function relies on the caller to align base and size of the address range you want to check. That's not a good API. I'd prefer a function that can check if a certain arbitrary range is cached a certain way: UC/RW/RO/WC... e.g.: checking if [0xff700000, 0xffffffff] is covered by RO caching type MTRR. You also don't want to make assumptions on the MTRR default caching type: read it out first. -- To view, visit https://review.coreboot.org/c/coreboot/+/56151 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0df698c1501ad678ec0d58e133d7e437b658f4ca Gerrit-Change-Number: 56151 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:54:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Switch to runtime generation of Intel Power Engine
by Tim Wawrzynczak (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Cliff Huang, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56006 ) Change subject: soc/intel/alderlake: Switch to runtime generation of Intel Power Engine ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/alderlake/pmc.c: https://review.coreboot.org/c/coreboot/+/56006/comment/9758fc6e_e57a142e PS1, Line 126: /* Add Intel Power Engine device */ > Please add #if CONFIG(CONFIG_SOC_INTEL_COMMON_BLOCK_ACPI_PEP) here so that we can still build even w […] `generate_acpi_power_engine` will not be compiled in unless `SOC_INTEL_COMMON_BLOCK_ACPI_PEP` is selected (which this CL already does), which would cause a compile error, I don't understand your concern? -- To view, visit https://review.coreboot.org/c/coreboot/+/56006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I617bc3d1c3cf4ac6b6cbbd790dcf62e731024834 Gerrit-Change-Number: 56006 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:48:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/adlrvp: Fix DDR5 Boot issue
by Meera Ravindranath (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Bora Guvendik, Furquan Shaikh, Selma Bensaid, Maulik V Vaghela, Paul Menzel, Subrata Banik, Angel Pons. Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50996 ) Change subject: mb/adlrvp: Fix DDR5 Boot issue ...................................................................... Patch Set 13: (1 comment) File src/mainboard/intel/adlrvp/romstage_fsp_params.c: https://review.coreboot.org/c/coreboot/+/50996/comment/d8e9def1_ede4c405 PS13, Line 60: ADL_P_DDR5_2 > @meera, can you please address this review comment ? […] Sure Subrata. Will do this. @Furquan: Will push the new changes in a day. Facing some issues in getting the DDR5 board at our end to verify the changes. -- To view, visit https://review.coreboot.org/c/coreboot/+/50996 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9998bfcd12b81c11fcc9f791da2a27d3c788e48a Gerrit-Change-Number: 50996 Gerrit-PatchSet: 13 Gerrit-Owner: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:45:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add cpu_fill_code_cache() to test eNEM
by build bot (Jenkins) (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Patrick Rudolph, EricR Lai. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55653 ) Change subject: soc/intel/common: Add cpu_fill_code_cache() to test eNEM ...................................................................... Patch Set 3: (5 comments) File src/soc/intel/common/block/cpu/cpulib.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123564): https://review.coreboot.org/c/coreboot/+/55653/comment/972db12e_47a93399 PS3, Line 462: char animation[4] = {'|','/','-','\\'}; space required after that ',' (ctx:VxV) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123564): https://review.coreboot.org/c/coreboot/+/55653/comment/8f463f72_5087665c PS3, Line 462: char animation[4] = {'|','/','-','\\'}; space required after that ',' (ctx:VxV) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123564): https://review.coreboot.org/c/coreboot/+/55653/comment/aad4dc8d_27b5bf0b PS3, Line 462: char animation[4] = {'|','/','-','\\'}; space required after that ',' (ctx:VxV) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123564): https://review.coreboot.org/c/coreboot/+/55653/comment/47377c48_9feffb7d PS3, Line 493: for (int i = 0; addr < (4ULL * GiB - CONFIG_C_ENV_BOOTBLOCK_SIZE); i ++) { space prohibited before that '++' (ctx:WxB) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123564): https://review.coreboot.org/c/coreboot/+/55653/comment/b89407e4_0ea21179 PS3, Line 500: } void function return statements are not generally useful -- To view, visit https://review.coreboot.org/c/coreboot/+/55653 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb1e644f154b8f8d8da41ba8084e99827e7eff23 Gerrit-Change-Number: 55653 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:43:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add cpu_fill_code_cache() to test eNEM
by Arthur Heymans (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Patrick Rudolph, EricR Lai. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55653 ) Change subject: soc/intel/common: Add cpu_fill_code_cache() to test eNEM ...................................................................... Patch Set 3: Code-Review+1 (2 comments) Patchset: PS3: Looks good File src/soc/intel/common/block/cpu/cpulib.c: https://review.coreboot.org/c/coreboot/+/55653/comment/61f57640_6288ed56 PS3, Line 493: CONFIG_C_ENV_BOOTBLOCK_SIZE This is not used anymore. use _bootblock. -- To view, visit https://review.coreboot.org/c/coreboot/+/55653 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb1e644f154b8f8d8da41ba8084e99827e7eff23 Gerrit-Change-Number: 55653 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:40:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add cpu_fill_code_cache() to test eNEM
by Subrata Banik (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Rizwan Qureshi, Arthur Heymans, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55653 ) Change subject: soc/intel/common: Add cpu_fill_code_cache() to test eNEM ...................................................................... Patch Set 3: (11 comments) File src/cpu/intel/common/Kconfig: https://review.coreboot.org/c/coreboot/+/55653/comment/110e1c30_323269cd PS2, Line 46: Cache > lowercase. Ack https://review.coreboot.org/c/coreboot/+/55653/comment/133e45ad_57f4c538 PS2, Line 48: Also, wish to run the stress test on the CAR region by filling : up the cache by a memory copy. > I'd split that functionality off? It sounds useful to print cache information regardless of CAR type […] Ack File src/cpu/intel/common/common_init.c: https://review.coreboot.org/c/coreboot/+/55653/comment/e444c9fd_67812364 PS2, Line 347: fill_code_cache > Return if not ENV_CACHE_AS_RAM? Ack https://review.coreboot.org/c/coreboot/+/55653/comment/2535e55e_6fdf653f PS2, Line 357: fast_spi_get_bios_region > Maybe also check if the RO flash is also covered by an MTRR? Otherwise this whole operation is a noo […] Ack https://review.coreboot.org/c/coreboot/+/55653/comment/ee90cb66_985d0911 PS2, Line 359: : * Don't need to run this stress test if available cache size is higher than : * BIOS region size > explain why, as it's not obvious to the reader I think. […] Ack https://review.coreboot.org/c/coreboot/+/55653/comment/29b90c93_4ee82918 PS2, Line 363: get_cache_info() >= size > You have to account for the cache used for cache as ram too. […] Ack https://review.coreboot.org/c/coreboot/+/55653/comment/37a0310c_4831e880 PS2, Line 369: 4ULL * GiB - 64 * KiB > > > I guess you are trying to avoid the bootblock here, while you're calling this inside the bootblo […] Ack https://review.coreboot.org/c/coreboot/+/55653/comment/8e165264_a4f5378f PS2, Line 370: 1 * KiB > sizeof(buf) Ack https://review.coreboot.org/c/coreboot/+/55653/comment/4a9a0515_e44cd1e9 PS2, Line 371: "TEST : %lx %x\n", addr, buf[0] > Is printing this even useful? It does not really tell you that CAR passed or failed a stress test. Its taking some time for memcpy hence added a printf to make sure folks understood tat something is going on and if that stops so consider as hang. Now i have modified this a small animated circle, hopefully that is more meaning here than redundant text. https://review.coreboot.org/c/coreboot/+/55653/comment/d5687760_8e96763d PS2, Line 371: BIOS_INFO > BIOS_SPEW. Ack https://review.coreboot.org/c/coreboot/+/55653/comment/c6f26f02_78ee5bff PS2, Line 372: 1 * KiB > sizeof(buf) Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/55653 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb1e644f154b8f8d8da41ba8084e99827e7eff23 Gerrit-Change-Number: 55653 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 15:07:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2021