Change in coreboot[master]: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common lo...
Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location
Some of the bootblock functions defined in soc files are common hence move them to common to reduce code redundancy.
BUG=None BRANCH=None TEST=Build and boot hatch board
Change-Id: Id7c563023c4b1ff4f42734f18b923d13afef4770 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/bootblock/bootblock.c A src/soc/intel/common/basecode/bootblock/Kconfig A src/soc/intel/common/basecode/bootblock/Makefile.inc A src/soc/intel/common/basecode/bootblock/bootblock.c M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/bootblock/bootblock.c M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/bootblock/bootblock.c 9 files changed, 45 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35277/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index d949fff..2c6a1ce 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -82,6 +82,8 @@ select PMC_GLOBAL_RESET_ENABLE_LOCK select SOC_INTEL_COMMON select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE + select SOC_INTEL_COMMON_BASECODE + select SOC_INTEL_COMMON_BASECODE_BOOTBLOCK select SOC_INTEL_COMMON_BLOCK select SOC_INTEL_COMMON_BLOCK_ACPI select SOC_INTEL_COMMON_BLOCK_CHIP_CONFIG diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c index 653ba30..386c40c 100644 --- a/src/soc/intel/cannonlake/bootblock/bootblock.c +++ b/src/soc/intel/cannonlake/bootblock/bootblock.c @@ -41,22 +41,6 @@ }; #endif
-asmlinkage void bootblock_c_entry(uint64_t base_timestamp) -{ - /* Call lib/bootblock.c main */ - bootblock_main_with_basetime(base_timestamp); -} - -void bootblock_soc_early_init(void) -{ - bootblock_systemagent_early_init(); - bootblock_pch_early_init(); - bootblock_cpu_init(); - pch_early_iorange_init(); - if (CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) - uart_bootblock_init(); -} - void bootblock_soc_init(void) { /* diff --git a/src/soc/intel/common/basecode/bootblock/Kconfig b/src/soc/intel/common/basecode/bootblock/Kconfig new file mode 100644 index 0000000..626685e --- /dev/null +++ b/src/soc/intel/common/basecode/bootblock/Kconfig @@ -0,0 +1,3 @@ +config SOC_INTEL_COMMON_BASECODE_BOOTBLOCK + bool + default n diff --git a/src/soc/intel/common/basecode/bootblock/Makefile.inc b/src/soc/intel/common/basecode/bootblock/Makefile.inc new file mode 100644 index 0000000..7b1a683 --- /dev/null +++ b/src/soc/intel/common/basecode/bootblock/Makefile.inc @@ -0,0 +1 @@ +bootblock-$(CONFIG_SOC_INTEL_COMMON_BASECODE_BOOTBLOCK) +=bootblock.c diff --git a/src/soc/intel/common/basecode/bootblock/bootblock.c b/src/soc/intel/common/basecode/bootblock/bootblock.c new file mode 100644 index 0000000..e2dd89c --- /dev/null +++ b/src/soc/intel/common/basecode/bootblock/bootblock.c @@ -0,0 +1,35 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2017-2018 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 + * 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 <bootblock_common.h> +#include <intelblocks/uart.h> +#include <soc/bootblock.h> +#include <soc/pch.h> + +asmlinkage void bootblock_c_entry(uint64_t base_timestamp) +{ + /* Call lib/bootblock.c main */ + bootblock_main_with_basetime(base_timestamp); +} + +void bootblock_soc_early_init(void) +{ + bootblock_systemagent_early_init(); + bootblock_pch_early_init(); + bootblock_cpu_init(); + pch_early_iorange_init(); + if (CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) + uart_bootblock_init(); +} diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 1bd478c..80f5f57 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -37,6 +37,8 @@ select SOC_AHCI_PORT_IMPLEMENTED_INVERT select PMC_GLOBAL_RESET_ENABLE_LOCK select SOC_INTEL_COMMON + select SOC_INTEL_COMMON_BASECODE + select SOC_INTEL_COMMON_BASECODE_BOOTBLOCK select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE select SOC_INTEL_COMMON_BLOCK select SOC_INTEL_COMMON_BLOCK_ACPI diff --git a/src/soc/intel/icelake/bootblock/bootblock.c b/src/soc/intel/icelake/bootblock/bootblock.c index db43e50..3ecb7b7 100644 --- a/src/soc/intel/icelake/bootblock/bootblock.c +++ b/src/soc/intel/icelake/bootblock/bootblock.c @@ -20,22 +20,6 @@ #include <soc/iomap.h> #include <soc/pch.h>
-asmlinkage void bootblock_c_entry(uint64_t base_timestamp) -{ - /* Call lib/bootblock.c main */ - bootblock_main_with_basetime(base_timestamp); -} - -void bootblock_soc_early_init(void) -{ - bootblock_systemagent_early_init(); - bootblock_pch_early_init(); - bootblock_cpu_init(); - pch_early_iorange_init(); - if (CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) - uart_bootblock_init(); -} - void bootblock_soc_init(void) { report_platform_info(); diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 9cb8d45..60c04e5 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -52,6 +52,8 @@ select PMC_GLOBAL_RESET_ENABLE_LOCK select SOC_INTEL_COMMON select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE + select SOC_INTEL_COMMON_BASECODE + select SOC_INTEL_COMMON_BASECODE_BOOTBLOCK select SOC_INTEL_COMMON_BLOCK select SOC_INTEL_COMMON_BLOCK_CAR select SOC_INTEL_COMMON_BLOCK_CHIP_CONFIG diff --git a/src/soc/intel/skylake/bootblock/bootblock.c b/src/soc/intel/skylake/bootblock/bootblock.c index e9ca2d8..4eb040f 100644 --- a/src/soc/intel/skylake/bootblock/bootblock.c +++ b/src/soc/intel/skylake/bootblock/bootblock.c @@ -19,23 +19,6 @@ #include <intelblocks/uart.h> #include <soc/bootblock.h>
-asmlinkage void bootblock_c_entry(uint64_t base_timestamp) -{ - /* Call lib/bootblock.c main */ - bootblock_main_with_basetime(base_timestamp); -} - -void bootblock_soc_early_init(void) -{ - bootblock_systemagent_early_init(); - bootblock_pch_early_init(); - bootblock_cpu_init(); - pch_early_iorange_init(); - - if (CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) - uart_bootblock_init(); -} - void bootblock_soc_init(void) { /* FSP 2.0 does not provide FSP-T/TempRamInit init support yet */
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Kconfig:
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... PS1, Line 4: help text ?
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... PS1, Line 1: bootblock-$(CONFIG_SOC_INTEL_COMMON_BASECODE_BOOTBLOCK) +=bootblock.c space ?
Hello Patrick Rudolph, Subrata Banik, 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/+/35277
to look at the new patch set (#3).
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location
Some of the bootblock functions defined in soc files are common hence move them to common to reduce code redundancy.
BUG=None BRANCH=None TEST=Build and boot hatch board
Change-Id: Id7c563023c4b1ff4f42734f18b923d13afef4770 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/bootblock/bootblock.c A src/soc/intel/common/basecode/bootblock/Kconfig A src/soc/intel/common/basecode/bootblock/Makefile.inc A src/soc/intel/common/basecode/bootblock/bootblock.c M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/bootblock/bootblock.c M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/bootblock/bootblock.c 9 files changed, 47 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35277/3
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Kconfig:
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... PS1, Line 4:
help text ?
Done
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35277/1/src/soc/intel/common/baseco... PS1, Line 1: bootblock-$(CONFIG_SOC_INTEL_COMMON_BASECODE_BOOTBLOCK) +=bootblock.c
space ?
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
What is the motivation for 'basecode' in the pathname and Konfig name? Looks a bit redundant to 'common'.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
Patch Set 3:
What is the motivation for 'basecode' in the pathname and Konfig name? Looks a bit redundant to 'common'.
basecode is something which is not conventional IP blocks that present inside soc/intel/common/block/.
the intention of basecode to have common non ip library code for an example: finalize.c, memmap.c, elog.c, stage files like bootblock.c, romstage.c, fsp_params.c etc.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
Patch Set 3:
What is the motivation for 'basecode' in the pathname and Konfig name? Looks a bit redundant to 'common'.
I found CB:25734 where Aaron had questions about what ends up in basecode/. Does this answer it now? This seems like the first implementation landing with 'select xxx_BASECODE_xxx' so is the tree layout clear now?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl I don't see apl in here. Is that intentional? What is your plan on enabling platforms other than big core? Do they fit into this model?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Kconfig:
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 5: all Is that true?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 18: soc/bootblock.h So, every SoC is expected to provide the declarations for the functions?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 19: soc/pch.h Why is this included?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 20: If a new platform decides to use this functionality, what are the functions that it is expected to provide? It is not clear at all from this change what is used from common/block and what is expected to be provided by SoC.
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 30: bootblock_pch_early_init This assumes that CONFIG_SOC_INTEL_COMMON_BLOCK_SA is selected by the SoC. Do you plan to auto select that based on the config selection? Similarly, whats the plan for other CONFIG_* that are assumed to be selected?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
I don't see apl in here. […]
APL/GLK, I intentionally left out since it is different from all other current platforms and the future onces. This model will fit well for the current SoCs (SKL/CNL/ICL/CML) and future (TGL/JSL etc). And the upcoming small core platforms will use similar code structure as in bigcore.
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Kconfig:
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 5: all
Is that true?
Nope.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
APL/GLK, I intentionally left out since it is different from all other current platforms and the fut […]
OTOH i see https://review.coreboot.org/c/coreboot/+/18457, this also covers APL, will check with the owner if this can be rebased and pushed.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
OTOH i see https://review.coreboot. […]
I would like to see why APL/GLK is different and what are the factors that make it a bad fit for this model in case it cannot be done.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
I would like to see why APL/GLK is different and what are the factors that make it a bad fit for thi […]
It is not the case of a bad fit. The patch is just moving some function definitions that are identical across SoC to a common location. For APL/GLK the function definitions are not same. Now, APL/GLK can be also be included in a common code model by making early init sequence as part of some common functions across SoCs which is being done in the pathset that I noted previously.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
It is not the case of a bad fit. […]
Also, as mentioned earlier the future ones (even the small core) would follow similar code structure as today's big core.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
APL/GLK can be also be included in a common code model by making early init sequence as part of some common functions across SoCs which is being done in the pathset that I noted previously.
Okay. So, I believe you are working on identifying how those changes can be rebased on top of this.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
APL/GLK can be also be included in a common code model by making early init sequence as part of so […]
Yes, that is the idea. This patch would just be a precursor to that one.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl
Yes, that is the idea. This patch would just be a precursor to that one.
Done
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35277?usp=email )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Furquan Shaikh (Code Review) -
Kyösti Mälkki (Code Review)
-
Martin L Roth (Code Review)
-
Rizwan Qureshi (Code Review)
-
Subrata Banik (Code Review)