Change in coreboot[master]: soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device
......................................................................

soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device

APL unhides the P2SB device in coreboot already. Do it the same on
other SoCs.
As the coreboot PCI allocator needs to be able to find the device,
unhide it after FSP-S.

Fixes "BUG: XXX requests hidden ...." warnings in coreboot log.

Change-Id: I0d14646098c34d3bf5cd49c35dcfcdce2c57431d
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/chip.c
M src/soc/intel/icelake/chip.c
M src/soc/intel/skylake/chip_fsp20.c
3 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/35620/1

diff --git a/src/soc/intel/cannonlake/chip.c b/src/soc/intel/cannonlake/chip.c
index 4e0dba5..e3962c0 100644
--- a/src/soc/intel/cannonlake/chip.c
+++ b/src/soc/intel/cannonlake/chip.c
@@ -21,6 +21,7 @@
 #include <intelblocks/chip.h>
 #include <intelblocks/itss.h>
 #include <intelblocks/xdci.h>
+#include <intelblocks/p2sb.h>
 #include <romstage_handoff.h>
 #include <soc/intel/common/vbt.h>
 #include <soc/pci_devs.h>
@@ -187,6 +188,13 @@
    /* Perform silicon specific init. */
    fsp_silicon_init(romstage_handoff_is_resume());

+	/*
+	 * Keep the P2SB device visible so it and the other devices are
+	 * visible in coreboot for driver support and PCI resource allocation.
+	 * There is no UPD setting for this.
+	 */
+	p2sb_unhide();
+
     /* Display FIRMWARE_VERSION_INFO_HOB */
    fsp_display_fvi_version_hob();

diff --git a/src/soc/intel/icelake/chip.c b/src/soc/intel/icelake/chip.c
index c4abb0c..abaae9f 100644
--- a/src/soc/intel/icelake/chip.c
+++ b/src/soc/intel/icelake/chip.c
@@ -21,6 +21,7 @@
 #include <intelblocks/chip.h>
 #include <intelblocks/itss.h>
 #include <intelblocks/xdci.h>
+#include <intelblocks/p2sb.h>
 #include <romstage_handoff.h>
 #include <soc/intel/common/vbt.h>
 #include <soc/itss.h>
@@ -128,6 +129,13 @@
    /* Perform silicon specific init. */
    fsp_silicon_init(romstage_handoff_is_resume());

+	/*
+	 * Keep the P2SB device visible so it and the other devices are
+	 * visible in coreboot for driver support and PCI resource allocation.
+	 * There is no UPD setting for this.
+	 */
+	p2sb_unhide();
+
     /* Display FIRMWARE_VERSION_INFO_HOB */
    fsp_display_fvi_version_hob();

diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c
index d1d7d6f..7fe1601 100644
--- a/src/soc/intel/skylake/chip_fsp20.c
+++ b/src/soc/intel/skylake/chip_fsp20.c
@@ -28,6 +28,7 @@
 #include <intelblocks/lpc_lib.h>
 #include <intelblocks/mp_init.h>
 #include <intelblocks/xdci.h>
+#include <intelblocks/p2sb.h>
 #include <intelpch/lockdown.h>
 #include <romstage_handoff.h>
 #include <soc/acpi.h>
@@ -175,6 +176,13 @@
    /* Perform silicon specific init. */
    fsp_silicon_init(romstage_handoff_is_resume());

+	/*
+	 * Keep the P2SB device visible so it and the other devices are
+	 * visible in coreboot for driver support and PCI resource allocation.
+	 * There is no UPD setting for this.
+	 */
+	p2sb_unhide();
+
    /* Restore GPIO IRQ polarities back to previous settings. */
    itss_restore_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device
......................................................................

Patch Set 1:

Currently APL hides the device in the END_OF_FIRMWARE event (see src/soc/intel/apollolake/chip.c). That was done because otherwise there were issues in OS with this device.
I would suggest to do the same with the platforms in this CL.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device
......................................................................

Patch Set 1:

