Change in ...coreboot[master]: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31229
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature
Enable Core Performance Boost feature in automatic mode.
Also enable C6 state which is a dependency for proper CPB operation.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I5e080bfaee06fd13cedf5151d4a598ec212213f2 --- M src/mainboard/pcengines/apu2/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31229/1
diff --git a/src/mainboard/pcengines/apu2/OemCustomize.c b/src/mainboard/pcengines/apu2/OemCustomize.c index a729860..700f4c7 100644 --- a/src/mainboard/pcengines/apu2/OemCustomize.c +++ b/src/mainboard/pcengines/apu2/OemCustomize.c @@ -97,4 +97,6 @@ ) { InitEarly->GnbConfig.PcieComplexList = &PcieComplex; + InitEarly->PlatformConfig.CStateMode = CStateModeC6; + InitEarly->PlatformConfig.CpbMode = CpbModeAuto; }
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1: Code-Review+1
Looks good to me, but since I don't know much about the binarypi stuff, I'll only +1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@9 PS1, Line 9: Enable Core Performance Boost feature in automatic mode. Could you please elaborate on what CPB actually does, and what effect the user will notice?
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@12 PS1, Line 12: Tested how?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@9 PS1, Line 9: Enable Core Performance Boost feature in automatic mode.
Could you please elaborate on what CPB actually does, and what effect the user will notice?
CPB allows to raise single core frequency from 1000MHz to 1200MHz during huge load. Processor has additional boosted P states when CPB is enabled, but these are hidden from OS.
I can add more information to commit message if You like.
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@12 PS1, Line 12:
Tested how?
I have performed some tests using https://github.com/kdlucas/byte-unixbench The suite showed c.a. 20% raise in overall index score for single core tests compared to the firmware without this patch.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
I have not encountered any issues. Would You mind sending these notes on priv so I can verify them?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
I have not encountered any issues. Would You mind sending these notes on priv so I can verify them?
Done; I understood apu2c4 was instable, apu3a2. Wondering if it was somehow memorymap or ECC related.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
I have not encountered any issues. Would You mind sending these notes on priv so I can verify them?
Done; I understood apu2c4 was instable, apu3a2. Wondering if it was somehow memorymap or ECC related.
Thank You. I have tested the patch on apu2c4 actually and instability seems to be gone.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@9 PS1, Line 9: Enable Core Performance Boost feature in automatic mode.
CPB allows to raise single core frequency from 1000MHz to 1200MHz during huge load. […]
Yes, it’d be great if you added both your answers to the commit message.
Hello Kyösti Mälkki, Patrick Rudolph, Felix Held, Piotr Król, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31229
to look at the new patch set (#2).
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature
Enable Core Performance Boost feature in automatic mode. Also enable C6 state which is a dependency for proper CPB operation.
CPB allows to raise single core frequency from 1000MHz to 1200MHz during huge load. Processor has additional boosted P-states when CPB is enabled, but these are hidden from OS.
Tested using byte-unixbench and with memtest86+ showing increased memory bandwidth.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I5e080bfaee06fd13cedf5151d4a598ec212213f2 --- M src/mainboard/pcengines/apu2/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31229/2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/1//COMMIT_MSG@9 PS1, Line 9: Enable Core Performance Boost feature in automatic mode.
Yes, it’d be great if you added both your answers to the commit message.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
I have not encountered any issues. Would You mind sending these notes on priv so I can verify them?
Done; I understood apu2c4 was instable, apu3a2. Wondering if it was somehow memorymap or ECC related.
Thank You. I have tested the patch on apu2c4 actually and instability seems to be gone.
Ok. It would be nice to have a definite answer if the previously applied patch on P-states was the culprit. So if you could revert the P-state change locally and see if it becomes unstable again and comment here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@17 PS2, Line 17: memory bandwidth. Well it's not really an increased memory bandwidth, just that those tests are CPU intensive memory read-write loops.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Michal, did you resolve the instability? I think I have some old notes about this.
I have not encountered any issues. Would You mind sending these notes on priv so I can verify them?
Done; I understood apu2c4 was instable, apu3a2. Wondering if it was somehow memorymap or ECC related.
Thank You. I have tested the patch on apu2c4 actually and instability seems to be gone.
Ok. It would be nice to have a definite answer if the previously applied patch on P-states was the culprit. So if you could revert the P-state change locally and see if it becomes unstable again and comment here.
Could You paste a link to the patch please?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@17 PS2, Line 17: memory bandwidth.
Well it's not really an increased memory bandwidth, just that those tests are CPU intensive memory r […]
Will "memtest86+ showing increased memory speed when boost is enabled" be ok?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 2:
(2 comments)
I was referring to SVI2 P-State patch: https://review.coreboot.org/c/coreboot/+/30367
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@13 PS2, Line 13: during huge load. Processor has additional boosted P-states when 'during high load if other cores idle'
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@17 PS2, Line 17: memory bandwidth.
Will "memtest86+ showing increased memory speed when boost is enabled" be ok?
Yet from specs we know that enabling CPB does not increase overall memory bandwidth?
'TEST: Higher single-core CPU performance is indicated by increased memory bandwidth as reported by memtest86+'
Hello Kyösti Mälkki, Patrick Rudolph, Felix Held, Piotr Król, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31229
to look at the new patch set (#3).
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature
Enable Core Performance Boost feature in automatic mode. Also enable C6 state which is a dependency for proper CPB operation.
CPB allows to raise single core frequency from 1000MHz to 1400MHz during high load if other cores idle. The processor has additional boosted P-states when CPB is enabled, but these are hidden from OS.
TEST: Higher single-core CPU performance is indicated by increased memory bandwidth as reported by memtest86+.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I5e080bfaee06fd13cedf5151d4a598ec212213f2 --- M src/mainboard/pcengines/apu2/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31229/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@13 PS2, Line 13: during huge load. Processor has additional boosted P-states when
'during high load if other cores idle'
Done. I have also changed the max frequency to 1400MHz, since memtest86+ shows overall ~40% increase in memory speed and the processor's specification says max frequency 1400MHz. The benchmark results showed ~20% increase in performance, but these are probably biased by background operations that OS performs.
https://review.coreboot.org/#/c/31229/2//COMMIT_MSG@17 PS2, Line 17: memory bandwidth.
Yet from specs we know that enabling CPB does not increase overall memory bandwidth? […]
Done
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31229 )
Change subject: src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature ......................................................................
src/mainboard/pcengines/apu2/OemCustomize.c: Enable CPB feature
Enable Core Performance Boost feature in automatic mode. Also enable C6 state which is a dependency for proper CPB operation.
CPB allows to raise single core frequency from 1000MHz to 1400MHz during high load if other cores idle. The processor has additional boosted P-states when CPB is enabled, but these are hidden from OS.
TEST: Higher single-core CPU performance is indicated by increased memory bandwidth as reported by memtest86+.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I5e080bfaee06fd13cedf5151d4a598ec212213f2 Reviewed-on: https://review.coreboot.org/c/31229 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/pcengines/apu2/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Felix Held: Looks good to me, but someone else must approve
diff --git a/src/mainboard/pcengines/apu2/OemCustomize.c b/src/mainboard/pcengines/apu2/OemCustomize.c index a729860..700f4c7 100644 --- a/src/mainboard/pcengines/apu2/OemCustomize.c +++ b/src/mainboard/pcengines/apu2/OemCustomize.c @@ -97,4 +97,6 @@ ) { InitEarly->GnbConfig.PcieComplexList = &PcieComplex; + InitEarly->PlatformConfig.CStateMode = CStateModeC6; + InitEarly->PlatformConfig.CpbMode = CpbModeAuto; }
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)