Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler
The xHCI S3/S4 quirk is only needed on PPT Revision A0. Fix always false condition by adding the missing read32 function call.
Untested.
Change-Id: Idc658aa9fe11c8ee93290e3d3ef66e73476f3c57 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/southbridge/intel/bd82x6x/smihandler.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/41852/1
diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index cddb4cf..fbc7b81 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -100,6 +100,9 @@ switch (slp_typ) { case ACPI_S3: case ACPI_S4: + if (!pch_silicon_supported(PCH_TYPE_PPT, PCH_STEP_A0)) + break; + reg16 = pci_read_config16(PCH_XHCI_DEV, 0x74); reg16 &= ~0x03UL; pci_write_config32(PCH_XHCI_DEV, 0x74, reg16); @@ -112,14 +115,13 @@ PCI_BASE_ADDRESS_0) & ~0xFUL;
if (!smm_validate_pointer((void *)(uintptr_t)xhci_bar)) { - /* FIXME: This looks broken */ - if ((xhci_bar + 0x4C0) & 1) + if (read32(xhci_bar + 0x4C0) & 1) pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4D0) & 1) + if (read32(xhci_bar + 0x4D0) & 1) pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4E0) & 1) + if (read32(xhci_bar + 0x4E0) & 1) pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4F0) & 1) + if (read32(xhci_bar + 0x4F0) & 1) pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); } reg32 = pci_read_config32(PCH_XHCI_DEV, PCI_COMMAND);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 1:
This needs a cast. Also, why was the condition always false?
Hello build bot (Jenkins), Bill XIE, Angel Pons, Arthur Heymans, Alexander Couzens, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41852
to look at the new patch set (#2).
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler
The xHCI S3/S4 quirk is only needed on PPT Revision A0. Fix always false condition by adding the missing read32 function call.
Untested.
Change-Id: Idc658aa9fe11c8ee93290e3d3ef66e73476f3c57 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/southbridge/intel/bd82x6x/smihandler.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/41852/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 2:
Patch Set 1:
This needs a cast. Also, why was the condition always false?
The lower 4 bits are always zero.
Hello build bot (Jenkins), Bill XIE, Angel Pons, Arthur Heymans, Alexander Couzens, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41852
to look at the new patch set (#3).
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler
The xHCI S3/S4 quirk is only needed on PPT Revision A0. Fix always false condition by adding the missing read32 function call.
Untested.
Change-Id: Idc658aa9fe11c8ee93290e3d3ef66e73476f3c57 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/southbridge/intel/bd82x6x/smihandler.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/41852/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 4: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 4:
Provided it gets tested.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... PS6, Line 116: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar)) { Shouldn't this check the actual addresses we are going to read?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 6: Code-Review-1
(1 comment)
definitely need to add smm_points_to_smram() checks for all of those random xhci_bar + ? reads
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... PS6, Line 116: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar)) {
Shouldn't this check the actual addresses we are going to read?
Agreed, all 5 addresses should be checked.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... PS6, Line 113: xhci_bar = pci_read_config32(PCH_XHCI_DEV, : PCI_BASE_ADDRESS_0) & ~0xFUL; Use `probe_resource`?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... PS6, Line 113: xhci_bar = pci_read_config32(PCH_XHCI_DEV, : PCI_BASE_ADDRESS_0) & ~0xFUL;
Use `probe_resource`?
no, it doesn't exist in SMM
Hello build bot (Jenkins), Bill XIE, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Alexander Couzens, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41852
to look at the new patch set (#7).
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler
The xHCI S3/S4 quirk is only needed on PPT Revision A0. Fix always false condition by adding the missing read32 function call.
Untested.
Change-Id: Idc658aa9fe11c8ee93290e3d3ef66e73476f3c57 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/southbridge/intel/bd82x6x/smihandler.c 1 file changed, 28 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/41852/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/6/src/southbridge/intel/bd82x... PS6, Line 116: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar)) {
Agreed, all 5 addresses should be checked.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 92: sizeof(uint16_t) this should be `sizeof(uint32_t)`, `read32` is used below
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 138: ~0xFUL `PCI_MEMORY_RANGE_MASK`
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 92: sizeof(uint16_t)
this should be `sizeof(uint32_t)`, `read32` is used below
I'd rather check if the entire xHCI MMIO window overlaps with SMRAM.
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 120: pci_read_config32 Isn't there some function to read a PCI BAR?
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 138: xhci_bar = pci_read_config32(PCH_XHCI_DEV, PCI_BASE_ADDRESS_0) & ~0xFUL; This read looks pointless.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 107: if (pch_silicon_type() == PCH_TYPE_PPT && : pch_silicon_revision() == PCH_STEP_A0) { : break; : } This skips over the `pci_or_config16(PCH_XHCI_DEV, 0x74, 0x03);` at the end of this case, but doesn't for S5. Am I missing something?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 107: if (pch_silicon_type() == PCH_TYPE_PPT && : pch_silicon_revision() == PCH_STEP_A0) { : break; : }
This skips over the `pci_or_config16(PCH_XHCI_DEV, 0x74, 0x03);` at the end of this case, but doesn' […]
Just checked, the S3/S4 workaround is indeed specific to PPT A0, but the S5 workaround applies to all steppings.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 107: if (pch_silicon_type() == PCH_TYPE_PPT && : pch_silicon_revision() == PCH_STEP_A0) { : break; : }
Just checked, the S3/S4 workaround is indeed specific to PPT A0, but the S5 workaround applies to al […]
Er, the condition is backwards.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 120: pci_read_config32
Isn't there some function to read a PCI BAR?
This BAR is 64-bit, so this can break if something moved the BAR above 4 GiB
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 92: sizeof(uint16_t)
I'd rather check if the entire xHCI MMIO window overlaps with SMRAM.
Do we know how big it is? There are just seemingly random offsets used below.
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 138: xhci_bar = pci_read_config32(PCH_XHCI_DEV, PCI_BASE_ADDRESS_0) & ~0xFUL;
This read looks pointless.
looks like copypasta, how could the BAR change in between? also not used after this
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 92: sizeof(uint16_t)
Do we know how big it is? There are just seemingly random offsets used below.
According to the datasheet, it's 64 KiB.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
I ended up making CB:49130 out of frustration
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41852 )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41852/7/src/southbridge/intel/bd82x... PS7, Line 112: /* FIXME: Unbalanced width in read/write ops (16-bit read then 32-bit write) */ : reg16 = pci_read_config16(PCH_XHCI_DEV, 0x74); : reg16 &= ~0x03UL; : pci_write_config32(PCH_XHCI_DEV, 0x74, reg16); This also needs to be fixed, the register is 16-bit.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41852?usp=email )
Change subject: sb/intel/bd82x6x/smihandler: Fix xHCI sleep handler ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Rudolph (Code Review)
-
Stefan Reinauer (Code Review)
-
Tim Wawrzynczak (Code Review)