Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36570 )
Change subject: soc/intel/common: pmclib: make use of the new ETR address API
......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block…
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block…
PS4, Line 432: void pmc_global_reset_lock(void)
: {
: uint32_t *etr = soc_read_pmc_etr_addr();
: uint32_t reg;
:
: reg = read32(etr);
: reg = (reg & ~CF9_GLB_RST) | CF9_LOCK;
: write32(etr, reg);
: }
> > besides that, why duplicate code again and again in soc/ when we can have a common function? […]
you can put anything on one line if you like to ;) I agree on the first one. The second one is not good coding style I'd say; but we could add a update32 macro, to do the same as with pci.
More important, where do you draw the line when something should be put to common? I would say the goal is to have as much in common code as possible and consilidate most code sooner or later. I've seen much code that could be moved from the socs to common, but that's not easy when some guys always only copypasta code :/
--
To view, visit https://review.coreboot.org/c/coreboot/+/36570
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I49f59efb4a7c7d3d629ac54a7922bbcc8a87714d
Gerrit-Change-Number: 36570
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner
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: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sun, 03 Nov 2019 08:37:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Michael Niewöhner
Gerrit-MessageType: comment
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36589
to look at the new patch set (#3).
Change subject: cpu/x86/smm: Add helper functions to deal with different save states
......................................................................
cpu/x86/smm: Add helper functions to deal with different save states
This abstracts the different save_states to get some common registers.
Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/cpu/x86/smm/Makefile.inc
A src/cpu/x86/smm/smihelper.c
M src/include/cpu/x86/smm.h
3 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36589/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/36589
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15
Gerrit-Change-Number: 36589
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36589
to look at the new patch set (#2).
Change subject: cpu/x86/smm: Add helper functions to deal with different save states
......................................................................
cpu/x86/smm: Add helper functions to deal with different save states
This abstracts the different save_states to get some common registers.
Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/cpu/x86/smm/Makefile.inc
A src/cpu/x86/smm/smihelper.c
M src/include/cpu/x86/smm.h
3 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36589/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/36589
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15
Gerrit-Change-Number: 36589
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36570 )
Change subject: soc/intel/common: pmclib: make use of the new ETR address API
......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block…
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block…
PS4, Line 434: soc_read_pmc_etr_addr();
Check for NULL and unaligned?
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block…
PS4, Line 432: void pmc_global_reset_lock(void)
: {
: uint32_t *etr = soc_read_pmc_etr_addr();
: uint32_t reg;
:
: reg = read32(etr);
: reg = (reg & ~CF9_GLB_RST) | CF9_LOCK;
: write32(etr, reg);
: }
> besides that, why duplicate code again and again in soc/ when we can have a common function?
If you go through the complexity of implementing a callback to fetch an offset and do sanity check on it, just to clear and set a few bits at that offset it might as well be done per soc. That likely ends up with less code.
if it's PCI config register it is one line:
pci_update_config32(..., ETR, ~CF9_GLB_RST, CF9_LOCK);
if it's a MMIO reg it's 2 lines:
uint8_t *base = get_base_...
write32(base + ETR, (read32(base + ETR) & ~CF9_GLB_RST) | CF9_LOCK);
--
To view, visit https://review.coreboot.org/c/coreboot/+/36570
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I49f59efb4a7c7d3d629ac54a7922bbcc8a87714d
Gerrit-Change-Number: 36570
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner
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: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sun, 03 Nov 2019 06:23:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Michael Niewöhner
Gerrit-MessageType: comment
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36537 )
Change subject: intel/broadwell: Switch to TSC_MONOTONIC_TIMER
......................................................................
intel/broadwell: Switch to TSC_MONOTONIC_TIMER
Change-Id: I01b73e20c8af1b00175dc6d9ee56e6b33ac5768d
Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
---
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/broadwell/Makefile.inc
D src/soc/intel/broadwell/monotonic_timer.c
3 files changed, 1 insertion(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/36537/1
diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig
index fadbb41..0bbb668 100644
--- a/src/soc/intel/broadwell/Kconfig
+++ b/src/soc/intel/broadwell/Kconfig
@@ -29,6 +29,7 @@
select SSE2
select TSC_SYNC_MFENCE
select UDELAY_TSC
+ select TSC_MONOTONIC_TIMER
select SOC_INTEL_COMMON
select INTEL_DESCRIPTOR_MODE_CAPABLE
select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE
diff --git a/src/soc/intel/broadwell/Makefile.inc b/src/soc/intel/broadwell/Makefile.inc
index 91a3da0..055a004 100644
--- a/src/soc/intel/broadwell/Makefile.inc
+++ b/src/soc/intel/broadwell/Makefile.inc
@@ -39,11 +39,6 @@
romstage-y += memmap.c
postcar-y += memmap.c
ramstage-y += minihd.c
-bootblock-y += monotonic_timer.c
-romstage-y += monotonic_timer.c
-postcar-y += monotonic_timer.c
-ramstage-y += monotonic_timer.c
-smm-y += monotonic_timer.c
ramstage-y += pch.c
romstage-y += pch.c
ramstage-y += pcie.c
diff --git a/src/soc/intel/broadwell/monotonic_timer.c b/src/soc/intel/broadwell/monotonic_timer.c
deleted file mode 100644
index 84eade8..0000000
--- a/src/soc/intel/broadwell/monotonic_timer.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * This file is part of the coreboot project.
- *
- * Copyright (C) 2014 Google 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
- * the Free Software Foundation; version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- */
-
-#include <stdint.h>
-#include <cpu/x86/msr.h>
-#include <timer.h>
-#include <soc/msr.h>
-
-static struct monotonic_counter {
- int initialized;
- struct mono_time time;
- uint32_t last_value;
-} mono_counter;
-
-static inline uint32_t read_counter_msr(void)
-{
- /* Even though the MSR is 64-bit it is assumed that the hardware
- * is polled frequently enough to only use the lower 32-bits. */
- msr_t counter_msr;
-
- counter_msr = rdmsr(MSR_COUNTER_24_MHZ);
-
- return counter_msr.lo;
-}
-
-void timer_monotonic_get(struct mono_time *mt)
-{
- uint32_t current_tick;
- uint32_t usecs_elapsed;
-
- if (!mono_counter.initialized) {
- mono_counter.last_value = read_counter_msr();
- mono_counter.initialized = 1;
- }
-
- current_tick = read_counter_msr();
- usecs_elapsed = (current_tick - mono_counter.last_value) / 24;
-
- /* Update current time and tick values only if a full tick occurred. */
- if (usecs_elapsed) {
- mono_time_add_usecs(&mono_counter.time, usecs_elapsed);
- mono_counter.last_value = current_tick;
- }
-
- /* Save result. */
- *mt = mono_counter.time;
-}
--
To view, visit https://review.coreboot.org/c/coreboot/+/36537
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I01b73e20c8af1b00175dc6d9ee56e6b33ac5768d
Gerrit-Change-Number: 36537
Gerrit-PatchSet: 1
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: newchange