Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Chap ......................................................................
soc/intel/cnl: Chap
Change-Id: I177c2dc91a507cc633b024cf388bcc97e0d0f2e6 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/pci_devs.h 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/44740/1
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 95da672..640252c 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -646,6 +646,9 @@
dev = pcidev_path_on_root(SA_DEVFN_GNA); params->GnaEnable = is_dev_enabled(dev); + + dev = pcidev_path_on_root(SA_DEVFN_CHAP); + tconfig->ChapDeviceEnable = is_dev_enabled(dev); }
/* Mainboard GPIO Configuration */ diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h index fd43bbd..b0430e4 100644 --- a/src/soc/intel/cannonlake/include/soc/pci_devs.h +++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h @@ -34,6 +34,10 @@ #define SA_DEVFN_IPU PCI_DEVFN(SA_DEV_SLOT_IPU, 0) #define SA_DEV_IPU PCI_DEV(0, SA_DEV_SLOT_IPU, 0)
+#define SA_DEV_SLOT_CHAP 0x07 +#define SA_DEVFN_CHAP PCI_DEVFN(SA_DEV_SLOT_CHAP, 0) +#define SA_DEV_CHAP PCI_DEV(0, SA_DEV_SLOT_CHAP, 0) + #define SA_DEV_SLOT_GNA 0x08 #define SA_DEVFN_GNA PCI_DEVFN(SA_DEV_SLOT_GNA, 0) #define SA_DEV_GNA PCI_DEV(0, SA_DEV_SLOT_GNA, 0)
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Set Ready For Review
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 4: Code-Review+1
same as GNA, I'd add a Kconfig instead since
Hello build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44740
to look at the new patch set (#13).
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
soc/intel/cnl: Allow enabling/disabling Chap device
Add PCI device definitions to pci_devs.h and allow enabling / disabling the Chap device depending on the devicetree configuration.
The purpose of this device is not clear, but I would like to add this for documentation and for anyone who needs that, or knows what to do with it. According to FspsUpd.h it's disabled by default and coreboot does not enable it unless it's explicitly turned on in the devicetree.
Change-Id: I177c2dc91a507cc633b024cf388bcc97e0d0f2e6 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/pci_devs.h 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/44740/13
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 13: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG@7 PS13, Line 7: Chap CHAP
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG@10 PS13, Line 10: Chap CHAP
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG@12 PS13, Line 12: The purpose of this device is not clear https://www.intel.com/content/dam/www/public/us/en/documents/specification-u...
It mentions something about the CHAP (Chipset Hardware Architecture Performance) counters.
Also see: https://patents.google.com/patent/US7773504
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... PS14, Line 532: tconfig->ChapDeviceEnable = is_dev_enabled(dev); FSP header doesn't seem to mention which value actually enables it. Please confirm that the default in the binaries is indeed 0.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... PS14, Line 532: tconfig->ChapDeviceEnable = is_dev_enabled(dev);
FSP header doesn't seem to mention which value actually enables it. Please […]
CFL FSP uses the same polarity for CHAP, Camarillo, IPU and GNA, and this approach is correct.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... PS14, Line 532: tconfig->ChapDeviceEnable = is_dev_enabled(dev);
CFL FSP uses the same polarity for CHAP, Camarillo, IPU and GNA, and this approach is correct.
@Nico wdym? FSP header says "Enable: Device 7 enabled, Disable (Default): Device 7 disabled", which is pretty clear IMO. However, I'm still not sure if we want to have the chap device in the devicetree instead of Kconfig/cmos
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/44740/14/src/soc/intel/cannonlake/f... PS14, Line 532: tconfig->ChapDeviceEnable = is_dev_enabled(dev);
@Nico wdym? FSP header says "Enable: Device 7 enabled, Disable (Default): Device 7 disabled", which […]
I've checked the code. The polarity is 0=off,1=on for all four
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG@12 PS13, Line 12: The purpose of this device is not clear
https://www.intel. […]
so, I haven't yet read the whole patent. when would one need that device? should it be on always, when supported? or does it even need user space software / drivers / whatever?
what I got is, that it can be used for performance monitoring
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44740/13//COMMIT_MSG@12 PS13, Line 12: The purpose of this device is not clear
so, I haven't yet read the whole patent. […]
We might want to wait for chipset devicetrees to allow configuring the device through Kconfig. I think it should be off unless someone needs it.
Attention is currently required from: Felix Singer, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Patch Set 14:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/44740/comment/356a483d_e2802469 PS13, Line 12: The purpose of this device is not clear
We might want to wait for chipset devicetrees to allow configuring the device through Kconfig. […]
If we want the dt to specify the default, we could add a Kconfig like this:
~~~ choice prompt "Override devicetree: CHAP" default DT_CHAP_NO_OVERRIDE
config DT_CHAP_NO_OVERRIDE bool "use devicetree value"
config DT_CHAP_ENABLE bool "enable"
config DT_CHAP_DISABLE bool "disable"
endchoice ~~~
Felix Singer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44740 )
Change subject: soc/intel/cnl: Allow enabling/disabling Chap device ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Felix Singer (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)