Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
mb/ocp/deltalake: Enable PCH thermal sensor via FSP
Tested=On OCP Delta Lake, oBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/mainboard/ocp/deltalake/romstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44075/1
diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c index f69ec60..40ac494 100644 --- a/src/mainboard/ocp/deltalake/romstage.c +++ b/src/mainboard/ocp/deltalake/romstage.c @@ -49,6 +49,8 @@ mupd->FspmConfig.DebugPrintLevel = FSP_LOG_LEVEL_DEFAULT; } } + /* Enable PCH Thermal sensor */ + mupd->FspmConfig.ThermalDeviceEnable = 1; }
/* Update bifurcation settings according to different Configs */
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 2:
This change is ready for review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44075/2//COMMIT_MSG@9 PS2, Line 9: oBMC OpenBMC?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... PS2, Line 53: mupd->FspmConfig.ThermalDeviceEnable = 1; Does this correspond to a PCI device? If so, I'd set it in chipset code according to devicetree
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44075
to look at the new patch set (#3).
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
mb/ocp/deltalake: Enable PCH thermal sensor via FSP
Tested=On OCP Delta Lake, OpenBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/mainboard/ocp/deltalake/devicetree.cb M src/mainboard/ocp/deltalake/romstage.c M src/soc/intel/xeon_sp/cpx/chip.h 3 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44075/3
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44075/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44075/2//COMMIT_MSG@9 PS2, Line 9: oBMC
OpenBMC?
Done
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... PS2, Line 53: mupd->FspmConfig.ThermalDeviceEnable = 1;
Does this correspond to a PCI device? If so, I'd set it in chipset code according to devicetree
Yes, updated.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... PS2, Line 53: mupd->FspmConfig.ThermalDeviceEnable = 1;
Yes, updated.
what is the address of this device (bus/device/function)?
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... PS3, Line 15: static const config_t *config = NULL; Why do we need this global variable? Maybe declare this variable in mainboard_memory_init_params()?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: mb/ocp/deltalake: Enable PCH thermal sensor via FSP ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... PS2, Line 53: mupd->FspmConfig.ThermalDeviceEnable = 1;
what is the address of this device (bus/device/function)?
I was thinking of doing something like CB:42557
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... PS3, Line 15: static const config_t *config = NULL;
Why do we need this global variable? […]
This should not be needed if doing something like CB:42557
Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44075
to look at the new patch set (#4).
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
xeon_sp/cpx: Enable PCH thermal device via FSP
Tested=On OCP Delta Lake, OpenBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h M src/soc/intel/xeon_sp/cpx/romstage.c 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44075/4
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/2/src/mainboard/ocp/deltalake... PS2, Line 53: mupd->FspmConfig.ThermalDeviceEnable = 1;
I was thinking of doing something like CB:42557
Yes, it's 0:14.2 PCH thermal subsystem, updated to follow CB:42557. Thanks for the tips.
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/3/src/mainboard/ocp/deltalake... PS3, Line 15: static const config_t *config = NULL;
This should not be needed if doing something like CB:42557
I changed to follow CB:42557
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 67: 2: Enabled in ACPI mode In this case, we cannot set this value
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 67: 2: Enabled in ACPI mode
In this case, we cannot set this value
Oh, hm. If we're not going to use it, it doesn't matter. If we need to be able to support ACPI mode, we would need to have something like `pch_thermal_dev_acpi_mode` in devicetree, and some more logic to handle it.
Or, just override the FSP-M UPD in `mainboard_memory_init_params`.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Patrick Rudolph, Maxim Polyakov, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44075
to look at the new patch set (#5).
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
xeon_sp/cpx: Enable PCH thermal device via FSP
Tested=On OCP Delta Lake, OpenBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/romstage.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44075/5
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 67: 2: Enabled in ACPI mode
Oh, hm. If we're not going to use it, it doesn't matter. […]
I set it in the chip code and people can still overwrite it in their mainboard_memory_init_params().
Hello build bot (Jenkins), Patrick Rudolph, Maxim Polyakov, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44075
to look at the new patch set (#6).
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
xeon_sp/cpx: Enable PCH thermal device via FSP
Tested=On OCP Delta Lake, OpenBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h M src/soc/intel/xeon_sp/cpx/romstage.c 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44075/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 6:
You'll need to add this device to deltalake's devicetree, though.
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/44075/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 67: 2: Enabled in ACPI mode
I set it in the chip code and people can still overwrite it in their mainboard_memory_init_params().
Oh sorry, I misunderstood what you meant, reverted back to patchset 4.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44075 )
Change subject: xeon_sp/cpx: Enable PCH thermal device via FSP ......................................................................
xeon_sp/cpx: Enable PCH thermal device via FSP
Tested=On OCP Delta Lake, OpenBMC sensor-util can see PCH Temp readings.
Change-Id: I39d0d0a982476f9fece51cfa19dcbd0da5dea690 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44075 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Maxim Polyakov max.senia.poliak@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h M src/soc/intel/xeon_sp/cpx/romstage.c 2 files changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Maxim Polyakov: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h index 71c68cf..17f801b 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h @@ -92,6 +92,8 @@
#define XHCI_BUS_NUMBER 0x0 #define PCH_DEV_SLOT_XHCI 0x14 +#define PCH_DEVFN_THERMAL _PCH_DEVFN(XHCI, 2) + #define XHCI_FUNC_NUM 0x0
#define HPET_BUS_NUM 0x0 diff --git a/src/soc/intel/xeon_sp/cpx/romstage.c b/src/soc/intel/xeon_sp/cpx/romstage.c index 96949fd..6c65994 100644 --- a/src/soc/intel/xeon_sp/cpx/romstage.c +++ b/src/soc/intel/xeon_sp/cpx/romstage.c @@ -3,6 +3,7 @@ #include <arch/romstage.h> #include <fsp/api.h> #include <soc/romstage.h> +#include <soc/pci_devs.h> #include "chip.h"
void __weak mainboard_memory_init_params(FSPM_UPD *mupd) @@ -13,6 +14,7 @@ void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { FSPM_CONFIG *m_cfg = &mupd->FspmConfig; + const struct device *dev;
/* ErrorLevel - 0 (disable) to 8 (verbose) */ m_cfg->DebugPrintLevel = 8; @@ -61,5 +63,10 @@ m_cfg->PEXPHIDE = 0x0; m_cfg->HidePEXPMenu = 0x0;
+ /* Enable PCH thermal device in FSP, the definition of ThermalDeviceEnable is + 0: Disable, 1: Enabled in PCI mode, 2: Enabled in ACPI mode */ + dev = pcidev_path_on_root(PCH_DEVFN_THERMAL); + m_cfg->ThermalDeviceEnable = dev && dev->enabled; + mainboard_memory_init_params(mupd); }
-
Angel Pons (Code Review) -
Johnny Lin (Code Review)
-
Maxim Polyakov (Code Review)
-
Paul Menzel (Code Review)