coreboot-gerrit November 2019

coreboot-gerrit@coreboot.org

1 participants
2418 discussions

Change in coreboot[master]: cpu/x86/smm: Add helper functions to deal with different save states
by Arthur Heymans (Code Review) 07 Nov '19

07 Nov '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36589 ) Change subject: cpu/x86/smm: Add helper functions to deal with different save states ...................................................................... Patch Set 3: > Patch Set 3: > > We have something similar in src/soc/intel/common/block/include/intelblocks/smihandler.h and src/soc/intel/common/block/smm/smihandler.c where the ops are about getting and setting a register. > > 1. Why is this about pointers to registers and not just getting and setting the explicit register? > 2. I prefer the ops-based approach because the chipset can say "these are my ops" and the other save state versions are garbage collected instead of carrying support for save state versions in the binary. > 3. Are we planning on converging/sharing the implementation? Attempt to provide an API in CB:36661. With that code there is no need anymore to have per chipset ops, since the ops can be guessed from the smm revision. -- To view, visit https://review.coreboot.org/c/coreboot/+/36589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15 Gerrit-Change-Number: 36589 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 15:01:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Arthur Heymans (Code Review) 07 Nov '19

07 Nov '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 ) Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices ...................................................................... Patch Set 20: > Patch Set 20: > > > Patch Set 20: > > > > > Patch Set 20: > > > > > > > Patch Set 20: > > > > > > > > > Patch Set 20: > > > > > > > > > > > Patch Set 20: > > > > > > Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix. > > > > > > > > > > This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing. > > > > > > > > > > What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)? > > > > > > > > The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think > > > > Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info. > > > > > > You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables. > > > > > > Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > > > Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past. > > > > Hi Arthur, I wanted to understand more on you below comment: > > > > [Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > [Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation: > > > > Name (PIRX, Package (0x11) > > { > > Package (0x04) > > { > > 0x0012FFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x0014FFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x0014FFFF, > > 0x01, > > Zero, > > 0x11 > > }, > > > > Package (0x04) > > { > > 0x0014FFFF, > > 0x02, > > Zero, > > 0x12 > > }, > > > > Package (0x04) > > { > > 0x0015FFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x0015FFFF, > > 0x01, > > Zero, > > 0x11 > > }, > > > > Package (0x04) > > { > > 0x0015FFFF, > > 0x02, > > Zero, > > 0x12 > > }, > > > > Package (0x04) > > { > > 0x0016FFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x0017FFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x0019FFFF, > > 0x03, > > Zero, > > 0x13 > > }, > > > > Package (0x04) > > { > > 0x001DFFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x001DFFFF, > > 0x01, > > Zero, > > 0x11 > > }, > > > > Package (0x04) > > { > > 0x001EFFFF, > > 0x00, > > Zero, > > 0x14 > > }, > > > > Package (0x04) > > { > > 0x001EFFFF, > > 0x01, > > Zero, > > 0x15 > > }, > > > > Package (0x04) > > { > > 0x001EFFFF, > > 0x02, > > Zero, > > 0x16 > > }, > > > > Package (0x04) > > { > > 0x001FFFFF, > > 0x00, > > Zero, > > 0x10 > > }, > > > > Package (0x04) > > { > > 0x001FFFFF, > > 0x01, > > Zero, > > 0x11 > > } > > }) > > > > And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs) > > https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/bloc… > > > > Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well. > > Hi Arthur, did you get a chance to review the acpi package, if no concerns, can you please review your -2 vote? > Anyways this CL is needed to populate irq table for FSP to consume. ACPI CL is here: https://review.coreboot.org/c/coreboot/+/34658/6 No, you are just 'lucky' that there are no double entries. Please try https://review.coreboot.org/c/coreboot/+/36437 and use that to generate acpi _PRT entries. It should be trivial to extend to use other settings you provide fsp. -- To view, visit https://review.coreboot.org/c/coreboot/+/34089 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7066432ff5f0d7017ac5a44922ca69f07da9556 Gerrit-Change-Number: 34089 Gerrit-PatchSet: 20 Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(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: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 14:19:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
by Mario Scheithauer (Code Review) 07 Nov '19

