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 * -- To view, visit https://review.coreboot.org/c/coreboot/+/35546 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If7ea6043d7b5473d0c16e83d7b2d4b620c125652 Gerrit-Change-Number: 35546 Gerrit-PatchSet: 14 Gerrit-Owner: Rizwan Qureshi <rizwan.qureshi@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Rizwan Qureshi <riz.pro@gmail.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi@intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla@intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla@intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Sun, 05 Jan 2020 18:40:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
participants (1)
-
Furquan Shaikh (Code Review)