Change in coreboot[master]: soc/intel/elkhartlake: Add Kconfig option to disable reset on TCO expire
Attention is currently required from: Nico Huber, Paul Menzel, Lean Sheng Tan, Patrick Rudolph. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61177 )
Change subject: soc/intel/elkhartlake: Add Kconfig option to disable reset on TCO expire ......................................................................
Patch Set 1:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/61177/comment/94bf2d10_0f32b48d PS1, Line 7: expire
expiration
Done
https://review.coreboot.org/c/coreboot/+/61177/comment/baf9b7e2_d4719397 PS1, Line 11: In this
“In these” or mayeb “For these”
Done
https://review.coreboot.org/c/coreboot/+/61177/comment/963fef9d_de8d3673 PS1, Line 11: In this : applications the TCO expire event generates an interrupt and software : takes care.
This is how it always works, isn't it? Can you please tell us what […]
Yes, the most meaningful state of the TCO is that it causes a reset on the second expiration, I agree. Unfortunately, on Broadwell-DE and on Apollo Lake Intel does set this NO_REBOOT-Bit in the FSP, of which I never was aware. Now, based on this, our application guys have used the watchdog (so here it is where the TCO gets enabled again) and one of the (bad) features there is that they let the watchdog expire to perform a soft reset of some parts. Now, with Elkhart Lake, this expiration triggers, other than before on Broadwell-DE and Apollo Lake, a real platform reset which breaks other assumptions in terms of data consistency in the system. I know, all quite weird.
The reason I decided to set it here is due to the fact that this "NO_REBOOT"-bit has different locations on Broadwell-DE, Apollo Lake and Elkhart Lake. The application is not aware of this and just uses a unified TCO library to control it. Moving this over to the application will require different code paths there.
In addition, the BIOS Writers Guide mentions that the system BIOS should set this NR-Bit and, once the application TCO driver takes over, it could reset the bit if needed. Again, all quite weird.
If settings this bit in bootblock on soc level is an issue, I could move it over to the mainboard. It just felt more right on soc level as the TCO is addressed there already and everything is around.
https://review.coreboot.org/c/coreboot/+/61177/comment/c306126c_53018b0e PS1, Line 13: There is a bit in the TCO1_CNT register on Elkhart Lake to : prevent this reset on expire (called NO_REBOOT).
Can you please reference the datasheet name and revision?
Done
https://review.coreboot.org/c/coreboot/+/61177/comment/a2228f8f_3a5bbb67 PS1, Line 14: expire
expiration
Done
https://review.coreboot.org/c/coreboot/+/61177/comment/0198dbee_c4adb37e PS1, Line 21: NO_BOOT
NO_REBOOT
Done
Patchset:
PS1:
I don't know if it changed for newer platforms, but on older ones […]
It is actually the application that enables the TCO again.
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/61177/comment/9349455f_79bd8703 PS1, Line 223: This can be needed for a given application depending on the TCO policy.
Maybe: […]
Done
File src/soc/intel/elkhartlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/61177/comment/fd64a3c0_fe63c502 PS1, Line 38: tco_write_reg(TCO1_CNT, reg);
Add a debug message?
Done
https://review.coreboot.org/c/coreboot/+/61177/comment/a5efd93a_1621a86d PS1, Line 39: }
Maybe add a message, what TCO1_CNT is set to? (Or can this be easily queried from the operating syst […]
I guess a debug message wouold be better instead of the value here. If needed, it can be read back on OS by reading the register (IO-mapped TCO control register).
-
Werner Zeh (Code Review)