07 Nov '19

Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36645 ) Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options ...................................................................... soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options The commit (1058dd84f06fa2fcbdd99eb99da07dccdf5b9722) breaks the Siemens APL mainboards as FSP-M never returns once it is called. This patch fixes an incomplete implementation of this change. TEST=tested on siemens/mc_apl5 Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550 Signed-off-by: Mario Scheithauer <mario.scheithauer(a)siemens.com> --- M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/denverton_ns/Makefile.inc M src/soc/intel/quark/Makefile.inc 3 files changed, 5 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36645/1 diff --git a/src/soc/intel/apollolake/Makefile.inc b/src/soc/intel/apollolake/Makefile.inc index 42cbab0..ef81e32 100644 --- a/src/soc/intel/apollolake/Makefile.inc +++ b/src/soc/intel/apollolake/Makefile.inc @@ -108,7 +108,7 @@ CPPFLAGS_common += -I$(src)/soc/intel/apollolake/include # Since FSP-M runs in CAR we need to relocate it to a specific address -$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR) +$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR) # Handle GLK paging requirements ifeq ($(CONFIG_PAGING_IN_CACHE_AS_RAM),y) diff --git a/src/soc/intel/denverton_ns/Makefile.inc b/src/soc/intel/denverton_ns/Makefile.inc index 51ae136..4050f61 100644 --- a/src/soc/intel/denverton_ns/Makefile.inc +++ b/src/soc/intel/denverton_ns/Makefile.inc @@ -90,8 +90,8 @@ ##Set FSP binary blobs memory location -$(FSP_T_CBFS)-options := -b $(CONFIG_FSP_T_ADDR) --xip -$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR) --xip -$(FSP_S_CBFS)-options := -b $(CONFIG_FSP_S_ADDR) --xip +$(call strip_quotes,$(CONFIG_FSP_T_CBFS))-options := -b $(CONFIG_FSP_T_ADDR) --xip +$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR) --xip +$(call strip_quotes,$(CONFIG_FSP_S_CBFS))-options := -b $(CONFIG_FSP_S_ADDR) --xip endif ## CONFIG_SOC_INTEL_DENVERTON_NS diff --git a/src/soc/intel/quark/Makefile.inc b/src/soc/intel/quark/Makefile.inc index f2413c8..bd120ab 100644 --- a/src/soc/intel/quark/Makefile.inc +++ b/src/soc/intel/quark/Makefile.inc @@ -71,7 +71,7 @@ CPPFLAGS_common += -I3rdparty/blobs/soc/intel/quark # Since FSP-M runs in CAR we need to relocate it to a specific address -$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_ESRAM_LOC) +$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_ESRAM_LOC) # Add the FSP binary to the CBFS image cbfs-files-$(CONFIG_ADD_FSP_RAW_BIN) += fsp.bin -- To view, visit https://review.coreboot.org/c/coreboot/+/36645 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550 Gerrit-Change-Number: 36645 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-MessageType: newchange

5 11

Change in coreboot[master]: soc/intel/icelake: Move pch_early_init from bootblock to romstage
by Subrata Banik (Code Review) 07 Nov '19

07 Nov '19

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36627 ) Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ...................................................................... soc/intel/icelake: Move pch_early_init from bootblock to romstage This patch moves required pch_early_init() function from bootblock/pch.c to romstage/pch.c. TEST=Able to build and boot ICL DE system. Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/soc/intel/icelake/bootblock/bootblock.c M src/soc/intel/icelake/bootblock/pch.c M src/soc/intel/icelake/include/soc/bootblock.h M src/soc/intel/icelake/include/soc/romstage.h M src/soc/intel/icelake/romstage/Makefile.inc A src/soc/intel/icelake/romstage/pch.c M src/soc/intel/icelake/romstage/romstage.c 7 files changed, 93 insertions(+), 56 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36627/1 diff --git a/src/soc/intel/icelake/bootblock/bootblock.c b/src/soc/intel/icelake/bootblock/bootblock.c index f348c1b..93e1762 100644 --- a/src/soc/intel/icelake/bootblock/bootblock.c +++ b/src/soc/intel/icelake/bootblock/bootblock.c @@ -40,5 +40,4 @@ void bootblock_soc_init(void) { report_platform_info(); - pch_early_init(); } diff --git a/src/soc/intel/icelake/bootblock/pch.c b/src/soc/intel/icelake/bootblock/pch.c index e95220b..d3bd3de 100644 --- a/src/soc/intel/icelake/bootblock/pch.c +++ b/src/soc/intel/icelake/bootblock/pch.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2018 Intel Corp. + * Copyright (C) 2018-2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -35,15 +35,6 @@ #include <soc/pm.h> #include <soc/smbus.h> -#define PCR_PSF3_TO_SHDW_PMC_REG_BASE 0x0600 -#define PCR_PSFX_TO_SHDW_BAR0 0 -#define PCR_PSFX_TO_SHDW_BAR1 0x4 -#define PCR_PSFX_TO_SHDW_BAR2 0x8 -#define PCR_PSFX_TO_SHDW_BAR3 0xC -#define PCR_PSFX_TO_SHDW_BAR4 0x10 -#define PCR_PSFX_TO_SHDW_PCIEN_IOEN 0x01 -#define PCR_PSFX_T0_SHDW_PCIEN 0x1C - #define PCR_DMI_DMICTL 0x2234 #define PCR_DMI_DMICTL_SRLOCK (1 << 31) @@ -94,30 +85,6 @@ soc_config_pwrmbase(); } - -static void soc_config_acpibase(void) -{ - uint32_t pmc_reg_value; - - pmc_reg_value = pcr_read32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + - PCR_PSFX_TO_SHDW_BAR4); - - if (pmc_reg_value != 0xFFFFFFFF) { - /* Disable Io Space before changing the address */ - pcr_rmw32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + - PCR_PSFX_T0_SHDW_PCIEN, - ~PCR_PSFX_TO_SHDW_PCIEN_IOEN, 0); - /* Program ABASE in PSF3 PMC space BAR4*/ - pcr_write32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + - PCR_PSFX_TO_SHDW_BAR4, - ACPI_BASE_ADDRESS); - /* Enable IO Space */ - pcr_rmw32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + - PCR_PSFX_T0_SHDW_PCIEN, - ~0, PCR_PSFX_TO_SHDW_PCIEN_IOEN); - } -} - static int pch_check_decode_enable(void) { uint32_t dmi_control; @@ -154,23 +121,3 @@ /* Program generic IO Decode Range */ pch_enable_lpc(); } - -void pch_early_init(void) -{ - /* - * Enabling ABASE for accessing PM1_STS, PM1_EN, PM1_CNT, - * GPE0_STS, GPE0_EN registers. - */ - soc_config_acpibase(); - - /* Programming TCO_BASE_ADDRESS and TCO Timer Halt */ - tco_configure(); - - /* Program SMBUS_BASE_ADDRESS and Enable it */ - smbus_common_init(); - - /* Set up GPE configuration */ - pmc_gpe_init(); - - enable_rtc_upper_bank(); -} diff --git a/src/soc/intel/icelake/include/soc/bootblock.h b/src/soc/intel/icelake/include/soc/bootblock.h index 4ca2c37..4222105 100644 --- a/src/soc/intel/icelake/include/soc/bootblock.h +++ b/src/soc/intel/icelake/include/soc/bootblock.h @@ -21,7 +21,6 @@ void bootblock_pch_early_init(void); /* Bootblock post console init programming */ -void pch_early_init(void); void pch_early_iorange_init(void); void report_platform_info(void); diff --git a/src/soc/intel/icelake/include/soc/romstage.h b/src/soc/intel/icelake/include/soc/romstage.h index e931811..dfb5695 100644 --- a/src/soc/intel/icelake/include/soc/romstage.h +++ b/src/soc/intel/icelake/include/soc/romstage.h @@ -20,6 +20,7 @@ void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); +void pch_early_init(void); /* Board type */ enum board_type { diff --git a/src/soc/intel/icelake/romstage/Makefile.inc b/src/soc/intel/icelake/romstage/Makefile.inc index baa4d46..b42f3f4 100644 --- a/src/soc/intel/icelake/romstage/Makefile.inc +++ b/src/soc/intel/icelake/romstage/Makefile.inc @@ -16,4 +16,5 @@ romstage-y += fsp_params.c romstage-y += ../../../../cpu/intel/car/romstage.c romstage-y += romstage.c +romstage-y += pch.c romstage-y += systemagent.c diff --git a/src/soc/intel/icelake/romstage/pch.c b/src/soc/intel/icelake/romstage/pch.c new file mode 100644 index 0000000..81551bb --- /dev/null +++ b/src/soc/intel/icelake/romstage/pch.c @@ -0,0 +1,88 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/mmio.h> +#include <device/device.h> +#include <device/pci_ops.h> +#include <intelblocks/fast_spi.h> +#include <intelblocks/gspi.h> +#include <intelblocks/lpc_lib.h> +#include <intelblocks/p2sb.h> +#include <intelblocks/pcr.h> +#include <intelblocks/pmclib.h> +#include <intelblocks/rtc.h> +#include <intelblocks/smbus.h> +#include <intelblocks/tco.h> +#include <soc/bootblock.h> +#include <soc/iomap.h> +#include <soc/romstage.h> +#include <soc/p2sb.h> +#include <soc/pch.h> +#include <soc/pci_devs.h> +#include <soc/pcr_ids.h> +#include <soc/pm.h> +#include <soc/smbus.h> + +#define PCR_PSF3_TO_SHDW_PMC_REG_BASE 0x0600 +#define PCR_PSFX_TO_SHDW_BAR0 0 +#define PCR_PSFX_TO_SHDW_BAR1 0x4 +#define PCR_PSFX_TO_SHDW_BAR2 0x8 +#define PCR_PSFX_TO_SHDW_BAR3 0xC +#define PCR_PSFX_TO_SHDW_BAR4 0x10 +#define PCR_PSFX_TO_SHDW_PCIEN_IOEN 0x01 +#define PCR_PSFX_T0_SHDW_PCIEN 0x1C + +static void soc_config_acpibase(void) +{ + uint32_t pmc_reg_value; + + pmc_reg_value = pcr_read32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + + PCR_PSFX_TO_SHDW_BAR4); + + if (pmc_reg_value != 0xFFFFFFFF) { + /* Disable Io Space before changing the address */ + pcr_rmw32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + + PCR_PSFX_T0_SHDW_PCIEN, + ~PCR_PSFX_TO_SHDW_PCIEN_IOEN, 0); + /* Program ABASE in PSF3 PMC space BAR4*/ + pcr_write32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + + PCR_PSFX_TO_SHDW_BAR4, + ACPI_BASE_ADDRESS); + /* Enable IO Space */ + pcr_rmw32(PID_PSF3, PCR_PSF3_TO_SHDW_PMC_REG_BASE + + PCR_PSFX_T0_SHDW_PCIEN, + ~0, PCR_PSFX_TO_SHDW_PCIEN_IOEN); + } +} + +void pch_early_init(void) +{ + /* + * Enabling ABASE for accessing PM1_STS, PM1_EN, PM1_CNT, + * GPE0_STS, GPE0_EN registers. + */ + soc_config_acpibase(); + + /* Programming TCO_BASE_ADDRESS and TCO Timer Halt */ + tco_configure(); + + /* Program SMBUS_BASE_ADDRESS and Enable it */ + smbus_common_init(); + + /* Set up GPE configuration */ + pmc_gpe_init(); + + enable_rtc_upper_bank(); +} diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index 2c4ba67..927e45d 100644 --- a/src/soc/intel/icelake/romstage/romstage.c +++ b/src/soc/intel/icelake/romstage/romstage.c @@ -116,6 +116,8 @@ /* Program MCHBAR, DMIBAR, GDXBAR and EDRAMBAR */ systemagent_early_init(); + /* Program PCH early init */ + pch_early_init(); /* initialize Heci interface */ heci_init(HECI1_BASE_ADDRESS); -- To view, visit https://review.coreboot.org/c/coreboot/+/36627 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Gerrit-Change-Number: 36627 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: newchange

