Change in coreboot[master]: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL
This patch performs below operations: 1. Refactor soc_gpio_pm_configuration() and create new API to fill gpio pm configuration as in fill_soc_gpio_pm_configuration() 2. Pass GPIO PM configuration values between devicetree.cb to platform ASL
Now SoC code should be able to handle dynamic GPIO PM programming through ACPI/ASL
BUG=b:144002424 TEST=Verify devicetree.cb gpio_pm[] variables and GPMx ASL variables are in sync.
Change-Id: I75246be01aa4ab4cfa1e184ab6a11b718471995e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/gpio_common.c M src/soc/intel/cannonlake/include/soc/gpio.h 3 files changed, 21 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/37794/1
diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c index 6846594..31e3227 100644 --- a/src/soc/intel/cannonlake/acpi.c +++ b/src/soc/intel/cannonlake/acpi.c @@ -233,6 +233,10 @@ /* Set USB2/USB3 wake enable bitmaps. */ gnvs->u2we = config->usb2_wake_enable_bitmap; gnvs->u3we = config->usb3_wake_enable_bitmap; + + /* Fill SoC GPIO PM configuration */ + gnvs->ogpm = config->gpio_override_pm; + fill_soc_gpio_pm_configuration(gnvs->gpmv, TOTAL_GPIO_COMM); }
uint32_t acpi_fill_soc_wake(uint32_t generic_pm1_en, diff --git a/src/soc/intel/cannonlake/gpio_common.c b/src/soc/intel/cannonlake/gpio_common.c index 360189a..b6ed74a 100644 --- a/src/soc/intel/cannonlake/gpio_common.c +++ b/src/soc/intel/cannonlake/gpio_common.c @@ -17,6 +17,18 @@ #include <intelblocks/gpio.h> #include <soc/soc_chip.h>
+/* SoC rotine to fill GPIO PM mask and value for GPIO_MISCCFG register */ +void fill_soc_gpio_pm_configuration(uint8_t *misccfg_pm_values, size_t size) +{ + const config_t *config = config_of_soc(); + + if (config->gpio_override_pm) + memcpy(misccfg_pm_values, config->gpio_pm, sizeof(uint8_t) * size); + else + memset(misccfg_pm_values, MISCCFG_ENABLE_GPIO_PM_CONFIG, + sizeof(uint8_t) * size); +} + /* * Routine to perform below operations: * 1. SoC rotine to fill GPIO PM mask and value for GPIO_MISCCFG register @@ -25,14 +37,8 @@ void soc_gpio_pm_configuration(void) { uint8_t value[TOTAL_GPIO_COMM]; - const config_t *config = config_of_soc();
- if (config->gpio_override_pm) - memcpy(value, config->gpio_pm, sizeof(uint8_t) * - TOTAL_GPIO_COMM); - else - memset(value, MISCCFG_ENABLE_GPIO_PM_CONFIG, sizeof(uint8_t) * - TOTAL_GPIO_COMM); + fill_soc_gpio_pm_configuration(value, TOTAL_GPIO_COMM);
gpio_pm_configure(value, TOTAL_GPIO_COMM); } diff --git a/src/soc/intel/cannonlake/include/soc/gpio.h b/src/soc/intel/cannonlake/include/soc/gpio.h index efed881..0625c63 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio.h +++ b/src/soc/intel/cannonlake/include/soc/gpio.h @@ -26,8 +26,12 @@ #include <intelblocks/gpio.h>
#ifndef __ACPI__ +#include <stdint.h> struct pad_config; void cnl_configure_pads(const struct pad_config *cfg, size_t num_pads); + +/* SoC rotine to fill GPIO PM mask and value for GPIO_MISCCFG register */ +void fill_soc_gpio_pm_configuration(uint8_t *misccfg_pm_values, size_t size); /* * Routine to perform below operations: * 1. SoC routine to fill GPIO PM mask and value for GPIO_MISCCFG register
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... PS1, Line 27: else Do we need this? FPS-S will enable it anyway right? If we can pass gpio_override_pm this to GNVS, I can use the flag to do overwrite or do nothing.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... PS1, Line 27: else
Do we need this? FPS-S will enable it anyway right?
valid point, i guess my proposal would be don't bother about what FSP sets, we are independent in this way.
If we can pass gpio_override_pm this to GNVS, I can use the flag to do overwrite or do nothing.
yes, gpio_override_pm is going into GNVS, look for OGPM, based on this you can decided to override those values else leave it to its default value as this else will anyway taken care. https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/...
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
I am okay with this. Let get google opinion.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... PS1, Line 27: else
valid point, i guess my proposal would be don't bother about what FSP sets, we are independent in this way.
+1. Let's stick with configuring this in coreboot rather than relying on FSP. In general, I would want to have a way to skip this configuration in FSP completely. It just adds unnecessary paths that we need to be worried about.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/1/src/soc/intel/cannonlake/gp... PS1, Line 27: else
valid point, i guess my proposal would be don't bother about what FSP sets, we are independent in […]
Yes, i'm on it already for coming platform. to get rid that in FSP
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Abandoned
Subrata Banik has restored this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass MISCCFG.bit 0-5 devicetree.cb to ASL ......................................................................
Restored
Hello Patrick Rudolph, EricR Lai, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37794
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL ......................................................................
soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL
This patch performs below operations: 1. Refactor soc_gpio_pm_configuration() and create new static API to fill gpio pm configuration as in fill_soc_gpio_pm_configuration() 2. Pass GPIO PM configuration values between devicetree.cb to platform ASL
BUG=b:144002424 TEST=Verify devicetree.cb "gpio_override_pm" variables and ogpm ASL variables are in sync.
Change-Id: I75246be01aa4ab4cfa1e184ab6a11b718471995e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/gpio_common.c 2 files changed, 16 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/37794/2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... PS2, Line 28: memset(misccfg_pm_values, MISCCFG_ENABLE_GPIO_PM_CONFIG, Do we still enable by default? And if we get rid of FSP-S, we can skip overwrite here just write the value that pass by device tree? We can set default in device tree.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... PS2, Line 28: memset(misccfg_pm_values, MISCCFG_ENABLE_GPIO_PM_CONFIG,
Do we still enable by default? And if we get rid of FSP-S, we can skip overwrite here just write the […]
did we discussed to not override ? i don't recall. I guess Furquan said last time to not dependent on FSP-S and if coreboot user doesn't enable gpio_override_pm then we should enable this feature by default and it might have power saving and we don't want to miss those on non-chrome platform with coreboot may be ?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... File src/soc/intel/cannonlake/gpio_common.c:
https://review.coreboot.org/c/coreboot/+/37794/2/src/soc/intel/cannonlake/gp... PS2, Line 28: memset(misccfg_pm_values, MISCCFG_ENABLE_GPIO_PM_CONFIG,
did we discussed to not override ? i don't recall. […]
OH, I thought Furquan mean let coreboot set the PM bits instead of FSP-S.If so, we don't care the overwrite anymore.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37794 )
Change subject: soc/intel/cannonlake: Pass GPIO PM configuration devicetree.cb to ASL ......................................................................
Abandoned
-
EricR Lai (Code Review) -
Furquan Shaikh (Code Review)
-
Subrata Banik (Code Review)