Change in coreboot[master]: soc/intel/cnl: PEG devices

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: PEG devices
......................................................................

soc/intel/cnl: PEG devices

Change-Id: Ie2dd90edf10f1cefed26fe577508a9c8af3eb532
Signed-off-by: Felix Singer 
---
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/cannonlake/romstage/fsp_params.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/44741/1

diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h
index b0430e4..35f0e59 100644
--- a/src/soc/intel/cannonlake/include/soc/pci_devs.h
+++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h
@@ -22,6 +22,16 @@
 #define  SA_DEV_ROOT		PCI_DEV(0, SA_DEV_SLOT_ROOT, 0)
 #endif

+#define SA_DEV_SLOT_PEG		0x01
+#define  SA_DEVFN_PEG0		PCI_DEVFN(SA_DEV_SLOT_PEG, 0)
+#define  SA_DEVFN_PEG1		PCI_DEVFN(SA_DEV_SLOT_PEG, 1)
+#define  SA_DEVFN_PEG2		PCI_DEVFN(SA_DEV_SLOT_PEG, 2)
+#define  SA_DEVFN_PEG3		PCI_DEVFN(SA_DEV_SLOT_PEG, 3)
+#define  SA_DEV_PEG0		PCI_DEV(0, SA_DEV_SLOT_PEG, 0)
+#define  SA_DEV_PEG1		PCI_DEV(0, SA_DEV_SLOT_PEG, 1)
+#define  SA_DEV_PEG2		PCI_DEV(0, SA_DEV_SLOT_PEG, 2)
+#define  SA_DEV_PEG3		PCI_DEV(0, SA_DEV_SLOT_PEG, 3)
+
 #define SA_DEV_SLOT_IGD		0x02
 #define  SA_DEVFN_IGD		PCI_DEVFN(SA_DEV_SLOT_IGD, 0)
 #define  SA_DEV_IGD		PCI_DEV(0, SA_DEV_SLOT_IGD, 0)
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index ac42e00..4b2ad50 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -131,6 +131,20 @@
    if (config->DisableHeciRetry)
    	tconfig->DisableHeciRetry = config->DisableHeciRetry;
 #endif
+
+#if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
+	dev = pcidev_path_on_root(SA_DEVFN_PEG0);
+	m_cfg->Peg0Enable = is_dev_enabled(dev);
+
+	dev = pcidev_path_on_root(SA_DEVFN_PEG1);
+	m_cfg->Peg1Enable = is_dev_enabled(dev);
+
+	dev = pcidev_path_on_root(SA_DEVFN_PEG2);
+	m_cfg->Peg2Enable = is_dev_enabled(dev);
+
+	dev = pcidev_path_on_root(SA_DEVFN_PEG3);
+	m_cfg->Peg3Enable = is_dev_enabled(dev);
+#endif
 }

void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version)

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Set Ready For Review

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44741

