Change in ...coreboot[master]: src/soc/intel/apollolake/cpu.c: Set up local APIC

[Mario Scheithauer (Code Review)]

Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31802

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

src/soc/intel/apollolake/cpu.c: Set up local APIC

Some Apollo Lake mainboards use SeaBIOS as payload. SeaBIOS requires the
initialization of the programmable interrupt controller (PIC) for
faultless operation. Therefore add setup_lapic() to configure the APIC.

Change-Id: I00b339ce1850729023db74da7f8845927a95dcc6
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/31802/1

diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index a08f1f0..7d42d6b 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -2,7 +2,7 @@
  * This file is part of the coreboot project.
  *
  * Copyright (C) 2015-2017 Intel Corp.
- * Copyright (C) 2017 Siemens AG, Inc.
+ * Copyright (C) 2017-2019 Siemens AG
  * (Written by Andrey Petrov  for Intel Corp.)
  * (Written by Alexandru Gagniuc  for Intel Corp.)
  *
@@ -23,6 +23,7 @@
 #include "chip.h"
 #include <cpu/cpu.h>
 #include <cpu/x86/cache.h>
+#include <cpu/x86/lapic.h>
 #include <cpu/x86/mp.h>
 #include <cpu/intel/microcode.h>
 #include <cpu/intel/turbo.h>
@@ -161,6 +162,10 @@
    }
    x86_setup_mtrrs_with_detect();
    x86_mtrr_check();
+
+	/* Enable the local CPU apics */
+	if (IS_ENABLED(CONFIG_PAYLOAD_SEABIOS))
+		setup_lapic();
 }

#if !IS_ENABLED(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU_MPINIT)

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 1: Code-Review+1

Why not always initialize it?

[Johannes Hahn (Code Review)]

Johannes Hahn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31802/1/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31802/1/src/soc/intel/apollolake/cpu.c@167 
PS1, Line 167: CONFIG_PAYLOAD_SEABIOS
May be you should remark that this KConfig switch is only set when integrating SeaBios during the coreboot build process implicitly. 
When building SeaBios explicitly and adding it as ELF executeable payload CONFIG_PAYLOAD_SEABIOS has to be set also explicitly.

[Mario Scheithauer (Code Review)]

Hello Johannes Hahn, Patrick Rudolph, Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

src/soc/intel/apollolake/cpu.c: Set up local APIC

Some Apollo Lake mainboards use SeaBIOS as payload. SeaBIOS requires the
initialization of the programmable interrupt controller (PIC) for
faultless operation. Therefore add setup_lapic() to configure the APIC.

Change-Id: I00b339ce1850729023db74da7f8845927a95dcc6
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/31802/2

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> Why not always initialize it?

Johannes wrote it a little later...of course, you can also include SeaBIOS as an ELF executable and then we don’t have the switch. I removed the switch.

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31802/1/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31802/1/src/soc/intel/apollolake/cpu.c@167 
PS1, Line 167: CONFIG_PAYLOAD_SEABIOS
> May be you should remark that this KConfig switch is only set when integrating SeaBios during the co […]

You're right. I removed the switch.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG@11 
PS2, Line 11: faultless
Please add the details on the error.

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG@12 
PS2, Line 12: 
Tested how?

[Mario Scheithauer (Code Review)]

Hello Johannes Hahn, Patrick Rudolph, Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

src/soc/intel/apollolake/cpu.c: Set up local APIC

Some Apollo Lake mainboards use SeaBIOS as payload. SeaBIOS requires the
initialization of the programmable interrupt controller (PIC) for
faultless operation. The PIC mode is need for USB support (e.g.
keyboard, memory stick) and for some Option ROMs (e.g. PXE ROM).
Therefore add setup_lapic() to configure the APIC.

Change-Id: I00b339ce1850729023db74da7f8845927a95dcc6
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/31802/3

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG@11 
PS2, Line 11: faultless
> Please add the details on the error.

Done

https://review.coreboot.org/#/c/31802/2//COMMIT_MSG@12 
PS2, Line 12:
> Tested how?

Hi Paul, I think with the addition in the description, the test clears itself as well.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 3: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 3:

lol, I was about to mention that we usually do this on all
cores and not just the BSP. The comments in `cpu/x86/
lapic/lapic.c` convinced me otherwise ;)

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31802/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31802/3/src/soc/intel/apollolake/cpu.c@5 
PS3, Line 5:  * Copyright (C) 2017-2019 Siemens AG
This `2017-2019` implies that you claim to have added something in 2018, too.

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

src/soc/intel/apollolake/cpu.c: Set up local APIC

Some Apollo Lake mainboards use SeaBIOS as payload. SeaBIOS requires the
initialization of the programmable interrupt controller (PIC) for
faultless operation. The PIC mode is need for USB support (e.g.
keyboard, memory stick) and for some Option ROMs (e.g. PXE ROM).
Therefore add setup_lapic() to configure the APIC.

Change-Id: I00b339ce1850729023db74da7f8845927a95dcc6
Signed-off-by: Mario Scheithauer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31802
Reviewed-by: Patrick Rudolph 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 5 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index 741e08c..8f1d933 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -2,7 +2,7 @@
  * This file is part of the coreboot project.
  *
  * Copyright (C) 2015-2017 Intel Corp.
- * Copyright (C) 2017 Siemens AG, Inc.
+ * Copyright (C) 2017-2019 Siemens AG
  * (Written by Andrey Petrov  for Intel Corp.)
  * (Written by Alexandru Gagniuc  for Intel Corp.)
  *
@@ -23,6 +23,7 @@
 #include "chip.h"
 #include <cpu/cpu.h>
 #include <cpu/x86/cache.h>
+#include <cpu/x86/lapic.h>
 #include <cpu/x86/mp.h>
 #include <cpu/intel/microcode.h>
 #include <cpu/intel/turbo.h>
@@ -161,6 +162,9 @@
    }
    x86_setup_mtrrs_with_detect();
    x86_mtrr_check();
+
+	/* Enable the local CPU apics */
+	setup_lapic();
 }

#if !CONFIG(SOC_INTEL_COMMON_BLOCK_CPU_MPINIT)

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31802 )

Change subject: src/soc/intel/apollolake/cpu.c: Set up local APIC
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31802/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31802/3/src/soc/intel/apollolake/cpu.c@5 
PS3, Line 5:  * Copyright (C) 2017-2019 Siemens AG
> This `2017-2019` implies that you claim to have added something in 2018, too.

All right, I'll keep that in mind next time.