Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
soc/intel/skylake: Fix platform reporting in console
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Fix the platforms reported in the console to be reported as "KBL/SKL <pch name>".
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/soc/intel/skylake/chip_fsp20.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/1
diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c index e46e52c..17bbc31d 100644 --- a/src/soc/intel/skylake/chip_fsp20.c +++ b/src/soc/intel/skylake/chip_fsp20.c @@ -55,7 +55,7 @@ * D29:[F0 - F7] 0xA118 - 0xA11F * D27:[F0 - F3] 0xA167 - 0xA16A */ -static const struct pcie_entry pcie_table_skl_pch_h[] = { +static const struct pcie_entry pcie_table_spt_pch_h[] = { {PCH_DEVFN_PCIE1, 8}, {PCH_DEVFN_PCIE9, 8}, {PCH_DEVFN_PCIE17, 4}, @@ -68,7 +68,7 @@ * D29:[F0 - F7] 0xA298 - 0xA29F * D27:[F0 - F7] 0xA2E7 - 0xA2EE */ -static const struct pcie_entry pcie_table_kbl_pch_h[] = { +static const struct pcie_entry pcie_table_unp_pch_h[] = { {PCH_DEVFN_PCIE1, 8}, {PCH_DEVFN_PCIE9, 8}, {PCH_DEVFN_PCIE17, 8}, @@ -80,7 +80,7 @@ * D28:[F0 - F7] 0x9D10 - 0x9D17 * D29:[F0 - F3] 0x9D18 - 0x9D1B */ -static const struct pcie_entry pcie_table_skl_pch_lp[] = { +static const struct pcie_entry pcie_table_spt_pch_lp[] = { {PCH_DEVFN_PCIE1, 8}, {PCH_DEVFN_PCIE9, 4}, }; @@ -148,17 +148,17 @@ id_mask = id & ~0xf; printk(BIOS_INFO, "Override DT after FSP-S, PCH is "); if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL/SKL PCH-LP SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_lp[0], - ARRAY_SIZE(pcie_table_skl_pch_lp)); + printk(BIOS_INFO, "KBL/SKL Sunrise Point PCH-LP SKU\n"); + pcie_update_device_tree(&pcie_table_spt_pch_lp[0], + ARRAY_SIZE(pcie_table_spt_pch_lp)); } else if (id_mask == (PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_kbl_pch_h[0], - ARRAY_SIZE(pcie_table_kbl_pch_h)); + printk(BIOS_INFO, "KBL/SKL Sunrise Point PCH-H SKU\n"); + pcie_update_device_tree(&pcie_table_spt_pch_h[0], + ARRAY_SIZE(pcie_table_spt_pch_h)); } else if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "SKL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_h[0], - ARRAY_SIZE(pcie_table_skl_pch_h)); + printk(BIOS_INFO, "KBL/SKL Union Point PCH-H SKU\n"); + pcie_update_device_tree(&pcie_table_unp_pch_h[0], + ARRAY_SIZE(pcie_table_unp_pch_h)); } else { printk(BIOS_ERR, "[BUG] PCIE Root Port id 0x%x" " is not found\n", id);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... PS1, Line 161: ARRAY_SIZE(pcie_table_unp_pch_h)); looks like the PCH-Hs were switched
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... PS1, Line 161: ARRAY_SIZE(pcie_table_unp_pch_h));
looks like the PCH-Hs were switched
yes, because this was wrong, I checked the ids
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... PS1, Line 161: ARRAY_SIZE(pcie_table_unp_pch_h));
yes, because this was wrong, I checked the ids
Well if there is a PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1 somewhere that is not SPT, that should be fixed and all users checked, first...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... PS1, Line 161: ARRAY_SIZE(pcie_table_unp_pch_h));
Well if there is a PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1 somewhere that […]
oops, now I see, what you mean... thank you, will be fixed
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 3:
This change is ready for review.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/1/src/soc/intel/skylake/chip_... PS1, Line 161: ARRAY_SIZE(pcie_table_unp_pch_h));
oops, now I see, what you mean... […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35763/3//COMMIT_MSG Commit Message:
PS3: Please mention that you chose UNP as Union-Point abbreviation.
https://review.coreboot.org/c/coreboot/+/35763/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/skylake: Fix platform reporting in console You also change identifier names and reorder platform detection which is the error-prone part. This summary hides that.
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... PS3, Line 156: unp nope, not anymore
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... PS3, Line 160: spt nope, not anymore
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35763
to look at the new patch set (#4).
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
soc/intel/skylake: Fix platform reporting in console
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Fix the platforms reported in the console to be reported as "KBL/SKL <pch name>".
As abbreviation for Union Point "kbp" has been used before here. This has been changed to "unp" now.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/soc/intel/skylake/chip_fsp20.c 1 file changed, 16 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/4
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35763
to look at the new patch set (#5).
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
soc/intel/skylake: Fix platform reporting in console
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Fix the platforms reported in the console to be reported as "KBL/SKL <pch name>".
While SPT was chosen as abbriviation for Sunrise Point, Union Point was shortened to KBL (KabyLake). To make this consistent, I replaced this by UNP.
Also this reorders platform detection to SPT-LP -> SPT-H -> UNP-H.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/soc/intel/skylake/chip_fsp20.c 1 file changed, 16 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/5
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: Fix platform reporting in console ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35763/3//COMMIT_MSG Commit Message:
PS3:
Please mention that you chose UNP as Union-Point abbreviation.
Done
https://review.coreboot.org/c/coreboot/+/35763/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/skylake: Fix platform reporting in console
You also change identifier names and reorder platform detection […]
Since this is if-else without any priority, I don't think this is really important but I added a short statement
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... PS3, Line 156: unp
nope, not anymore
Done
https://review.coreboot.org/c/coreboot/+/35763/3/src/soc/intel/skylake/chip_... PS3, Line 160: spt
nope, not anymore
Done
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35763
to look at the new patch set (#6).
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
soc/intel/skylake: rework and fix platform detection
This fixes multiple problems by completely reworking platform detection:
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Modify the platforms reported in the console to be reported as "SKL/KBL <pch name>".
Further, platform detection currently can fail if PCIe root port 1 is disabled. However, PCIe port swapping, which should work around exactly this problem in general, depends on platform detection.
This chicken-and-egg problem is solved by relying on the LPC device id instead of the PCIe port 1 device id, since this is what FSP does successfully (found out by reversing FSP-S). Required device id bases and masks got added to the device id list, while resorting some misplaced ones.
Also, the platform detection now has been implemented in a more generic way that will make it possible to move this (and coreboot-native PCIe root port swapping, which is already being worked on) to the common section and make it available for other platforms.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/include/device/pci_ids.h M src/soc/intel/skylake/chip_fsp20.c 2 files changed, 66 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35763/6/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/6/src/soc/intel/skylake/chip_... PS6, Line 99: .name = (const char*) "SKL/KBL Sunrise/Union Point PCH-LP SKU", "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/35763/6/src/soc/intel/skylake/chip_... PS6, Line 105: .name = (const char*) "SKL/KBL Sunrise Point PCH-H SKU", "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/35763/6/src/soc/intel/skylake/chip_... PS6, Line 111: .name = (const char*) "SKL/KBL Union Point PCH-H SKU", "(foo*)" should be "(foo *)"
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35763
to look at the new patch set (#7).
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
soc/intel/skylake: rework and fix platform detection
This fixes multiple problems by completely reworking platform detection:
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Modify the platforms reported in the console to be reported as "SKL/KBL <pch name>".
Further, platform detection currently can fail if PCIe root port 1 is disabled. However, PCIe port swapping, which should work around exactly this problem in general, depends on platform detection.
This chicken-and-egg problem is solved by relying on the LPC device id instead of the PCIe port 1 device id, since this is what FSP does successfully (found out by reversing FSP-S). Required device id bases and masks got added to the device id list, while resorting some misplaced ones.
Also, the platform detection now has been implemented in a more generic way that will make it possible to move this (and coreboot-native PCIe root port swapping, which is already being worked on) to the common section and make it available for other platforms.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/include/device/pci_ids.h M src/soc/intel/skylake/chip_fsp20.c 2 files changed, 66 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/7
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(6 comments)
Looks like you are overdoing the original cosmetic change a bit? Please explain the problem you are trying to fix before putting any more work into it.
Patch set #5 looked rather good?
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection. I still don't see how it fails. Can you give an example for what IDs it fails?
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@23 PS7, Line 23: misplaced ones. FSP-S isn't really the place to look for good examples.
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... PS7, Line 2701: #define PCI_DEVICE_ID_INTEL_SPT_LP_LPC_MASK 0x001f Why is the mask inverted?
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 114: }, we generally indent with tabs
`}, {` can stick on one line.
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 126: { This was correctly indented before. Either two tabs or aligned with the `(`. But _never_ aligned with the body.
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 178: if ((id & platform->lpc_mask) == platform->lpc_base) Doesn't seem to make sense given the masks provided.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection.
I still don't see how it fails. Can you give an example for what IDs […]
This does not depend on IDs. When the root port was disabled by FSP, it returns 0xFFFF as device ID, so we can't guess this
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@23 PS7, Line 23: misplaced ones.
FSP-S isn't really the place to look for good examples.
In this case it is the right way to do this, as explained here
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 178: if ((id & platform->lpc_mask) == platform->lpc_base)
Doesn't seem to make sense given the masks provided.
right... I forgot the invert in PCI ids... will be fixed
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection.
This does not depend on IDs. […]
* can't guess the platform by this
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
Patch Set 7:
(6 comments)
Looks like you are overdoing the original cosmetic change a bit? Please explain the problem you are trying to fix before putting any more work into it.
Patch set #5 looked rather good?
I did. Read the commit message ;)
Also, the platform detection now has been implemented in a more generic way that will make it possible to move this (and coreboot-native PCIe root port swapping, which is already being worked on) to the common section and make it available for other platforms.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
Patch Set 7:
(6 comments)
Looks like you are overdoing the original cosmetic change a bit? Please explain the problem you are trying to fix before putting any more work into it.
Patch set #5 looked rather good?
I did. Read the commit message ;)
Also, the platform detection now has been implemented in a more generic way that will make it possible to move this (and coreboot-native PCIe root port swapping, which is already being worked on) to the common section and make it available for other platforms.
and why is improving code quality a bad thing?
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... PS7, Line 2701: #define PCI_DEVICE_ID_INTEL_SPT_LP_LPC_MASK 0x001f
Why is the mask inverted?
because of brainf... will be fixed
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 126: {
This was correctly indented before. Either two tabs or aligned with the `(`. […]
oops... that wasn't intended
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35763
to look at the new patch set (#8).
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
soc/intel/skylake: rework and fix platform detection
This fixes multiple problems by completely reworking platform detection:
The 100 series PCH is reported as "SKL" platform which does support SKL and KBL and thus leads to confusion. Modify the platforms reported in the console to be reported as "SKL/KBL <pch name>".
Further, platform detection currently can fail if PCIe root port 1 is disabled. However, PCIe port swapping, which should work around exactly this problem in general, depends on platform detection.
This chicken-and-egg problem is solved by relying on the LPC device id instead of the PCIe port 1 device id, since this is what FSP does successfully (found out by reversing FSP-S). Required device id bases and masks got added to the device id list, while resorting some misplaced ones.
Also, the platform detection now has been implemented in a more generic way that will make it possible to move this (and coreboot-native PCIe root port swapping, which is already being worked on) to the common section and make it available for other platforms.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: Ida60f619308388adc180a5652908e5a947c17c0f --- M src/include/device/pci_ids.h M src/soc/intel/skylake/chip_fsp20.c 2 files changed, 66 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35763/8
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Uploaded patch set 8.
(4 comments)
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... PS7, Line 2701: #define PCI_DEVICE_ID_INTEL_SPT_LP_LPC_MASK 0x001f
because of brainf... […]
Done
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 114: },
we generally indent with tabs […]
Done
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 126: {
oops... […]
Done
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 178: if ((id & platform->lpc_mask) == platform->lpc_base)
right... I forgot the invert in PCI ids... […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35763/8/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/8/src/soc/intel/skylake/chip_... PS8, Line 103: },{ space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35763/8/src/soc/intel/skylake/chip_... PS8, Line 109: },{ space required after that ',' (ctx:VxV)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection.
- can't guess the platform by this
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
(1 comment)
I would prefer to treat the platform detection and changes to the RP swapping in separate commits. The former can be generally useful, e.g. as simple functions is_spt() is_unp() (the LP can actually be derived from Kconfig, could still implement a func- tion for that, that acts on the config setting).
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection.
Done
Ok, I'm starting to understand what you say. An example would have made this more clear. It fails to call pcie_update_device_tree() if all ports of the first group are disabled (otherwise we would not read 0xffff but the swapped IDs)... Which is really not what you said, but a potential problem.
So the real problem is that we make decisions for all groups based on the state of one group...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection.
but a potential problem.
not only potential, I see this on X11SSM-F
So the real problem is that we make decisions for all groups based on the state of one group...
Yep, and I use LPC now for the detection (hellsenberg suggested this, and then I found that FSP-S does exactly this)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
Patch Set 8:
(1 comment)
I would prefer to treat the platform detection and changes to the RP swapping in separate commits. The former can be generally useful, e.g. as simple functions is_spt() is_unp() (the LP can actually be derived from Kconfig, could still implement a func- tion for that, that acts on the config setting).
Well, atm this commit is only about platform detection. That this fixes the root port thing is just a positive side effect. I do NOT plan to add my non-fsp root port swapping implementation to this commit. This will definitely go to a follow-up commit :-)
I aggree that platform detection can be useful for other things, so what do you think about moving this directly to common code here? This is what I plan for the root port swapping implementation, too.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 8:
Patch Set 8:
Patch Set 8:
(1 comment)
I would prefer to treat the platform detection and changes to the RP swapping in separate commits. The former can be generally useful, e.g. as simple functions is_spt() is_unp() (the LP can actually be derived from Kconfig, could still implement a func- tion for that, that acts on the config setting).
Well, atm this commit is only about platform detection. That this fixes the root port thing is just a positive side effect. I do NOT plan to add my non-fsp root port swapping implementation to this commit. This will definitely go to a follow-up commit :-)
I aggree that platform detection can be useful for other things, so what do you think about moving this directly to common code here? This is what I plan for the root port swapping implementation, too.
Hmm.. actually what I said isn't completely true. I will try to make SKU detection completely independent. Let's see..
Michael Niewöhner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Abandoned
abandoned in favor of CB:35985
-
build bot (Jenkins) (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)