Attention is currently required from: Hung-Te Lin, Kevin Chiu.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50276 )
Change subject: mb/google/kukui: Add LPDDR4X byte mode/single rank DRAM support for burnet/esche
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50276/comment/c1cd5285_7a411180
PS1, Line 7: mb/google/kukui: Add LPDDR4X byte mode/single rank DRAM support for burnet/esche
Line too long
https://review.coreboot.org/c/coreboot/+/50276/comment/57222d68_c4e75003
PS1, Line 13: master
none
--
To view, visit https://review.coreboot.org/c/coreboot/+/50276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iaa735c23889860218f6f6571cf0bc0b21b304b51
Gerrit-Change-Number: 50276
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Chiu <kevin.chiu.17802(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Kevin Chiu <kevin.chiu.17802(a)gmail.com>
Gerrit-Comment-Date: Thu, 04 Feb 2021 08:11:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
chris wang has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/49933 )
Change subject: mb/google/zork: set USB_PD port0 to USB3.1 mode(flip)
......................................................................
Abandoned
--
To view, visit https://review.coreboot.org/c/coreboot/+/49933
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I13629763690c4cb40a1c917ca2c628332b2de916
Gerrit-Change-Number: 49933
Gerrit-PatchSet: 4
Gerrit-Owner: chris wang <Chris.Wang(a)amd.com>
Gerrit-Reviewer: Chris Wang <chris.wang(a)amd.corp-partner.google.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alan Lee <alan_lee(a)compal.corp-partner.google.com>
Gerrit-CC: Frank Wu <frank_wu(a)compal.corp-partner.google.com>
Gerrit-CC: Ivy Jian <ivy_jian(a)compal.corp-partner.google.com>
Gerrit-MessageType: abandon
Attention is currently required from: Martin Roth, Tim Wawrzynczak, Meera Ravindranath, Aamir Bohra, Andrey Petrov, Patrick Rudolph.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49474 )
Change subject: drivers/intel/fsp2_0: Add support for MP PPI services2 APIs
......................................................................
Patch Set 6:
(18 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49474/comment/736b11ad_9ffac746
PS6, Line 7: MP PPI services2
nit: MP services2 PPI
https://review.coreboot.org/c/coreboot/+/49474/comment/ba1ca19b_d9527816
PS6, Line 13:
BUG=b:169196864
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/49474/comment/d108dbec_75935716
PS6, Line 267:
: if FSP_PEIM_TO_PEIM_INTERFACE
: source "src/drivers/intel/fsp2_0/ppi/Kconfig"
: endif
:
Why was this dropped?
File src/drivers/intel/fsp2_0/include/fsp/ppi/mp_service_ppi.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/be5cb13b_52936fc8
PS6, Line 18: efi_pei_mp_services_ppi
I think it would be good to just set this to void so that we can void having to provide the definition of this type. With V1 and V2 providing different definitions, it is unnecessarily going to create a dependency on the header files.
https://review.coreboot.org/c/coreboot/+/49474/comment/d12e10f3_29b912d7
PS6, Line 19:
: /*
: * get the number of logical processor in the platform
: */
nit: Recommended comment style for single line comments is either /* ... */ or //
https://review.coreboot.org/c/coreboot/+/49474/comment/104bfe01_525f2ea9
PS6, Line 36: )
I think you are going to need an additional parameter here to decide whether the procedure should be run in parallel or in serial fashion. b/169114674. Are you planning to handle that as a follow-up?
https://review.coreboot.org/c/coreboot/+/49474/comment/4105c7b3_a9edf868
PS6, Line 41: )
What about timeout_usec?
https://review.coreboot.org/c/coreboot/+/49474/comment/897b4831_ddcc066a
PS6, Line 49: /*
: * switches the requested AP to be the BSP
: */
: efi_return_status_t mp_switch_bsp(void);
:
: /*
: * enable or disable an AP. This service may only be called from the BSP.
: */
: efi_return_status_t mp_enable_disable_ap(void);
:
These return FSP_UNSUPPORTED. Do we really want to have common helpers for these?
File src/drivers/intel/fsp2_0/ppi/Kconfig:
PS6:
I think we can simplify this file somewhat.
1. I know I asked to use "choice". But, one thing I realized is that choice requires user prompt, which though harmless, is unnecessary. The user/mainboard does not really have an option to select one v/s other since it is pretty much governed by the FSP being used for the platform. Given, that I think we can just use configs without choice and add a check in mp_service_ppi.c to ensure that only one is selected.
2. "PLATFORM_USES.." and "FSP_USES.." looks really confusing. What do you think about: CB:50274 and CB:50275. I think we can add following configs in this file:
config MP_SERVICES_PPI (added in CB:50275)
...
config MP_SERVICES_PPI_V1
bool
default n
select MP_SERVICES_PPI
help
...
config MP_SERVICES_PPI_V2
bool
default n
select MP_SERVICES_PPI
help
...
File src/drivers/intel/fsp2_0/ppi/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/49474/comment/05abea61_fba9605e
PS6, Line 3: CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
And this can be CONFIG_MP_SERVICES_PPI
https://review.coreboot.org/c/coreboot/+/49474/comment/1ef7a4a2_5a4d547b
PS6, Line 4: CONFIG_FSP_USES_MP_SERVICES_PPI
... and CONFIG_MP_SERVICES_PPI_V1
File src/drivers/intel/fsp2_0/ppi/mp_service1.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/03fddcf4_6b1cb0c2
PS6, Line 13:
remove extra blank lines.
https://review.coreboot.org/c/coreboot/+/49474/comment/cdcebf63_fb0f061c
PS6, Line 61: /*
: * EDK2 UEFIPKG Open Source MP Service PPI to be installed
: */
Single line comment style: // or /* ... */
File src/drivers/intel/fsp2_0/ppi/mp_service2.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/f8d1d2cc_9411a4a4
PS6, Line 14:
extra blank line not required.
https://review.coreboot.org/c/coreboot/+/49474/comment/c1551958_1f543e81
PS6, Line 69: /*
: * EDK2 UEFIPKG Open Source MP Services 2 PPI to be installed
: */
:
Use single line comment style.
File src/include/efi/efi_datatype.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/eac097ec_8db29a67
PS6, Line 9: #include <Ppi/MpServices.h>
: #include <Ppi/MpServices2.h>
I don't think we should be including either of these here.
https://review.coreboot.org/c/coreboot/+/49474/comment/57a0b673_672df5d3
PS6, Line 59: #if CONFIG(FSP_USES_MP_SERVICES2_PPI)
: typedef EDKII_PEI_MP_SERVICES2_PPI efi_pei_mp_services_ppi;
: #else
: typedef EFI_PEI_MP_SERVICES_PPI efi_pei_mp_services_ppi;
: typedef EFI_PEI_SERVICES efi_pei_services;
: #endif
I think these can be moved to mp_service1.c and mp_service2.c.
File src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Ppi/MpServices2.h:
PS6:
This file is not really present in UDK2017. I don't think we should be adding this here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/49474
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id74baf17fb90147d229c78be90268fdc3ec1badc
Gerrit-Change-Number: 49474
Gerrit-PatchSet: 6
Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Meera Ravindranath <meera.ravindranath(a)intel.com>
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: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Meera Ravindranath <meera.ravindranath(a)intel.com>
Gerrit-Attention: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 04 Feb 2021 07:47:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/50254 )
Change subject: vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h
......................................................................
vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h
The recent merge of Intel ADL FSP 2017.00 appears to have introduced a
new dependency within the file MemInfoHob.h. Adding required macros to
resolve the dependency.
BUG=b:178846328
Change-Id: I18370edca481bac5fdd483680cd7b05b216d10fc
Signed-off-by: Subrata Banik <subrata.banik(a)intel.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50254
Reviewed-by: Furquan Shaikh <furquan(a)google.com>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/vendorcode/intel/fsp/fsp2_0/alderlake/MemInfoHob.h
1 file changed, 10 insertions(+), 17 deletions(-)
Approvals:
build bot (Jenkins): Verified
Furquan Shaikh: Looks good to me, approved
Angel Pons: Looks good to me, but someone else must approve
Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/vendorcode/intel/fsp/fsp2_0/alderlake/MemInfoHob.h b/src/vendorcode/intel/fsp/fsp2_0/alderlake/MemInfoHob.h
index 816ce06..31047af 100644
--- a/src/vendorcode/intel/fsp/fsp2_0/alderlake/MemInfoHob.h
+++ b/src/vendorcode/intel/fsp/fsp2_0/alderlake/MemInfoHob.h
@@ -4,7 +4,7 @@
data hobs.
@copyright
- Copyright (c) 1999 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 1999 - 2021, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials are licensed and made available under
the terms and conditions of the BSD License that accompanies this distribution.
The full text of the license may be found at
@@ -24,10 +24,8 @@
extern EFI_GUID gSiMemoryInfoDataGuid;
extern EFI_GUID gSiMemoryPlatformDataGuid;
-#define MAX_TRACE_CACHE_TYPE 3
-
-#define MAX_NODE 1
-#define MAX_CH 2
+#define MAX_NODE 2
+#define MAX_CH 4
#define MAX_DIMM 2
///
@@ -153,6 +151,9 @@
#define MAX_PROFILE_NUM 4 // number of memory profiles supported
#define MAX_XMP_PROFILE_NUM 2 // number of XMP profiles supported
+#define MAX_TRACE_REGION 5
+#define MAX_TRACE_CACHE_TYPE 2
+
//
// DIMM timings
//
@@ -243,10 +244,11 @@
UINT32 TotalPhysicalMemorySize;
UINT32 DefaultXmptCK[MAX_XMP_PROFILE_NUM];///< Stores the tCK value read from SPD XMP profiles if they exist.
UINT8 XmpProfileEnable; ///< If XMP capable DIMMs are detected, this will indicate which XMP Profiles are common among all DIMMs.
- UINT8 Ratio;
+ UINT8 Ratio; ///< DDR Frequency Ratio, Max Value 255
UINT8 RefClk;
UINT32 VddVoltage[MAX_PROFILE_NUM];
CONTROLLER_INFO Controller[MAX_NODE];
+ UINT16 Ratio_UINT16; ///< DDR Frequency Ratio, used for programs that require ratios higher then 255
} MEMORY_INFO_DATA_HOB;
/**
@@ -265,21 +267,12 @@
UINT32 TsegBase;
UINT32 PrmrrSize;
UINT64 PrmrrBase;
- UINT32 PramSize;
- UINT64 PramBase;
- UINT64 DismLimit;
- UINT64 DismBase;
UINT32 GttBase;
UINT32 MmioSize;
UINT32 PciEBaseAddress;
-//
-// CPU:RestrictedBegin
-//
- UINT32 SharedMailboxBase;
-//
-// CPU:RestrictedEnd
-//
PSMI_MEM_INFO PsmiInfo[MAX_TRACE_CACHE_TYPE];
+ PSMI_MEM_INFO PsmiRegionInfo[MAX_TRACE_REGION];
+ BOOLEAN MrcBasicMemoryTestPass;
} MEMORY_PLATFORM_DATA;
typedef struct {
--
To view, visit https://review.coreboot.org/c/coreboot/+/50254
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I18370edca481bac5fdd483680cd7b05b216d10fc
Gerrit-Change-Number: 50254
Gerrit-PatchSet: 4
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged