Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34239
to review the following change.
Change subject: Documentation: Add FSP bugs ......................................................................
Documentation: Add FSP bugs
As Intel doesn't even document known bugs add a list of FSP bugs here.
Change-Id: I07819b83fb0c9437fc237472dfe943f78738347a Signed-off-by: Patrick Rudolph siro@das-labor.org --- M Documentation/soc/intel/fsp/index.md 1 file changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34239/1
diff --git a/Documentation/soc/intel/fsp/index.md b/Documentation/soc/intel/fsp/index.md index d7f44c6..0f0d197 100644 --- a/Documentation/soc/intel/fsp/index.md +++ b/Documentation/soc/intel/fsp/index.md @@ -2,6 +2,30 @@
This section contains documentation about Intel-FSP in public domain.
+## Bugs +As Intel doesn't even list known bugs, they are collected here until +those fixed. If possible a workaround is described here as well. + +### BroadwellDEFsp +* IA32_FEATURE_CONTROL MSR is locked in FSP-M + * Writing the MSR is required in ramstage for Intel TXT + * No workaround is known. + +* FSP-S asserts if the thermal PCI device 00:1f.6 is disabled + * FSP expects the PCI device to be enabled + * FSP expects BARs to be properly assigned + * Workaround: Don't disable this PCI device + +### KabylakeFsp +* MfgId and ModulePartNum in the DIMM_INFO struct are empty + * Those values are typically consumed by SMBIOS type 17 + * No workaround is known. + +### BraswellFsp +* Internal UART can't be disabled using PcdEnableHsuart* + * Workaround: Disable internal UART manually after calling FSP + + ## Open Source Intel FSP specification
* [About Intel FSP](https://firmware.intel.com/learn/fsp/about-intel-fsp) @@ -15,3 +39,7 @@ ## Additional Features in FSP 2.1 specification
- [PPI](ppi/ppi.md) + +## Official bugtracker + +- [IntelFSP/FSP](https://github.com/IntelFsp/FSP/issues)
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... File Documentation/soc/intel/fsp/index.md:
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 7: those fixed *are fixed* or *get fixed*
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 12: * No workaround is known. Maybe:
Workaround: none
to format it as below.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... File Documentation/soc/intel/fsp/index.md:
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 12: * No workaround is known.
Maybe: […]
I agree
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 22: No workaround is known Same here
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
-1 to bring attention to the requested changes.
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... File Documentation/soc/intel/fsp/index.md:
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 9: BroadwellDEFsp For each section, can we document which version these issues are found in?
Maybe link to the bugs in the FSP tracker so it's easy to see if there are changes?
Hello Patrick Rudolph, Angel Pons, Philipp Deppenwiese, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34239
to look at the new patch set (#2).
Change subject: Documentation: Add FSP bugs ......................................................................
Documentation: Add FSP bugs
As Intel doesn't even document known bugs add a list of FSP bugs here.
Change-Id: I07819b83fb0c9437fc237472dfe943f78738347a Signed-off-by: Patrick Rudolph siro@das-labor.org --- M Documentation/soc/intel/fsp/index.md 1 file changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34239/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... File Documentation/soc/intel/fsp/index.md:
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 7: those fixed
*are fixed* or *get fixed*
Done
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 9: BroadwellDEFsp
For each section, can we document which version these issues are found in? […]
Done
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 12: * No workaround is known.
I agree
Done
https://review.coreboot.org/c/coreboot/+/34239/1/Documentation/soc/intel/fsp... PS1, Line 22: No workaround is known
Same here
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2: Code-Review+1
CB:34285 has commentary about hiding and locking active PCI devices. But they are probably called "features" now.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+1
CB:34285 has commentary about hiding and locking active PCI devices. But they are probably called "features" now.
As it affects coreboot in a way that you'd call a bug, it's a bug. Please file a bug report, as I don't know all details there. I personally try to avoid touching P2SB whenever I can.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2: Code-Review-1
Patch Set 2:
Patch Set 2: Code-Review+1
CB:34285 has commentary about hiding and locking active PCI devices. But they are probably called "features" now.
As it affects coreboot in a way that you'd call a bug, it's a bug. Please file a bug report, as I don't know all details there. I personally try to avoid touching P2SB whenever I can.
[Non-blocking] -1 to bring attention to the requested changes. I agree with this change, though.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
Patch Set 2:
Patch Set 2: Code-Review+1
CB:34285 has commentary about hiding and locking active PCI devices. But they are probably called "features" now.
As it affects coreboot in a way that you'd call a bug, it's a bug. Please file a bug report, as I don't know all details there. I personally try to avoid touching P2SB whenever I can.
[Non-blocking] -1 to bring attention to the requested changes. I agree with this change, though.
What?
Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Documentation: Add FSP bugs
As Intel doesn't even document known bugs add a list of FSP bugs here.
Change-Id: I07819b83fb0c9437fc237472dfe943f78738347a Signed-off-by: Patrick Rudolph siro@das-labor.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34239 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/soc/intel/fsp/index.md 1 file changed, 43 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Martin Roth: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
Objections: Angel Pons: I would prefer that you didn't submit this
diff --git a/Documentation/soc/intel/fsp/index.md b/Documentation/soc/intel/fsp/index.md index d7f44c6..6269445 100644 --- a/Documentation/soc/intel/fsp/index.md +++ b/Documentation/soc/intel/fsp/index.md @@ -2,6 +2,39 @@
This section contains documentation about Intel-FSP in public domain.
+## Bugs +As Intel doesn't even list known bugs, they are collected here until +those are fixed. If possible a workaround is described here as well. + +### BroadwellDEFsp + +* IA32_FEATURE_CONTROL MSR is locked in FSP-M + * Release MR2 + * Writing the MSR is required in ramstage for Intel TXT + * Workaround: none + * Issue on public tracker: [Issue 10] + +* FSP-S asserts if the thermal PCI device 00:1f.6 is disabled + * Release MR2 + * FSP expects the PCI device to be enabled + * FSP expects BARs to be properly assigned + * Workaround: Don't disable this PCI device + * Issue on public tracker: [Issue 13] + +### KabylakeFsp +* MfgId and ModulePartNum in the DIMM_INFO struct are empty + * Release 3.7.1 + * Those values are typically consumed by SMBIOS type 17 + * Workaround: none + * Issue on public tracker: [Issue 22] + +### BraswellFsp +* Internal UART can't be disabled using PcdEnableHsuart* + * Release MR2 + * Workaround: Disable internal UART manually after calling FSP + * Issue on public tracker: [Issue 10] + + ## Open Source Intel FSP specification
* [About Intel FSP](https://firmware.intel.com/learn/fsp/about-intel-fsp) @@ -15,3 +48,13 @@ ## Additional Features in FSP 2.1 specification
- [PPI](ppi/ppi.md) + +## Official bugtracker + +- [IntelFSP/FSP](https://github.com/IntelFsp/FSP/issues) + +[Issue 10]: https://github.com/IntelFsp/FSP/issues/10 +[Issue 13]: https://github.com/IntelFsp/FSP/issues/13 +[Issue 15]: https://github.com/IntelFsp/FSP/issues/15 +[Issue 22]: https://github.com/IntelFsp/FSP/issues/22 +
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34239 )
Change subject: Documentation: Add FSP bugs ......................................................................
Patch Set 3: Code-Review+2
Patch Set 2:
Patch Set 2: Code-Review-1
Patch Set 2:
Patch Set 2: Code-Review+1
CB:34285 has commentary about hiding and locking active PCI devices. But they are probably called "features" now.
As it affects coreboot in a way that you'd call a bug, it's a bug. Please file a bug report, as I don't know all details there. I personally try to avoid touching P2SB whenever I can.
[Non-blocking] -1 to bring attention to the requested changes. I agree with this change, though.
What?
Not sure of what I was thinking. It can be easily amended in a follow-up.
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)