Change in coreboot[master]: soc/amd: factor out common family 17h&19h TSC and monotonic timer code

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

soc/amd: factor out common family 17h&19h TSC and monotonic timer code

The corresponding MSRs of all AMD family 17h and 19h CPUs/APUs match the
code.

Change-Id: I29cfef5d8920c29e36c55fc46a90eb579a042b64
Signed-off-by: Felix Held 
---
A src/soc/amd/common/block/tsc/Kconfig
A src/soc/amd/common/block/tsc/Makefile.inc
R src/soc/amd/common/block/tsc/monotonic_timer.c
R src/soc/amd/common/block/tsc/tsc_freq.c
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
6 files changed, 31 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/48305/1

diff --git a/src/soc/amd/common/block/tsc/Kconfig b/src/soc/amd/common/block/tsc/Kconfig
new file mode 100644
index 0000000..49f2589
--- /dev/null
+++ b/src/soc/amd/common/block/tsc/Kconfig
@@ -0,0 +1,10 @@
+config SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
+	bool
+	select COLLECT_TIMESTAMPS_NO_TSC # selected use SoC-specific timestamp function
+	select TSC_SYNC_LFENCE
+	select UDELAY_TSC
+	default n
+	help
+	  Select this option to add the common functions for getting the TSC
+	  frequency of AMD family 17h and 19h CPUs/APUs and to provide TSC-
+	  based monotonic timer functionality to the build.
diff --git a/src/soc/amd/common/block/tsc/Makefile.inc b/src/soc/amd/common/block/tsc/Makefile.inc
new file mode 100644
index 0000000..75c6061
--- /dev/null
+++ b/src/soc/amd/common/block/tsc/Makefile.inc
@@ -0,0 +1,20 @@
+ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H),y)
+
+subdirs-y += ../../../../../cpu/x86/tsc
+
+bootblock-y += tsc_freq.c
+bootblock-y += monotonic_timer.c
+
+verstage_x86-y += tsc_freq.c
+verstage_x86-y += monotonic_timer.c
+
+romstage-y += tsc_freq.c
+romstage-y += monotonic_timer.c
+
+ramstage-y += tsc_freq.c
+ramstage-y += monotonic_timer.c
+
+smm-y += tsc_freq.c
+smm-y += monotonic_timer.c
+
+endif # CONFIG_SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
diff --git a/src/soc/amd/picasso/monotonic_timer.c b/src/soc/amd/common/block/tsc/monotonic_timer.c
similarity index 100%
rename from src/soc/amd/picasso/monotonic_timer.c
rename to src/soc/amd/common/block/tsc/monotonic_timer.c
diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/common/block/tsc/tsc_freq.c
similarity index 100%
rename from src/soc/amd/picasso/tsc_freq.c
rename to src/soc/amd/common/block/tsc/tsc_freq.c
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index 2ac1235..79fc3be 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -24,10 +24,7 @@
    select IOAPIC
    select HAVE_EM100_SUPPORT
    select HAVE_USBDEBUG_OPTIONS
-	select COLLECT_TIMESTAMPS_NO_TSC
    select SOC_AMD_COMMON_BLOCK_SPI
-	select TSC_SYNC_LFENCE
-	select UDELAY_TSC
    select SOC_AMD_COMMON
    select SOC_AMD_COMMON_BLOCK_NONCAR
    select SOC_AMD_COMMON_BLOCK_HAS_ESPI
@@ -44,6 +41,7 @@
    select SOC_AMD_COMMON_BLOCK_SMBUS
    select SOC_AMD_COMMON_BLOCK_SMI
    select SOC_AMD_COMMON_BLOCK_SMU
+	select SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
    select SOC_AMD_COMMON_BLOCK_PSP_GEN2
    select PROVIDES_ROM_SHARING
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index c772783..d86d97e 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -3,7 +3,6 @@
 ifeq ($(CONFIG_SOC_AMD_PICASSO),y)

subdirs-y += ../../../cpu/amd/mtrr/
-subdirs-y += ../../../cpu/x86/tsc
 subdirs-y += ../../../cpu/x86/lapic
 subdirs-y += ../../../cpu/x86/cache
 subdirs-y += ../../../cpu/x86/mtrr
@@ -17,8 +16,6 @@
 bootblock-y += i2c.c
 bootblock-y += uart.c
 bootblock-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-bootblock-y += monotonic_timer.c
-bootblock-y += tsc_freq.c
 bootblock-y += gpio.c
 bootblock-y += config.c
 bootblock-y += reset.c
@@ -30,8 +27,6 @@
 romstage-y += memmap.c
 romstage-y += uart.c
 romstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-romstage-y += monotonic_timer.c
-romstage-y += tsc_freq.c
 romstage-y += aoac.c
 romstage-y += southbridge.c
 romstage-y += psp.c
