coreboot-gerrit May 2020

coreboot-gerrit@coreboot.org

1 participants
2588 discussions

Change in coreboot[master]: src/device/dram/ddr{2,3}: Rewrire printram(Capacity)
by HAOUAS Elyes (Code Review) 03 May '20

03 May '20

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40427 ) Change subject: src/device/dram/ddr{2,3}: Rewrire printram(Capacity) ...................................................................... src/device/dram/ddr{2,3}: Rewrire printram(Capacity) GCC will complain when using '-Wduplicated-branches' option as printram will expaind to nothing. Rewrite it to make gcc happy when '-Wduplicated-branches' is used. Change-Id: Ice6245db0a209ce970a8d62cba8b7e0cda2c9968 Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M src/device/dram/ddr2.c M src/device/dram/ddr3.c 2 files changed, 7 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/40427/1 diff --git a/src/device/dram/ddr2.c b/src/device/dram/ddr2.c index 5319806..bf3f815 100644 --- a/src/device/dram/ddr2.c +++ b/src/device/dram/ddr2.c @@ -522,10 +522,10 @@ dimm->ranksize_mb = 128 * reg8; /* Module density */ dimm->size_mb = dimm->ranksize_mb * dimm->ranks; - if (dimm->size_mb < 1024) - printram(" Capacity : %u MB\n", dimm->size_mb); - else - printram(" Capacity : %u GB\n", dimm->size_mb >> 10); + + printram(" Capacity : ", (dimm->size_mb < 1024) ? + "%u MB\n", dimm->size_mb : + "%u GB\n", dimm->size_mb >> 10); /* SDRAM Maximum Cycle Time (tCKmax) */ if (spd_decode_bcd_time(&dimm->tCK, spd[43]) != CB_SUCCESS) { diff --git a/src/device/dram/ddr3.c b/src/device/dram/ddr3.c index bef3c78..de72bb3 100644 --- a/src/device/dram/ddr3.c +++ b/src/device/dram/ddr3.c @@ -162,11 +162,9 @@ printram(" Invalid module capacity\n"); ret = SPD_STATUS_INVALID_FIELD; } - if (capacity_shift < 0x02) { - printram(" Capacity : %u Mb\n", 256 << capacity_shift); - } else { - printram(" Capacity : %u Gb\n", 1 << (capacity_shift - 2)); - } + printram(" Capacity : ", (capacity_shift < 0x02) ? + "%u Mb\n", 256 << capacity_shift : + "%u Gb\n", 1 << (capacity_shift - 2)); reg8 = spd[5]; /* Row address bits */ -- To view, visit https://review.coreboot.org/c/coreboot/+/40427 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ice6245db0a209ce970a8d62cba8b7e0cda2c9968 Gerrit-Change-Number: 40427 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: soc/intel/{apl, cnl, skl}: Add sanity check of SA_PCIEX_LENGTH_MIB
by Subrata Banik (Code Review) 03 May '20

03 May '20

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40337 ) Change subject: soc/intel/{apl, cnl, skl}: Add sanity check of SA_PCIEX_LENGTH_MIB ...................................................................... soc/intel/{apl, cnl, skl}: Add sanity check of SA_PCIEX_LENGTH_MIB Not all IA SoC has provision for higher PCIEXBAR length hence added required check inside SoC to avoid wrong length programming. TEST=Select PCIEX_LENGTH_4096MB config in Cannonlake Kconfig file and build causes compilation error. Change-Id: I0dd67868df373c6ab5c724a21371bcc206968036 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/skylake/systemagent.c 3 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/40337/1 diff --git a/src/soc/intel/apollolake/systemagent.c b/src/soc/intel/apollolake/systemagent.c index f564e8f..27686ee 100644 --- a/src/soc/intel/apollolake/systemagent.c +++ b/src/soc/intel/apollolake/systemagent.c @@ -23,6 +23,10 @@ #include <soc/iomap.h> #include <soc/systemagent.h> +#if (CONFIG_SA_PCIEX_LENGTH_MIB > 0x100) /* Max length is 256MB */ +#error "Invalid SA_PCIEX_LENGTH_MIB selection!" +#endif + /* * SoC implementation * diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index 54b16e2..9f56890 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -12,6 +12,10 @@ #include <soc/systemagent.h> #include "chip.h" +#if (CONFIG_SA_PCIEX_LENGTH_MIB > 0x100) /* Max length is 256MB */ +#error "Invalid SA_PCIEX_LENGTH_MIB selection!" +#endif + /* * SoC implementation * diff --git a/src/soc/intel/skylake/systemagent.c b/src/soc/intel/skylake/systemagent.c index 10d79d3..22a764a 100644 --- a/src/soc/intel/skylake/systemagent.c +++ b/src/soc/intel/skylake/systemagent.c @@ -14,6 +14,10 @@ #include <soc/systemagent.h> #include "chip.h" +#if (CONFIG_SA_PCIEX_LENGTH_MIB > 0x100) /* Max length is 256MB */ +#error "Invalid SA_PCIEX_LENGTH_MIB selection!" +#endif + bool soc_is_vtd_capable(void) { struct device *const root_dev = pcidev_path_on_root(SA_DEVFN_ROOT); -- To view, visit https://review.coreboot.org/c/coreboot/+/40337 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0dd67868df373c6ab5c724a21371bcc206968036 Gerrit-Change-Number: 40337 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: soc/intel/common/basecode: Implement CSE update flow
by Furquan Shaikh (Code Review) 03 May '20

