David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32222
Change subject: mb/facebook/watson: Disable turbo ......................................................................
mb/facebook/watson: Disable turbo
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com --- M src/mainboard/facebook/watson/mainboard.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/32222/1
diff --git a/src/mainboard/facebook/watson/mainboard.c b/src/mainboard/facebook/watson/mainboard.c index e6b7850..8b0f129 100644 --- a/src/mainboard/facebook/watson/mainboard.c +++ b/src/mainboard/facebook/watson/mainboard.c @@ -3,6 +3,7 @@ * * Copyright (C) 2007-2009 coresystems GmbH * Copyright (C) 2011 Google Inc. + * Copyright (C) 2019-present Facebook Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -14,6 +15,7 @@ * GNU General Public License for more details. */
+#include <cpu/intel/turbo.h> #include <device/device.h>
/* @@ -25,6 +27,12 @@
}
+static void mainboard_init(void *chip_info) +{ + disable_turbo(); +} + struct chip_operations mainboard_ops = { .enable_dev = mainboard_enable, + .init = mainboard_init, };
David Hendricks has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
mb/facebook/watson: Disable turbo
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com --- M src/mainboard/facebook/watson/Kconfig M src/mainboard/facebook/watson/mainboard.c 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/32222/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... File src/mainboard/facebook/watson/Kconfig:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... PS2, Line 45: def_bool n Why the Kconfig?
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... File src/mainboard/facebook/watson/mainboard.c:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... PS2, Line 6: -present A copyright notice should state each year when (a part of) the work was first published. If `-present` is supposed to mean present at the time of writing that'd be useless, I guess. If it's supposed to mean present at the time of reading, it would become a lie next year (unless you'd add something noticeable at each January 1st?).
So what's the point here?
Hello Philipp Deppenwiese, build bot (Jenkins), John Looney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32222
to look at the new patch set (#3).
Change subject: mb/facebook/watson: Disable turbo ......................................................................
mb/facebook/watson: Disable turbo
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com --- M src/mainboard/facebook/watson/Kconfig M src/mainboard/facebook/watson/mainboard.c 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/32222/3
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... File src/mainboard/facebook/watson/Kconfig:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... PS2, Line 45: def_bool n
Why the Kconfig?
The requirement for whether or not turbo is enabled in all cases is not well-defined, and >1 customer may deploy this board. So this gives them flexibility to change this setting by enabling/disabling it in the config they use rather than changing code.
Some day I hope to have this and other settings that different users may want to change stored in VPD.
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... File src/mainboard/facebook/watson/mainboard.c:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... PS2, Line 6: -present
A copyright notice should state each year when (a part of) […]
Thanks for bringing this up. I actually went back and checked our legal template and as it happens one of the lawyers modified it last year and removed this part (see patch set 3).
BTW, I'm not a lawyer but I'm pretty sure copyright extends well past the next year. The "-present" thing only made it so people don't try to change the copyright line every time a file is updated. I'm sure we can find plenty of examples in the coreboot tree where people needlessly changed copyright years which in fact makes things a lot more confusing if the origin of the code were ever in dispute.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32222/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32222/3//COMMIT_MSG@8 PS3, Line 8: Please add the reasoning to the commit message.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 3:
Also, shouldn’t this be run-time configurable?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... File src/mainboard/facebook/watson/Kconfig:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... PS2, Line 45: def_bool n
The requirement for whether or not turbo is enabled in all cases is not well-defined, and >1 custome […]
Ok, maybe I should have asked "Why a Kconfig without prompt?" :) This is what was irritating me: You'd have to change the code or add some in site-local/ to toggle it.
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... File src/mainboard/facebook/watson/mainboard.c:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/mainbo... PS2, Line 6: -present
Thanks for bringing this up. […]
Yes, copyright extends, but not indefinitely. That's why one usually notes the year of the first publication, it's when the countdown starts.
I guess the urge to have a more generic copyright notice that doesn't have to be updated comes from the (seemingly new) practice to add/update notices for touching files (I've seen it going as far as adding a note for removing code!?! lately) no matter how little the change was. IANAL either, but it seems it only makes sense to have such copyright notices for more creative things. IMHO, if a code (addition) doesn't contain an `if` or a loop, it's not a creative work and doesn't deserve a copyright notice.
Hello Philipp Deppenwiese, build bot (Jenkins), John Looney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32222
to look at the new patch set (#4).
Change subject: mb/facebook/watson: Disable turbo ......................................................................
mb/facebook/watson: Disable turbo
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com --- M src/mainboard/facebook/watson/Kconfig M src/mainboard/facebook/watson/mainboard.c 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/32222/4
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 4:
(1 comment)
Patch Set 3:
Also, shouldn’t this be run-time configurable?
Possibly, but we lack a good runtime config UI at present and that is beyond the scope of this patch.
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... File src/mainboard/facebook/watson/Kconfig:
https://review.coreboot.org/#/c/32222/2/src/mainboard/facebook/watson/Kconfi... PS2, Line 45: def_bool n
Ok, maybe I should have asked "Why a Kconfig without prompt?" :) […]
The current deployments use site-local configs. I think adding it to the prompt is overkill in this case, but I don't really have a strong preference so I went ahead and added it (see PS4).
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Disable turbo ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32222/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32222/3//COMMIT_MSG@8 PS3, Line 8:
Please add the reasoning to the commit message.
From coreboot's standpoint it's just a CPU feature that can be enabled (or not). The rationale for why it should be enabled/disabled is up to the user.
I'll improve this message to say it's configurable, at least, since that more accurately reflects what the patch does.
Hello Philipp Deppenwiese, build bot (Jenkins), John Looney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32222
to look at the new patch set (#5).
Change subject: mb/facebook/watson: Make turbo mode configurable (disabled by default) ......................................................................
mb/facebook/watson: Make turbo mode configurable (disabled by default)
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com --- M src/mainboard/facebook/watson/Kconfig M src/mainboard/facebook/watson/mainboard.c 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/32222/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Make turbo mode configurable (disabled by default) ......................................................................
Patch Set 5: Code-Review+2
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Make turbo mode configurable (disabled by default) ......................................................................
Patch Set 5: Code-Review+2
David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32222 )
Change subject: mb/facebook/watson: Make turbo mode configurable (disabled by default) ......................................................................
mb/facebook/watson: Make turbo mode configurable (disabled by default)
Change-Id: Ief1eaab960c8fdab5bd5041b1a4f0c6ba1dd833f Signed-off-by: David Hendricks dhendrix@fb.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32222 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/facebook/watson/Kconfig M src/mainboard/facebook/watson/mainboard.c 2 files changed, 14 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/mainboard/facebook/watson/Kconfig b/src/mainboard/facebook/watson/Kconfig index 609b14a..6c2f954 100644 --- a/src/mainboard/facebook/watson/Kconfig +++ b/src/mainboard/facebook/watson/Kconfig @@ -41,4 +41,8 @@ string default "src/mainboard/$(CONFIG_MAINBOARD_DIR)/board.fmd"
+config ENABLE_TURBO + bool "Enable turbo frequency" + default n + endif # BOARD_FACEBOOK_WATSON diff --git a/src/mainboard/facebook/watson/mainboard.c b/src/mainboard/facebook/watson/mainboard.c index e6b7850..3a1692f 100644 --- a/src/mainboard/facebook/watson/mainboard.c +++ b/src/mainboard/facebook/watson/mainboard.c @@ -3,6 +3,7 @@ * * Copyright (C) 2007-2009 coresystems GmbH * Copyright (C) 2011 Google Inc. + * Copyright (C) Facebook, Inc. and its affiliates * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -14,6 +15,7 @@ * GNU General Public License for more details. */
+#include <cpu/intel/turbo.h> #include <device/device.h>
/* @@ -25,6 +27,14 @@
}
+static void mainboard_init(void *chip_info) +{ +#if !IS_ENABLED(CONFIG_ENABLE_TURBO) + disable_turbo(); +#endif +} + struct chip_operations mainboard_ops = { .enable_dev = mainboard_enable, + .init = mainboard_init, };
-
David Hendricks (Code Review) -
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)