Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 39: * to prevent a crash in FSP-M. Please don't add comments about what you do. AIUI, it was asked for a comment, because the details why it is done in this particular way were unclear (not why it is done at all).
So
!dev || !dev->enabled
this is to check if the device was unmentioned in the devicetree or explicitly turned `off`,
(dev &&
this is redundant and should be removed. By the evaluation rules of `||` the right-hand side is only evaluated if the left-hand side is false. If the left-hand side here (`!dev || !dev->enabled`) is false, it implies `dev` is non-NULL.
(pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff)
this checks if the device is actually there. If it isn't, the devicetree is inconsistent (says `on` for a device that doesn't even exist). IMHO, this is the part that needs to be commented. Why assume inconsistency? because we might not know at compile time if the CPU that is put into a socket supports it. <-- this I can only assume, because it isn't mentioned in the comment.
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0; Please merge this with the original check above. I know that involves passing `tconfig`, but that's just how things are with FSP...