Attention is currently required from: Felix Singer, Nico Huber, Tim Wawrzynczak, 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 HECI1 at pre-boot ......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/60721/comment/d2a23d28_de136d00 PS3, Line 9: mainboard users
Sorry for the delay.
As I understand the latest IA SoCs, the CSE/HECI1 is default enable and its recommended to keep enable even after booting to OS as well. The only problem occurs when a kernel doesn't have corresponding HECI driver, it might yellow bang for windows or binding generic PCI driver for linux. In ChromeOS, we had seen an issue where in absence of specific HECI driver in kernel, it causes S0ix entry issue. This brings the requirement where platform integrator/owner can decide to hide/disable the HECI1 device prior booting to OS. So, hopefully this might helps to clarify its not the HW specific requirement, rather its platform specific where per board can decide if they wish to disable/hide HECI1.
IMO OS issues are best fixed in the OS. Especially when a vendor has both firmware and the OS under control, like it's the case for ChromeOS, why add odd workarounds to the firmware instead of fixing the OS? Also, please when there are workarounds in the firmware, document! A little comment that a setting is actually a workaround and for what can help a lot :) coreboot is supposed to be about the hardware. Any requirement that is not about hardware should be explained.
Anyway, that devices ship with ChromeOS doesn't mean everybody is using ChromeOS on them and devices that ship without ChromeOS can be used with it. So a `default y if CHROMEOS` would fit your story much better than per-board selects,
I agree with making `default y if CHROMEOS` as you said, we shouldn't assume coreboot == chromeOS alone. will fix it.
I guess. Please keep in mind that all of coreboot including the mainboard ports is supposed to be for everyone, not just the original vendor.
If it is hardware specific and has to be decided per board, it should be a devicetree setting, IMHO.
OTOH, there are two cases where a Kconfig setting makes sense, IMHO.
- if it isn't hardware specific, then it should have a prompt, let
the user decide.
true, thats what we tries to implement here, where a prompt would let users to decide.
- if there is more than C code that depends on an
option (e.g. different build-system behavior), but should it have a prompt then?
A Kconfig with a user-visible prompt but forced state doesn't make any sense to me.
So, you can educate me on this please. Now that we have added a Kconfig with prompt that ask user if would like to `disable HECI1`. This Kconfig is `default n`, meaning unless override in corresponding mainboard, it would act as `keep heci1 enable`. Now, we have two options
A. Having a select in Kconfig B. Override the default from 'n' to 'y' in local mainboard.
I have opt for #A above but looks like you are suggesting #B.
Yes, that's right. #A would defeat the prompt. One can't change a setting that has a `select`. So it rarely makes sense to have a prompt and use `select`. In this case, it would be very inconsistent: users of non-chromebooks could choose and users of chromebooks would be shown a prompt but they couldn't choose?
Thanks, I will keep a note on this in future.