Attention is currently required from: Nico Huber, Michał Żygowski, Martin L Roth, Michał Kopeć, Angel Pons, Arthur Heymans.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68944 )
Change subject: soc/intel/common/block/oc_wdt: Add OC watchdog common block ......................................................................
Patch Set 14:
(15 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68944/comment/1270ba83_6243b163 PS14, Line 18: driveless driverless
File src/soc/intel/common/block/include/intelblocks/oc_wdt.h:
https://review.coreboot.org/c/coreboot/+/68944/comment/085ea05b_2c7da7b4 PS14, Line 8: void wdt_reload_and_start(unsigned int timeout); : void wdt_disable(void); : bool is_wdt_enabled(void); : unsigned int wdt_get_current_timeout(void); To avoid confusions with the TCO watchdog, would it make sense to add '_oc' to the function names?
File src/soc/intel/common/block/oc_wdt/Kconfig:
https://review.coreboot.org/c/coreboot/+/68944/comment/a6bd1a8e_9f2e1740 PS14, Line 24: timeout the timeout
https://review.coreboot.org/c/coreboot/+/68944/comment/b4e2eb8b_418d3503 PS14, Line 24: reload maybe 'preload'?
https://review.coreboot.org/c/coreboot/+/68944/comment/2054b0e4_f1d567a1 PS14, Line 27: platform the platform
https://review.coreboot.org/c/coreboot/+/68944/comment/2d6ddf1d_c52cce94 PS14, Line 30: Feed Maybe better 'Reload'?
File src/soc/intel/common/block/oc_wdt/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/68944/comment/52ae59be_90063928 PS14, Line 1: bootblock-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : verstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : postcar-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : smm-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c Since you want this module being available in all stages, you could use:
all-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c
instead to improve readability.
File src/soc/intel/common/block/oc_wdt/oc_wdt.c:
https://review.coreboot.org/c/coreboot/+/68944/comment/89c9c456_58bf36fa PS14, Line 10: (ACPI_BASE_ADDRESS + 0x54) nit: At least my documents for Apollo Lake does not mention neither the OC watchdog nor this register at all. But I can see it on Elkhart Lake.
https://review.coreboot.org/c/coreboot/+/68944/comment/732142d6_2ae8806a PS14, Line 25: uint32_t readback; I would move this right at the start of the function. And maybe give it better name, e.g. oc_wdt_ctrl or the like?
https://review.coreboot.org/c/coreboot/+/68944/comment/b2493565_547672fc PS14, Line 27: timeout - 1) > PCH_OC_WDT_CTL_TOV_MASK || timeout == 0 Who about: ``` if ((timeout == 0) || (timeout > (PCH_OC_WDT_CTL_TOV_MASK + 1))) {``` This will avoid the bad '-1' case for timeout values of 0.
https://review.coreboot.org/c/coreboot/+/68944/comment/55cce624_33a7e02c PS14, Line 29: Watchdog Here again: to avoid confusions witht he old watchdog (TCO), how about a 'OC' prefix?
https://review.coreboot.org/c/coreboot/+/68944/comment/1d385829_289c60d1 PS14, Line 40: & PCH_OC_WDT_CTL_TOV_MASK nit: Not really necessary here since you have made sure already that the range fits into 10 bits at the start of this function.
https://review.coreboot.org/c/coreboot/+/68944/comment/3d150a86_29466e05 PS14, Line 48: readback nit: Same register name argument here.
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/68944/comment/3bf0c386_1bedd0a5 PS14, Line 217: if (CONFIG(SOC_INTEL_COMMON_OC_WDT_ENABLE)) : wdt_disable(); This reads strange. If OC_WDT is enabled then disable it? Is this your real intention here? If yes, an explaining comment would be helpful.
https://review.coreboot.org/c/coreboot/+/68944/comment/f5b6a7dc_44756459 PS14, Line 488: if (CONFIG(SOC_INTEL_COMMON_OC_WDT_RELOAD_IN_PERIODIC_SMI)) : wdt_reload_and_start(CONFIG_SOC_INTEL_COMMON_OC_WDT_TIMEOUT_SECONDS); It seems like the oc_wdt is only enabled when SOC_INTEL_COMMON_OC_WDT_RELOAD_IN_PERIODIC_SMI is selected. What about the described possibility to let the driver/SW do the reload? Where is it started in this case?