6 18

Change in coreboot[master]: Rangeley: Fix incorrect BCLK
by Name of user not set (Code Review) 07 Nov '19

07 Nov '19

hannah.williams(a)dell.com has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35348 ) Change subject: Rangeley: Fix incorrect BCLK ...................................................................... Rangeley: Fix incorrect BCLK Not all Rangeley SKUs have a fixed 100MHz BCLK. As per BIOS Writer's Guide, BCLK is available in MSR PSB_CLOCK_STS[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz. Signed-off-by: Hannah Williams <hannah.williams(a)dell.com> Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f --- M src/cpu/intel/fsp_model_406dx/acpi.c M src/cpu/intel/fsp_model_406dx/model_406dx.h 2 files changed, 31 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/1 diff --git a/src/cpu/intel/fsp_model_406dx/acpi.c b/src/cpu/intel/fsp_model_406dx/acpi.c index 6672eab..d6177b6 100644 --- a/src/cpu/intel/fsp_model_406dx/acpi.c +++ b/src/cpu/intel/fsp_model_406dx/acpi.c @@ -154,6 +154,33 @@ return (int)power; } +static int get_core_frequency_mhz(int ratio) +{ + msr_t msr; + unsigned int cpu_bclk_hz[] = { + 83333333, + 100000000, + 133333333, + 116666666 + }; + int core_freq; + int index; + + /* Get BCLK - different SKUs can have different BCLK */ + msr = rdmsr(MSR_PSB_CLOCK_STS); + index = msr.lo & CPU_BCLK_MASK; + if (index >= ARRAY_SIZE(cpu_bclk_hz)) + index = 0; + + printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", + msr.hi, msr.lo, cpu_bclk_hz[index], ratio); + core_freq = (cpu_bclk_hz[index] * ratio) / 1000000; + if ((cpu_bclk_hz[index] * ratio) % 1000000 > 500000) + core_freq++; + printk(BIOS_DEBUG, "core frequency for ratio(%d) %dMHz\n", ratio, core_freq); + return core_freq; +} + static void generate_P_state_entries(int core, int cores_per_package) { int ratio_min, ratio_max, ratio_turbo, ratio_step; @@ -177,7 +204,7 @@ /* Max Non-Turbo Ratio */ ratio_max = (msr.lo >> 8) & 0xff; } - clock_max = ratio_max * RANGELEY_BCLK; + clock_max = get_core_frequency_mhz(ratio_max); /* Calculate CPU TDP in mW */ msr = rdmsr(MSR_PKG_POWER_SKU_UNIT); @@ -240,7 +267,7 @@ /* Calculate power at this ratio */ power = calculate_power(power_max, ratio_max, ratio); - clock = ratio * RANGELEY_BCLK; + clock = get_core_frequency_mhz(ratio); acpigen_write_PSS_package( clock, /*MHz*/ diff --git a/src/cpu/intel/fsp_model_406dx/model_406dx.h b/src/cpu/intel/fsp_model_406dx/model_406dx.h index 53a77a9..dcfe914 100644 --- a/src/cpu/intel/fsp_model_406dx/model_406dx.h +++ b/src/cpu/intel/fsp_model_406dx/model_406dx.h @@ -15,8 +15,6 @@ #ifndef _CPU_INTEL_MODEL_406DX_H #define _CPU_INTEL_MODEL_406DX_H -/* Rangeley bus clock is fixed at 100MHz */ -#define RANGELEY_BCLK 100 #define MSR_FEATURE_CONFIG 0x13c #define MSR_FLEX_RATIO 0x194 @@ -27,6 +25,8 @@ #define MSR_NO_EVICT_MODE 0x2e0 #define MSR_PIC_MSG_CONTROL 0x2e +#define MSR_PSB_CLOCK_STS 0xcd +#define CPU_BCLK_MASK 0x3 #define MSR_PLATFORM_INFO 0xce #define PLATFORM_INFO_SET_TDP (1 << 29) #define MSR_PKG_CST_CONFIG_CONTROL 0xe2 -- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 1 Gerrit-Owner: hannah.williams(a)dell.com Gerrit-MessageType: newchange

8 28

Change in coreboot[master]: security/vboot: Build vboot library with same .a that depthcharge uses
by Joel Kitching (Code Review) 07 Nov '19

07 Nov '19

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36341 ) Change subject: security/vboot: Build vboot library with same .a that depthcharge uses ...................................................................... Patch Set 18: > > This is all good but it still leaves the problem that we need to deal with the TLCL duplicate references (see my comment early on). Are you guys clear on who's going to deal with that (Tim or Joel)? Ah, it's only after seeing this CL that I realize the purpose is for handling the duplicate references. Might want to mention that in the vboot_reference CL. -- To view, visit https://review.coreboot.org/c/coreboot/+/36341 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I905b781c3596965ec7ef45a2a7eafe15fdd4d9cc Gerrit-Change-Number: 36341 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Joel Kitching <kitching(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 14:04:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE
by Frans Hendriks (Code Review) 07 Nov '19

07 Nov '19

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36635 ) Change subject: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE ...................................................................... Patch Set 2: Code-Review+2 Working fine on Facebook FBG1701 -- To view, visit https://review.coreboot.org/c/coreboot/+/36635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id56a63a67b7eb70dce6687bb9c2734a711f611b3 Gerrit-Change-Number: 36635 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn(a)eltan.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: wvervoorn <wvervoorn(a)eltan.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 13:41:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE
by Wim Vervoorn (Code Review) 07 Nov '19

