John Zhao 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:
(5 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: […]
Ack
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: […]
Ack
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 125: reg16 = pci_read_config16(dev, PCI_COMMAND); : : /* Check if BME bit is enabled before */ : if ((reg16 & PCI_COMMAND_MASTER) == PCI_COMMAND_MASTER) { : reg16 &= ~PCI_COMMAND_MASTER; : pci_write_config16(dev, PCI_COMMAND, reg16); : }
in terms of performance - no need to do read compare write - just write 0 to the bit.
Ack
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: […]
END_OF_FIMWARE is only one of boot stages that could be handled by platform_fsp_notify_status. Note: AFTER_PCI_ENUM(0x20), READY_TO_BOOT(0x40), END_OF_FIRMWARE(0xF0). "if (phase != END_OF_FRIMWARE) return" would not be considered.
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 […]
L142 checks for device associated with SA_DEVFN_ROOT. Other SA_DEVFN_TBTx devices are checked in the clear_tbt_pcie_rt_bme function.