Change in coreboot[master]: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 13 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35170/1
diff --git a/src/soc/intel/skylake/bootblock/pch.c b/src/soc/intel/skylake/bootblock/pch.c index c95a8d8..740ce89 100644 --- a/src/soc/intel/skylake/bootblock/pch.c +++ b/src/soc/intel/skylake/bootblock/pch.c @@ -35,6 +35,7 @@ #include <soc/pm.h> #include <soc/pmc.h> #include <soc/smbus.h> +#include <soc/systemagent.h>
#include "../chip.h"
@@ -50,6 +51,12 @@ fast_spi_early_init(SPI_BASE_ADDRESS); p2sb_enable_bar(); p2sb_configure_hpet(); + + /* Write the IOAPIC Bus:Dev.Fun value to P2SB IBDF register */ + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_IBDF, V_DEFAULT_IBDF); + + /* Write the HPET Bus:Dev.Fun value to P2SB HBDF register */ + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF); }
static void soc_config_acpibase(void) diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c index 43bf27c..ed33c20 100644 --- a/src/soc/intel/skylake/chip_fsp20.c +++ b/src/soc/intel/skylake/chip_fsp20.c @@ -484,10 +484,8 @@ params->X2ApicOptOut = 0; tconfig->VtdDisable = 0;
- params->PchIoApicBdfValid = 1; - params->PchIoApicBusNumber = V_P2SB_IBDF_BUS; - params->PchIoApicDeviceNumber = V_P2SB_IBDF_DEV; - params->PchIoApicFunctionNumber = V_P2SB_IBDF_FUN; + /* HPET Bus:Dev.Fun already set in coreboot, so tell FSP to ignore UPDs */ + params->PchIoApicBdfValid = 0; }
soc_irq_settings(params); diff --git a/src/soc/intel/skylake/include/soc/systemagent.h b/src/soc/intel/skylake/include/soc/systemagent.h index 565c885..91209c8 100644 --- a/src/soc/intel/skylake/include/soc/systemagent.h +++ b/src/soc/intel/skylake/include/soc/systemagent.h @@ -66,9 +66,11 @@ #define V_P2SB_IBDF_BUS 250 #define V_P2SB_IBDF_DEV 31 #define V_P2SB_IBDF_FUN 0 +#define V_DEFAULT_IBDF ((V_P2SB_IBDF_BUS << 8) | PCI_DEVFN(V_P2SB_IBDF_DEV, V_P2SB_IBDF_FUN))
#define V_P2SB_HBDF_BUS 250 #define V_P2SB_HBDF_DEV 15 #define V_P2SB_HBDF_FUN 0 +#define V_DEFAULT_HBDF ((V_P2SB_HBDF_BUS << 8) | PCI_DEVFN(V_P2SB_HBDF_DEV, V_P2SB_HBDF_FUN))
#endif diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 4844660..a4d1d50 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -246,10 +246,8 @@ cpu_flex_override(m_cfg);
if (!config->ignore_vtd) { - m_cfg->PchHpetBdfValid = 1; - m_cfg->PchHpetBusNumber = V_P2SB_HBDF_BUS; - m_cfg->PchHpetDeviceNumber = V_P2SB_HBDF_DEV; - m_cfg->PchHpetFunctionNumber = V_P2SB_HBDF_FUN; + /* HPET Bus:Dev.Fun already set in coreboot, so tell FSP to ignore UPDs */ + m_cfg->PchHpetBdfValid = 0; } }
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35170/2/src/soc/intel/skylake/chip_... PS2, Line 487: HPET IOAPIC?
Hello Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35170
to look at the new patch set (#3).
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 13 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35170/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35170/2/src/soc/intel/skylake/chip_... PS2, Line 487: HPET
IOAPIC?
woups, copypasta error. Done
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3: Code-Review+2
tested and works well the code looks good to me
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3:
(1 comment)
I don't see yet that this improves anything and it obviously doesn't fix anything either. I don't care enough, though, if somebody merges it as is.
This is just configuration of the P2SB for later use by the OS, nothing that coreboot depends on. Hence, it would preferably be done somewhere in the P2SB's device driver (I haven't found it yet, `soc/intel/common/block/p2sb/p2sb.c` doesn't seem to mention SKL) before FSP hides the device. That much of the code in soc/intel/ doesn't follow basic coreboot structures is no excuse to make it even messier. I would also prefer that such things are unified across all the newer platforms. That we have different directories and code structures for them seems more like an accident.
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF); Looks like this would align better if you'd put it into something like
void p2sb_configure_vtd_originators(void);
called from here. It would also make the implementation available to other platforms (if they ever need it).
Also, as such configuration usually isn't done in the bootblock, we should at least comment, why. Maybe even move it to a more reasonable place before FSP hides the P2SB?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
Looks like this would align better if you'd put it into something like […]
Right, I could do that. Should I then place that function on the common P2SB intelblock?
I'd move this elsewhere, but where? I know the P2SB device is working fine here. Plus, if I were to move this elsewhere, the HPET write seems to be done by FSP-M and the IOAPIC write is done by FSP-S... Not sure why these are done on different phases, but I don't think Intel would want to tell me the reasons to do so.
And since the B:D.F numbers aren't the same for all platforms (cannonlake uses different stuffs, for instance), I think the p2sb_configure_vtd_originators() function would then have to take the B:D.F values as arguments... Not sure if a function to do two specific register writes is really worth it.
But I do agree that the comments for this function deserve a different comment treatment. The comments explain what the code does, but do not mention why.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
Right, I could do that. Should I then place that function on the common P2SB intelblock?
Sounds good.
I'd move this elsewhere, but where? I know the P2SB device is working fine here. Plus, if I were to move this elsewhere, the HPET write seems to be done by FSP-M and the IOAPIC write is done by FSP-S... Not sure why these are done on different phases, but I don't think Intel would want to tell me the reasons to do so.
I assume, it doesn't matter when it is done. The FSP source is probably just not organized around the P2SB device. HPET is a CPU thing, IOAPIC a PCH thing. For instance, we could also decide to put the HPET BDF writing in p2sb_configure_hpet()? Just an example, please ignore.
And since the B:D.F numbers aren't the same for all platforms (cannonlake uses different stuffs, for instance), I think the p2sb_configure_vtd_originators() function would then have to take the B:D.F values as arguments... Not sure if a function to do two specific register writes is really worth it.
The soc/intel/common/ code uses #includes as abstraction mechanism. It would just have to #include <soc/systemagent.h> and would get the correct numbers for the SoC it is built for.
But I do agree that the comments for this function deserve a different comment treatment. The comments explain what the code does, but do not mention why.
Yeah, your comments are actually just rephrasing the code (the worst kind of comments, beside lies, imho). What is really needed is the information that it's done early because FSP hides the device. Either in a comment or at least the commit message.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF); Ack, will do.
Yeah, your comments are actually just rephrasing the code (the worst kind of comments, beside lies, imho). What is really needed is the information that it's done early because FSP hides the device. Either in a comment or at least the commit message
Just wanted to state that useless wastes of space are less bad than misleading information :D Still bad, though. Will take care.
Maxim Polyakov has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Removed Code-Review+2 by Maxim Polyakov max.senia.poliak@gmail.com
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 3: Code-Review+2
Hello Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35170
to look at the new patch set (#4).
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage.c M src/soc/intel/skylake/romstage/systemagent.c 5 files changed, 13 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35170/4
Hello Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35170
to look at the new patch set (#5).
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/chip.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage.c M src/soc/intel/skylake/romstage/systemagent.c 4 files changed, 12 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35170/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 5:
(1 comment)
Patch Set 3:
(1 comment)
I don't see yet that this improves anything and it obviously doesn't fix anything either. I don't care enough, though, if somebody merges it as is.
This is just configuration of the P2SB for later use by the OS, nothing that coreboot depends on. Hence, it would preferably be done somewhere in the P2SB's device driver (I haven't found it yet, `soc/intel/common/block/p2sb/p2sb.c` doesn't seem to mention SKL) before FSP hides the device. That much of the code in soc/intel/ doesn't follow basic coreboot structures is no excuse to make it even messier. I would also prefer that such things are unified across all the newer platforms. That we have different directories and code structures for them seems more like an accident.
I am afraid of touching newer platforms as they can't be tested as easily. The idea of this change is to make the black box (FSP) weigh less (do less things) so that it can be carried away more easily where it belongs.
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
Ack, will do. […]
After thinking about it for a while, I realized I am unconditionally setting these values when they only make sense if VT-d is supported and enabled, so I moved this to romstage vtd init. It also makes more sense because #include <soc/systemagent.h> is already there.
Hello Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35170
to look at the new patch set (#6).
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/chip.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage.c M src/soc/intel/skylake/romstage/systemagent.c 4 files changed, 13 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35170/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
Grrr... I should build test before pushing...
Could somebody please boot-test this? Also, it would be good to test whether virtualization breaks with this change, since it changes stuff around VT-d.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/6//COMMIT_MSG Commit Message:
PS6: Needs testing!
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
Patch Set 5:
(1 comment)
Patch Set 3:
(1 comment)
I don't see yet that this improves anything and it obviously doesn't fix anything either. I don't care enough, though, if somebody merges it as is.
This is just configuration of the P2SB for later use by the OS, nothing that coreboot depends on. Hence, it would preferably be done somewhere in the P2SB's device driver (I haven't found it yet, `soc/intel/common/block/p2sb/p2sb.c` doesn't seem to mention SKL) before FSP hides the device. That much of the code in soc/intel/ doesn't follow basic coreboot structures is no excuse to make it even messier. I would also prefer that such things are unified across all the newer platforms. That we have different directories and code structures for them seems more like an accident.
I am afraid of touching newer platforms as they can't be tested as easily. The idea of this change is to make the black box (FSP) weigh less (do less things) so that it can be carried away more easily where it belongs.
The idea of this change is to make the black box (FSP) weigh less (do less things)
Full ACK! It's a small step in the right direction :-)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
After thinking about it for a while, I realized I am unconditionally setting these values when they […]
FSP hides P2SB but recently a CB got merged unhiding it again after FSP-S, so this is not true anymore
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
FSP hides P2SB but recently a CB got merged unhiding it again after FSP-S, so this is not true anymore
With "this", I guess you are referring to "this is done early because FSP hides the P2SB in ramstage"? I don't think it matters for this change.
The writes are now done in romstage instead of bootblock, which is more sound. And even then, since only FSP-S seems to hide the P2SB, it is still early enough.
Since this is a long thread on an older patchset, I am marking it as resolved. Please make a new comment on a more current patchset instead of making this thread even longer.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
FSP hides P2SB but recently a CB got merged unhiding it again after FSP-S, so this is not true any […]
Yep, definitely resolved (did I mark it as unresolved? :/) With "this" I meant "FSP hides the P2SB" - which is true, but we have a solution for it now :-)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6: Code-Review+1
Tested successfully on X11SSM-F
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
Patch Set 6: Code-Review+1
Tested successfully on X11SSM-F
Tested by comparing registers and starting linux in qemu-kvm
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/6//COMMIT_MSG Commit Message:
PS6:
Needs testing!
done ;)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6: Code-Review+1
Still not sure if this is worth the hassle. If you write the registers before FSP runs, we can't align it with newer platforms (that don't have the UPD and would probably overwrite our choice). Unless the registers are R/WO, ofc, who knows?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
Patch Set 6: Code-Review+1
Still not sure if this is worth the hassle. If you write the registers before FSP runs, we can't align it with newer platforms (that don't have the UPD and would probably overwrite our choice). Unless the registers are R/WO, ofc, who knows?
The registers are RW and the values that get programmed into it match those in the reference documentation. Moreover, I'm not sure if aligning the interfaces of different FSP binaries is worth the hassle either. I'd rather try aligning code that directly does things to the hardware.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35170/6//COMMIT_MSG Commit Message:
PS6:
done ;)
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 6:
Still not sure if this is worth the hassle. If you write the registers before FSP runs, we can't align it with newer platforms (that don't have the UPD and would probably overwrite our choice). Unless the registers are R/WO, ofc, who knows?
The registers are RW and the values that get programmed into it match those in the reference documentation. Moreover, I'm not sure if aligning the interfaces of different FSP binaries is worth the hassle either. I'd rather try aligning code that directly does things to the hardware.
That's what I mean, you make it harder to align this code. The same change would probably work for newer platforms if the register write would happen later. Doesn't matter, I don't see any code yet that tries to align some- thing.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot
Do it in coreboot code instead of letting FSP do it.
Change-Id: Ic5e8a62141608463ade398432253bad460a9a79d Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35170 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/chip.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage.c M src/soc/intel/skylake/romstage/systemagent.c 4 files changed, 13 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, approved
diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c index 1e0803c..de11a9e 100644 --- a/src/soc/intel/skylake/chip.c +++ b/src/soc/intel/skylake/chip.c @@ -391,17 +391,15 @@ /* Set TccActivationOffset */ tconfig->TccActivationOffset = config->tcc_offset;
+ /* Already handled in coreboot code, so tell FSP to ignore UPDs */ + params->PchIoApicBdfValid = 0; + /* Enable VT-d and X2APIC */ if (!config->ignore_vtd && soc_is_vtd_capable()) { params->VtdBaseAddress[0] = GFXVT_BASE_ADDRESS; params->VtdBaseAddress[1] = VTVC0_BASE_ADDRESS; params->X2ApicOptOut = 0; tconfig->VtdDisable = 0; - - params->PchIoApicBdfValid = 1; - params->PchIoApicBusNumber = V_P2SB_IBDF_BUS; - params->PchIoApicDeviceNumber = V_P2SB_IBDF_DEV; - params->PchIoApicFunctionNumber = V_P2SB_IBDF_FUN; }
dev = pcidev_path_on_root(SA_DEVFN_IGD); diff --git a/src/soc/intel/skylake/include/soc/systemagent.h b/src/soc/intel/skylake/include/soc/systemagent.h index 565c885..91209c8 100644 --- a/src/soc/intel/skylake/include/soc/systemagent.h +++ b/src/soc/intel/skylake/include/soc/systemagent.h @@ -66,9 +66,11 @@ #define V_P2SB_IBDF_BUS 250 #define V_P2SB_IBDF_DEV 31 #define V_P2SB_IBDF_FUN 0 +#define V_DEFAULT_IBDF ((V_P2SB_IBDF_BUS << 8) | PCI_DEVFN(V_P2SB_IBDF_DEV, V_P2SB_IBDF_FUN))
#define V_P2SB_HBDF_BUS 250 #define V_P2SB_HBDF_DEV 15 #define V_P2SB_HBDF_FUN 0 +#define V_DEFAULT_HBDF ((V_P2SB_HBDF_BUS << 8) | PCI_DEVFN(V_P2SB_HBDF_DEV, V_P2SB_HBDF_FUN))
#endif diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index 2904f05..d381caa 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -248,12 +248,9 @@
cpu_flex_override(m_cfg);
- if (!config->ignore_vtd) { - m_cfg->PchHpetBdfValid = 1; - m_cfg->PchHpetBusNumber = V_P2SB_HBDF_BUS; - m_cfg->PchHpetDeviceNumber = V_P2SB_HBDF_DEV; - m_cfg->PchHpetFunctionNumber = V_P2SB_HBDF_FUN; - } + /* HPET BDF already handled in coreboot code, so tell FSP to ignore UPDs */ + m_cfg->PchHpetBdfValid = 0; + m_cfg->HyperThreading = CONFIG(FSP_HYPERTHREADING); }
diff --git a/src/soc/intel/skylake/romstage/systemagent.c b/src/soc/intel/skylake/romstage/systemagent.c index bf0d506..e1272a1 100644 --- a/src/soc/intel/skylake/romstage/systemagent.c +++ b/src/soc/intel/skylake/romstage/systemagent.c @@ -19,6 +19,7 @@ #include <device/pci_ops.h> #include <intelblocks/systemagent.h> #include <soc/iomap.h> +#include <soc/p2sb.h> #include <soc/pci_devs.h> #include <soc/romstage.h> #include <soc/systemagent.h> @@ -38,6 +39,10 @@ if (!vtd_capable) return;
+ /* Configure P2SB VT-d originators (HPET and IOAPIC) */ + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF); + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_IBDF, V_DEFAULT_IBDF); + if (igd_dev && igd_dev->enabled) sa_set_mch_bar(&soc_gfxvt_mmio_descriptor, 1);
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Maxim Polyakov (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)