03 May '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35403 ) Change subject: soc/intel/common/basecode: Implement CSE update flow ...................................................................... Patch Set 79: (43 comments) https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@14 PS79, Line 14: The CSME size is 3.9MB. For what platform? https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@17 PS79, Line 17: CSE binary. What is the difference here between ME update binary and CSE binary? https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@18 PS79, Line 18: CSME I understand that you are referring to the same thing with CSME, CSE and ME. But it would be good to be consistent. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@19 PS79, Line 19: which are ~1.5 MB for what platform? I don't think this number is true for every Intel platform. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@33 PS79, Line 33: - Send global reset command to reset only the CSME I thought we agreed on not doing a CSME only reset. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@34 PS79, Line 34: Wait for CSME to enter SOFT_TEMP_DISABLE operation mode : (indicated by HFSTS1 register bit 19:16) BIOS/CPU wouldn't really wait. It gets reset as well. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@44 PS79, Line 44: Verified TEST= https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… File Documentation/soc/intel/cse_fw_update/cse_fw_update.md: https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 12: are nit: that are https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 26: RW Partition update is supported by CSE FW. I am not really sure what this statement means. You already mentioned above that CSE FW layout has two partitions. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 38: Start/End nit: 's' and 'e' should be lowercase https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 58: components nit: '.' at the end. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 63: next boot partition just say "partition" https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 78: ME I mentioned this on the commit message as well. ME, CSME and CSE are used interchangeably. Can you make it consistent so that a single terminology is used? https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 94: accommodate larger CSE binary This is confusing. I posted a comment about this on commit message as well. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 95: respective RW What does this mean? https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 103: Send GLOBAL_RESET HECI command to reset only the CSE. : - Wait for CSE to enter Soft Temporary Disable Mode. This needs update. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 121: The RW FW checks if the system is is in recovery mode, if it's in then : skips CSE FW uupdate flow. This statement doesn't make sense. In recovery mode, RW FW is not selected at all. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 24: "Name of CSE Region in FMAP" I don't think this has to be a user-visible option. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 25: default "SI_ME" Can you please add a help text indicating what this option is? https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 28: "CBFS entry name" Same here. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/custom_bp.c: PS79: This file name should be updated to cse_custom_sku or cse_lite_sku https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 29: 4 sizeof(uint32_t) https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 31: /* : * If CSE RW signature is valid, : * then last update was successful : */ : #define CSE_RW_SIGN_VALID 1 : : /* : * If CSE RW signature is not valid, : * then last update was not successful : */ : #define CSE_RW_SIGN_INVALID 0 These are unused. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 176: static void cse_trigger_recovery(uint8_t rec_sub_code) : { : if (CONFIG(VBOOT)) { : struct vb2_context *ctx; : ctx = vboot_get_context(); : vb2api_fail(ctx, VB2_RECOVERY_CSME_FAILURE, rec_sub_code); : vboot_save_data(ctx); : vboot_reboot(); : } else : do_global_reset(); : } : Looking at the entire CL, I think addition of just this function can be put as a separate CL and can be added before this change. We can push this in sooner and in smaller parts. That way we have the CSME Lite SKU booting setup complete. And this CL would be the next step i.e. performing an actual update. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 225: return cse_get_bp_entry_version(RW, cse_bp_info); Just place the contents of cse_get_bp_entry_version here? https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 375: cse_set_and_boot_from_next_bp Can you please add comments to the functions? In this case, it is important to mention that this function results in a reset and so it will never return control. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 381: die("cse_bp: Failed to reset system\n"); We should trigger recovery in this case as well. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 404: uint32_t size_t https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 405: uint32_t size_t https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 415: locate %s in CBFS\n" This is not locating in CBFS. It is actually finding the region using FMAP and returning a region device pointer to it. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 447: It calculates CSE RW region's start offset and size This function does more than just calculating the offset and size. In fact, offset and size are already present in cse_bp_info, right? https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 458: region_device_offset(&cse_region_rdev)); This can fit on the previous line. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 462: : if (boot_device_rw_subregion(&cse_rw_region, target_rdev) < 0) : return false; There is no need to do the math for getting the actual offset. You can use rdev_chain(). https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 486: cse_get_cbfs_rw_version(cse_rw_n_ver_data) Why create a function? It is not really doing much. BTW, you can simply pass source_rdev here and it can do a rdev_readat() instead of doing a rdev_mmap_full() right away. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 550: return !cse_check_version_mismatch(cse_bp_info, cse_cbfs_rw_n_ver); : } I think the contents of cse_check_version_mismatch() can simply be placed into this function. No use of a function which just calls another function. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 571: void *cse_cbfs_rw = cse_get_cbfs_rw_map(cse_cbfs_rw_n_ver); : size_t cse_cbfs_rw_sz = cse_cbfs_rw_n_ver_sz - CSE_RW_VERSION_SZ; : You don't really have to do the math here. As mentioned above you can simply pass the source_rdev and then use rdev_chain here to clip the region device as per what you want. Then you don't really have to do any of this math. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 592: if (!cse_hmrfpo_enable()) : return false; : : return true; Just "return cse_hmrfo_enable();" https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 608: Layout of SOC_INTEL_CSE_RW_CBFS = [<CSE RW Version>(16 bytes) + <CSE RW Firmware>] I want to revisit this as mentioned on the bug. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 609: cse_cbfs_rw_n_ver cse_cbfs_rw_n_ver is confusing. cse_cbfs_rw should be fine? Also, you don't really need to map the whole thing right away. You can pass in source_rdev structure to the functions and the function that cares only about the version can do a rdev_readat(). Then the function which does the write can do a rdev_chain followed by rdev_mmap and use that to write the RW partition. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 615: if (!cse_is_rw_bp_signature_valid(&target_rdev) || : !cse_is_rw_version_latest(cse_bp_info, cse_cbfs_rw_n_ver)) { This whole if block can be arranged such that: if (cse_is_update_required()) ret = cse_trigger_update(); rdev_munmap(...); Then you don't need to sprinkle the munmap calls in multiple places. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 622: cse_trigger_recovery(CSME_FAILURE_COMMUNICATION_ERROR); It looks like the triggering of recovery is currently happening from multiple places and so it is not very clear what functions really return and which don't. Can we plumb back all failure cases to the top such that the triggering of recovery happens by the top-level function cse_fw_sync? You can change the return type to status code i.e. 0 = success non-zero = recovery reason. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 631: rdev_munmap(source_rdev, cse_cbfs_rw_n_ver); Why is rdev_munmap() called conditionally? https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 660: Otherwise, nit: CSE is set to boot from RW in both cases right? i.e. whether CSME RW binary is present in CBFS or not. -- To view, visit https://review.coreboot.org/c/coreboot/+/35403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I12f6bba3324069d65edabaccd234006b0840e700 Gerrit-Change-Number: 35403 Gerrit-PatchSet: 79 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Andrey Petrov <anpetrov(a)fb.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sun, 03 May 2020 06:16:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Furquan Shaikh (Code Review) 03 May '20

