[coreboot-gerrit] Change in coreboot[master]: Consolidate reset API, add generic reset_prepare mechanism

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/19789 )

Change subject: Consolidate reset API, add generic reset_prepare mechanism
......................................................................


Patch Set 1:

(4 comments)

https://review.coreboot.org/#/c/19789/1/src/include/reset.h
File src/include/reset.h:

Line 7: __attribute__((noreturn)) void hard_reset(void);
> The thing is hard_reset() on x86 actually is a cold reset which drops power
Okay, I'll try to clarify the descriptions a little more. So the only difference between soft_reset and hard_reset is DRAM? Or is soft_reset a vague "some but not all board components will be reset"? Maybe we should come up with a platform-independent list of what should and shouldn't be reset for each level. For example, if we can say that all soft_resets should reset the TPM we could switch vboot to that and have a better chance of persisting RAM. (My Falco has little trouble persisting RAM across a hard_reset... I think dropping power even for a second or two often doesn't do much more that a slight bit of corruption.)

The other question is of course why we even need so many resets in the first place. We seem to hardly use most of them, and where we do it's often not quite clear why a particular type is chosen. Maybe less would be better there?


Line 23: 	GLOBAL_RESET,
> Global's actually harder than hard.
Oh... hmm... okay. I'd still like hard_reset() to be the general fallback because it's the most commonly implemented one right now (and the one that has a Kconfig to check for), but I can change the order here.


https://review.coreboot.org/#/c/19789/1/src/lib/reset.c
File src/lib/reset.c:

Line 41: 	dcache_clean_all();
> I don't know how this works if caching is disabled on x86 systems. It might
Do you mean disabled by CR0 or some more magical mechanism? For the former I'm pretty sure it just flushes the cache anyway with whatever was still in it... for example, my Intel manual says in in section 11.5.3 (Preventing Caching):

> To disable the L1, L2, and L3 caches after they have been enabled and have received cache fills, perform the following steps:
> 1. Enter the no-fill cache mode. (Set the CD flag in control register CR0 to 1 and the NW flag to 0.
> 2. Flush all caches using the WBINVD instruction.
> ...


Line 47: 	printk(BIOS_INFO, "%s() called!\n", __func__);
> We need these printks() everywhere?
I just thought a general "I'm resetting now" print would be useful to have? Might make it easier to understand if your code intentionally reset or had some weird power glitch, watchdog, etc. event.


-- 
To view, visit https://review.coreboot.org/19789
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b89ce4a923102f0748922496e1dd9bce8a610f
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes