Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
soc/intel/common/pch: Add Intel common reset code
Move all common SoC reset functionality into pch/reset and let respective SoC to select required Kconfig to make use of the common reset code block.
TEST=Able to boot CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/pch/reset/Kconfig A src/soc/intel/common/pch/reset/Makefile.inc A src/soc/intel/common/pch/reset/reset.c 3 files changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/1
diff --git a/src/soc/intel/common/pch/reset/Kconfig b/src/soc/intel/common/pch/reset/Kconfig new file mode 100644 index 0000000..8d129ce --- /dev/null +++ b/src/soc/intel/common/pch/reset/Kconfig @@ -0,0 +1,5 @@ +config SOC_INTEL_COMMON_PCH_RESET + bool + default n + help + This option allows to have common function to handle chipset reset request. diff --git a/src/soc/intel/common/pch/reset/Makefile.inc b/src/soc/intel/common/pch/reset/Makefile.inc new file mode 100644 index 0000000..3decf69 --- /dev/null +++ b/src/soc/intel/common/pch/reset/Makefile.inc @@ -0,0 +1,2 @@ +romstage-$(CONFIG_SOC_INTEL_COMMON_PCH_RESET) += reset.c +ramstage-$(CONFIG_SOC_INTEL_COMMON_PCH_RESET) += reset.c diff --git a/src/soc/intel/common/pch/reset/reset.c b/src/soc/intel/common/pch/reset/reset.c new file mode 100644 index 0000000..d37ff54 --- /dev/null +++ b/src/soc/intel/common/pch/reset/reset.c @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cf9_reset.h> +#include <console/console.h> +#include <intelblocks/cse.h> +#include <intelblocks/pmclib.h> +#include <fsp/util.h> +#include <soc/intel/common/reset.h> +#include <soc/pci_devs.h> + +void do_global_reset(void) +{ + /* Ask CSE to do the global reset */ + if (cse_request_global_reset(GLOBAL_RESET)) + return; + + /* global reset if CSE fail to reset */ + pmc_global_reset_enable(1); + do_full_reset(); +} + +void chipset_handle_reset(uint32_t status) +{ + switch (status) { + case FSP_STATUS_RESET_REQUIRED_3: /* Global Reset */ + printk(BIOS_DEBUG, "GLOBAL RESET!!\n"); + global_reset(); + break; + default: + printk(BIOS_ERR, "unhandled reset type %x\n", status); + die("unknown reset type"); + break; + } +}
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... PS1, Line 14: cse_request_global_reset I think this depends on CSE support? SKL has a different reset.c file (see next patch)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... PS1, Line 14: cse_request_global_reset
I think this depends on CSE support? SKL has a different reset. […]
initiate the test on SKL and KBL platform, will get back on result
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
PS1: I just remembered we already have `src/soc/intel/common/reset.c`, maybe we could merge both files in a follow-up commit?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45336
to look at the new patch set (#2).
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
soc/intel/common/pch: Add Intel common reset code
Move all common SoC reset functionality into pch/reset and let respective SoC to select required Kconfig to make use of the common reset code block.
TEST=Able to boot CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/pch/reset/Kconfig A src/soc/intel/common/pch/reset/Makefile.inc A src/soc/intel/common/pch/reset/reset.c 3 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
PS1:
I just remembered we already have `src/soc/intel/common/reset. […]
yes, i believe that would be good idea to move this file from here to soc/intel/common/reset.c as incremental CL
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45336/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/2//COMMIT_MSG@13 PS2, Line 13: TEST=Able to boot CML and TGL platform. Are you able to reset them too? 😊
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 14: * BIOS should ensure it does a global reset : * to reset both host and Intel ME by setting : * PCH PMC [B0:D31:F2 register offset 0xAC bit 20] : */ 96 chars wide on new files
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 20: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port : * to global reset platform */ same here
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 28: /* If ME unable to reset platform then : * force global reset using PMC CF9GR register*/ same
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 37: FSP_STATUS_RESET_REQUIRED_3 I think this value is chipset FSP-dependent? I see that we use _3 for most but APL uses _5. Should there be a macro FSP_STATUS_GLOBAL_RESET which can be defined by SoCs as per their FSP implementation?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 14: * BIOS should ensure it does a global reset : * to reset both host and Intel ME by setting : * PCH PMC [B0:D31:F2 register offset 0xAC bit 20] : */
96 chars wide on new files
For running text, I'd prefer shorter lines. I think reflowing this to fit in two lines would be good enough.
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 20: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port : * to global reset platform */
same here
Should fit in one line
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 28: /* If ME unable to reset platform then : * force global reset using PMC CF9GR register*/
same
I'd place the comment outside of the if-block, then it might fit in a single line. Plus, the braces can then be dropped.
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 37: FSP_STATUS_RESET_REQUIRED_3
I think this value is chipset FSP-dependent? I see that we use _3 for most but APL uses _5. […]
The only exceptions are Apollo Lake and Denverton-NS. APL uses _3 for shutdown reset (although the integration guide says it is not currently used) and _5 for global reset, whereas DNV does not seem to support requesting any resets at all. If only APL needs special treatment, we could have an exception for it in here, but it's a bit ugly. We could also have a Kconfig option that only gets selected by APL. In any case, this difference is documented in integration guides and is small enough to account for it in common code.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/pch: Add Intel common reset code ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45336/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/2//COMMIT_MSG@13 PS2, Line 13: TEST=Able to boot CML and TGL platform.
Are you able to reset them too? 😊
😎
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 14: * BIOS should ensure it does a global reset : * to reset both host and Intel ME by setting : * PCH PMC [B0:D31:F2 register offset 0xAC bit 20] : */
For running text, I'd prefer shorter lines. […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 20: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port : * to global reset platform */
Should fit in one line
Ack
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 28: /* If ME unable to reset platform then : * force global reset using PMC CF9GR register*/
I'd place the comment outside of the if-block, then it might fit in a single line. […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/2/src/soc/intel/common/pch/re... PS2, Line 37: FSP_STATUS_RESET_REQUIRED_3
The only exceptions are Apollo Lake and Denverton-NS. […]
Ack
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45336
to look at the new patch set (#3).
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
soc/intel/common/block: Add Intel common reset code
Move all common SoC reset functionality into common block and let respective SoC to select required Kconfig to make use of the common reset code block.
TEST=Able to boot and reset CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/block/reset/Kconfig A src/soc/intel/common/block/reset/Makefile.inc A src/soc/intel/common/block/reset/reset.c 3 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/3
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 5: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
PS1:
yes, i believe that would be good idea to move this file from here to soc/intel/common/reset. […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 5:
(1 comment)
BTW, there i
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block BTW, I just noticed that there is already a reset.c in soc/intel/common: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/rese...
How does this common "reset" compare to that? Shouldn't the two be really combined into a single file?
Another thing: soc/intel/common/block is always used for an IP block. I don't think "reset" can be termed as an IP block. Shouldn't this code live directly under soc/intel/common as the reset.c file is already right now?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
BTW, I just noticed that there is already a reset.c in soc/intel/common: https://review.coreboot. […]
yes, i had thought on this as well but due to below reason I have decided to kept inside common block code (although reset is not any IP block but still a common place where we can refer both core and atom family code like power_limit, acpi etc)
1. Unresolved common code macros and function from "src/soc/intel/common/reset.c" like below because "src/soc/intel/common/reset.c" is included by older SoCs like BSW, Quark are not part of IA common code scope.
#include <intelblocks/cse.h> #include <intelblocks/pmclib.h>
cse_request_global_reset pmc_global_reset_enable
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45336
to look at the new patch set (#6).
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
soc/intel/common/block: Add Intel common reset code
Move all common SoC reset functionality into common block and let respective SoC to select required Kconfig to make use of the common reset code block.
Don't expect to call do_global_reset() from verstage and/or postcar.
TEST=Able to boot and reset CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/block/reset/Kconfig A src/soc/intel/common/block/reset/Makefile.inc A src/soc/intel/common/block/reset/reset.c 3 files changed, 51 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
yes, i had thought on this as well but due to below reason I have decided to kept inside common bloc […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... File src/soc/intel/common/pch/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/1/src/soc/intel/common/pch/re... PS1, Line 14: cse_request_global_reset
initiate the test on SKL and KBL platform, will get back on result
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 35: global_reset Why prefer `global_reset` here over `do_global_reset`? Although I will say, the naming here is getting kinda confusing.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset so, do_force_global_reset does not call force_global_reset? ;) this naming is *really* confusing. why not have this in (do_)force_global_reset()?
if (cse_request_global_reset()) return;
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
so, do_force_global_reset does not call force_global_reset? ;) this naming is *really* confusing. […]
First we should try to send global reset using CSE command GLOBAL_RESET if failure then we will use PMC global reset enable bit to get guaranteed global reset enforce.
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 35: global_reset
Why prefer `global_reset` here over `do_global_reset`? Although I will say, the naming here is getti […]
Tim, Prior to send CSE command we need to do some additional work as below hence prefer to have global_reset() call which will eventually redirect to here do_global_reset
vim src/soc/intel/common/reset.c +11
void global_reset(void) { printk(BIOS_INFO, "%s() called!\n", __func__); cf9_reset_prepare(); dcache_clean_all(); do_global_reset(); halt(); }
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
@Furquan/Angel, if you could help to review further.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
(although reset is not any IP block but still a common place where we can refer both core and atom family code like power_limit, acpi etc)
In my opinion, that is not correct. Even power_limit and ACPI should really not be part of common/block. Instead, `basecode` might be a better location? There is nothing under `basecode` right now, so I am unsure what that is supposed to contain. I think we need to keep the separation clean w.r.t. IP blocks and rest of the common code.
Unresolved common code macros and function from
Wouldn't those be optimized out by the linker since the older SoCs won't use the cse/pmc related functions?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
(although reset is not any IP block but still a common place where we can refer both core and atom family code like power_limit, acpi etc)
In my opinion, that is not correct. Even power_limit and ACPI should really not be part of common/block. Instead, `basecode` might be a better location? There is nothing under `basecode` right now, so I am unsure what that is supposed to contain. I think we need to keep the separation clean w.r.t. IP blocks and rest of the common code.
we can use basecode, let me try moving this code into basecode and later power_limit and acpi later.
Unresolved common code macros and function from
Wouldn't those be optimized out by the linker since the older SoCs won't use the cse/pmc related functions?
without any guard, its giving compilation error saying cse_request_global_reset unknown function reference. We could use guard (#if) for sure but i really don't know if that is right way
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
First we should try to send global reset using CSE command GLOBAL_RESET if failure then we will use […]
Sure, I just don't like the naming scheme :-)
First, I'd like to find a better name for do_global_reset, since naming whise it's not clear what the difference between global_reset vs. do_global_reset is. That's simply confusing. (another, exaggerated example would be having global_reset, do_global_reset, really_do_global_reset, really_really_do_global_reset, ...) You know what I mean?
Second, do_global_reset is only ever called from do_force_global_reset, so why not move the prior to the latter?
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 34: !! isn't one enough?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Sure, I just don't like the naming scheme :-)
First, I'd like to find a better name for do_global_reset, since naming whise it's not clear what the difference between global_reset vs. do_global_reset is. That's simply confusing. (another, exaggerated example would be having global_reset, do_global_reset, really_do_global_reset, really_really_do_global_reset, ...) You know what I mean?
Yup 😊
Second, do_global_reset is only ever called from do_force_global_reset, so why not move the prior to the latter?
do_global_reset() is calling from below common reference hence we might need to change that code as well to make something meaning full. Finally do_force_global_reset() can be renamed as pmc_assist_global_reset() which is the actual case ?
vim src/soc/intel/common/reset.c +11
void global_reset(void) {
printk(BIOS_INFO, "%s() called!\n", __func__); cf9_reset_prepare(); dcache_clean_all(); do_global_reset(); halt(); }
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Second, do_global_reset is only ever called from do_force_global_reset, so why not move the prior to the latter?
do_global_reset() is calling from below common reference hence we might need to change that code as well to make something meaning full. Finally do_force_global_reset() can be renamed as pmc_assist_global_reset() which is the actual case ?
See, how I am confused? :P Actually I meant "do_force_global_reset is only ever called from do_global_reset".
What about this?
static void force_global_reset(void) { if (cse_request_global_reset()) return; /* TODO: or halt()? */
/* * If ME is unable to reset platform then enable the PMC CF9GR register and * force a global reset by writing 0x06 or 0x0E. */
pmc_global_reset_enable(true);  /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */  do_full_reset(); // another really_really_do_... ;-) } 
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/block: Add Intel common reset code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Second, do_global_reset is only ever called from do_force_global_reset, so why not move the prior to the latter?
do_global_reset() is calling from below common reference hence we might need to change that code as well to make something meaning full. Finally do_force_global_reset() can be renamed as pmc_assist_global_reset() which is the actual case ?
See, how I am confused? :P Actually I meant "do_force_global_reset is only ever called from do_global_reset".
Never mind, I understood what u meant 😊
What about this?
static void force_global_reset(void) { if (cse_request_global_reset()) return; /* TODO: or halt()? */
/* * If ME is unable to reset platform then enable the PMC CF9GR register and * force a global reset by writing 0x06 or 0x0E. */
pmc_global_reset_enable(true);  /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */  do_full_reset(); // another really_really_do_... ;-) } 
Is this reasonable enough ? hence we can avoid the need of another static function ?
void do_global_reset(void) { /* Ask CSE to do the global reset */ if (cse_request_global_reset(GLOBAL_RESET)) return;
/* * If ME is unable to reset platform then enable the PMC CF9GR register and * force a global reset by writing 0x06 or 0x0E. */ pmc_global_reset_enable(true); /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */ do_full_reset(); }
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45336
to look at the new patch set (#8).
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
soc/intel/common/basecode: Add Intel common reset code
Move all common SoC reset functionality into common basecode and let respective SoC to select required Kconfig to make use of the common reset code block.
Don't expect to call do_global_reset() from verstage and/or postcar.
TEST=Able to boot and reset CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/basecode/reset/Kconfig A src/soc/intel/common/basecode/reset/Makefile.inc A src/soc/intel/common/basecode/reset/reset.c 3 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
(although reset is not any IP block but still a common place where we can refer both core and at […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 34: !!
isn't one enough?
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Second, do_global_reset is only ever called from do_force_global_reset, so why not move the pr […]
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 29: case FSP_STATUS_GLOBAL_RESET: nit: Maybe put a comment indicating where this is supposed to be defined?
/* Defined in each SoC's chip.h */
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 11: do_global_reset force_global_reset?
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 22: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */ I'd say that's included above, so drop that comment maybe
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Ack
Well, we still have do_global_reset and global_reset; why is my force_global_reset (which the function actually does, right? enforce it?) wrong?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
without any guard, its giving compilation error saying cse_request_global_reset unknown function reference.
I think we can add a check for CONFIG(SOC_INTEL_COMMON_BLOCK_CSE). In general, I think it is confusing that there are two different reset files under common and with very similar functionality.
Can we please make an attempt to consolidate this into a single place? I don't think the cse function call requires #if. You can have a C check: if (CONFIG(SOC_INTEL_COMMON_BLOCK_CSE)) cse_request_global_reset();
One thing that I think we would need to keep separate is the chipset_handle_reset which is used only by FSP2.0+. Considering that, I think we can have two files under soc/intel/common/basecode/reset:
reset.c - Provides implementation of global_reset, do_global_reset, do_board_reset. fsp_reset.c - Provides implementation of chipset_handle_reset. This file will get included only if PLATFORM_USES_FSP2_0.
Thoughts?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
without any guard, its giving compilation error saying cse_request_global_reset unknown function r […]
Also, I think `do_global_reset()` doesn't really need to be exposed out of the common/basecode/reset/reset.c It should really only be used by `global_reset()`. There is a single caller of `do_global_reset()` which is within cse_lite.c. I think that should really be calling `global_reset()` and not `do_global_reset()`.
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 4: endif Why was postcar dropped?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 11: do_global_reset
force_global_reset?
I'd rather keep the name as-is, for consistency with `do_full_reset` and `do_system_reset` (cf9_reset.h)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 22: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */
I'd say that's included above, so drop that comment maybe
I might be weird, but I like to have a comment explaining why `do_full_reset` doesn't do a full reset. 😄
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 31: global_reset Hmmm, shouldn't this be `do_global_reset`?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 11: do_global_reset
I'd rather keep the name as-is, for consistency with `do_full_reset` and `do_system_reset` (cf9_rese […]
well, IMHO that should be reworded, too :S see what I wrote here https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/...
TL;DR wording wise there is no difference between do_<whatever> and <whatever>; it's simply confusing
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 31: global_reset
Hmmm, shouldn't this be `do_global_reset`?
see https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
Also, I think `do_global_reset()` doesn't really need to be exposed out of the common/basecode/reset […]
@Furquan see https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... and let's continue that discussion there, maybe? :)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... File src/soc/intel/common/block/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/7/src/soc/intel/common/block/... PS7, Line 27: do_force_global_reset
Well, we still have do_global_reset and global_reset; why is my force_global_reset (which the functi […]
+1. I think these names need to be clear. Currently, the functions are named too similar to each other and it is not very clear what the difference is.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 4: endif
Why was postcar dropped?
postcar being such a thin stage, do we see any scope for global reset. so far reset.c has only included for APL postcar hence dropped
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45336
to look at the new patch set (#9).
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
soc/intel/common/basecode: Add Intel common reset code
Move all common SoC reset functionality into common basecode and let respective SoC to select required Kconfig to make use of the common reset code block.
Don't expect to call force_global_reset() from verstage.
TEST=Able to boot and reset CML and TGL platform.
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: Icafe931ba9920501cf5448290ddd9f897760bb75 --- A src/soc/intel/common/basecode/reset/Kconfig A src/soc/intel/common/basecode/reset/Makefile.inc A src/soc/intel/common/basecode/reset/reset.c 3 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/45336/9
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Abandoned
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45336 )
Change subject: soc/intel/common/basecode: Add Intel common reset code ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45336/5//COMMIT_MSG@7 PS5, Line 7: soc/intel/common/block
@Furquan see https://review.coreboot. […]
@Furquan, please look at https://review.coreboot.org/c/coreboot/+/45541/1, its taken care
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 4: endif
postcar being such a thin stage, do we see any scope for global reset. so far reset. […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/reset/reset.c:
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 11: do_global_reset
well, IMHO that should be reworded, too :S see what I wrote here https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/45336/8/src/soc/intel/common/baseco... PS8, Line 22: /* Now BIOS can write 0x06 or 0x0E to 0xCF9 port to global reset platform */
I might be weird, but I like to have a comment explaining why `do_full_reset` doesn't do a full rese […]
Ack
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
HAOUAS Elyes (Code Review)
-
Michael Niewöhner (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)