03 May '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 80: Code-Review+2 (10 comments) Just some minor nits to update comments. Rest looks okay. https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG@32 PS80, Line 32: coreboot ensure CSE to boot from RO(BP1) if system is in Recovery mode. This is no longer true. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 21: SKU Custom Still pending. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/custom_bp.c: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 23: nit: No space before /* https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 29: It's a valid bootable partition. nit: Does this fit on the previous line? https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 44: present based on layout, but it is not valid. nit: I think part of this comment can go to previous line. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 50: present per initial image layout. nit: This can go on previous line. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 58: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ nit: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 331: do_global_reset Just making a note: This is not the right way to handle the failure case. It is just being done until it is replaced with proper triggering of recovery in follow-up CLs. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/include/intelblocks/cse.h: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 231: triggers recovery mode if CSE fails to jump to RW Just a note: This is not yet implemented in this CL. It will be done in follow-up CLs. https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 232: RW partition nit: whatever is the currently selected partition. -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 80 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sun, 03 May 2020 05:08:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/picasso: Select CHROMEOS_RAMOOPS_DYNAMIC
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40949 ) Change subject: soc/amd/picasso: Select CHROMEOS_RAMOOPS_DYNAMIC ...................................................................... soc/amd/picasso: Select CHROMEOS_RAMOOPS_DYNAMIC For boards that select CHROMEOS, select CHROMEOS_RAMOOPS_DYNAMIC by default. BUG=b:155345589 Change-Id: Id215f3a2c8d1e9e713a628283af9586a1f117ef4 Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/soc/amd/picasso/Kconfig 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/40949/1 diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 5996cc6..7de918e 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -194,6 +194,9 @@ Specify the amount of DRAM reserved for gathering the data used to generate the ACPI table. +config CHROMEOS + select CHROMEOS_RAMOOPS_DYNAMIC + config RO_REGION_ONLY string depends on CHROMEOS -- To view, visit https://review.coreboot.org/c/coreboot/+/40949 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id215f3a2c8d1e9e713a628283af9586a1f117ef4 Gerrit-Change-Number: 40949 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

4 4

Change in coreboot[master]: acpi: Move ACPI table support out of arch/x86 (5/5)
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40940 ) Change subject: acpi: Move ACPI table support out of arch/x86 (5/5) ...................................................................... acpi: Move ACPI table support out of arch/x86 (5/5) This change moves all ACPI table support in coreboot currently living under arch/x86 into common code to make it architecture independent. ACPI table generation is not really tied to any architecture and hence it makes sense to move this to its own directory. In order to make it easier to review, this change is being split into multiple CLs. This is change 5/5 which moves the addition of ACPI table related files from arch/x86/Makefile.inc to acpi/Makefile.inc. BUG=b:155428745 Change-Id: I8143fd37357aeb0561516450adddc6714d539ada Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/acpi/Makefile.inc M src/arch/x86/Makefile.inc 2 files changed, 8 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/40940/1 diff --git a/src/acpi/Makefile.inc b/src/acpi/Makefile.inc index 068c592..09ff9f1 100644 --- a/src/acpi/Makefile.inc +++ b/src/acpi/Makefile.inc @@ -12,4 +12,12 @@ ramstage-y += sata.c +ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/acpi_tables.c),) +ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/acpi_tables.c +endif +$(eval $(call asl_template,dsdt)) +ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/fadt.c),) +ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/fadt.c +endif + endif # CONFIG_GENERATE_ACPI_TABLES diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index aa1f5fe..c29d5ed 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -281,15 +281,6 @@ ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/reset.c),) ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/reset.c endif -ifeq ($(CONFIG_HAVE_ACPI_TABLES),y) -ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/acpi_tables.c),) -ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/acpi_tables.c -endif -$(eval $(call asl_template,dsdt)) -ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/fadt.c),) -ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/fadt.c -endif -endif # CONFIG_GENERATE_ACPI_TABLES ramstage-libs ?= -- To view, visit https://review.coreboot.org/c/coreboot/+/40940 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8143fd37357aeb0561516450adddc6714d539ada Gerrit-Change-Number: 40940 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

7 8