07 Nov '19

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36635 ) Change subject: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/36635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id56a63a67b7eb70dce6687bb9c2734a711f611b3 Gerrit-Change-Number: 36635 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn(a)eltan.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: wvervoorn <wvervoorn(a)eltan.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 13:15:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE
by Frans Hendriks (Code Review) 07 Nov '19

07 Nov '19

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36635 ) Change subject: eltan/security: Replace __PRE_RAM__ with ENV_ROMSTAGE_OR_BEFORE ...................................................................... Patch Set 2: Will verify this patchset (and the other related patches) on Facebook FBG1701 and let you know the result. -- To view, visit https://review.coreboot.org/c/coreboot/+/36635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id56a63a67b7eb70dce6687bb9c2734a711f611b3 Gerrit-Change-Number: 36635 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: wvervoorn <wvervoorn(a)eltan.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 11:24:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Aamir Bohra (Code Review) 07 Nov '19

07 Nov '19

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 ) Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices ...................................................................... Patch Set 20: > Patch Set 20: > > > Patch Set 20: > > > > > Patch Set 20: > > > > > > > Patch Set 20: > > > > > > > > > Patch Set 20: > > > > > Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix. > > > > > > > > This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing. > > > > > > > > What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)? > > > > > > The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think > > > Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info. > > > > You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables. > > > > Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past. > > Hi Arthur, I wanted to understand more on you below comment: > > [Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > [Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation: > > Name (PIRX, Package (0x11) > { > Package (0x04) > { > 0x0012FFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x0014FFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x0014FFFF, > 0x01, > Zero, > 0x11 > }, > > Package (0x04) > { > 0x0014FFFF, > 0x02, > Zero, > 0x12 > }, > > Package (0x04) > { > 0x0015FFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x0015FFFF, > 0x01, > Zero, > 0x11 > }, > > Package (0x04) > { > 0x0015FFFF, > 0x02, > Zero, > 0x12 > }, > > Package (0x04) > { > 0x0016FFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x0017FFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x0019FFFF, > 0x03, > Zero, > 0x13 > }, > > Package (0x04) > { > 0x001DFFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x001DFFFF, > 0x01, > Zero, > 0x11 > }, > > Package (0x04) > { > 0x001EFFFF, > 0x00, > Zero, > 0x14 > }, > > Package (0x04) > { > 0x001EFFFF, > 0x01, > Zero, > 0x15 > }, > > Package (0x04) > { > 0x001EFFFF, > 0x02, > Zero, > 0x16 > }, > > Package (0x04) > { > 0x001FFFFF, > 0x00, > Zero, > 0x10 > }, > > Package (0x04) > { > 0x001FFFFF, > 0x01, > Zero, > 0x11 > } > }) > > And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs) > https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/bloc… > > Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well. Hi Arthur, did you get a chance to review the acpi package, if no concerns, can you please review your -2 vote? Anyways this CL is needed to populate irq table for FSP to consume. ACPI CL is here: https://review.coreboot.org/c/coreboot/+/34658/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/34089 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7066432ff5f0d7017ac5a44922ca69f07da9556 Gerrit-Change-Number: 34089 Gerrit-PatchSet: 20 Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(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: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 11:08:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2019