Hello Werner Zeh, Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38824
to look at the new patch set (#4).
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
soc/intel/apollolake: Display apollolake platform information
This patch includes the change required to display apollolake platform
information which reports CPU, MCH, PCH and IGD information in romstage.
BUG=None
TEST=
1. Boot to OS on Bobba board.
2. Verified below info from CPU Console log in romstage
CPU: Intel(R) Celeron(R) N4000 CPU @ 1.10GHz
CPU: ID 706a1, Geminilake B0, ucode: 00000031
CPU: AES supported, TXT NOT supported, VT supported
MCH: device id 31f0 (rev 03) is Geminilake
PCH: device id 3197 (rev 03) is Geminilake
IGD: device id 3185 (rev 03) is Geminilake EU12
Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Signed-off-by: Usha P <usha.p(a)intel.com>
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/apollolake/include/soc/romstage.h
A src/soc/intel/apollolake/report_platform.c
M src/soc/intel/apollolake/romstage.c
4 files changed, 177 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/38824/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 4
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> > > Compiles fine as by Aamir's suggestion, btw.
> >
> > yes compilation will work as long as #define __SIMPLE_DEVICE__ is there.
>
> No, I meant this patch but with PCH_DEV_LPC from soc/pci_devs.h instead
> of your new macro works fine.
Do you mean PCH_DEV_LPC from soc/pci_devs.h will work without having #define __SIMPLE_DEVICE__ at line 18?
>
> >
> > But i don't know why we have this guard at first place and want to compile this entire file as part of simple device. is that saving anything ? general query.
>
> It provides a stable API even if the file is compiled for ramstage and
> pre-ram stages.
Intention is not to break the stability here for sure, just wondering if you have real reason why #define __SIMPLE_DEVICE__ been added at first place ?
> Hence, avoids the need for any #if in platform code.
FYI, this is soc code not the platform code. hatch, poppy are the platform code in my understanding. this is soc library. i was adhering old review comments from Arthur who has suggested to have a macro rather function names.
> because you can't use them anymore to get a `struct device *`. That's
> why there are some explicit calls to pcidev_on_root() below.
yes, that's the point. this could have been avoided but it has some other assumptions :) about "struct device *"
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
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: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.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: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 09:52:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> > Compiles fine as by Aamir's suggestion, btw.
>
> yes compilation will work as long as #define __SIMPLE_DEVICE__ is there.
No, I meant this patch but with PCH_DEV_LPC from soc/pci_devs.h instead
of your new macro works fine.
>
> But i don't know why we have this guard at first place and want to compile this entire file as part of simple device. is that saving anything ? general query.
It provides a stable API even if the file is compiled for ramstage and
pre-ram stages. Hence, avoids the need for any #if in platform code. It
makes it harder, though, to have common definitions like PCH_DEV_LPC,
because you can't use them anymore to get a `struct device *`. That's
why there are some explicit calls to pcidev_on_root() below.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
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: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.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: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 09:42:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> Compiles fine as by Aamir's suggestion, btw.
yes compilation will work as long as #define __SIMPLE_DEVICE__ is there.
But i don't know why we have this guard at first place and want to compile this entire file as part of simple device. is that saving anything ? general query.
>
> That you had to get rid of the declaration in line 179, doesn't
> mean we need yet another macro.
I believe this new macro help it to compile as per need, i mean till romstage as simple device else regularly. unlike entire file is compile as simple device (again i don't know the goodness or bad about this)
also another reason to pick this is p2sb.c is also has same problem where we have redundant function name which can be avoided.
infact looking at old southbridge code that arthur has written has same implementation. Also in some AMD latest common code.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
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: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.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: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 09:27:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> Why do you want to remove the `#define __SIMPLE_DEVICE__`? It's been […]
Compiles fine as by Aamir's suggestion, btw.
That you had to get rid of the declaration in line 179, doesn't
mean we need yet another macro.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
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: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.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: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 09:14:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> right now we have a […]
Why do you want to remove the `#define __SIMPLE_DEVICE__`? It's been
used here exactly the way it was intended to be used. `#if
defined(__SIMPLE_DEVICE__)` is discouraged, OTOH.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
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: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.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: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 09:07:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-MessageType: comment