@@ -44,8 +39,6 @@
 verstage_x86-y += gpio.c
 verstage_x86-y += uart.c
 verstage_x86-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-verstage_x86-y += monotonic_timer.c
-verstage_x86-y += tsc_freq.c
 verstage_x86-y += reset.c

ramstage-y += i2c.c
@@ -66,8 +59,6 @@
 ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c
 ramstage-y += uart.c
 ramstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-ramstage-y += monotonic_timer.c
-ramstage-y += tsc_freq.c
 ramstage-y += finalize.c
 ramstage-y += soc_util.c
 ramstage-y += psp.c
@@ -80,8 +71,6 @@
 ramstage-y += dmi.c

smm-y += smihandler.c
-smm-y += monotonic_timer.c
-smm-y += tsc_freq.c
 ifeq ($(CONFIG_DEBUG_SMI),y)
 smm-y += uart.c
 smm-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

Patch Set 1:

(1 comment)

Could consider putting under block/cpu instead of a new block.

Eventually we could also point other devices to use similar code, with different decoding of the p-state register, and subject to the various limitations (e.g. Stoney's TSC frequency changes after a ucode patch, and IIRC Kyõsti had measured some differences in Family 14h and Family 16h).  But, I'm not sure whether that might change your Kconfig symbol.

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
File src/soc/amd/common/block/tsc/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
PS1, Line 7: 
Use all-y instead?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
File src/soc/amd/common/block/tsc/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
PS1, Line 7:
> Use all-y instead?

we can't use that for verstage on psp. i'll have a look if some other guard will prevent things from breaking then

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Patrick Georgi, Martin Roth, Marshall Dawson,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48305

to look at the new patch set (#2).

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

soc/amd: factor out common family 17h&19h TSC and monotonic timer code

The corresponding MSRs of all AMD family 17h and 19h CPUs/APUs match the
code.

Change-Id: I29cfef5d8920c29e36c55fc46a90eb579a042b64
Signed-off-by: Felix Held 
---
M src/soc/amd/common/block/cpu/Kconfig
A src/soc/amd/common/block/cpu/tsc/Makefile.inc
R src/soc/amd/common/block/cpu/tsc/monotonic_timer.c
R src/soc/amd/common/block/cpu/tsc/tsc_freq.c
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
6 files changed, 32 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/48305/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

Patch Set 2:

(1 comment)
> Patch Set 1:
>
> (1 comment)
>
> Could consider putting under block/cpu instead of a new block.
>
> Eventually we could also point other devices to use similar code, with different decoding of the p-state register, and subject to the various limitations (e.g. Stoney's TSC frequency changes after a ucode patch, and IIRC Kyõsti had measured some differences in Family 14h and Family 16h).  But, I'm not sure whether that might change your Kconfig symbol.

good idea. i've updated the patch and added a patch in between to prepare soc/amd/common/block/cpu for this

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
File src/soc/amd/common/block/tsc/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
PS1, Line 7:
> we can't use that for verstage on psp. […]

there is currently no guard for that and i don't think that we should add clutter to coreboot-common code due to verstage on psp slightly breaking some assumptions

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
File src/soc/amd/common/block/tsc/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/48305/1/src/soc/amd/common/block/ts... 
PS1, Line 7:
> there is currently no guard for that and i don't think that we should add clutter to coreboot-common […]

Good point on verstage on psp.

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48305 )

Change subject: soc/amd: factor out common family 17h&19h TSC and monotonic timer code
......................................................................

soc/amd: factor out common family 17h&19h TSC and monotonic timer code

The corresponding MSRs of all AMD family 17h and 19h CPUs/APUs match the
code.

Change-Id: I29cfef5d8920c29e36c55fc46a90eb579a042b64
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48305
Tested-by: build bot (Jenkins) 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/common/block/cpu/Kconfig
A src/soc/amd/common/block/cpu/tsc/Makefile.inc
R src/soc/amd/common/block/cpu/tsc/monotonic_timer.c
R src/soc/amd/common/block/cpu/tsc/tsc_freq.c
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
6 files changed, 32 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marshall Dawson: Looks good to me, approved

diff --git a/src/soc/amd/common/block/cpu/Kconfig b/src/soc/amd/common/block/cpu/Kconfig
index 826f80b..c155975 100644
--- a/src/soc/amd/common/block/cpu/Kconfig
+++ b/src/soc/amd/common/block/cpu/Kconfig
@@ -27,3 +27,14 @@
    default "src/soc/amd/common/block/cpu/noncar/memlayout.ld"

endif # SOC_AMD_COMMON_BLOCK_NONCAR
+
+config SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
+	bool
+	select COLLECT_TIMESTAMPS_NO_TSC # selected use SoC-specific timestamp function
+	select TSC_SYNC_LFENCE
+	select UDELAY_TSC
+	default n
+	help
+	  Select this option to add the common functions for getting the TSC
+	  frequency of AMD family 17h and 19h CPUs/APUs and to provide TSC-
+	  based monotonic timer functionality to the build.
diff --git a/src/soc/amd/common/block/cpu/tsc/Makefile.inc b/src/soc/amd/common/block/cpu/tsc/Makefile.inc
new file mode 100644
index 0000000..4f2729b
--- /dev/null
+++ b/src/soc/amd/common/block/cpu/tsc/Makefile.inc
@@ -0,0 +1,20 @@
+ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H),y)
+
+subdirs-y += ../../../../../../cpu/x86/tsc
+
+bootblock-y += tsc_freq.c
+bootblock-y += monotonic_timer.c
+
+verstage_x86-y += tsc_freq.c
+verstage_x86-y += monotonic_timer.c
+
+romstage-y += tsc_freq.c
+romstage-y += monotonic_timer.c
+
+ramstage-y += tsc_freq.c
+ramstage-y += monotonic_timer.c
+
+smm-y += tsc_freq.c
+smm-y += monotonic_timer.c
+
+endif # CONFIG_SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
diff --git a/src/soc/amd/picasso/monotonic_timer.c b/src/soc/amd/common/block/cpu/tsc/monotonic_timer.c
similarity index 100%
rename from src/soc/amd/picasso/monotonic_timer.c
rename to src/soc/amd/common/block/cpu/tsc/monotonic_timer.c
diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/common/block/cpu/tsc/tsc_freq.c
similarity index 100%
rename from src/soc/amd/picasso/tsc_freq.c
rename to src/soc/amd/common/block/cpu/tsc/tsc_freq.c
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index 2ac1235..79fc3be 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -24,10 +24,7 @@
    select IOAPIC
    select HAVE_EM100_SUPPORT
    select HAVE_USBDEBUG_OPTIONS
