Usha P has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
soc/intel/skylake: 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 soraka.
Change-Id: Idf7b04edc3fce147f7857591ce7d5a0cd03f43fe Signed-off-by: Usha P usha.p@intel.com --- M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/include/soc/romstage.h M src/soc/intel/skylake/romstage/Makefile.inc A src/soc/intel/skylake/romstage/pch.c M src/soc/intel/skylake/romstage/romstage.c 5 files changed, 32 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/36672/1
diff --git a/src/soc/intel/skylake/bootblock/pch.c b/src/soc/intel/skylake/bootblock/pch.c index c95a8d8..5916589 100644 --- a/src/soc/intel/skylake/bootblock/pch.c +++ b/src/soc/intel/skylake/bootblock/pch.c @@ -2,7 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2014 Google Inc. - * Copyright (C) 2015-2018 Intel Corporation. + * Copyright (C) 2015-2019 Intel Corporation. * * 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 @@ -24,8 +24,6 @@ #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/p2sb.h> @@ -34,8 +32,6 @@ #include <soc/pcr_ids.h> #include <soc/pm.h> #include <soc/pmc.h> -#include <soc/smbus.h> - #include "../chip.h"
#define PCR_DMI_DMICTL 0x2234 @@ -164,12 +160,6 @@ */ soc_config_pwrmbase();
- /* 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/skylake/include/soc/romstage.h b/src/soc/intel/skylake/include/soc/romstage.h index 364bf52..6746526 100644 --- a/src/soc/intel/skylake/include/soc/romstage.h +++ b/src/soc/intel/skylake/include/soc/romstage.h @@ -21,6 +21,7 @@
void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); +void pch_init(void); int smbus_read_byte(unsigned int device, unsigned int address); /* Board type */ enum board_type { diff --git a/src/soc/intel/skylake/romstage/Makefile.inc b/src/soc/intel/skylake/romstage/Makefile.inc index dff89ce..1b069b6 100644 --- a/src/soc/intel/skylake/romstage/Makefile.inc +++ b/src/soc/intel/skylake/romstage/Makefile.inc @@ -1,3 +1,4 @@ romstage-y += ../../../../cpu/intel/car/romstage.c romstage-y += romstage.c romstage-y += systemagent.c +romstage-y += pch.c diff --git a/src/soc/intel/skylake/romstage/pch.c b/src/soc/intel/skylake/romstage/pch.c new file mode 100644 index 0000000..88a7cc7 --- /dev/null +++ b/src/soc/intel/skylake/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/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index a72b261..2904f05 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -146,7 +146,8 @@
/* Program MCHBAR, DMIBAR, GDXBAR and EDRAMBAR */ systemagent_early_init(); - + /* Program PCH init */ + pch_init(); ps = pmc_get_power_state(); s3wake = pmc_fill_power_state(ps) == ACPI_S3; fsp_memory_init(s3wake);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure(); Why is halting the TCO timer done in romstage? It would reset the PC if verstage takes longer than 2TCO cycles to boot.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... PS1, Line 149: pch_early_init pch_init?
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure();
Why is halting the TCO timer done in romstage? […]
can you please refer me the document ?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure();
can you please refer me the document ?
Document Number: 332691-003EN Chapter 30.1.6.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure();
Document Number: 332691-003EN Chapter 30.1.6.
unable to locate the document, can you please tell me the document name ?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure();
unable to locate the document, can you please tell me the document name ?
Intel ® 100 Series and Intel ® C230 Series Chipset Family Platform Controller Hub (PCH)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/romst... PS1, Line 23: tco_configure();
Intel ® 100 Series and Intel ® C230 Series Chipset Family Platform Controller Hub (PCH)
got it, you are referring to TCO timer halt bit implementation. it doesn't refer what you have written above.
this is programming recommendation for TCO in BIOS
After booting, the System BIOS must disable the TCO logic within 4.8 seconds or this reset will occur. The System BIOS should disable the TCO watchdog timer from resetting the system by setting the NR bit. The OS or device driver will be responsible for resetting this bit after the TCO driver loads.
The TCO_TMR_HALT bit, TCOBASE + 08h[11], if set can halt the TCO timer. Since TCO SMIs can be undesirable in some implementations, especially if the timer reload software had not been enabled, this provides a way for the software to halt the TCO timer.
doc : 33742
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36672
to look at the new patch set (#2).
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
soc/intel/skylake: 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 soraka.
Change-Id: Idf7b04edc3fce147f7857591ce7d5a0cd03f43fe Signed-off-by: Usha P usha.p@intel.com --- M src/soc/intel/skylake/bootblock/bootblock.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/include/soc/bootblock.h M src/soc/intel/skylake/include/soc/romstage.h M src/soc/intel/skylake/romstage/Makefile.inc A src/soc/intel/skylake/romstage/pch.c M src/soc/intel/skylake/romstage/romstage.c 7 files changed, 35 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/36672/2
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... PS1, Line 149: pch_early_init
pch_init?
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 2: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/36672/1/src/soc/intel/skylake/bootb... PS1, Line 149: pch_early_init
Done
Done
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
soc/intel/skylake: 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 soraka.
Change-Id: Idf7b04edc3fce147f7857591ce7d5a0cd03f43fe Signed-off-by: Usha P usha.p@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36672 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/skylake/bootblock/bootblock.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/include/soc/bootblock.h M src/soc/intel/skylake/include/soc/romstage.h M src/soc/intel/skylake/romstage/Makefile.inc A src/soc/intel/skylake/romstage/pch.c M src/soc/intel/skylake/romstage/romstage.c 7 files changed, 35 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Subrata Banik: Looks good to me, approved
diff --git a/src/soc/intel/skylake/bootblock/bootblock.c b/src/soc/intel/skylake/bootblock/bootblock.c index 596e3f1..d1fbb83 100644 --- a/src/soc/intel/skylake/bootblock/bootblock.c +++ b/src/soc/intel/skylake/bootblock/bootblock.c @@ -44,7 +44,6 @@ * and abase, i2c programming and print platform info */ report_platform_info(); - pch_early_init(); - + pch_init(); gspi_early_bar_init(); } diff --git a/src/soc/intel/skylake/bootblock/pch.c b/src/soc/intel/skylake/bootblock/pch.c index c95a8d8..332060e 100644 --- a/src/soc/intel/skylake/bootblock/pch.c +++ b/src/soc/intel/skylake/bootblock/pch.c @@ -2,7 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2014 Google Inc. - * Copyright (C) 2015-2018 Intel Corporation. + * Copyright (C) 2015-2019 Intel Corporation. * * 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 @@ -24,8 +24,6 @@ #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/p2sb.h> @@ -34,8 +32,6 @@ #include <soc/pcr_ids.h> #include <soc/pm.h> #include <soc/pmc.h> -#include <soc/smbus.h> - #include "../chip.h"
#define PCR_DMI_DMICTL 0x2234 @@ -150,7 +146,7 @@ pch_enable_lpc(); }
-void pch_early_init(void) +void pch_init(void) { /* * Enabling ABASE for accessing PM1_STS, PM1_EN, PM1_CNT, @@ -164,12 +160,6 @@ */ soc_config_pwrmbase();
- /* 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/skylake/include/soc/bootblock.h b/src/soc/intel/skylake/include/soc/bootblock.h index a40f439..302db50 100644 --- a/src/soc/intel/skylake/include/soc/bootblock.h +++ b/src/soc/intel/skylake/include/soc/bootblock.h @@ -24,7 +24,7 @@
/* Bootblock post console init programming */ void i2c_early_init(void); -void pch_early_init(void); +void pch_init(void); void pch_early_iorange_init(void); void report_platform_info(void); void report_memory_config(void); diff --git a/src/soc/intel/skylake/include/soc/romstage.h b/src/soc/intel/skylake/include/soc/romstage.h index 364bf52..6746526 100644 --- a/src/soc/intel/skylake/include/soc/romstage.h +++ b/src/soc/intel/skylake/include/soc/romstage.h @@ -21,6 +21,7 @@
void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); +void pch_init(void); int smbus_read_byte(unsigned int device, unsigned int address); /* Board type */ enum board_type { diff --git a/src/soc/intel/skylake/romstage/Makefile.inc b/src/soc/intel/skylake/romstage/Makefile.inc index dff89ce..1b069b6 100644 --- a/src/soc/intel/skylake/romstage/Makefile.inc +++ b/src/soc/intel/skylake/romstage/Makefile.inc @@ -1,3 +1,4 @@ romstage-y += ../../../../cpu/intel/car/romstage.c romstage-y += romstage.c romstage-y += systemagent.c +romstage-y += pch.c diff --git a/src/soc/intel/skylake/romstage/pch.c b/src/soc/intel/skylake/romstage/pch.c new file mode 100644 index 0000000..88a7cc7 --- /dev/null +++ b/src/soc/intel/skylake/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/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index a72b261..2904f05 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -146,7 +146,8 @@
/* Program MCHBAR, DMIBAR, GDXBAR and EDRAMBAR */ systemagent_early_init(); - + /* Program PCH init */ + pch_init(); ps = pmc_get_power_state(); s3wake = pmc_fill_power_state(ps) == ACPI_S3; fsp_memory_init(s3wake);
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36672 )
Change subject: soc/intel/skylake: Refactor pch_early_init() code ......................................................................
Patch Set 3:
Having 2 functions with identical names in different stages is not a good idea. It makes some headers mutually exclusive in one file and is just confusing in general. Why not have prefixes like bootblock_pch_init and romstage_pch_init?
-
Arthur Heymans (Code Review) -
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)
-
Usha P (Code Review)