Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35730 )
Change subject: sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35730/4/src/northbridge/intel/fsp_r... File src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c:
https://review.coreboot.org/c/coreboot/+/35730/4/src/northbridge/intel/fsp_r... PS4, Line 112: UpdData->PcdEnableLan);
This code ties PCI device BDF hard to the functionality it provides. Sure, it works with static BDF assigments, but would it not be better to have PCI device driver (in the context of detected device ID and class) to modify UpdData instead?
Are you suggesting different call back ops for determining things like this?How would you bind a driver to the semantics the FSP is expecting here?
Well I was thinking to delay the initialisation sequence from here to chip->enable_dev(), but realised now there is no per-device .enable_dev callback.
With alias names, this could work:
UpdData->PcdEnableSata2 = PCI_ALIAS(sata2)->enabled;
That would take the designated BDF out of the equation, and remove the loop here as well. Not sure if that makes any difference.
I would want to reduce the use of all _DEVFN_ as much as possible, but we cannot use symbol names for these case blocks, can we?
Yes, we definitely can do that. I fully intend on working with you and Nico on doing that.
This aliasing bothered me:
src/soc/intel/icelake/include/soc/pci_devs.h: #define PCH_DEV_LPC PCH_DEV_ESPI
AFAICS that was done to fulfill soc/intel/common naming conventions.
A related question; do we want to allow common code to include platform-specific <soc/pci_devs.h> headers and reference these xx_DEVFN_xx directly? Shouldn't the common code side declare some API instead to find the PCI devices they need?
Do you have ideas on the API?
Well.. simply get_lpc_dev() declared under common/ with implementations in platform would avoid eg. that direct PCH_DEV_LPC reference under common/?
I think we need to look at the current use-cases and figure out how/why <soc/pci_devs.h> are being used.
I know the SA_SOMETHING_DEV was used in the intel common code base with the assumption that it was consistent across soc naming. If we went to symbol based resolution that would work w/o including <soc/pci_devs.h>. Do we lose any information? We essentially have magic symbols now. How do we convey their magic/special-ness so that people aren't super confused?