-	select COLLECT_TIMESTAMPS_NO_TSC
    select SOC_AMD_COMMON_BLOCK_SPI
-	select TSC_SYNC_LFENCE
-	select UDELAY_TSC
    select SOC_AMD_COMMON
    select SOC_AMD_COMMON_BLOCK_NONCAR
    select SOC_AMD_COMMON_BLOCK_HAS_ESPI
@@ -44,6 +41,7 @@
    select SOC_AMD_COMMON_BLOCK_SMBUS
    select SOC_AMD_COMMON_BLOCK_SMI
    select SOC_AMD_COMMON_BLOCK_SMU
+	select SOC_AMD_COMMON_BLOCK_TSC_FAM17H_19H
    select SOC_AMD_COMMON_BLOCK_PSP_GEN2
    select PROVIDES_ROM_SHARING
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index c772783..d86d97e 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -3,7 +3,6 @@
 ifeq ($(CONFIG_SOC_AMD_PICASSO),y)

subdirs-y += ../../../cpu/amd/mtrr/
-subdirs-y += ../../../cpu/x86/tsc
 subdirs-y += ../../../cpu/x86/lapic
 subdirs-y += ../../../cpu/x86/cache
 subdirs-y += ../../../cpu/x86/mtrr
@@ -17,8 +16,6 @@
 bootblock-y += i2c.c
 bootblock-y += uart.c
 bootblock-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-bootblock-y += monotonic_timer.c
-bootblock-y += tsc_freq.c
 bootblock-y += gpio.c
 bootblock-y += config.c
 bootblock-y += reset.c
@@ -30,8 +27,6 @@
 romstage-y += memmap.c
 romstage-y += uart.c
 romstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-romstage-y += monotonic_timer.c
-romstage-y += tsc_freq.c
 romstage-y += aoac.c
 romstage-y += southbridge.c
 romstage-y += psp.c
@@ -44,8 +39,6 @@
 verstage_x86-y += gpio.c
 verstage_x86-y += uart.c
 verstage_x86-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-verstage_x86-y += monotonic_timer.c
-verstage_x86-y += tsc_freq.c
 verstage_x86-y += reset.c

ramstage-y += i2c.c
@@ -66,8 +59,6 @@
 ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c
 ramstage-y += uart.c
 ramstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c
-ramstage-y += monotonic_timer.c
-ramstage-y += tsc_freq.c
 ramstage-y += finalize.c
 ramstage-y += soc_util.c
 ramstage-y += psp.c
@@ -80,8 +71,6 @@
 ramstage-y += dmi.c

smm-y += smihandler.c
-smm-y += monotonic_timer.c
-smm-y += tsc_freq.c
 ifeq ($(CONFIG_DEBUG_SMI),y)
 smm-y += uart.c
 smm-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c