Prashant Malani has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 123: if (dev) { nit: Kindly use a guard clause ; it will save one level of indentation:
if (!dev) return;
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 124: if (pci_read_config16(dev, PCI_DEVICE_ID) == device_id) { nit: Same here; a guard clause will save another level of indentation:
if (!dev) return;
if (pci_read_config(dev, PCI_DEVICE_ID) != device_id) return;
reg16 = pci_read_config16(...);
....
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 138: if (phase == END_OF_FIRMWARE) { nit: Guard clause here please:
if (phase != END_OF_FIRMWARE) return;
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 149: dev = pcidev_path_on_root(SA_DEVFN_TBT0); Does |dev} need to be checked for an error here? It seems like it's being done on L142, but I'm not sure what the semantics are for this function.