BTW: It is done for skylake as well (see src/soc/intel/skylake/finalize.c)

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/{skl,icl,cnl}/chip: Unhide P2SB device
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Currently APL hides the device in the END_OF_FIRMWARE event (see src/soc/intel/apollolake/chip.c). That was done because otherwise there were issues in OS with this device.
> I would suggest to do the same with the platforms in this CL.

It's already hidden by all SoCs in the finalize event.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, PraveenX Hodagatta Pranesh, Subrata Banik, Michael Niewöhner, Aamir Bohra, Keith Short, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Werner Zeh, Ronak Kanabar, Maxim Polyakov,

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

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

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

soc/intel/skylake/chip: Unhide P2SB device

APL unhides the P2SB device in coreboot already. Do the same on SKL/BKL.
As the coreboot PCI allocator needs to be able to find the device,
unhide it after FSP-S.
The device is hidden in the SoC finalize function already and not visible
in the OS.

Other SoCs aren't updated, because they are to broken.

Fixes "BUG: XXX requests hidden ...." warnings in coreboot log.
Tested on Supermicro X11SSH-TF.

Change-Id: I0d14646098c34d3bf5cd49c35dcfcdce2c57431d
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/skylake/chip_fsp20.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/35620/2

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@15 
PS2, Line 15: to
too? Do you mean they are even more broken?

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Keith Short (Code Review)]

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@9 
PS2, Line 9: BKL
KBL?

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+2

The picture is slowly coming into focus about the consistency p2sb handling.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: -Code-Review

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
i guess there are some compliance to make P2SB disable before booting to OS, can you please give me 1 day time to get back on this and kindly hold this CL.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> i guess there are some compliance to make P2SB disable before booting to OS, can you please give me  […]

https://github.com/coreboot/coreboot/blob/master/src/soc/intel/skylake/final...

This is mirroring the same sequence as apollolake:

https://github.com/coreboot/coreboot/blob/master/src/soc/intel/apollolake/ch...

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> https://github.com/coreboot/coreboot/blob/master/src/soc/intel/skylake/final... […]

on core platform, i remember to have some recommendation that FSP-S should make p2sb hidden and then if CB like to use it then just unhide and make sure before booting to OS ,we are hiding p2sb again.

looks like src/soc/intel/common/block/systemagent/systemagent.c will eventually make p2sb hidden so we are good to meet the recommendation.

i got confused with commit msg saying unhide p2sb, but looks like thats is intermediate one to bypass warning

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> on core platform, i remember to have some recommendation that FSP-S should make p2sb hidden and then […]

now i remember, why we had FSP-S making p2sb hidden from bus

It avoids the BAR0 been modified by PCI BUS enumeration (either by bootloader or by OS). BIOS all hard code the BAR0 as 0xFD000000 so it must not be changed to any other setting.

Please check if PCI enumeration is allocating some other address than above over p2sb bar0 if yes, then we might need to right another p2sb.c to make sure we are doing fixed resource allocation.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> now i remember, why we had FSP-S making p2sb hidden from bus
>
> It avoids the BAR0 been modified by PCI BUS enumeration (either by bootloader or by OS). BIOS all hard code the BAR0 as 0xFD000000 so it must not be changed to any other setting.
>
> Please check if PCI enumeration is allocating some other address than above over p2sb bar0 if yes, then we might need to right another p2sb.c to make sure we are doing fixed resource allocation.

