Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44579 )
Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
What is Heci3? Is it enabled by any devicetree? is it even useful with coreboot? If not, why not always disable it?
Heci3 is another ME interface (there are four, actually. FSP only has options for two (three?))
I guess I'll place my comments inline in the future so they get answered before merge... Again, is enabling Heci3 useful with coreboot? Doesn't it require additional configuration/software?
I agree with Nico that questions should be resolved before submitting the patch.
Anyway, I don't know what this device is used for but I think we should give the possibility to enable it. There might be users outside of this tree. Always disabling it makes me feel we withhold something from the user.
I consider that the limits of what a user can do is limited to that which won't result in a dirty image (anything past that threshold would be hacking/development, and I wouldn't expect most people to do that). Changing a setting on the devicetree will make the coreboot image dirty.
If you want the user to have control over this setting, it needs to be a Kconfig option. However, I firmly believe this should *NOT* be user-configurable. Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.