Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... PS17, Line 189: same as CNL
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 188: if (!is_heci_enable()) return;
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 189: if (!me_read_config32(PCI_ME_HFSTS1, &hfsts1.raw)) : return; don't need any change here
hfsts1.raw = me_read_config32(PCI_ME_HFSTS1);
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 249: same as above. just check if (!is_heci_enable()) return; at line 232 and then don't need to make any change
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 505: add helper function to check HECI is enable/visible before asking for read status
bool is_heci_enable(const struct device *dev) { if (!cse_dev || !cse_dev->enabled) { printk(BIOS_WARNING, "HECI: No CSE device\n"); return false; }
if (pci_read_config16(PCH_DEV_CSE, PCI_VENDOR_ID) == 0xFFFF) { printk(BIOS_WARNING, "HECI: CSE device is hidden\n"); return false; } return true; }
let all caller check is_heci_enable() returns true before calling me_read_config32() in that case we can avoid multiple time device presence check which looks very redundant IMHO
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 506: int uint32_t me_read_config32(int offset) { return pci_read_config32(PCH_DEV_CSE, offset); }
so your caller doesn't required much changes
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c... PS17, Line 241: if (!me_read_config32(PCI_ME_HFSTS1, &hfs.data)) same as CNL