Attention is currently required from: Felix Singer, Nico Huber, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60721 )
Change subject: soc/intel/common/cse: Add config to disable CSE at pre-boot ......................................................................
Patch Set 2:
(1 comment)
File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/60721/comment/ca3bbe1a_2509d9c1 PS2, Line 13: This config decides the state of CSE/Heci1 device at the end of boot.
I also wouldn't use "CSE" in the name, since for me CSE refers to the firmware, some internals of it or the hardware part and this is just about the communication interface. However, I would just use HECI1.
Sure
Also, I wouldn't invert the option. I think we should be more consistent regarding the direction (enable or disable) of options. If find it pretty hard and also annoying to use the Kconfig menu if some have "enable" and others "disable" in their name. From my perspective, people expect the "enable" terminology in user interfaces. So I would name it "Enable HECI1" and set it to enable by default.
I have little different opinion here, CSE/Heci1 is by default enable and it stays in that way hence a Kconfig saying ENABLE_HECI1 doesn't make much sense as its *already enabled* (isn't it). The reason we need a Kconfig to decide if we want to leave CSE/heci1 in that state of enabled or wish to put into Disable (aka function disable) or also known as Hidden mode. Hence, having either a config that says "DISABLE_HECI1" or "HIDE_HECI1"can be use on its positive side to ensure we wish to call "heci_disable()" function or not.
Any thoughts on this.
-
Subrata Banik (Code Review)