to look at the new patch set (#5).

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

soc/intel/cnl: Allow enabling/disabling PEG devices

Add PCI device definitions to pci_devs.h and allow enabling /
disabling the PEG devices depending on the devicetree configuration.

Change-Id: Ie2dd90edf10f1cefed26fe577508a9c8af3eb532
Signed-off-by: Felix Singer 
---
M src/mainboard/siemens/chili/variants/chili/devicetree.cb
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/cannonlake/romstage/fsp_params.c
3 files changed, 24 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/44741/5

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Patch Set 7:

(1 comment)

Have all the devicetrees been checked that they enable available
PEG ports?

https://review.coreboot.org/c/coreboot/+/44741/7/src/soc/intel/cannonlake/ro... 
File src/soc/intel/cannonlake/romstage/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44741/7/src/soc/intel/cannonlake/ro... 
PS7, Line 135: #if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
Why the preprocessor guard? It's the same FSP headers for PCH-H and SoC, isn't it? If a C-if works, please use that.

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44741

to look at the new patch set (#13).

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

soc/intel/cnl: Allow enabling/disabling PEG devices

Add PCI device definitions to pci_devs.h and allow enabling /
disabling the PEG devices depending on the devicetree configuration.

Change-Id: Ie2dd90edf10f1cefed26fe577508a9c8af3eb532
Signed-off-by: Felix Singer 
---
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/cannonlake/romstage/fsp_params.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/44741/13

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Patch Set 13: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
File src/soc/intel/cannonlake/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
PS13, Line 26: #define  SA_DEVFN_PEG0		PCI_DEVFN(SA_DEV_SLOT_PEG, 0)
On other platforms with a PEG device on PCI_DEV(0, 6, 0) we needed to define each device. But here?

#define  SA_DEVFN_PEG(x) PCI_DEVFN(SA_DEV_SLOT_PEG, x)
 #define  SA_DEV_PEG(x)   PCI_DEV(0, SA_DEV_SLOT_PEG, x)

This allows using the macros to iterate over all PEG functions 😜

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Patch Set 14:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
File src/soc/intel/cannonlake/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
PS13, Line 29: #define  SA_DEVFN_PEG3		PCI_DEVFN(SA_DEV_SLOT_PEG, 3)
I wonder what this is? datasheet doesn't seem to mention it.

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/r... 
File src/soc/intel/cannonlake/romstage/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/r... 
PS13, Line 146: 	m_cfg->Peg3Enable = is_dev_enabled(dev);
Is there any SKU that could make use of this?

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
File src/soc/intel/cannonlake/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
PS13, Line 29: #define  SA_DEVFN_PEG3		PCI_DEVFN(SA_DEV_SLOT_PEG, 3)
> I wonder what this is? datasheet doesn't seem to mention it.

see Intel doc# 615211-005, section "2.2 PCI Express* Graphics Interface (PEG)" for example; interestingly function 0x01 is not documented in Volume 2 (Intel doc# 615211-003).

Fspmupd.h has this:
Peg0Enable:
Offset 0x0113 - Enable/Disable PEG 0
Disabled(0x0): Disable PEG Port, Enabled(0x1): Enable PEG Port (If Silicon SKU permits it), Auto(0x2)(Default): If an endpoint is present, enable the PEG Port, Disable otherwise 0:Disable, 1:Enable, 2:AUTO

PrimaryDisplay:
Offset 0x0177 - Selection of the primary display device
0=iGFX, 1=PEG, 2=PCIe Graphics on PCH, 3(Default)=AUTO, 4=Switchable Graphics

=> could it be, that this is the interface for external graphics, like nvidia or whatever?

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44741 )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
File src/soc/intel/cannonlake/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/44741/13/src/soc/intel/cannonlake/i... 
PS13, Line 29: #define  SA_DEVFN_PEG3		PCI_DEVFN(SA_DEV_SLOT_PEG, 3)
> see Intel doc# 615211-005, section "2.2 PCI Express* Graphics Interface (PEG)"

Doesn't mention function numbers AFAICS.
> for example; interestingly function 0x01 is not documented in Volume 2 (Intel doc# 615211-003).

That's 615212-003. It's easier to name things, e.g. CML DS vol2.

Also, top of the chapter it says that it's for functions 0..2.
> Fspmupd.h has this:
> Peg0Enable:
> Offset 0x0113 - Enable/Disable PEG 0
> Disabled(0x0): Disable PEG Port, Enabled(0x1): Enable PEG Port (If Silicon SKU permits it), Auto(0x2)(Default): If an endpoint is present, enable the PEG Port, Disable otherwise 0:Disable, 1:Enable, 2:AUTO
>
> PrimaryDisplay:
> Offset 0x0177 - Selection of the primary display device
> 0=iGFX, 1=PEG, 2=PCIe Graphics on PCH, 3(Default)=AUTO, 4=Switchable Graphics
>
> => could it be, that this is the interface for external graphics, like nvidia or whatever?

??? PEG means PCIe for Graphics. Intel uses it as a pseudonym for the
PCIe lanes directly at the CPU. PEG0..2 are bifurcation options of these
16 lanes. So, yes, one can expect external graphics there. But the one I
asked about, PEG3, isn't part of it. Maybe something for the future,
Rocket Lake is said to have 4 additional lanes (for NVMe) IIRC.

[S] Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling PEG devices

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44741?usp=email )

Change subject: soc/intel/cnl: Allow enabling/disabling PEG devices
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.