Change in coreboot[master]: soc/intel/icelake: Move pch_early_init from bootblock to romstage
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@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);
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ......................................................................
Patch Set 2:
(17 comments)
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG@10 PS2, Line 10: to romstage/pch.c. Commit message describes what is being done but not why it is being done. Can you please add the reasoning behind making this change?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 16: <device/mmio.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 17: <device/device.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 18: <device/pci_ops.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 19: intelblocks/fast_spi.h No Fast SPI operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 20: intelblocks/gspi.h No GSPI operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 21: intelblocks/lpc_lib.h No LPC operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 22: intelblocks/p2sb.h Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 28: #include <soc/bootblock.h> Why is bootblock.h included in a file for romstage?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 31: #include <soc/p2sb.h> Why is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 32: #include <soc/pch.h> Not required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 33: #include <soc/pci_devs.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 35: #include <soc/pm.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 36: #include <soc/smbus.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 70: early What does "early" signify in case of romstage? In bootblock, it is used to differentiate initialization that is done before console init v/s after. What is the definition for romstage?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init Have you evaluated/tested the impact of making this change? If GPE init is moved to romstage, then anything that relies on having the GPE routing setup will fail. One example that comes to mind is TPM driver accessing the state of GPE for TPM IRQ line in verstage.
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 87: enable_rtc_upper_bank Is anything stored in CMOS in upper bank that is accessed in early stages before romstage?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ......................................................................
Patch Set 2:
(17 comments)
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG@10 PS2, Line 10: to romstage/pch.c.
Commit message describes what is being done but not why it is being done. […]
i guess the idea is to do limited programming in bootblock and move the not that urgent bootblock programming into romstage?
will add the same in commit msg
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 16: <device/mmio.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 17: <device/device.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 18: <device/pci_ops.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 19: intelblocks/fast_spi.h
No Fast SPI operations in this file. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 20: intelblocks/gspi.h
No GSPI operations in this file. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 21: intelblocks/lpc_lib.h
No LPC operations in this file. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 22: intelblocks/p2sb.h
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 28: #include <soc/bootblock.h>
Why is bootblock. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 31: #include <soc/p2sb.h>
Why is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 32: #include <soc/pch.h>
Not required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 33: #include <soc/pci_devs.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 35: #include <soc/pm.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 36: #include <soc/smbus.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 70: early
What does "early" signify in case of romstage? In bootblock, it is used to differentiate initializat […]
it was added to maintain more align name with system_early_init but make sense to remove _early_ from function
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init
Have you evaluated/tested the impact of making this change? If GPE init is moved to romstage, then a […]
yes, no issue
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 87: enable_rtc_upper_bank
Is anything stored in CMOS in upper bank that is accessed in early stages before romstage?
i believe this is valid because vboot might use cmos during verstage while creating vboot_buffer. Although i don't see any error
Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36627
to look at the new patch set (#3).
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 in order to maintain only required chipset programming for bootblock and verstage.
TEST=Able to build and boot ICL DE system.
Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik subrata.banik@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, 79 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36627/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init
yes, no issue
How are TPM interrupts working with this change? I would not expect the interrupt status to update at all since GPE routing wouldn't be set.
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 87: enable_rtc_upper_bank
i believe this is valid because vboot might use cmos during verstage while creating vboot_buffer. […]
Probably because it lives in lower bank?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init
How are TPM interrupts working with this change? I would not expect the interrupt status to update a […]
TPM is working. here is the log with this changes
coreboot-4.10-1405-g7bf7601-dirty Wed Nov 6 05:10:16 UTC 2019 verstage starting (log level: 8)... Probing TPM: . done! TPM ready after 3 ms Connected to device vid:did:rid of 1ae0:0028:00 Firmware version: B2-C:0 RO_A:0.0.10/29d77172 RW_B:0.3.10/cr50_v1.9308_87_mp.104-7508e4e Initialized TPM device CR50 revision 0 tlcl_send_startup: Startup return code is 0 TPM: setup succeeded Chrome EC: UHEPI supported Phase 1 FMAP: Found "FLASH" version 1.1 at 1c04000. FMAP: base = fe000000 size = 2000000 #areas = 31 FMAP: area GBB found @ 1c05000 (978944 bytes) VB2:vb2_check_recovery() Recovery reason from previous boot: 0x0 / 0x0 Phase 2 Phase 3 FMAP: area GBB found @ 1c05000 (978944 bytes) VB2:vb2_report_dev_firmware() This is developer signed firmware FMAP: area VBLOCK_A found @ 1400000 (65536 bytes) FMAP: area VBLOCK_A found @ 1400000 (65536 bytes) VB2:vb2_verify_keyblock() Checking key block signature... FMAP: area VBLOCK_A found @ 1400000 (65536 bytes) FMAP: area VBLOCK_A found @ 1400000 (65536 bytes) VB2:vb2_verify_fw_preamble() Verifying preamble. Phase 4 FMAP: area FW_MAIN_A found @ 1410000 (2883520 bytes) VB2:vb2api_init_hash() HW crypto for hash_alg 2 not supported, using SW Saving vboot hash. Saving nvdata Saving secdata tlcl_extend: response is 0 tlcl_extend: response is 0 Slot A is selected CBFS: 'VBOOT' located CBFS at [1410000:153d580) CBFS: Locating 'fallback/romstage' CBFS: Found @ offset ff80 size 12b2c
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 87: enable_rtc_upper_bank
Probably because it lives in lower bank?
might be the case. but i have moved it to bootblock now
Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36627
to look at the new patch set (#4).
Change subject: soc/intel/icelake: Move early pch initialization from bootblock to romstage ......................................................................
soc/intel/icelake: Move early pch initialization from bootblock to romstage
This patch moves required pch_early_init() function from bootblock/pch.c to romstage/pch.c in order to maintain only required chipset programming for bootblock and verstage.
TEST=Able to build and boot ICL DE system.
Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik subrata.banik@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, 79 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36627/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move early pch initialization from bootblock to romstage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG@10 PS2, Line 10: to romstage/pch.c.
i guess the idea is to do limited programming in bootblock and move the not that urgent bootblock pr […]
We had a similar topic on IRC and the question was what does the bootblock need to initialize? As there's no checklist, the code is usually just copied from earlier generations. While it might be SoC/mainboard specific, having a checklist seems a good idea. I'll create a change for that.
https://review.coreboot.org/c/coreboot/+/36627/4/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/4/src/soc/intel/icelake/romst... PS4, Line 65: TCO When should the TCO watchdog be disabled? Will there be a platform reset if verstage takes too many TCO cycles measuring the flash regions?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move early pch initialization from bootblock to romstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init
TPM is working. here is the log with this changes […]
What do the GPE registers look like in verstage? I am curious how GPE STS bits are getting set.
Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36627
to look at the new patch set (#5).
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
soc/intel/icelake: Refactor pch_early_init() code
This patch keeps required pch_early_init() function like GPE and RTC init into bootblock and moves remaining functions like ACPI base programming, TCO configuration and SMBUS init into romstage/pch.c in order to maintain only required chipset programming for bootblock and verstage.
TEST=Able to build and boot ICL DE system.
Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/pch.c 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 5 files changed, 75 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36627/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG@10 PS2, Line 10: to romstage/pch.c.
We had a similar topic on IRC and the question was what does the bootblock need to initialize? As there's no checklist, the code is usually just copied from earlier generations.
checklist is not there because its bootloader design to decide what we need and from soc over soc we copy because chrome (as our main vehicle for coreboot development holds same design). For an example. platform might need to have uart console enable and for that we need below programming in bootblock
1. LPSS UART controller init code 2. GPIO programming 3. GPIO programming need PCR programming 4. PCR programming needs P2SB programming 5. P2SB programming needs PMC programming
so bootblock_early_pch_init() should do those programming now if some design doesn't need that they don;t do the same. But that doesn't change with soc rather its bootloader or platform design
While it might be SoC/mainboard specific, having a checklist seems a good idea. I'll create a change for that.
yes, may be something like minimum bootloader requirement for IA soc. i had a white paper in past on the same i remember
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init
What do the GPE registers look like in verstage? I am curious how GPE STS bits are getting set.
we were lucky it was working but realized it might not hold good for any cases. hence pulled in gpe programming into bootblock itself
https://review.coreboot.org/c/coreboot/+/36627/4/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/4/src/soc/intel/icelake/romst... PS4, Line 65: TCO
When should the TCO watchdog be disabled?
Inside FSP-S or finalize.c
Will there be a platform reset if verstage takes too many TCO cycles measuring the flash regions?
Are you trying to related how kernel triggers soft reset due to TCO waterdog expires. i don't see the same in FW space as we use PCH Timer for timeouts
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36627/5/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/5/src/soc/intel/icelake/romst... PS5, Line 58: /* : * Enabling ABASE for accessing PM1_STS, PM1_EN, PM1_CNT, : * GPE0_STS, GPE0_EN registers. : */ Wouldn't the acpibase configuration also have to be done in bootblock since it is required to access GPE STS?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36627/5/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/5/src/soc/intel/icelake/romst... PS5, Line 58: /* : * Enabling ABASE for accessing PM1_STS, PM1_EN, PM1_CNT, : * GPE0_STS, GPE0_EN registers. : */
Wouldn't the acpibase configuration also have to be done in bootblock since it is required to access […]
yes it should
Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36627
to look at the new patch set (#6).
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
soc/intel/icelake: Refactor pch_early_init() code
This patch keeps required pch_early_init() function like ABASE programming, GPE and RTC init into bootblock and moves remaining functions like TCO configuration and SMBUS init into romstage/pch.c in order to maintain only required chipset programming for bootblock and verstage.
TEST=Able to build and boot ICL DE system.
Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/pch.c 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 5 files changed, 32 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36627/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 6:
@Furquan: can you please help to take a look ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 6: Code-Review+2
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 6: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Refactor pch_early_init() code ......................................................................
soc/intel/icelake: Refactor pch_early_init() code
This patch keeps required pch_early_init() function like ABASE programming, GPE and RTC init into bootblock and moves remaining functions like TCO configuration and SMBUS init into romstage/pch.c in order to maintain only required chipset programming for bootblock and verstage.
TEST=Able to build and boot ICL DE system.
Change-Id: I4f0914242c3215f6bf76e41c468f544361a740d8 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36627 Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Maulik V Vaghela maulik.v.vaghela@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/icelake/bootblock/pch.c 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 5 files changed, 32 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Aamir Bohra: Looks good to me, approved Maulik V Vaghela: Looks good to me, approved
diff --git a/src/soc/intel/icelake/bootblock/pch.c b/src/soc/intel/icelake/bootblock/pch.c index e95220b..b8a404b 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 @@ -23,17 +23,13 @@ #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/espi.h> #include <soc/iomap.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 @@ -94,7 +90,6 @@ soc_config_pwrmbase(); }
- static void soc_config_acpibase(void) { uint32_t pmc_reg_value; @@ -163,12 +158,6 @@ */ 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();
diff --git a/src/soc/intel/icelake/include/soc/romstage.h b/src/soc/intel/icelake/include/soc/romstage.h index e931811..977c7c0 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_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..88a7cc7 --- /dev/null +++ b/src/soc/intel/icelake/romstage/pch.c @@ -0,0 +1,27 @@ +/* + * 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 <intelblocks/smbus.h> +#include <intelblocks/tco.h> +#include <soc/romstage.h> + +void pch_init(void) +{ + /* Programming TCO_BASE_ADDRESS and TCO Timer Halt */ + tco_configure(); + + /* Program SMBUS_BASE_ADDRESS and Enable it */ + smbus_common_init(); +} diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index 2c4ba67..7f1be73 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 init */ + pch_init(); /* initialize Heci interface */ heci_init(HECI1_BASE_ADDRESS);
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Maulik V Vaghela (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)