Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Patrick Georgi, HAOUAS Elyes, Vanny E, Huang Jin, Philipp Deppenwiese, Nico Huber, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33928
to look at the new patch set (#15).
Change subject: cpu/x86/tsc: Flip and rename TSC_CONSTANT_RATE to UNKNOWN_TSC_RATE
......................................................................
cpu/x86/tsc: Flip and rename TSC_CONSTANT_RATE to UNKNOWN_TSC_RATE
The x86 timers are a bit of a mess. Cases where different stages use
different counters and timestamps use different counters from udelays.
The original intention was to only flip TSC_CONSTANT_RATE Kconfig
to NOT_CONSTANT_TSC_RATE. The name would be incorrect though, those
counters do run with a constant rate but we just lack tsc_freq_mhz()
implementation for three platforms.
Note that for boards with UNKNOWN_TSC_RATE=y, each stage will have a
slow run of calibrate_tsc_with_pit(). This is easy enough to fix with
followup implementation of tsc_freq_mhz() for the platforms.
Implementations with LAPIC_MONOTONIC_TIMER typically will not have
tsc_freq_mhz() implemented and default to UNKNOWN_TSC_RATE. However,
as they don't use TSC for udelay() the slow calibrate_tsc_with_pit()
is avoided.
Because x86/tsc_delay.tsc was using two different guards and nb/via/vx900
claimed UDELAY_TSC, but pulled UDELAY_IO implementation, we also switch
that romstage to use UDELAY_TSC.
Change-Id: I1690cb80295d6b006b75ed69edea28899b674b68
Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
---
M src/arch/x86/cpu.c
M src/arch/x86/timestamp.c
M src/cpu/intel/fsp_model_406dx/Kconfig
M src/cpu/intel/haswell/Kconfig
M src/cpu/intel/model_1067x/Kconfig
M src/cpu/intel/model_106cx/Kconfig
M src/cpu/intel/model_2065x/Kconfig
M src/cpu/intel/model_206ax/Kconfig
M src/cpu/intel/model_6ex/Kconfig
M src/cpu/intel/model_6fx/Kconfig
M src/cpu/intel/slot_1/Kconfig
M src/cpu/intel/socket_mPGA604/Kconfig
M src/cpu/qemu-x86/Kconfig
M src/cpu/via/nano/Kconfig
M src/cpu/x86/Kconfig
M src/cpu/x86/tsc/Makefile.inc
M src/cpu/x86/tsc/delay_tsc.c
M src/drivers/pc80/pc/i8254.c
M src/include/cpu/x86/tsc.h
M src/northbridge/via/vx900/Makefile.inc
M src/soc/amd/picasso/Kconfig
M src/soc/intel/apollolake/Kconfig
M src/soc/intel/baytrail/Kconfig
M src/soc/intel/braswell/Kconfig
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/fsp_baytrail/Kconfig
M src/soc/intel/fsp_broadwell_de/Kconfig
M src/soc/intel/icelake/Kconfig
M src/soc/intel/quark/Kconfig
M src/soc/intel/skylake/Kconfig
32 files changed, 43 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/33928/15
--
To view, visit https://review.coreboot.org/c/coreboot/+/33928
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1690cb80295d6b006b75ed69edea28899b674b68
Gerrit-Change-Number: 33928
Gerrit-PatchSet: 15
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Vanny E <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36527 )
Change subject: timestamps: COLLECT_TIMESTAMPS is mostly optional
......................................................................
Patch Set 4: -Code-Review
(4 comments)
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG@14
PS3, Line 14: the use of get_us_since_boot() with COLLECT_TIMESTAMPS=n.
> The code changes from 'has 5 seconds from power-on elapsed' to 'lets wait here for 5 seconds'.
Done
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG@15
PS3, Line 15: We could implement get_us_since_boot() over monotonic timer.
> Ya. It's partially my fault, I think. […]
Ack
https://review.coreboot.org/c/coreboot/+/36527/3/src/mainboard/intel/galile…
File src/mainboard/intel/galileo/Kconfig:
https://review.coreboot.org/c/coreboot/+/36527/3/src/mainboard/intel/galile…
PS3, Line 109: select COLLECT_TIMESTAMPS
> I'm not sure what the dependency is for CYRPTO_SHIELD necessarily. […]
Done
https://review.coreboot.org/c/coreboot/+/36527/3/src/vendorcode/google/chro…
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/36527/3/src/vendorcode/google/chro…
PS3, Line 25: select COLLECT_TIMESTAMPS
> This needs to stay.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 4
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 18:19:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: comment
Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, HAOUAS Elyes, Julius Werner, Arthur Heymans, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36527
to look at the new patch set (#4).
Change subject: timestamps: COLLECT_TIMESTAMPS is mostly optional
......................................................................
timestamps: COLLECT_TIMESTAMPS is mostly optional
It is a user-visible option and enabled by default, some
consider it as debugging aid only. Therefore platform design
should not depend on it.
It must remain selected with CHROMEOS and boards are allowed
to explicitly select it as well.
For siemens/mc_bdx1,mc_aplX boot time will be increased due
the use of get_us_since_boot() with COLLECT_TIMESTAMPS=n.
When unable to determine if N seconds has elapsed from boot,
this turns into a delay of N seconds.
Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/stoneyridge/Kconfig
M src/soc/intel/apollolake/Kconfig
M src/soc/intel/braswell/Kconfig
M src/soc/intel/skylake/Kconfig
5 files changed, 0 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36527/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 4
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36527 )
Change subject: timestamps: COLLECT_TIMESTAMPS is always optional
......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG@15
PS3, Line 15: We could implement get_us_since_boot() over monotonic timer.
> I doubt if amd/stoneyridge/monotonic_timer.c version does the same. Or intel/quark/tsc_freq.c. […]
Ya. It's partially my fault, I think. My point was that we could use it in that way once we remove that bit of code in src/cpu/x86/tsc/delay_tsc.c and formalize on the semantics.
Current comment in timer.h:
Obtain the current monotonic time. The assumption is that the time counts
* up from the value 0 with value 0 being the point when the timer was
* initialized. Additionally, the timer is assumed to only be valid for the
* duration of the boot.
It's ambiguous, I admit, but we currently make no claims about it continuing to count up across stages. We can definitely do that, though.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 18:05:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: comment
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36527 )
Change subject: timestamps: COLLECT_TIMESTAMPS is always optional
......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG@14
PS3, Line 14: the use of get_us_since_boot() with COLLECT_TIMESTAMPS=n.
> Why is it that there is an increase in those conditions?
The code changes from 'has 5 seconds from power-on elapsed' to 'lets wait here for 5 seconds'.
https://review.coreboot.org/c/coreboot/+/36527/3//COMMIT_MSG@15
PS3, Line 15: We could implement get_us_since_boot() over monotonic timer.
> Currently monotonic timer is relative to the stage. It's not global and free running. […]
I doubt if amd/stoneyridge/monotonic_timer.c version does the same. Or intel/quark/tsc_freq.c. Or nvidia/tegra210/monotonic_timer.c.
Probably something that was never defined?
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 17:59:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36527 )
Change subject: timestamps: COLLECT_TIMESTAMPS is always optional
......................................................................
Patch Set 3:
> Patch Set 3: Code-Review-2
>
> I am unable to deduce the answers to galileo and chromeos.
COLLECT_TIMESTAMPS for Chrome OS needs to stay selected. It's a product decision, and we shouldn't be changing that here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 17:53:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36527 )
Change subject: timestamps: COLLECT_TIMESTAMPS is always optional
......................................................................
Patch Set 3:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > Martin, Aaron, the Chromebooks' coreboot-9999.ebuild may require modifying to accommodate this?
>
> Not exactly. We have COLLECT_TIMESTAMPS under the CHROMEOS Kconfig option in src/vendorcode/google/chromeos/Kconfig so as long as that doesn't change we should be good.
Shoot. I saw that Chrome OS default policy was changed. We can't have that.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36527
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ee4195d266440143344781d39db9578cd8bdcb3
Gerrit-Change-Number: 36527
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 17:53:13 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment