Hello Karthikeyan Ramasubramanian,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32448
to review the following change.
Change subject: mb/google/octopus: Clear the GPI IS & IE registers ......................................................................
mb/google/octopus: Clear the GPI IS & IE registers
Clear the GPI Interrupt Status & Enable registers before configuring the GPIO pad to prevent any interrupt storms due to GPI.
BUG=b:130593883 BRANCH=octopus TEST=Ensure that the Interrupt status & enable registers are reset during the boot up when the system is brought out of G3, S5 & S3. Ensure that the system boots fine to ChromeOS.
Change-Id: Ia3b9d3bf08472219348e20b53bae470c589039fb Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/mainboard/google/octopus/mainboard.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32448/1
diff --git a/src/mainboard/google/octopus/mainboard.c b/src/mainboard/google/octopus/mainboard.c index 4316ffe..f3a015d 100644 --- a/src/mainboard/google/octopus/mainboard.c +++ b/src/mainboard/google/octopus/mainboard.c @@ -71,6 +71,7 @@ base_pads = variant_base_gpio_table(&base_num); override_pads = variant_override_gpio_table(&override_num);
+ gpi_clear_int_cfg(); gpio_configure_pads_with_override(base_pads, base_num, override_pads, override_num);
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: mb/google/octopus: Clear the GPI IS & IE registers ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... PS1, Line 74: gpi_clear_int_cfg I don't think this is something mainboard specific. It applies to all boards. Can we put the call in intel common code?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: mb/google/octopus: Clear the GPI IS & IE registers ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... PS1, Line 74: gpi_clear_int_cfg
I don't think this is something mainboard specific. It applies to all boards. […]
I prefer that too. But I did this way since I wasn't sure about the right location in intel common code to invoke this function. I can see GPI SMI STS registers are cleared in common smihandler.c. But I am not sure if that is the right location to invoke this function.
Another lame reason is to rollout invoking this function in a phased manner so that I am not causing any side effect in other SoCs/mainboards. But definitely open to putting in common intel code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: mb/google/octopus: Clear the GPI IS & IE registers ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... PS1, Line 74: gpi_clear_int_cfg
I prefer that too. […]
It can be done in a few ways:
1. bootblock_soc_init can make a call to gpio_clear_int_cfg() to make sure any stable interrupt configuration is cleared. That can be done on a per-SoC basis.
2. You can also do this in gpio_configure_pads to ensure that the interrupt configuration is cleared out before any pad is configured. This will apply to all SoCs using the common code.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: mb/google/octopus: Clear the GPI IS & IE registers ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... PS1, Line 74: gpi_clear_int_cfg
It can be done in a few ways: […]
I will pick up option 1, since that seems to give more control regarding where this function is used.
Hello Karthikeyan Ramasubramanian, Kane Chen, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32448
to look at the new patch set (#2).
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers
Clear the GPI Interrupt Status & Enable registers to prevent any interrupt storms due to GPI.
BUG=b:130593883 BRANCH=octopus TEST=Ensure that the Interrupt status & enable registers are reset during the boot up when the system is brought out of G3, S5 & S3. Ensure that the system boots fine to ChromeOS.
Change-Id: Ia3b9d3bf08472219348e20b53bae470c589039fb Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/apollolake/bootblock/bootblock.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32448/2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/32448/1/src/mainboard/google/octopus/mainboa... PS1, Line 74: gpi_clear_int_cfg
I will pick up option 1, since that seems to give more control regarding where this function is used […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32448/2/src/soc/intel/apollolake/bootblock/b... File src/soc/intel/apollolake/bootblock/bootblock.c:
https://review.coreboot.org/#/c/32448/2/src/soc/intel/apollolake/bootblock/b... PS2, Line 128: gpi_clear_int_cfg Can you please add a comment indicating why this is necessary?
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Kane Chen, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32448
to look at the new patch set (#3).
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers
Clear the GPI Interrupt Status & Enable registers to prevent any interrupt storms due to GPI.
BUG=b:130593883 BRANCH=octopus TEST=Ensure that the Interrupt status & enable registers are reset during the boot up when the system is brought out of G3, S5 & S3. Ensure that the system boots fine to ChromeOS.
Change-Id: Ia3b9d3bf08472219348e20b53bae470c589039fb Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/apollolake/bootblock/bootblock.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32448/3
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Kane Chen, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32448
to look at the new patch set (#4).
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers
Clear the GPI Interrupt Status & Enable registers to prevent any interrupt storms due to GPI.
BUG=b:130593883 BRANCH=octopus TEST=Ensure that the Interrupt status & enable registers are reset during the boot up when the system is brought out of G3, S5 & S3. Ensure that the system boots fine to ChromeOS.
Change-Id: Ia3b9d3bf08472219348e20b53bae470c589039fb Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/apollolake/bootblock/bootblock.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32448/4
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32448/2/src/soc/intel/apollolake/bootblock/b... File src/soc/intel/apollolake/bootblock/bootblock.c:
https://review.coreboot.org/#/c/32448/2/src/soc/intel/apollolake/bootblock/b... PS2, Line 128: gpi_clear_int_cfg
Can you please add a comment indicating why this is necessary?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32448/4/src/soc/intel/apollolake/bootblock/b... File src/soc/intel/apollolake/bootblock/bootblock.c:
https://review.coreboot.org/#/c/32448/4/src/soc/intel/apollolake/bootblock/b... PS4, Line 130: * interrupt storm during the kernel bootup. It would be good to also mentioned that these registers do not get reset to default when booting back from S5.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32448 )
Change subject: soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers ......................................................................
soc/intel/apollolake/bootblock: Clear the GPI IS & IE registers
Clear the GPI Interrupt Status & Enable registers to prevent any interrupt storms due to GPI.
BUG=b:130593883 BRANCH=octopus TEST=Ensure that the Interrupt status & enable registers are reset during the boot up when the system is brought out of G3, S5 & S3. Ensure that the system boots fine to ChromeOS.
Change-Id: Ia3b9d3bf08472219348e20b53bae470c589039fb Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32448 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/apollolake/bootblock/bootblock.c 1 file changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/bootblock/bootblock.c b/src/soc/intel/apollolake/bootblock/bootblock.c index ac6903a..c791378 100644 --- a/src/soc/intel/apollolake/bootblock/bootblock.c +++ b/src/soc/intel/apollolake/bootblock/bootblock.c @@ -122,3 +122,12 @@ paging_enable_for_car("pdpt", "pt"); } } + +void bootblock_soc_init(void) +{ + /* + * Clear the GPI interrupt enable & status registers to avoid any + * interrupt storm during the kernel bootup. + */ + gpi_clear_int_cfg(); +}
-
Furquan Shaikh (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)