Change in coreboot[master]: soc/intel/common/p2sb: Add helper function to determine p2sb state
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
soc/intel/common/p2sb: Add helper function to determine p2sb state
Change-Id: I1d6f9c18160806e289e98c2fa5d290c61434112f Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/p2sb/p2sb.c 1 file changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47530/1
diff --git a/src/soc/intel/common/block/p2sb/p2sb.c b/src/soc/intel/common/block/p2sb/p2sb.c index d97cd8d..b5f9a07 100644 --- a/src/soc/intel/common/block/p2sb/p2sb.c +++ b/src/soc/intel/common/block/p2sb/p2sb.c @@ -15,6 +15,20 @@
#define HIDE_BIT (1 << 0)
+static bool p2sb_is_hiden(void) +{ + bool valid_vid = pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) == + PCI_VENDOR_ID_INTEL; + bool read_ones = pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) == 0xffff; + + if (read_ones) + return true; + if (valid_vid) + return false; + printk(BIOS_ERR, "P2SB PCI_VENDOR_ID is invalid, unknown if hidden\n"); + return true; +} + void p2sb_enable_bar(void) { /* Enable PCR Base address in PCH */ @@ -59,8 +73,7 @@ { p2sb_set_hide_bit(0);
- if (pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) != - PCI_VENDOR_ID_INTEL) + if (p2sb_is_hiden()) die_with_post_code(POST_HW_INIT_FAILURE, "Unable to unhide PCH_DEV_P2SB device !\n"); } @@ -69,8 +82,7 @@ { p2sb_set_hide_bit(1);
- if (pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) != - 0xFFFF) + if (!p2sb_is_hiden()) die_with_post_code(POST_HW_INIT_FAILURE, "Unable to hide PCH_DEV_P2SB device !\n"); }
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47530/1/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/1/src/soc/intel/common/block/... PS1, Line 18: p2sb_is_hiden p2sb_is_hidden
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47530
to look at the new patch set (#2).
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
soc/intel/common/p2sb: Add helper function to determine p2sb state
Change-Id: I1d6f9c18160806e289e98c2fa5d290c61434112f Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/p2sb/p2sb.c 1 file changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47530/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47530/1/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/1/src/soc/intel/common/block/... PS1, Line 18: p2sb_is_hiden
p2sb_is_hidden
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47530/2/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/2/src/soc/intel/common/block/... PS2, Line 20: pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) Why not store this in a constant variable?
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47530
to look at the new patch set (#3).
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
soc/intel/common/p2sb: Add helper function to determine p2sb state
Change-Id: I1d6f9c18160806e289e98c2fa5d290c61434112f Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/p2sb/p2sb.c 1 file changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47530/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47530/2/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/2/src/soc/intel/common/block/... PS2, Line 20: pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID)
Why not store this in a constant variable?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Sorry for going back and forth so often.
https://review.coreboot.org/c/coreboot/+/47530/3/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/3/src/soc/intel/common/block/... PS3, Line 21: bool valid_vid = pci_vid == PCI_VENDOR_ID_INTEL; : bool read_ones = pci_vid == 0xffff; Why not move these inside the if-blocks?
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47530
to look at the new patch set (#4).
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
soc/intel/common/p2sb: Add helper function to determine p2sb state
Change-Id: I1d6f9c18160806e289e98c2fa5d290c61434112f Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/p2sb/p2sb.c 1 file changed, 14 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47530/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47530/3/src/soc/intel/common/block/... File src/soc/intel/common/block/p2sb/p2sb.c:
https://review.coreboot.org/c/coreboot/+/47530/3/src/soc/intel/common/block/... PS3, Line 21: bool valid_vid = pci_vid == PCI_VENDOR_ID_INTEL; : bool read_ones = pci_vid == 0xffff;
Why not move these inside the if-blocks?
Done
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
Patch Set 4: Code-Review+2
Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47530 )
Change subject: soc/intel/common/p2sb: Add helper function to determine p2sb state ......................................................................
soc/intel/common/p2sb: Add helper function to determine p2sb state
Change-Id: I1d6f9c18160806e289e98c2fa5d290c61434112f Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/47530 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Christian Walter christian.walter@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/p2sb/p2sb.c 1 file changed, 14 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Christian Walter: Looks good to me, approved
diff --git a/src/soc/intel/common/block/p2sb/p2sb.c b/src/soc/intel/common/block/p2sb/p2sb.c index d97cd8d..9673a2c 100644 --- a/src/soc/intel/common/block/p2sb/p2sb.c +++ b/src/soc/intel/common/block/p2sb/p2sb.c @@ -15,6 +15,18 @@
#define HIDE_BIT (1 << 0)
+static bool p2sb_is_hidden(void) +{ + const uint16_t pci_vid = pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID); + + if (pci_vid == 0xffff) + return true; + if (pci_vid == PCI_VENDOR_ID_INTEL) + return false; + printk(BIOS_ERR, "P2SB PCI_VENDOR_ID is invalid, unknown if hidden\n"); + return true; +} + void p2sb_enable_bar(void) { /* Enable PCR Base address in PCH */ @@ -59,8 +71,7 @@ { p2sb_set_hide_bit(0);
- if (pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) != - PCI_VENDOR_ID_INTEL) + if (p2sb_is_hidden()) die_with_post_code(POST_HW_INIT_FAILURE, "Unable to unhide PCH_DEV_P2SB device !\n"); } @@ -69,8 +80,7 @@ { p2sb_set_hide_bit(1);
- if (pci_read_config16(PCH_DEV_P2SB, PCI_VENDOR_ID) != - 0xFFFF) + if (!p2sb_is_hidden()) die_with_post_code(POST_HW_INIT_FAILURE, "Unable to hide PCH_DEV_P2SB device !\n"); }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Marc Jones (Code Review)