Change in coreboot[master]: src/soc/intel/denverton_ns: Update to several improvement in coreboot...

[Julien Viard de Galbert (Code Review)]

Julien Viard de Galbert has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Update to several improvement in coreboot since 4.9
......................................................................

src/soc/intel/denverton_ns: Update to several improvement in coreboot since 4.9

Signed-off-by: Julien Viard de Galbert 
Change-Id: Ibc5ce91118c6052af23642fb3461f574cd888dea
---
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/Makefile.inc
2 files changed, 22 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/47340/1

diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index 89bbbb0..888713a 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -7,6 +7,10 @@

if SOC_INTEL_DENVERTON_NS

+config CPU_INTEL_NUM_FIT_ENTRIES
+	int
+	default 1
+
 config CPU_SPECIFIC_OPTIONS
    def_bool y
    select ARCH_ALL_STAGES_X86_32
@@ -21,6 +25,7 @@
    select CACHE_MRC_SETTINGS
    select PARALLEL_MP
    select PCR_COMMON_IOSF_1_0
+	select SUPPORT_CPU_UCODE_IN_CBFS
    select INTEL_DESCRIPTOR_MODE_CAPABLE
    select SOC_INTEL_COMMON_BLOCK
    select SOC_INTEL_COMMON_BLOCK_CPU
@@ -153,10 +158,14 @@
    hex
    default 0x8000

-config DENVERTON_NS_CAR_NEM_ENHANCED
+choice
+	prompt "Cache-as-ram implementation"
+	default USE_DENVERTON_NS_CAR_NEM_ENHANCED
+	help
+	  This option allows you to select how cache-as-ram (CAR) is set up.
+
+config USE_DENVERTON_NS_CAR_NEM_ENHANCED
    bool "Enhanced Non-evict mode"
-	depends on !FSP_CAR
-	default y
    select SOC_INTEL_COMMON_BLOCK_CAR
    select USE_CAR_NEM_ENHANCED_V1
    help
@@ -167,4 +176,12 @@
      ENHANCED NEM guarantees that modified data is always
      kept in cache while clean data is replaced.

+config USE_DENVERTON_NS_FSP_CAR
+	bool "Use FSP CAR"
+	select FSP_CAR
+	help
+	  Use FSP APIs to initialize and tear down the Cache-As-Ram.
+
+endchoice
+
 endif ## SOC_INTEL_DENVERTON_NS
diff --git a/src/soc/intel/denverton_ns/Makefile.inc b/src/soc/intel/denverton_ns/Makefile.inc
index 105f866..c854037 100644
--- a/src/soc/intel/denverton_ns/Makefile.inc
+++ b/src/soc/intel/denverton_ns/Makefile.inc
@@ -2,6 +2,8 @@

ifeq ($(CONFIG_SOC_INTEL_DENVERTON_NS),y)

+cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-5f-01
+
 subdirs-y += ../../../cpu/intel/microcode
 subdirs-y += ../../../cpu/intel/turbo
 subdirs-y += ../../../cpu/x86/lapic

[Julien Viard de Galbert (Code Review)]

Julien Viard de Galbert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Update to several improvement in coreboot since 4.9
......................................................................

Patch Set 1:

This is basically the changes that I needed to have my board build and start again with coreboot master.

It includes:
 - re-enabling FSP CAR (I didn't check why the alternative didn't work yet)
 - enabling microcode in cbfs (which is actually required)
 - loading the microcode from the 3rdparty repo

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Julien Viard de Galbert (Code Review)]

Hello build bot (Jenkins), David Guckian, Patrick Georgi, Martin Roth, Vanessa Eusebio, Patrick Rudolph,

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

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

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

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

Signed-off-by: Julien Viard de Galbert 
Change-Id: Ibc5ce91118c6052af23642fb3461f574cd888dea
---
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/Makefile.inc
2 files changed, 25 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/47340/2

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Felix Singer (Code Review)]

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

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/47340/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47340/2//COMMIT_MSG@8 
PS2, Line 8: 
Maybe sum up the changes here

https://review.coreboot.org/c/coreboot/+/47340/2/src/soc/intel/denverton_ns/... 
File src/soc/intel/denverton_ns/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47340/2/src/soc/intel/denverton_ns/... 
PS2, Line 5: cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-5f-01
Can be removed. Same as in line 84.

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Julien Viard de Galbert (Code Review)]

Hello  Felix Singer, build bot (Jenkins), David Guckian, Patrick Georgi, Martin Roth, Vanessa Eusebio, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