https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Nico Huber (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@15 
PS2, Line 15: to
> too? Do you mean they are even more broken?

Assuming yes. And yep, newer FSP is even more annoying. Intel
drags us all the way down.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@15 
PS2, Line 15: to
> Assuming yes. And yep, newer FSP is even more annoying. Intel […]

there's the undocumented "POST_SAI" feature which requires *some* registers to be written from SMM (That's what I guess by looking at the smihandler.c, but I can't say for sure).

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> > now i remember, why we had FSP-S making p2sb hidden from bus […]

Done

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

Tested successfully on X11SSM-F

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> Done

I think that p2sb.c driver needs the SKL, KBL, and AML DIDs added to the table.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

soc/intel/skylake/chip: Unhide P2SB device

APL unhides the P2SB device in coreboot already. Do the same on SKL/BKL.
As the coreboot PCI allocator needs to be able to find the device,
unhide it after FSP-S.
The device is hidden in the SoC finalize function already and not visible
in the OS, as more P2SB device IDs have been added.

Other SoCs aren't updated, because they are to broken.

Fixes "BUG: XXX requests hidden ...." warnings in coreboot log.
Tested on Supermicro X11SSH-TF.

Change-Id: I0d14646098c34d3bf5cd49c35dcfcdce2c57431d
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/skylake/chip_fsp20.c
M src/soc/intel/skylake/finalize.c
2 files changed, 12 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/35620/3

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> I think that p2sb.c driver needs the SKL, KBL, and AML DIDs added to the table.

done in CB:35791

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Angel Pons (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@15 
PS2, Line 15: to
> Yes, that's shedding permissions by transitioning to a new fabric id that is associated with future  […]

I believe that these P2SB SAI (Security Attributes of Initiator) getting lowered at some point during boot is what makes the P2SB on newer chips (CNL and later, I think) impossible to unhide (from the OS, at least).

This is what I understood/guessed so far. I don't have access to the interesting documents which describe SAI and sideband stuffs (they are likely only available under a NDA or two, I guess).

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Angel Pons (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35620/2/src/soc/intel/skylake/chip_... 
PS2, Line 184: p2sb_unhide
> now i remember, why we had FSP-S making p2sb hidden from bus
>
> It avoids the BAR0 been modified by PCI BUS enumeration (either by bootloader or by OS). BIOS all hard code the BAR0 as 0xFD000000 so it must not be changed to any other setting.

@Subrata, do you know if the hardcoded BAR0 value of 0xFD000000 is specifically required by hardware? It's the only reason I can think of that would require BAR0 to always be set to 0xFD000000.

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG@12 
PS3, Line 12: The device is hidden in the SoC finalize function already and not visible
Please add a blank line between paragraphs.

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG@15 
PS3, Line 15: to
too

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Felix Held (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3: Code-Review+1

looks good to me

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3: Code-Review+2

while this fixes the "BUG" warnings for P2SB, there are still others for calls accessing 13.0, 08.0, and 1e.0 on a Librem 13v2 SKL device (and other SKL/KBL devices I'd assume) which need to be addressed as well

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+2
>
> while this fixes the "BUG" warnings for P2SB, there are still others for calls accessing 13.0, 08.0, and 1e.0 on a Librem 13v2 SKL device (and other SKL/KBL devices I'd assume) which need to be addressed as well

Matt,

I don't see entries in the devicetree for those devices:

src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb

Are there other mainboards you re seeing this on? And what was the exact warning?

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3: Code-Review+1
> Patch Set 3:
> > Patch Set 3: Code-Review+2
> >
> > while this fixes the "BUG" warnings for P2SB, there are still others for calls accessing 13.0, 08.0, and 1e.0 on a Librem 13v2 SKL device (and other SKL/KBL devices I'd assume) which need to be addressed as well
>
> Matt,
>
> I don't see entries in the devicetree for those devices:
>
> src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
> src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
>
> Are there other mainboards you re seeing this on? And what was the exact warning?

This is another bug but not related to p2sb. I vote for merging this and working on the other warnings independent, as we already have multiple +2

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 3:
> This is another bug but not related to p2sb. I vote for merging this and working on the other warnings independent, as we already have multiple +2

yes, just related to hidden PCI devices. But agree completely

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, PraveenX Hodagatta Pranesh, Subrata Banik, Arthur Heymans, Michael Niewöhner, Aamir Bohra, Keith Short, Matt DeVillier, Paul Menzel, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Werner Zeh, Felix Held, Ronak Kanabar, Maxim Polyakov,

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

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

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

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

soc/intel/skylake/chip: Unhide P2SB device

APL unhides the P2SB device in coreboot already. Do the same on SKL/KBL.
As the coreboot PCI allocator needs to be able to find the device,
unhide it after FSP-S.

The device is hidden in the SoC finalize function already and not visible
in the OS, as more P2SB device IDs have been added.

Other SoCs aren't updated, because they are too broken.

Fixes "BUG: XXX requests hidden ...." warnings in coreboot log.
Tested on Supermicro X11SSH-TF.

Change-Id: I0d14646098c34d3bf5cd49c35dcfcdce2c57431d
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/skylake/chip_fsp20.c
M src/soc/intel/skylake/finalize.c
2 files changed, 12 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/35620/4

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/2//COMMIT_MSG@9 
PS2, Line 9: BKL
> KBL?

Done

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG@12 
PS3, Line 12: The device is hidden in the SoC finalize function already and not visible
> Please add a blank line between paragraphs.

Done

https://review.coreboot.org/c/coreboot/+/35620/3//COMMIT_MSG@15 
PS3, Line 15: to
> too

Done

Change in coreboot[master]: soc/intel/skylake/chip: Unhide P2SB device

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35620 )

Change subject: soc/intel/skylake/chip: Unhide P2SB device
......................................................................

soc/intel/skylake/chip: Unhide P2SB device

APL unhides the P2SB device in coreboot already. Do the same on SKL/KBL.
As the coreboot PCI allocator needs to be able to find the device,
unhide it after FSP-S.

The device is hidden in the SoC finalize function already and not visible
in the OS, as more P2SB device IDs have been added.

Other SoCs aren't updated, because they are too broken.

Fixes "BUG: XXX requests hidden ...." warnings in coreboot log.
Tested on Supermicro X11SSH-TF.

Change-Id: I0d14646098c34d3bf5cd49c35dcfcdce2c57431d
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35620
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
Reviewed-by: Aaron Durbin 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Matt DeVillier 
Reviewed-by: Michael Niewöhner
---
M src/soc/intel/skylake/chip_fsp20.c
M src/soc/intel/skylake/finalize.c
2 files changed, 12 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Felix Held: Looks good to me, but someone else must approve
  Furquan Shaikh: Looks good to me, approved
  Matt DeVillier: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c
index 55fedd3..c4f4e50 100644
--- a/src/soc/intel/skylake/chip_fsp20.c
+++ b/src/soc/intel/skylake/chip_fsp20.c
@@ -28,6 +28,7 @@
 #include <intelblocks/lpc_lib.h>
 #include <intelblocks/mp_init.h>
 #include <intelblocks/xdci.h>
+#include <intelblocks/p2sb.h>
 #include <intelpch/lockdown.h>
 #include <romstage_handoff.h>
 #include <soc/acpi.h>
@@ -175,6 +176,13 @@
    /* Perform silicon specific init. */
    fsp_silicon_init(romstage_handoff_is_resume());

+	/*
+	 * Keep the P2SB device visible so it and the other devices are
+	 * visible in coreboot for driver support and PCI resource allocation.
+	 * There is no UPD setting for this.
+	 */
+	p2sb_unhide();
+
    /* Restore GPIO IRQ polarities back to previous settings. */
    itss_restore_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);

diff --git a/src/soc/intel/skylake/finalize.c b/src/soc/intel/skylake/finalize.c
index 0d6f737..4cc9c83 100644
--- a/src/soc/intel/skylake/finalize.c
+++ b/src/soc/intel/skylake/finalize.c
@@ -46,14 +46,6 @@
 #define PCR_PSFX_T0_SHDW_PCIEN	0x1C
 #define PCR_PSFX_T0_SHDW_PCIEN_FUNDIS	(1 << 8)

-static void disable_sideband_access(void)
-{
-	p2sb_disable_sideband_access();
-
-	/* hide p2sb device */
-	p2sb_hide();
-}
-
 static void pch_disable_heci(void)
 {
    /* unhide p2sb device */
@@ -63,7 +55,7 @@
    pcr_or32(PID_PSF1, PSF_BASE_ADDRESS + PCR_PSFX_T0_SHDW_PCIEN,
    	PCR_PSFX_T0_SHDW_PCIEN_FUNDIS);

-	disable_sideband_access();
+	p2sb_disable_sideband_access();
 }

static void pch_finalize_script(struct device *dev)
@@ -113,6 +105,9 @@
    /* we should disable Heci1 based on the devicetree policy */
    if (config->HeciEnabled == 0)
    	pch_disable_heci();
+
+	/* Hide p2sb device as the OS must not change BAR0. */
+	p2sb_hide();
 }

static void soc_lockdown(struct device *dev)