Change in coreboot[master]: acpi: Update sata files to be more aligned with rest of acpi files
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40933 ) Change subject: acpi: Update sata files to be more aligned with rest of acpi files ...................................................................... acpi: Update sata files to be more aligned with rest of acpi files This change renames sata.c to acpi_sata.c and moves sata.h to include/acpi/acpi_sata.h to align them with the rest of the files under acpi/. BUG=b:155428745 Change-Id: I3f97e5c12535a331d7347c0ecad00b07b5f13f37 Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/acpi/Makefile.inc R src/acpi/acpi_sata.c R src/include/acpi/acpi_sata.h M src/southbridge/intel/bd82x6x/sata.c M src/southbridge/intel/ibexpeak/sata.c 5 files changed, 4 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/40933/1 diff --git a/src/acpi/Makefile.inc b/src/acpi/Makefile.inc index 0010fea..d311278 100644 --- a/src/acpi/Makefile.inc +++ b/src/acpi/Makefile.inc @@ -6,12 +6,11 @@ ramstage-y += acpi.c ramstage-y += acpi_device.c ramstage-y += acpi_pld.c +ramstage-y += acpi_sata.c ramstage-y += acpigen.c ramstage-y += acpigen_dsm.c ramstage-y += acpigen_ps2_keybd.c -ramstage-y += sata.c - ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/acpi_tables.c),) ramstage-srcs += src/mainboard/$(MAINBOARDDIR)/acpi_tables.c endif diff --git a/src/acpi/sata.c b/src/acpi/acpi_sata.c similarity index 97% rename from src/acpi/sata.c rename to src/acpi/acpi_sata.c index f2f53e6..110742e 100644 --- a/src/acpi/sata.c +++ b/src/acpi/acpi_sata.c @@ -1,10 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ -#include "sata.h" - #include <acpi/acpi.h> #include <acpi/acpigen.h> +#include <acpi/acpi_sata.h> /* e.g. * generate_sata_ssdt_ports("\_SB.PCI0.SATA", 0x3); diff --git a/src/acpi/sata.h b/src/include/acpi/acpi_sata.h similarity index 100% rename from src/acpi/sata.h rename to src/include/acpi/acpi_sata.h diff --git a/src/southbridge/intel/bd82x6x/sata.c b/src/southbridge/intel/bd82x6x/sata.c index e65fd6e..57eb7e7 100644 --- a/src/southbridge/intel/bd82x6x/sata.c +++ b/src/southbridge/intel/bd82x6x/sata.c @@ -8,7 +8,7 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <option.h> -#include <acpi/sata.h> +#include <acpi/acpi_sata.h> #include <types.h> #include "chip.h" diff --git a/src/southbridge/intel/ibexpeak/sata.c b/src/southbridge/intel/ibexpeak/sata.c index 11ac078..df13989 100644 --- a/src/southbridge/intel/ibexpeak/sata.c +++ b/src/southbridge/intel/ibexpeak/sata.c @@ -8,7 +8,7 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <option.h> -#include <acpi/sata.h> +#include <acpi/acpi_sata.h> #include <types.h> #include "chip.h" -- To view, visit https://review.coreboot.org/c/coreboot/+/40933 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3f97e5c12535a331d7347c0ecad00b07b5f13f37 Gerrit-Change-Number: 40933 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

4 9

Change in coreboot[master]: acpi: Remove acpi_ from filenames
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40941 ) Change subject: acpi: Remove acpi_ from filenames ...................................................................... acpi: Remove acpi_ from filenames This change drops acpi_ prefix from filenames under src/acpi/. BUG=b:155428745 Change-Id: Iadda2b848701367e51f4f74706154f7e36a87df6 Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/acpi/Makefile.inc R src/acpi/device.c R src/acpi/pld.c 3 files changed, 2 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40941/1 diff --git a/src/acpi/Makefile.inc b/src/acpi/Makefile.inc index 09ff9f1..09b9906 100644 --- a/src/acpi/Makefile.inc +++ b/src/acpi/Makefile.inc @@ -4,12 +4,11 @@ ifeq ($(CONFIG_HAVE_ACPI_TABLES),y) ramstage-y += acpi.c -ramstage-y += acpi_device.c -ramstage-y += acpi_pld.c ramstage-y += acpigen.c ramstage-y += acpigen_dsm.c ramstage-y += acpigen_ps2_keybd.c - +ramstage-y += device.c +ramstage-y += pld.c ramstage-y += sata.c ifneq ($(wildcard src/mainboard/$(MAINBOARDDIR)/acpi_tables.c),) diff --git a/src/acpi/acpi_device.c b/src/acpi/device.c similarity index 100% rename from src/acpi/acpi_device.c rename to src/acpi/device.c diff --git a/src/acpi/acpi_pld.c b/src/acpi/pld.c similarity index 100% rename from src/acpi/acpi_pld.c rename to src/acpi/pld.c -- To view, visit https://review.coreboot.org/c/coreboot/+/40941 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iadda2b848701367e51f4f74706154f7e36a87df6 Gerrit-Change-Number: 40941 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 5

