Change in ...coreboot[master]: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gat...
Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31534
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter
Change-Id: Id346173ac7ae5246de0b38b9dd23be7b72e70f1e --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31534/1
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index c276c86..534065f 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -128,6 +128,10 @@ /* S0ix */ params->PchPmSlpS0Enable = config->s0ix_enable;
+ /* Legacy 8254 timer support */ + params->Enable8254ClockGating = config->clock_gate_8254; + params->Enable8254ClockGatingOnS3 = config->clock_gate_8254; + /* disable Legacy PME */ memset(params->PcieRpPmSci, 0, sizeof(params->PcieRpPmSci));
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31534/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31534/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 132: params->Enable8254ClockGating = config->clock_gate_8254; : params->Enable8254ClockGatingOnS3 = config->clock_gate_8254; are these always synchronized? Assume there's a use case where they aren't, if there are separate UPDs
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31534
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter
Tested on system76 galp3-c
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: Id346173ac7ae5246de0b38b9dd23be7b72e70f1e --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31534/2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
(1 comment)
Enable8254ClockGatingOnS3 is only applicable when Enable8254ClockGating is disabled. FSP will do the 8254 CGE programming on S3 resume when Enable8254ClockGatingOnS3 is enabled.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31534/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31534/2//COMMIT_MSG@6 PS2, Line 6: : soc/intel/cannonlake: Please use a short commit summary.
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+1
(1 comment)
I am unsure how I could make it shorter. Any suggestions?
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
Patch Set 1:
(1 comment)
So, there are three cases identified in the FSP documentation. I think I will make this an enum like so:
{ OFF, // There is no 8254 clock gating S3, // 8254 clock gating happens in S3 ON, // 8254 clock gating happens on boot and in S3 }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... PS2, Line 132: params->Enable8254ClockGating = config->clock_gate_8254; : params->Enable8254ClockGatingOnS3 = config->clock_gate_8254; Default value for both Enable8254ClockGating and Enable8254ClockGatingOnS3 is 1. With this change, it will be set to clock_gate_8254 which defaults to 0 and I don't see any coreboot mainboard setting it. IIUC, that can result in S0ix failures because of the change in UPD values. Any board that is using CNL should first set clock_gate_8254 before this change goes in.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... PS2, Line 132: params->Enable8254ClockGating = config->clock_gate_8254; : params->Enable8254ClockGatingOnS3 = config->clock_gate_8254;
Default value for both Enable8254ClockGating and Enable8254ClockGatingOnS3 is 1. […]
Phew, I guess this means the surrounding code in coreboot (e.g. itss_clock_gate8254()) is dead and was never tested? I don't under- stand why we keep adding or copying dead code (that is redundant to FSP).
Jeremy, I think the best thing to do is to drop `clock_gate_8254` and the code around it. And introduce a Kconfig option that defaults to enabling the 8254 if the payload is SeaBIOS (IIRC, this was why you wanted to enable it?).
Whether or not this clock should be gated isn't a hardware design decision anyway, so why have such a setting the the devicetree in the first place? We could also remove the devicetree option for other platforms (that don't use it anyway) and give the Kconfig a nice default for each platform that reflects the current state?
Jeremy Soller has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Abandoned
Fixed by https://review.coreboot.org/c/coreboot/+/33512
-
Furquan Shaikh (Code Review) -
Jeremy Soller (Code Review)
-
Lijian Zhao (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)