Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35730 )
Change subject: sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC ......................................................................
sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC
Change-Id: I6ead2bc5e41a86c9aeef730f5664a30406414c8c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c M src/southbridge/intel/fsp_rangeley/pci_devs.h 2 files changed, 27 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/35730/1
diff --git a/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c b/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c index 43e71f4..a9d2ba7 100644 --- a/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c +++ b/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c @@ -52,7 +52,7 @@ DEVTREE_CONST config_t *config; printk(BIOS_DEBUG, "Configure Default UPD Data\n");
- dev = pcidev_path_on_root(SOC_DEV_FUNC); + dev = pcidev_path_on_root(XXX_DEVFN_SOC); config = dev->chip_info;
/* Set SPD addresses */ @@ -103,30 +103,30 @@ continue;
switch (dev->path.pci.devfn) { - case GBE1_DEV_FUNC: - case GBE2_DEV_FUNC: - case GBE3_DEV_FUNC: - case GBE4_DEV_FUNC: + case XXX_DEVFN_GBE1: + case XXX_DEVFN_GBE2: + case XXX_DEVFN_GBE3: + case XXX_DEVFN_GBE4: UpdData->PcdEnableLan |= dev->enabled; printk(BIOS_DEBUG, "PcdEnableLan %d\n", UpdData->PcdEnableLan); break; - case SATA2_DEV_FUNC: + case XXX_DEVFN_SATA2: UpdData->PcdEnableSata2 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableSata2 %d\n", UpdData->PcdEnableSata2); break; - case SATA3_DEV_FUNC: + case XXX_DEVFN_SATA3: UpdData->PcdEnableSata3 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableSata3 %d\n", UpdData->PcdEnableSata3); break; - case IQAT_DEV_FUNC: + case XXX_DEVFN_IQAT: UpdData->PcdEnableIQAT |= dev->enabled; printk(BIOS_DEBUG, "PcdEnableIQAT %d\n", UpdData->PcdEnableIQAT); break; - case USB2_DEV_FUNC: + case XXX_DEVFN_USB2: UpdData->PcdEnableUsb20 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableUsb20 %d\n", UpdData->PcdEnableUsb20); diff --git a/src/southbridge/intel/fsp_rangeley/pci_devs.h b/src/southbridge/intel/fsp_rangeley/pci_devs.h index 89f3c5c..69edae6 100644 --- a/src/southbridge/intel/fsp_rangeley/pci_devs.h +++ b/src/southbridge/intel/fsp_rangeley/pci_devs.h @@ -26,72 +26,72 @@ /* Host Bridge */ #define SOC_DEV 0x0 #define SOC_FUNC 0 -# define SOC_DEV_FUNC PCI_DEVFN(SOC_DEV,SOC_FUNC) +# define XXX_DEVFN_SOC PCI_DEVFN(SOC_DEV,SOC_FUNC)
/* PCIE Port 1 */ #define PCIE_PORT1_DEV 0x1 #define PCIE_PORT1_FUNC 0 -# define PCIE_PORT1_DEV_FUNC PCI_DEVFN(PCIE_PORT1_DEV,PCIE_PORT1_FUNC) +# define XXX_DEVFN_PCIE_PORT1 PCI_DEVFN(PCIE_PORT1_DEV,PCIE_PORT1_FUNC)
/* PCIE Port 2 */ #define PCIE_PORT2_DEV 0x2 #define PCIE_PORT2_FUNC 0 -# define PCIE_PORT2_DEV_FUNC PCI_DEVFN(PCIE_PORT2_DEV,PCIE_PORT2_FUNC) +# define XXX_DEVFN_PCIE_PORT2 PCI_DEVFN(PCIE_PORT2_DEV,PCIE_PORT2_FUNC)
/* PCIE Port 3 */ #define PCIE_PORT3_DEV 0x3 #define PCIE_PORT3_FUNC 0 -# define PCIE_PORT3_DEV_FUNC PCI_DEVFN(PCIE_PORT3_DEV,PCIE_PORT3_FUNC) +# define XXX_DEVFN_PCIE_PORT3 PCI_DEVFN(PCIE_PORT3_DEV,PCIE_PORT3_FUNC)
/* PCIE Port 4 */ #define PCIE_PORT4_DEV 0x4 #define PCIE_PORT4_FUNC 0 -# define PCIE_PORT4_DEV_FUNC PCI_DEVFN(PCIE_PORT4_DEV,PCIE_PORT4_FUNC) +# define XXX_DEVFN_PCIE_PORT4 PCI_DEVFN(PCIE_PORT4_DEV,PCIE_PORT4_FUNC)
/* Host Bridge, Fabric, and RAS Registers */ #define HOST_BRIDGE_DEV 0xe #define HOST_BRIDGE_FUNC 0 -# define HOST_BRIDGE_DEV_FUNC PCI_DEVFN(HOST_BRIDGE_DEV,HOST_BRIDGE_FUNC) +# define XXX_DEVFN_HOST_BRIDGE PCI_DEVFN(HOST_BRIDGE_DEV,HOST_BRIDGE_FUNC)
/* Root Complex Event Collector (RCEC) */ #define RCEC_DEV 0xf #define RCEC_FUNC 0 -# define RCEC_DEV_FUNC PCI_DEVFN(RCEC_DEV,RCEC_FUNC) +# define XXX_DEVFN_RCEC PCI_DEVFN(RCEC_DEV,RCEC_FUNC)
/* SMBus 2.0 1 */ #define SMBUS1_DEV 0x13 #define SMBUS1_FUNC 0 -# define SMBUS1_DEV_FUNC PCI_DEVFN(SMBUS1_DEV,SMBUS1_FUNC) +# define XXX_DEVFN_SMBUS1 PCI_DEVFN(SMBUS1_DEV,SMBUS1_FUNC)
/* Gigabit Ethernet (GbE) */ #define GBE_DEV 0x14 #define GBE1_DEV GBE_DEV #define GBE1_FUNC 0 -# define GBE1_DEV_FUNC PCI_DEVFN(GBE1_DEV,GBE1_FUNC) +# define XXX_DEVFN_GBE1 PCI_DEVFN(GBE1_DEV,GBE1_FUNC) #define GBE2_DEV GBE_DEV #define GBE2_FUNC 1 -# define GBE2_DEV_FUNC PCI_DEVFN(GBE2_DEV,GBE2_FUNC) +# define XXX_DEVFN_GBE2 PCI_DEVFN(GBE2_DEV,GBE2_FUNC) #define GBE3_DEV GBE_DEV #define GBE3_FUNC 2 -# define GBE3_DEV_FUNC PCI_DEVFN(GBE3_DEV,GBE3_FUNC) +# define XXX_DEVFN_GBE3 PCI_DEVFN(GBE3_DEV,GBE3_FUNC) #define GBE4_DEV GBE_DEV #define GBE4_FUNC 3 -# define GBE4_DEV_FUNC PCI_DEVFN(GBE4_DEV,GBE4_FUNC) +# define XXX_DEVFN_GBE4 PCI_DEVFN(GBE4_DEV,GBE4_FUNC)
/* USB 2.0 */ #define USB2_DEV 0x16 #define USB2_FUNC 0 -# define USB2_DEV_FUNC PCI_DEVFN(USB2_DEV,USB2_FUNC) +# define XXX_DEVFN_USB2 PCI_DEVFN(USB2_DEV,USB2_FUNC)
/* SATA Gen 2 */ #define SATA2_DEV 0x17 #define SATA2_FUNC 0 -# define SATA2_DEV_FUNC PCI_DEVFN(SATA2_DEV,SATA2_FUNC) +# define XXX_DEVFN_SATA2 PCI_DEVFN(SATA2_DEV,SATA2_FUNC)
/* SATA Gen 3 */ #define SATA3_DEV 0x18 #define SATA3_FUNC 0 -# define SATA3_DEV_FUNC PCI_DEVFN(SATA3_DEV,SATA3_FUNC) +# define XXX_DEVFN_SATA3 PCI_DEVFN(SATA3_DEV,SATA3_FUNC)
/* Platform Control Unit (PCU) */ #define PCU_DEV 0x1f @@ -99,18 +99,18 @@ /* Low Pin Count (LPC/ISA) */ #define LPC_DEV PCU_DEV #define LPC_FUNC 0 -# define LPC_DEV_FUNC PCI_DEVFN(LPC_DEV,LPC_FUNC) +# define XXX_DEVFN_LPC PCI_DEVFN(LPC_DEV,LPC_FUNC) # define LPC_BDF PCI_DEV(BUS0, LPC_DEV, LPC_FUNC)
/* SMBus 2.0 0 */ #define SMBUS0_DEV PCU_DEV #define SMBUS0_FUNC 3 -# define SMBUS0_DEV_FUNC PCI_DEVFN(SMBUS0_DEV,SMBUS0_FUNC) +# define XXX_DEVFN_SMBUS0 PCI_DEVFN(SMBUS0_DEV,SMBUS0_FUNC)
/* Intel QuickAssist Integrated Accelerator (IQIA) */ #define IQAT_DEV 0xb #define IQAT_FUNC 0 -# define IQAT_DEV_FUNC PCI_DEVFN(IQAT_DEV,IQAT_FUNC) +# define XXX_DEVFN_IQAT PCI_DEVFN(IQAT_DEV,IQAT_FUNC)
#define SOC_DEVID 0x1f08 #define PCIE_PORT1_DEVID 0x1f10
David Guckian 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 1:
Hi, I presume you have a reason for doing this but I'm confused about the purpose of making these changes. From a functional perspective this change is fine but why do this?
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 1:
Patch Set 1:
Hi, I presume you have a reason for doing this but I'm confused about the purpose of making these changes. From a functional perspective this change is fine but why do this?
Conventions and consitency with naming helps. We are currently evaluating ways to replace some/most/lot of pci_devfn_t with struct device *, and potentially auto-generating these _DEVFN_ from devicetree.cb where necessary.
The prefix XXX_ is arbitrary. Would SCH_ or PCH_ be somewhat correct?
Aaron Durbin 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 1:
I know you are discussing with Werner in another thread, but I think it'd be helpful to have the intention explicitly noted as to the purpose of the rename. I'm personally not sure it's necessary just yet. But maybe I haven't see the full patch stack yet that provides that clarity.
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins), Furquan Shaikh, David Guckian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35730
to look at the new patch set (#4).
Change subject: sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC ......................................................................
sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC
Intel adopted xx_DEVFN_xx naming for macros expanding to PCI_DEVFN() starting with apollolake. The ones named xx_DEV_FUNC are being renamed, or dropped, if they were generally not used at all for a platform.
Change-Id: I6ead2bc5e41a86c9aeef730f5664a30406414c8c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c M src/southbridge/intel/fsp_rangeley/pci_devs.h 2 files changed, 27 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/35730/4
Aaron Durbin 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//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG@11 PS4, Line 11: renamed Can you explain why you explicitly went with the prefix XXX as opposed to SOC in the implementation?
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//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG@11 PS4, Line 11: renamed
Can you explain why you explicitly went with the prefix XXX as opposed to SOC in the implementation?
I made the following comment somewhere in baytrail:
The prefix XXX_ is arbitrary. Would SCH_ or PCH_ be somewhat correct?
Later intels use SA_ and PCH_ prefixes.
AMD does this:
#if !defined(__SIMPLE_DEVICE__ #include <device/device.h> #define _SOC_DEV(slot, func) pcidev_on_root(slot, func) #else #define _SOC_DEV(slot, func) PCI_DEV(0, slot, func) #endif
#define GNB_DEV 0x0 #define GNB_FUNC 0 #define GNB_DEVID 0x1576 #define GNB_DEVFN PCI_DEVFN(GNB_DEV, GNB_FUNC) #define SOC_GNB_DEV _SOC_DEV(GNB_DEV, GNB_FUNC)
So.. prefix SOC_ there is conversion to either struct device or PCI_DEV(0,d,f).
Aaron Durbin 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: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG@11 PS4, Line 11: renamed
I made the following comment somewhere in baytrail: […]
Ah ok. The SA (system agent) is a proxy for north cluster and pch a proxy for south cluster. Intel hasn't done a monolithic die since baytrail/braswell iirc.
What are you thoughts about being more descriptive? Let it go? Or at least name rangely or something? It feels odd to me use XXX proper.
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)
Sure, the prefix XXX_ should change before merge.
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?
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?
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?
Aaron Durbin 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?
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.
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?
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?
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?
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins), Nico Huber, Furquan Shaikh, David Guckian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35730
to look at the new patch set (#5).
Change subject: sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC ......................................................................
sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC
Intel adopted xx_DEVFN_xx naming for macros expanding to PCI_DEVFN() starting with apollolake. The ones named xx_DEV_FUNC are being renamed, or dropped, if they were generally not used at all for a platform.
Change-Id: I6ead2bc5e41a86c9aeef730f5664a30406414c8c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c M src/southbridge/intel/fsp_rangeley/pci_devs.h 2 files changed, 27 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/35730/5
build bot (Jenkins) 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 6:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35730/6/src/northbridge/intel/fsp_r... File src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c:
https://review.coreboot.org/c/coreboot/+/35730/6/src/northbridge/intel/fsp_r... PS6, Line 105: switch (dev->path.pci.devfn) { switch and case should be at the same indent
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... File src/southbridge/intel/fsp_rangeley/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 29: # define SOC_DEVFN_SOC PCI_DEVFN(SOC_DEV,SOC_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 34: # define SOC_DEVFN_PCIE_PORT1 PCI_DEVFN(PCIE_PORT1_DEV,PCIE_PORT1_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 39: # define SOC_DEVFN_PCIE_PORT2 PCI_DEVFN(PCIE_PORT2_DEV,PCIE_PORT2_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 44: # define SOC_DEVFN_PCIE_PORT3 PCI_DEVFN(PCIE_PORT3_DEV,PCIE_PORT3_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 49: # define SOC_DEVFN_PCIE_PORT4 PCI_DEVFN(PCIE_PORT4_DEV,PCIE_PORT4_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 54: # define SOC_DEVFN_HOST_BRIDGE PCI_DEVFN(HOST_BRIDGE_DEV,HOST_BRIDGE_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 59: # define SOC_DEVFN_RCEC PCI_DEVFN(RCEC_DEV,RCEC_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 64: # define SOC_DEVFN_SMBUS1 PCI_DEVFN(SMBUS1_DEV,SMBUS1_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 70: # define SOC_DEVFN_GBE1 PCI_DEVFN(GBE1_DEV,GBE1_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 73: # define SOC_DEVFN_GBE2 PCI_DEVFN(GBE2_DEV,GBE2_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 76: # define SOC_DEVFN_GBE3 PCI_DEVFN(GBE3_DEV,GBE3_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 79: # define SOC_DEVFN_GBE4 PCI_DEVFN(GBE4_DEV,GBE4_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 84: # define SOC_DEVFN_USB2 PCI_DEVFN(USB2_DEV,USB2_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 89: # define SOC_DEVFN_SATA2 PCI_DEVFN(SATA2_DEV,SATA2_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 94: # define SOC_DEVFN_SATA3 PCI_DEVFN(SATA3_DEV,SATA3_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 102: # define SOC_DEVFN_LPC PCI_DEVFN(LPC_DEV,LPC_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 108: # define SOC_DEVFN_SMBUS0 PCI_DEVFN(SMBUS0_DEV,SMBUS0_FUNC) space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35730/6/src/southbridge/intel/fsp_r... PS6, Line 113: # define SOC_DEVFN_IQAT PCI_DEVFN(IQAT_DEV,IQAT_FUNC) space required after that ',' (ctx:VxV)
Werner Zeh 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 6: Code-Review+2
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 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35730/4//COMMIT_MSG@11 PS4, Line 11: renamed
Ah ok. The SA (system agent) is a proxy for north cluster and pch a proxy for south cluster. […]
Ack
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. […]
Ack
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35730 )
Change subject: sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC ......................................................................
sb,nb/intel/fsp_rangeley: Rename from xx_DEV_FUNC
Intel adopted xx_DEVFN_xx naming for macros expanding to PCI_DEVFN() starting with apollolake. The ones named xx_DEV_FUNC are being renamed, or dropped, if they were generally not used at all for a platform.
Change-Id: I6ead2bc5e41a86c9aeef730f5664a30406414c8c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35730 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Werner Zeh werner.zeh@siemens.com --- M src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c M src/southbridge/intel/fsp_rangeley/pci_devs.h 2 files changed, 27 insertions(+), 27 deletions(-)
Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved
diff --git a/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c b/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c index 43e71f4..9acce5b 100644 --- a/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c +++ b/src/northbridge/intel/fsp_rangeley/fsp/chipset_fsp_util.c @@ -52,7 +52,7 @@ DEVTREE_CONST config_t *config; printk(BIOS_DEBUG, "Configure Default UPD Data\n");
- dev = pcidev_path_on_root(SOC_DEV_FUNC); + dev = pcidev_path_on_root(SOC_DEVFN_SOC); config = dev->chip_info;
/* Set SPD addresses */ @@ -103,30 +103,30 @@ continue;
switch (dev->path.pci.devfn) { - case GBE1_DEV_FUNC: - case GBE2_DEV_FUNC: - case GBE3_DEV_FUNC: - case GBE4_DEV_FUNC: + case SOC_DEVFN_GBE1: + case SOC_DEVFN_GBE2: + case SOC_DEVFN_GBE3: + case SOC_DEVFN_GBE4: UpdData->PcdEnableLan |= dev->enabled; printk(BIOS_DEBUG, "PcdEnableLan %d\n", UpdData->PcdEnableLan); break; - case SATA2_DEV_FUNC: + case SOC_DEVFN_SATA2: UpdData->PcdEnableSata2 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableSata2 %d\n", UpdData->PcdEnableSata2); break; - case SATA3_DEV_FUNC: + case SOC_DEVFN_SATA3: UpdData->PcdEnableSata3 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableSata3 %d\n", UpdData->PcdEnableSata3); break; - case IQAT_DEV_FUNC: + case SOC_DEVFN_IQAT: UpdData->PcdEnableIQAT |= dev->enabled; printk(BIOS_DEBUG, "PcdEnableIQAT %d\n", UpdData->PcdEnableIQAT); break; - case USB2_DEV_FUNC: + case SOC_DEVFN_USB2: UpdData->PcdEnableUsb20 = dev->enabled; printk(BIOS_DEBUG, "PcdEnableUsb20 %d\n", UpdData->PcdEnableUsb20); diff --git a/src/southbridge/intel/fsp_rangeley/pci_devs.h b/src/southbridge/intel/fsp_rangeley/pci_devs.h index 89f3c5c..c5ef6b8 100644 --- a/src/southbridge/intel/fsp_rangeley/pci_devs.h +++ b/src/southbridge/intel/fsp_rangeley/pci_devs.h @@ -26,72 +26,72 @@ /* Host Bridge */ #define SOC_DEV 0x0 #define SOC_FUNC 0 -# define SOC_DEV_FUNC PCI_DEVFN(SOC_DEV,SOC_FUNC) +# define SOC_DEVFN_SOC PCI_DEVFN(SOC_DEV,SOC_FUNC)
/* PCIE Port 1 */ #define PCIE_PORT1_DEV 0x1 #define PCIE_PORT1_FUNC 0 -# define PCIE_PORT1_DEV_FUNC PCI_DEVFN(PCIE_PORT1_DEV,PCIE_PORT1_FUNC) +# define SOC_DEVFN_PCIE_PORT1 PCI_DEVFN(PCIE_PORT1_DEV,PCIE_PORT1_FUNC)
/* PCIE Port 2 */ #define PCIE_PORT2_DEV 0x2 #define PCIE_PORT2_FUNC 0 -# define PCIE_PORT2_DEV_FUNC PCI_DEVFN(PCIE_PORT2_DEV,PCIE_PORT2_FUNC) +# define SOC_DEVFN_PCIE_PORT2 PCI_DEVFN(PCIE_PORT2_DEV,PCIE_PORT2_FUNC)
/* PCIE Port 3 */ #define PCIE_PORT3_DEV 0x3 #define PCIE_PORT3_FUNC 0 -# define PCIE_PORT3_DEV_FUNC PCI_DEVFN(PCIE_PORT3_DEV,PCIE_PORT3_FUNC) +# define SOC_DEVFN_PCIE_PORT3 PCI_DEVFN(PCIE_PORT3_DEV,PCIE_PORT3_FUNC)
/* PCIE Port 4 */ #define PCIE_PORT4_DEV 0x4 #define PCIE_PORT4_FUNC 0 -# define PCIE_PORT4_DEV_FUNC PCI_DEVFN(PCIE_PORT4_DEV,PCIE_PORT4_FUNC) +# define SOC_DEVFN_PCIE_PORT4 PCI_DEVFN(PCIE_PORT4_DEV,PCIE_PORT4_FUNC)
/* Host Bridge, Fabric, and RAS Registers */ #define HOST_BRIDGE_DEV 0xe #define HOST_BRIDGE_FUNC 0 -# define HOST_BRIDGE_DEV_FUNC PCI_DEVFN(HOST_BRIDGE_DEV,HOST_BRIDGE_FUNC) +# define SOC_DEVFN_HOST_BRIDGE PCI_DEVFN(HOST_BRIDGE_DEV,HOST_BRIDGE_FUNC)
/* Root Complex Event Collector (RCEC) */ #define RCEC_DEV 0xf #define RCEC_FUNC 0 -# define RCEC_DEV_FUNC PCI_DEVFN(RCEC_DEV,RCEC_FUNC) +# define SOC_DEVFN_RCEC PCI_DEVFN(RCEC_DEV,RCEC_FUNC)
/* SMBus 2.0 1 */ #define SMBUS1_DEV 0x13 #define SMBUS1_FUNC 0 -# define SMBUS1_DEV_FUNC PCI_DEVFN(SMBUS1_DEV,SMBUS1_FUNC) +# define SOC_DEVFN_SMBUS1 PCI_DEVFN(SMBUS1_DEV,SMBUS1_FUNC)
/* Gigabit Ethernet (GbE) */ #define GBE_DEV 0x14 #define GBE1_DEV GBE_DEV #define GBE1_FUNC 0 -# define GBE1_DEV_FUNC PCI_DEVFN(GBE1_DEV,GBE1_FUNC) +# define SOC_DEVFN_GBE1 PCI_DEVFN(GBE1_DEV,GBE1_FUNC) #define GBE2_DEV GBE_DEV #define GBE2_FUNC 1 -# define GBE2_DEV_FUNC PCI_DEVFN(GBE2_DEV,GBE2_FUNC) +# define SOC_DEVFN_GBE2 PCI_DEVFN(GBE2_DEV,GBE2_FUNC) #define GBE3_DEV GBE_DEV #define GBE3_FUNC 2 -# define GBE3_DEV_FUNC PCI_DEVFN(GBE3_DEV,GBE3_FUNC) +# define SOC_DEVFN_GBE3 PCI_DEVFN(GBE3_DEV,GBE3_FUNC) #define GBE4_DEV GBE_DEV #define GBE4_FUNC 3 -# define GBE4_DEV_FUNC PCI_DEVFN(GBE4_DEV,GBE4_FUNC) +# define SOC_DEVFN_GBE4 PCI_DEVFN(GBE4_DEV,GBE4_FUNC)
/* USB 2.0 */ #define USB2_DEV 0x16 #define USB2_FUNC 0 -# define USB2_DEV_FUNC PCI_DEVFN(USB2_DEV,USB2_FUNC) +# define SOC_DEVFN_USB2 PCI_DEVFN(USB2_DEV,USB2_FUNC)
/* SATA Gen 2 */ #define SATA2_DEV 0x17 #define SATA2_FUNC 0 -# define SATA2_DEV_FUNC PCI_DEVFN(SATA2_DEV,SATA2_FUNC) +# define SOC_DEVFN_SATA2 PCI_DEVFN(SATA2_DEV,SATA2_FUNC)
/* SATA Gen 3 */ #define SATA3_DEV 0x18 #define SATA3_FUNC 0 -# define SATA3_DEV_FUNC PCI_DEVFN(SATA3_DEV,SATA3_FUNC) +# define SOC_DEVFN_SATA3 PCI_DEVFN(SATA3_DEV,SATA3_FUNC)
/* Platform Control Unit (PCU) */ #define PCU_DEV 0x1f @@ -99,18 +99,18 @@ /* Low Pin Count (LPC/ISA) */ #define LPC_DEV PCU_DEV #define LPC_FUNC 0 -# define LPC_DEV_FUNC PCI_DEVFN(LPC_DEV,LPC_FUNC) +# define SOC_DEVFN_LPC PCI_DEVFN(LPC_DEV,LPC_FUNC) # define LPC_BDF PCI_DEV(BUS0, LPC_DEV, LPC_FUNC)
/* SMBus 2.0 0 */ #define SMBUS0_DEV PCU_DEV #define SMBUS0_FUNC 3 -# define SMBUS0_DEV_FUNC PCI_DEVFN(SMBUS0_DEV,SMBUS0_FUNC) +# define SOC_DEVFN_SMBUS0 PCI_DEVFN(SMBUS0_DEV,SMBUS0_FUNC)
/* Intel QuickAssist Integrated Accelerator (IQIA) */ #define IQAT_DEV 0xb #define IQAT_FUNC 0 -# define IQAT_DEV_FUNC PCI_DEVFN(IQAT_DEV,IQAT_FUNC) +# define SOC_DEVFN_IQAT PCI_DEVFN(IQAT_DEV,IQAT_FUNC)
#define SOC_DEVID 0x1f08 #define PCIE_PORT1_DEVID 0x1f10
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
David Guckian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Werner Zeh (Code Review)