- enable microcode in cbfs (won't boot without microcode)
 - force num fit entry to 1 to avoid crash in cbfstool/fit.c
 - re-enable FSP-CAR (tested to boot, while I couldn't boot with NEM)
 - enable io driver for uart in legacy mode (ie emulating legacy port by
   configuring the pci to legacy io address and hiding the pci device)

Signed-off-by: Julien Viard de Galbert 
Change-Id: Ibc5ce91118c6052af23642fb3461f574cd888dea
---
M src/soc/intel/denverton_ns/Kconfig
1 file changed, 23 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/47340/3

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Julien Viard de Galbert (Code Review)]

Julien Viard de Galbert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3:

(2 comments)

Thanks for the review !

https://review.coreboot.org/c/coreboot/+/47340/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47340/2//COMMIT_MSG@8 
PS2, Line 8:
> Maybe sum up the changes here

Done

https://review.coreboot.org/c/coreboot/+/47340/2/src/soc/intel/denverton_ns/... 
File src/soc/intel/denverton_ns/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47340/2/src/soc/intel/denverton_ns/... 
PS2, Line 5: cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-5f-01
> Can be removed. Same as in line 84.

oh, as the microcode was not enabled I didn't see this was already in, thanks!

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Felix Singer (Code Review)]

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

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Mariusz Szafrański (Code Review)]

Mariusz Szafrański has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3: Code-Review+1

There are issues with setting NEM on Denverton platform reliable.
FSP-CAR works correctly and reliable so it the suggested method to setup CAR.
NEM could be removed in the future.
The same for serial legacy/io mode - There is no easy way to make it 100% legacy.
So I`m thinking about removing it and leave only serial mem mode.

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+1
>
> There are issues with setting NEM on Denverton platform reliable.
> FSP-CAR works correctly and reliable so it the suggested method to setup CAR.
> NEM could be removed in the future.

Um, no. If coreboot CAR init isn't working well on Denverton, the expected approach is to investigate why. Given that APL uses CQOS, maybe that's what DNV should also be using instead?

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Mariusz Szafrański (Code Review)]

Mariusz Szafrański has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

Patch Set 3:
> Patch Set 3:
> > Patch Set 3: Code-Review+1
> >
> > There are issues with setting NEM on Denverton platform reliable.
> > FSP-CAR works correctly and reliable so it the suggested method to setup CAR.
> > NEM could be removed in the future.
>
> Um, no. If coreboot CAR init isn't working well on Denverton, the expected approach is to investigate why. Given that APL uses CQOS, maybe that's what DNV should also be using instead?

To be clear. In the past I`ve only deeply checked setting temp ram using FSP-T TempRamInit call. And can confirm that this methods works correctly and is reliable on DNV. So my suggestion to use it instead of others.

Change in coreboot[master]: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47340 )

Change subject: src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9
......................................................................

src/soc/intel/denverton_ns: Use improvement in coreboot since 4.9

- enable microcode in cbfs (won't boot without microcode)
 - force num fit entry to 1 to avoid crash in cbfstool/fit.c
 - re-enable FSP-CAR (tested to boot, while I couldn't boot with NEM)
 - enable io driver for uart in legacy mode (ie emulating legacy port by
   configuring the pci to legacy io address and hiding the pci device)

Signed-off-by: Julien Viard de Galbert 
Change-Id: Ibc5ce91118c6052af23642fb3461f574cd888dea
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47340
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by:  Felix Singer 
Reviewed-by: Mariusz Szafrański 
---
M src/soc/intel/denverton_ns/Kconfig
1 file changed, 23 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Mariusz Szafrański: Looks good to me, but someone else must approve
   Felix Singer: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index 89bbbb0..4981205 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -7,6 +7,10 @@

if SOC_INTEL_DENVERTON_NS

+config CPU_INTEL_NUM_FIT_ENTRIES
+	int
+	default 1
+
 config CPU_SPECIFIC_OPTIONS
    def_bool y
    select ARCH_ALL_STAGES_X86_32
@@ -21,6 +25,7 @@
    select CACHE_MRC_SETTINGS
    select PARALLEL_MP
    select PCR_COMMON_IOSF_1_0
+	select SUPPORT_CPU_UCODE_IN_CBFS
    select INTEL_DESCRIPTOR_MODE_CAPABLE
    select SOC_INTEL_COMMON_BLOCK
    select SOC_INTEL_COMMON_BLOCK_CPU
@@ -134,6 +139,9 @@
    bool "Legacy Mode"
    help
      Enable legacy UART mode
+	select CONSOLE_SERIAL
+	select DRIVERS_UART
+	select DRIVERS_UART_8250IO
 endchoice

config ENABLE_HSUART
@@ -153,10 +161,14 @@
    hex
    default 0x8000

-config DENVERTON_NS_CAR_NEM_ENHANCED
+choice
+	prompt "Cache-as-ram implementation"
+	default USE_DENVERTON_NS_CAR_NEM_ENHANCED
+	help
+	  This option allows you to select how cache-as-ram (CAR) is set up.
+
+config USE_DENVERTON_NS_CAR_NEM_ENHANCED
    bool "Enhanced Non-evict mode"
-	depends on !FSP_CAR
-	default y
    select SOC_INTEL_COMMON_BLOCK_CAR
    select USE_CAR_NEM_ENHANCED_V1
    help
@@ -167,4 +179,12 @@
      ENHANCED NEM guarantees that modified data is always
      kept in cache while clean data is replaced.

+config USE_DENVERTON_NS_FSP_CAR
+	bool "Use FSP CAR"
+	select FSP_CAR
+	help
+	  Use FSP APIs to initialize and tear down the Cache-As-Ram.
+
+endchoice
+
 endif ## SOC_INTEL_DENVERTON_NS