Change in coreboot[master]: acpi: Reorganize ACPI configs
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40932 ) Change subject: acpi: Reorganize ACPI configs ...................................................................... acpi: Reorganize ACPI configs This change moves HAVE_ACPI_TABLES to acpi/Kconfig and ACPI_BERT to arch/x86/Kconfig to keep them in the same directory where the corresponding code lives. BUG=b:155428745 Change-Id: I289565f38e46bd106ff89685aaf8f57e53d9827a Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/Kconfig M src/acpi/Kconfig M src/arch/x86/Kconfig 3 files changed, 12 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/40932/1 diff --git a/src/Kconfig b/src/Kconfig index cf4df18..6540499 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -624,12 +624,6 @@ help Enable Unified Memory Architecture for graphics. -config HAVE_ACPI_TABLES - bool - help - This variable specifies whether a given board has ACPI table support. - It is usually set in mainboard/*/Kconfig. - config HAVE_MP_TABLE bool help @@ -656,12 +650,6 @@ help Build support for NHLT (non HD Audio) ACPI table generation. -config ACPI_BERT - bool - depends on HAVE_ACPI_TABLES - help - Build an ACPI Boot Error Record Table. - #These Options are here to avoid "undefined" warnings. #The actual selection and help texts are in the following menu. diff --git a/src/acpi/Kconfig b/src/acpi/Kconfig index 8c8bb87..52925fe 100644 --- a/src/acpi/Kconfig +++ b/src/acpi/Kconfig @@ -12,3 +12,9 @@ help Provide common definitions for AMD hardware PM1_CNT register sleep values. + +config HAVE_ACPI_TABLES + bool + help + This variable specifies whether a given board has ACPI table support. + It is usually set in mainboard/*/Kconfig. diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 11733bd..a7d9b10 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -246,6 +246,12 @@ Note that it is the responsibility of the payload to reset the normal boot bit to 1 after each successful boot. +config ACPI_BERT + bool + depends on HAVE_ACPI_TABLES + help + Build an ACPI Boot Error Record Table. + config ACPI_NO_PCAT_8259 bool help -- To view, visit https://review.coreboot.org/c/coreboot/+/40932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I289565f38e46bd106ff89685aaf8f57e53d9827a Gerrit-Change-Number: 40932 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

4 7

Change in coreboot[master]: acpi: Make header #ifdefs consistent
by Furquan Shaikh (Code Review) 02 May '20

02 May '20

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40931 ) Change subject: acpi: Make header #ifdefs consistent ...................................................................... acpi: Make header #ifdefs consistent Now that all ACPI header files are moved to src/include/acpi, this change updates the #ifdef to __ACPI_${FILENAME}__. BUG=b:155428745 Change-Id: Id24ee35bac318278871a26f98be7092604de01c0 Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/include/acpi/acpi.h M src/include/acpi/acpi_device.h M src/include/acpi/acpi_ivrs.h M src/include/acpi/acpi_pld.h M src/include/acpi/acpigen.h M src/include/acpi/acpigen_dsm.h M src/include/acpi/acpigen_ps2_keybd.h 7 files changed, 16 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/40931/1 diff --git a/src/include/acpi/acpi.h b/src/include/acpi/acpi.h index 86b3932..6bcbb7f 100644 --- a/src/include/acpi/acpi.h +++ b/src/include/acpi/acpi.h @@ -5,8 +5,8 @@ * coreboot ACPI support - headers and defines. */ -#ifndef __ASM_ACPI_H -#define __ASM_ACPI_H +#ifndef __ACPI_ACPI_H__ +#define __ACPI_ACPI_H__ /* * The type and enable fields are common in ACPI, but the diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index ed64cd8..1f55423 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ -#ifndef __ACPI_DEVICE_H -#define __ACPI_DEVICE_H +#ifndef __ACPI_ACPI_DEVICE_H__ +#define __ACPI_ACPI_DEVICE_H__ #include <device/i2c.h> #include <stdint.h> diff --git a/src/include/acpi/acpi_ivrs.h b/src/include/acpi/acpi_ivrs.h index 83abfb6..1c0987c 100644 --- a/src/include/acpi/acpi_ivrs.h +++ b/src/include/acpi/acpi_ivrs.h @@ -9,8 +9,8 @@ * I/O Virtualization Reporting Structure (IVRS) */ -#ifndef __ARCH_ACPI_IVRS_H -#define __ARCH_ACPI_IVRS_H +#ifndef __ACPI_ACPI_IVRS_H__ +#define __ACPI_ACPI_IVRS_H__ /* I/O Virtualization Reporting Structure (IVRS) */ #define IVHD_BLOCK_TYPE_LEGACY__FIXED 0x10 diff --git a/src/include/acpi/acpi_pld.h b/src/include/acpi/acpi_pld.h index c518077..bbc1924 100644 --- a/src/include/acpi/acpi_pld.h +++ b/src/include/acpi/acpi_pld.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ -#ifndef __ACPI_PLD_H -#define __ACPI_PLD_H +#ifndef __ACPI_ACPI_PLD_H__ +#define __ACPI_ACPI_PLD_H__ #include <acpi/acpi.h> #include <stdint.h> diff --git a/src/include/acpi/acpigen.h b/src/include/acpi/acpigen.h index 8d66e2b..9776d00 100644 --- a/src/include/acpi/acpigen.h +++ b/src/include/acpi/acpigen.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ -#ifndef LIBACPI_H -#define LIBACPI_H +#ifndef __ACPI_ACPIGEN_H__ +#define __ACPI_ACPIGEN_H__ #include <stdint.h> #include <acpi/acpi.h> diff --git a/src/include/acpi/acpigen_dsm.h b/src/include/acpi/acpigen_dsm.h index c51c12b..28b8974 100644 --- a/src/include/acpi/acpigen_dsm.h +++ b/src/include/acpi/acpigen_dsm.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ -#ifndef __ARCH_ACPIGEN_DSM_H__ -#define __ARCH_ACPIGEN_DSM_H__ +#ifndef __ACPI_ACPIGEN_DSM_H__ +#define __ACPI_ACPIGEN_DSM_H__ #include <stdint.h> @@ -12,4 +12,4 @@ void acpigen_write_dsm_i2c_hid(struct dsm_i2c_hid_config *config); -#endif /* __ARCH_ACPIGEN_DSM_H__ */ +#endif /* __ACPI_ACPIGEN_DSM_H__ */ diff --git a/src/include/acpi/acpigen_ps2_keybd.h b/src/include/acpi/acpigen_ps2_keybd.h index c0228bc..aeeacae 100644 --- a/src/include/acpi/acpigen_ps2_keybd.h +++ b/src/include/acpi/acpigen_ps2_keybd.h @@ -4,8 +4,8 @@ * SPDX-License-Identifier: GPL-2.0-or-later */ -#ifndef __ACPIGEN_PS2_KEYBD_H__ -#define __ACPIGEN_PS2_KEYBD_H__ +#ifndef __ACPI_ACPIGEN_PS2_KEYBD_H__ +#define __ACPI_ACPIGEN_PS2_KEYBD_H__ #include <types.h> @@ -38,4 +38,4 @@ bool can_send_function_keys, bool has_numeric_keypad, bool has_scrnlock_key); -#endif /* __ACPIGEN_PS2_KEYBD_H__ */ +#endif /* __ACPI_ACPIGEN_PS2_KEYBD_H__ */ -- To view, visit https://review.coreboot.org/c/coreboot/+/40931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id24ee35bac318278871a26f98be7092604de01c0 Gerrit-Change-Number: 40931 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

5 11

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2020