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: Code-Review-2
I am unable to deduce the answers to galileo and chromeos.
--
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:52:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
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:
(1 comment)
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
> What happens when someone decides to change the default to 'n' for COLLECT_TIMESTAMPS going by the s […]
I'm not sure what the dependency is for CYRPTO_SHIELD necessarily. But I think i'm ok given my last comment to Marshall about Chrome OS.
--
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:51:20 +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:
>
> 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.
--
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:47:55 +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:
(3 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?
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. It can't be used for get_us_sinc_boot(). If you nuke all the logic in timer_monotonic_get() from src/cpu/x86/tsc/delay_tsc.c then it will be more free running. But right now the implementation is only giving you microseconds elapsed from the first instance of calling timer_monotonic_get().
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
What happens when someone decides to change the default to 'n' for COLLECT_TIMESTAMPS going by the similar logic that it's only a debugging aid?
--
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:45:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36312 )
Change subject: [TESTME]mb/asus/{p5gc-mx,p5qpl-am}: Use system_reset instead of full_reset
......................................................................
[TESTME]mb/asus/{p5gc-mx,p5qpl-am}: Use system_reset instead of full_reset
The GMCH needs to sample BSEL again after the SuperIO GPIO to
configure BSEL have been set. This happens when RSTIN# is asserted,
which in turn happens when PCIRST# is asserted by the southbridge.
This needs to be tested on hardware.
Change-Id: I246ac15d0d12ffc18686c7d22a9e6c2b43dce535
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/mainboard/asus/p5gc-mx/romstage.c
M src/mainboard/asus/p5qpl-am/romstage.c
2 files changed, 10 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36312/1
diff --git a/src/mainboard/asus/p5gc-mx/romstage.c b/src/mainboard/asus/p5gc-mx/romstage.c
index 20a2b56..0814203 100644
--- a/src/mainboard/asus/p5gc-mx/romstage.c
+++ b/src/mainboard/asus/p5gc-mx/romstage.c
@@ -182,7 +182,11 @@
if (!s3resume && setup_sio_gpio(c_bsel)) {
printk(BIOS_DEBUG,
"Needs reset to configure CPU BSEL straps\n");
- full_reset();
+ /* BSEL is resampled if GMCH RSTIN# is asserted which
+ in it turn gets asserted by PCIRST#. */
+ pci_update_config16(PCI_DEV(0, 0x1e, 0), PCI_BRIDGE_CONTROL, 0,
+ PCI_BRIDGE_CTL_BUS_RESET);
+ system_reset();
}
/* Enable SPD ROMs and DDR-II DRAM */
diff --git a/src/mainboard/asus/p5qpl-am/romstage.c b/src/mainboard/asus/p5qpl-am/romstage.c
index 30480ad..c60b4d2 100644
--- a/src/mainboard/asus/p5qpl-am/romstage.c
+++ b/src/mainboard/asus/p5qpl-am/romstage.c
@@ -184,7 +184,11 @@
if (!s3_resume && setup_sio_gpio()) {
printk(BIOS_DEBUG,
"Needs reset to configure CPU BSEL straps\n");
- full_reset();
+ /* BSEL is resampled if GMCH RSTIN# is asserted which
+ in it turn gets asserted by PCIRST#. */
+ pci_update_config16(PCI_DEV(0, 0x1e, 0), PCI_BRIDGE_CONTROL, 0,
+ PCI_BRIDGE_CTL_BUS_RESET);
+ system_reset();
}
sdram_initialize(boot_path, spd_addrmap);
--
To view, visit https://review.coreboot.org/c/coreboot/+/36312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I246ac15d0d12ffc18686c7d22a9e6c2b43dce535
Gerrit-Change-Number: 36312
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33928 )
Change subject: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG@7
PS14, Line 7: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
> The x86 timers are a mess. […]
Could you please add that info to the commit description?
--
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: 14
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-Comment-Date: Fri, 01 Nov 2019 17:29:51 +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/+/33928 )
Change subject: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG@7
PS14, Line 7: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
> I don't understand this subject nor the reason for removing the explicitness of TSC_CONSTANT_RATE. […]
The x86 timers are a mess. Different stages using different counters, timestamps using different counters from udelays..
The original intention was to only flip TSC_CONSTANT_RATE Kconfig to NO_TSC_CONSTANT_RATE. The name would be incorrect, those TSCs do run with a constant rate but we just lack the implementation of tsc_freq_mhz() on three platforms.
It then got more complicated because x86/tsc_delay.tsc was using two different guards, with nb/via/vx900 claiming UDELAY_TSC but pulling in udelay_io.c for romstage without guards at all.
The net result here is we should always define tsc_freq_mhz() and thus get rid of the couple remaining UNKNOWN_TSC_RATE cases.
--
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: 14
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-Comment-Date: Fri, 01 Nov 2019 17:25:23 +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/+/33928 )
Change subject: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/33928/14//COMMIT_MSG@7
PS14, Line 7: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
I don't understand this subject nor the reason for removing the explicitness of TSC_CONSTANT_RATE. Are you wanting to ensure the default now is semantically equivalent to TSC_CONSTANT_RATE with only the platforms not having that select UNKNOWN_TSC_RATE?
https://review.coreboot.org/c/coreboot/+/33928/14/src/cpu/x86/tsc/delay_tsc…
File src/cpu/x86/tsc/delay_tsc.c:
https://review.coreboot.org/c/coreboot/+/33928/14/src/cpu/x86/tsc/delay_tsc…
PS14, Line 23: (void)tsc_freq_mhz();
Wouldn't we want the caching in one place?
--
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: 14
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-Comment-Date: Fri, 01 Nov 2019 16:54:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment