Subrata Banik has posted comments on this change by Jamie Ryu. ( https://review.coreboot.org/c/coreboot/+/83784?usp=email )
Change subject: soc/intel/common: Add SoC QDF read function ......................................................................
Patch Set 6:
(8 comments)
File src/soc/intel/common/block/include/intelblocks/pmc_ipc.h:
https://review.coreboot.org/c/coreboot/+/83784/comment/f7037d10_24e2d97c?usp... : PS6, Line 45: /* IPC command for accessing SoC registers */ : #define PMC_IPC_CMD_SOC_REG_ACC 0xAA : #define PMC_IPC_CMD_SUBCMD_SOC_REG_RD 0x00 : #if CONFIG(SOC_QDF_DYNAMIC_READ_PMC) : #define PMC_IPC_CMD_REGID_SOC_QDF 0x03 : #endif /* SOC_QDF_DYNAMIC_READ_PMC */ please don't use any CPP to guard the macro. Also, if there is no other users of these macros, then it can go into pmclib.c file itself
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/83784/comment/6a5d3f4a_b3a67261?usp... : PS6, Line 271: #if CONFIG(SOC_QDF_DYNAMIC_READ_PMC) u don't need this guard
File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/83784/comment/6126d7be_ba59d4a4?usp... : PS6, Line 89: depends on SOC_INTEL_COMMON_BLOCK_PMC : depends on PMC_IPC_ACPI_INTERFACE ```suggestion depends on SOC_INTEL_COMMON_BLOCK_PMC && PMC_IPC_ACPI_INTERFACE ```
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/83784/comment/1f103f9f_ba36a012?usp... : PS6, Line 888: This function reads SoC QDF information using PMC interface. ```suggestion * This function reads SoC QDF information using PMC interface if SOC_QDF_DYNAMIC_READ_PMC config is enabled otherwise return -1. The caller function should check the return value before processing the QDF. ```
https://review.coreboot.org/c/coreboot/+/83784/comment/8c175278_73f0fd0f?usp... : PS6, Line 891: int pmc_read_qdf(void) looks like we are not returning anything, rather we are just dumping?
hence, the function should just say `pmc_dump_silicon_qdf_info()`. Ideally you could call this from report platform if possible.
Also, if this function is not expected to return any value then mark as void.
https://review.coreboot.org/c/coreboot/+/83784/comment/b7ee66c9_682f2fc8?usp... : PS6, Line 891: int pmc_read_qdf(void) : { ```suggestion int pmc_read_qdf(void) { if (CONFIG(!SOC_QDF_DYNAMIC_READ_PMC)) return -1;
... ...
```
https://review.coreboot.org/c/coreboot/+/83784/comment/8f647988_8bd5580f?usp... : PS6, Line 908: 0 error as in `-1`
https://review.coreboot.org/c/coreboot/+/83784/comment/94c9a245_526442b8?usp... : PS6, Line 918: } else { : printk(BIOS_INFO, "SoC QDF not available\n"); : } do we really care about this print?