Change in coreboot[master]: soc/intel/common/block/cse: Minor clean up some late comments
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35546 )
Change subject: soc/intel/common/block/cse: Minor clean up some late comments ......................................................................
Patch Set 14:
(17 comments)
I started adding comments here and later noticed that some of the components that I commented on are also touched upon in follow up CLs. If you have already addressed any of the comments in the follow up CLs, please ACK/mark DONE for the comment here. Thanks!
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 78: GR_ORIGIN_BIOS_POST Is this a command id or group id? From the name and the comment, it is not really clear to me what this macro is.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 84: operation and working modes Actually, this should be CWS = Current Working State and COM = Current Operation Mode
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 85: HFS I think it would be better to add HFSTS1 or HFS1 to indicate that this is a field in HFSTS1 register. Same for the macros below.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 86: MODE I think it would be better to call this COM = Current Operation Mode. There are different things that can be referred to as mode.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 598: rst_type Are the different reset types supported by ME firmware for all platforms? For CML, I don't see all the different reset types in the latest BWG.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 606: heci_reset(); Why is heci_reset() done here?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 609: memset(&reply, 0, reply_size); What is the use of setting this to 0?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 650: padding Rather than padding, can we name this reserved just like it is in BWG?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 662: or temporary disable mode Is this correct? I don't see this in BWG.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 664: hfs1.fields.working_state Not for this change, but I think we should add helper fucntions for CWS and COP i.e. me_get_cws(hfs1) and me_get_cop(hfs1). Just makes it easier to read and write the struct fields.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 678: } Why is resp.status not checked here?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 679: return 1; HMRFPO enabling needs to be followed by a global reset. Why is that not done as part of this function?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 687: DISABLES DISABLED
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 698: padding reserved
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 711: This command can be sent only if CWS is normal. Shouldn't that be checked here?
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 143: nit: extra blank line not required.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 152: * nit: space after *
-
Furquan Shaikh (Code Review)