[M] Change in coreboot[master]: soc/intel/common/block/oc_wdt: Add OC watchdog common block

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans, Benjamin Doron, Martin L Roth, Maximilian Brune, Michał Kopeć, Michał Żygowski, Werner Zeh.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68944?usp=email )

Change subject: soc/intel/common/block/oc_wdt: Add OC watchdog common block
......................................................................

Patch Set 17: Code-Review+1

(6 comments)

File src/soc/intel/common/block/oc_wdt/Kconfig:

https://review.coreboot.org/c/coreboot/+/68944/comment/232952da_03a0bcb6 :
PS16, Line 7: config SOC_INTEL_COMMON_OC_WDT_ENABLE
> The main reason the OC WDT has an enable switch is because we don't know what payload an OS the boar […]

I agree with Michał. It's also common to have one Kconfig to express SoC support
and one for the user's choice.

https://review.coreboot.org/c/coreboot/+/68944/comment/b61cbd1c_f094751a :
PS16, Line 21: 	default 90
> True, DDR5 takes much longer to train. […]

LGTM. Maximilian, what do you think?

File src/soc/intel/common/block/oc_wdt/Kconfig:

https://review.coreboot.org/c/coreboot/+/68944/comment/f27abfef_a26f54b2 :
PS17, Line 41: 
Just wondering, should there be an option to disable the WDT at the end of
coreboot when everything seems fine? Then one could use it for the memory
training, but doesn't need special OS support or the periodic SMI slowing
things down.

File src/soc/intel/common/block/oc_wdt/oc_wdt.c:

https://review.coreboot.org/c/coreboot/+/68944/comment/d323d8a7_15c5b0e5 :
PS17, Line 36: PCH_OC_WDT_CTL_FORCE_ALL
I don't fully understand the description of this bit. Do you have
any details about what it does? experienced a difference with it
being set/unset?

https://review.coreboot.org/c/coreboot/+/68944/comment/d1b8ce22_8683b126 :
PS17, Line 62: unsigned int oc_wdt_get_current_timeout(void)
             : {
             : 	return (inl(PCH_OC_WDT_CTL) & PCH_OC_WDT_CTL_TOV_MASK) + 1;
             : }
Was this tested? I couldn't find a hint in the register description that
this would reflect the current value.

File src/soc/intel/common/block/smm/smm.c:

https://review.coreboot.org/c/coreboot/+/68944/comment/bc168ebe_0ec651e8 :
PS15, Line 45: Also we do not allow timeouts lower than 60s
> Added it to the comment.

Acknowledged