Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39795
to review the following change.
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
soc/intel/cnl: Fix `PcieClkSrcUsage` setting
Remove unnecessary for-loop because the setting gets overwritten with memcpy afterwards.
Change-Id: Idb2192e68e29ae53fcdd0775ac7fd6cea2930a0e Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/39795/1
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 4634806..bbacfc1 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -320,10 +320,6 @@ params->PchCnviMode = 0; #endif /* PCI Express */ - for (i = 0; i < ARRAY_SIZE(config->PcieClkSrcUsage); i++) { - if (config->PcieClkSrcUsage[i] == 0) - config->PcieClkSrcUsage[i] = PCIE_CLK_NOTUSED; - } memcpy(params->PcieClkSrcUsage, config->PcieClkSrcUsage, sizeof(config->PcieClkSrcUsage)); memcpy(params->PcieClkSrcClkReq, config->PcieClkSrcClkReq,
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... PS1, Line 327: params memcpy happens to params->PcieClkSrcUsage, whereas the for loop above was setting config->PcieClkSrcUsage. I don't think the for loop is unnecessary here.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... PS1, Line 327: params
memcpy happens to params->PcieClkSrcUsage, whereas the for loop above was setting config->PcieClkSrc […]
You are right, sorry. I will abandon this patch.
Felix Singer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Abandoned
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
Yeah, sorry, the commit message was stale. That's why the original, vanished? change was [WIP]. The actual problem with the for-loop is that it replaces Clock Source 0 with garbage, i.e. you can't configure 0 for any board because somebody took the weirdest shortcut, using the fact that unset devicetree options default to 0. But 0 is a valid value :-/
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
Patch Set 1:
Yeah, sorry, the commit message was stale. That's why the original, vanished? change was [WIP]. The actual problem with the for-loop is that it replaces Clock Source 0 with garbage, i.e. you can't configure 0 for any board because somebody took the weirdest shortcut, using the fact that unset devicetree options default to 0. But 0 is a valid value :-/
s/Clock Source 0/Clock Source for the first root port/
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
Patch Set 1:
Yeah, sorry, the commit message was stale. That's why the original, vanished? change was [WIP]. The actual problem with the for-loop is that it replaces Clock Source 0 with garbage, i.e. you can't configure 0 for any board because somebody took the weirdest shortcut, using the fact that unset devicetree options default to 0. But 0 is a valid value :-/
Yes, I noticed this some time back and found it really weird that value 0 cannot really be used. I have some thoughts on it. Let me see if I can write it down and propose some changes here.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... PS1, Line 327: params
You are right, sorry. I will abandon this patch.
Well, the loop is wrong. One can't ever set PcieClkSrcUsage for RP #0
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39795 )
Change subject: soc/intel/cnl: Fix `PcieClkSrcUsage` setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39795/1/src/soc/intel/cannonlake/fs... PS1, Line 327: params
Well, the loop is wrong. […]
Oh, you already realized that in the main comments ;)
-
Felix Singer (Code Review) -
Furquan Shaikh (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)