[coreboot-gerrit] Change in coreboot[master]: soc/amd/stoney: clean up and update reset.c

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/22439 )

Change subject: soc/amd/stoney: clean up and update reset.c
......................................................................


Patch Set 1:

(6 comments)

https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/include/soc/reset.h
File src/soc/amd/stoneyridge/include/soc/reset.h:

https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/include/soc/reset.h@16
PS1, Line 16: 
I would probably add these to northbridge.h and southbridge.h instead of creating a new file.


https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/include/soc/reset.h@21
PS1, Line 21: HTIC_BIOSR_DETECT
This is actually a 3-bit field.  Besides b[5], also b[10:9].  We should probably clear all three for completeness.  BTW, I did a quick scan through the AGESA code, and from what I can tell, it ignores b[10:9] unless b[5] is set.


https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/reset.c
File src/soc/amd/stoneyridge/reset.c:

https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/reset.c@17
PS1, Line 17: // Use simple device model for this file even in ramstage
            : #define __SIMPLE_DEVICE__
This could probably go away.  Not sure why it was here, but changing to SOC_HT_DEV below likely obsoletes it.


https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/reset.c@27
PS1, Line 27:  * Clearing bit 5 of HT_INIT_CONTROL signals that this reset came from the
Something doesn't add up.  According to the BKDG, this is 0 on a cold reset.  It suggests the BIOS can set it to 1 before initiating a BIOS-generated reset event.

The code below clears the bit though, i.e. coreboot basically says he doesn't want to track that he generated the reset.

So maybe the comment is backwards.  (I can't remember whether I saw a discussion of AGESA using this status; I know PMx10 was brought up.)


https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/reset.c@30
PS1, Line 30: set_bios_reset
Should we be calling this clear_bios_reset() instead?


https://review.coreboot.org/#/c/22439/1/src/soc/amd/stoneyridge/reset.c@53
PS1, Line 53:  
extra space



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0cc4c04b53b7fec38d45e962ff1292d8c717269c
Gerrit-Change-Number: 22439
Gerrit-PatchSet: 1
Gerrit-Owner: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Mon, 13 Nov 2017 17:27:24 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20171113/fba17341/attachment.html>