Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )
Change subject: soc/intel: Fix regression with hidden PCI devices ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 3:
(1 comment)
Patch Set 3:
Also we have pci_s_read_config() that takes BDF as argument, no need for device node pointer. I can use that on code like p2sb_hide() and p2sb_unhide():
Why not do it for all devices that are currently accessed using PCH_DEV_* i.e. just use _s_ variant to pass in PCH_DEVFN_* and get rid of all PCH_DEV_* completely.
IMO what p2sb_hide/unhide does is a specs violation, sending configuration cycles to a PCI device that does not respond to the PCI ID register query. Currently I am assuming the register with hide/unide bit is the only one hardware always decodes.
The device responds back with all 1s. Basically, the EDS says that when the hide bit gets set, this device will read 1s for any PCI config read. All other transactions are unaffected. I am not sure if it is a violation of spec to return back all 1s. That is a special value which basically indicates no device. But, I am not a PCI spec expert.
IIRC, Linux kernel follows a similar way of doing this i.e. accessing P2SB device.
I would rather make that one an explicit quirk, complain in the logs (maybe BIOS_DEBUG, not BIOS_ERR) of having to do so, and maybe even make it require a "select PCI_QUIRK_SPECS_VIOLATION" in the platform configs. I did not check PCI space of that device, I bet there would be bunch of interesting bits for firmware, maybe even OS. Like SERR/PERR perhaps.
What purpose would this "PCI_QUIRK_SPECS_VIOLATION" be used for?