Attention is currently required from: Francois Toguo Fotso, Martin Roth, Duncan Laurie, Tim Wawrzynczak, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49943 )
Change subject: soc/intel/tigerlake: Add CrashLog implementation for intel TGL. ......................................................................
Patch Set 6: Code-Review+1
(10 comments)
Patchset:
PS6: Overall looks pretty good to me.
Francois, just in general, how much of this is common between TGL & ADL ?
File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/49943/comment/1505b7c1_ffc50d21 PS6, Line 266: config ACPI_BERT : bool "Enables ACPI BERT" : default y : help : Enables ACPI Boot Error Record Table generation. : : config SOC_INTEL_CRASHLOG : def_bool n : help : Enables CrashLog. : I think this better captures the dependencies:
``` config SOC_INTEL_CRASHLOG def_bool n depends on ACPI_BERT help Enables CPU & PMC CrashLog support ```
which requires a mainboard to select ACPI_BERT and SOC_INTEL_CRASHLOG together (IOW the crashlog is useless without a means to report the error).
File src/soc/intel/tigerlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/49943/comment/4a3efc24_500b482b PS6, Line 367: if (crashlog_get_total_data_size() > bert_storage_remaining()) { : printk(BIOS_ERR, "Error: Crashlog entry would exceed " : "available region\n"); : return; : } Is this expected to happen? If it does, would it still be useful to dump the first 64KB of this?
https://review.coreboot.org/c/coreboot/+/49943/comment/41478fe2_7f76ff04 PS6, Line 415: nit: extra blank line
File src/soc/intel/tigerlake/crashlog_lib.c:
https://review.coreboot.org/c/coreboot/+/49943/comment/c11dfc66_a92e6667 PS6, Line 47: tale table
https://review.coreboot.org/c/coreboot/+/49943/comment/4b8fece6_b0ae14c3 PS6, Line 57: nit: extra empty line
https://review.coreboot.org/c/coreboot/+/49943/comment/2c03bed3_1649d114 PS6, Line 150: cmd_reg = pmc_make_ipc_cmd(PMC_IPC_CMD_CRASHLOG, nit: blank line after `int r;`
https://review.coreboot.org/c/coreboot/+/49943/comment/35db02dd_c32c8e96 PS6, Line 452: nit: extra space
File src/soc/intel/tigerlake/include/soc/crashlog_lib.h:
https://review.coreboot.org/c/coreboot/+/49943/comment/73055279_e40177ca PS6, Line 3: /* : * This file is part of the coreboot project. : * : * Copyright 2021 Intel Corporation : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; either version 2 of the License, or : * (at your option) any later version. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ We don't include this header anymore, the SPDX is enough 😊
https://review.coreboot.org/c/coreboot/+/49943/comment/68d5b93b_8b44ab4f PS6, Line 136: : int pmc_cl_get_descriptor_table(u32 desc_table_addr); : bool pmc_cl_discovery(void); : int pmc_cl_disable(void); : int pmc_cl_clear(void); : int pmc_cl_re_arm_after_reset(void); : int pmc_cl_en_gen_on_all_reboot(void); : void pmc_cl_get_sram_data(void); : : bool discover_crashlog(void); : int crashlog_get_total_data_size(void); : bool cl_copy_data_from_sram(u32 src_bar, : u32 offset, : u32 size, : u32 *dest_addr, : u32 buffer_index, : bool pmc_sram); : : int cpu_cl_poll_mailbox_ready(u32 cl_mailbox_addr); : int cpu_cl_mailbox_cmd(u8 cmd, u8 param); : bool cpu_cl_get_capability(tel_crashlog_devsc_cap_t *cl_devsc_cap); : u32 cpu_cl_get_bar_addr(tel_crashlog_devsc_cap_t *cl_devsc_cap); : int cpu_cl_clear_data(void); : void cpu_cl_get_header(u32 base_addr, : cpu_crashlog_header_t *cl_header_buff); : bool cpu_cl_disable(void); : bool cpu_cl_get_discovery_table(void); : bool cpu_cl_discovery(void); : : void cpu_cl_get_telemtry_sram_data(void); : void collect_pmc_and_cpu_crashlog_from_srams(void); : : int crashlog_get_cpu_record_size(void); : int crashlog_get_pmc_record_size(void); : bool crashlog_fill_cpu_records(void *cl_record); : bool crashlog_fill_pmc_records(void *cl_record); : Do these all need to be publicly exported? I think only a few of these are used in tigerlake/acpi.c, e.g.: crashlog_fill_pmc_records() crashlog_fill_cpu_records() crashlog_get_cpu_record_size() crashlog_get_pmc_record_size()
are all that I see used outside of this module; could the rest of the functions be static to crashlog_lib.c and not exported here?