Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
soc/intel/common: Add GPIO PM configuration into NVS
This patch creates options for ASL code to dynamically program MISCCFG.bit 0-5 to perform local clock gating based on devicetree.cb
Change-Id: Ib4f950d97c7d1dbe22f6e57cd502afde6935d831 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/globalnvs.asl M src/soc/intel/common/block/include/intelblocks/nvs.h 2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/37793/1
diff --git a/src/soc/intel/common/block/acpi/acpi/globalnvs.asl b/src/soc/intel/common/block/acpi/acpi/globalnvs.asl index 8e8241b..ce9ebef 100644 --- a/src/soc/intel/common/block/acpi/acpi/globalnvs.asl +++ b/src/soc/intel/common/block/acpi/acpi/globalnvs.asl @@ -47,6 +47,12 @@ U2WE, 16, // 0x2b - 0x2c USB2 Wake Enable Bitmap U3WE, 16, // 0x2d - 0x2e USB3 Wake Enable Bitmap UIOR, 8, // 0x2f - UART debug controller init on S3 resume + OGPM, 8, // 0x30 - Override GPIO Power Management + GPM0, 8, // 0x31 - GPIO Community 0 + GPM1, 8, // 0x32 - GPIO Community 1 + GPM2, 8, // 0x33 - GPIO Community 2 + GPM3, 8, // 0x34 - GPIO Community 3 + GPM4, 8, // 0x35 - GPIO Community 4
/* ChromeOS specific */ Offset (0x100), diff --git a/src/soc/intel/common/block/include/intelblocks/nvs.h b/src/soc/intel/common/block/include/intelblocks/nvs.h index 5f367b6..d3b3bb6 100644 --- a/src/soc/intel/common/block/include/intelblocks/nvs.h +++ b/src/soc/intel/common/block/include/intelblocks/nvs.h @@ -17,6 +17,7 @@ #define SOC_INTEL_COMMON_BLOCK_NVS_H
#include <commonlib/helpers.h> +#include <intelblocks/gpio.h> #include <vendorcode/google/chromeos/gnvs.h>
typedef struct global_nvs_t { @@ -38,7 +39,9 @@ u16 u2we; /* 0x2b - 0x2c USB2 Wake Enable Bitmap */ u16 u3we; /* 0x2d - 0x2e USB3 Wake Enable Bitmap */ u8 uior; /* 0x2f - UART debug controller init on S3 resume */ - u8 unused[208]; + u8 ogpm; /* 0x30 - Override GPIO Power Management */ + u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ + u8 unused[202];
/* ChromeOS specific (0x100 - 0xfff) */ chromeos_acpi_t chromeos;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 55: GPIO Community 4 This is Intel common block code. Does the number of GPIO communities remain the same across platforms?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 55: GPIO Community 4
This is Intel common block code. […]
yes, we are currently using this common nvs file in CNL, ICL and TGL and JSL will use the same.
And as i could see Google plan might be disable/enable GPIO PM config for all GPIO community, isn't it?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202]; Doesn't the 202 depend on how each SoC defines TOTAL_GPIO_COMM ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
Doesn't the 202 depend on how each SoC defines TOTAL_GPIO_COMM ?
this is true, right now CNL (CML), ICL, TGL and JSL we have visibility of 5 GPIO communitym hence TOTAL_GPIO_COMM is 5.
Are you suggesting to have some buffer so we are not running into issue further ? like a reserve_gpio_comm[10] more ?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
this is true, right now CNL (CML), ICL, TGL and JSL we have visibility of 5 GPIO communitym hence TO […]
Yes the issue would be if a newer SoC defines TOTAL_GPIO_COMM > 5. I guess we do have the check_member assert below to ensure that the CrOS GNVS stuff lives at the appropriate place, so that should catch any issues.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
Yes the issue would be if a newer SoC defines TOTAL_GPIO_COMM > 5. […]
yes, if TOTAL_GPIO_COMM > 5 then check_number would assert. i don't think we need any additional check. Correct ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 55: GPIO Community 4
yes, we are currently using this common nvs file in CNL, ICL and TGL and JSL will use the same. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
yes, if TOTAL_GPIO_COMM > 5 then check_number would assert. […]
Looks OK.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
Looks OK.
It looks like TGL already has GPIO comm 0, 1, 2, 4, 5 as per the code under soc/intel/tigerlake/. Doesn't it mean that this assumption is already broken?
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
Abandoned
Subrata Banik has restored this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM configuration into NVS ......................................................................
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/+/37793
to look at the new patch set (#2).
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
soc/intel/common: Add GPIO PM override configuration into NVS
This patch creates options for ASL code to get information about GPIO PM override based on devicetree.cb
Change-Id: Ib4f950d97c7d1dbe22f6e57cd502afde6935d831 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/globalnvs.asl M src/soc/intel/common/block/include/intelblocks/nvs.h 2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/37793/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/nvs.h:
https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/... PS1, Line 43: u8 gpmv[TOTAL_GPIO_COMM]; /* 0x31 - GPIO PM Value */ : u8 unused[202];
It looks like TGL already has GPIO comm 0, 1, 2, 4, 5 as per the code under soc/intel/tigerlake/. […]
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/3/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/37793/3/src/soc/intel/common/block/... PS3, Line 50: OGPM Do we really need this? Can we have a policy that GPIO PM bits are saved before entering lower power state and restored on exit irrespective of what is configured. Then, it doesn't matter what the mainboard configures i.e. even if the override is not set by mainboard, we would still save --> enable all --> restore. This would avoid the need to have a NVS variable.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
@Furquan, maybe you can discuss with Tim. He want mainboard can choice have enable PM bits before enter s0ix or not.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
(1 comment)
@Furquan, maybe you can discuss with Tim. He want mainboard can choice have enable PM bits before enter s0ix or not.
Can you please point me to the discussion thread?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
(1 comment)
@Furquan, maybe you can discuss with Tim. He want mainboard can choice have enable PM bits before enter s0ix or not.
Can you please point me to the discussion thread?
Here https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/...
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37793/3/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/37793/3/src/soc/intel/common/block/... PS3, Line 50: OGPM
Do we really need this? Can we have a policy that GPIO PM bits are saved before entering lower power […]
Ack
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
(1 comment)
@Furquan, maybe you can discuss with Tim. He want mainboard can choice have enable PM bits before enter s0ix or not.
Can you please point me to the discussion thread?
@Furquan, if you don't like NVS. I propose use Kconfig as well.. And can solve the APL issue in the same time. Add CONFIG_SUPPORT_GPIO_PM in soc level and we can override it in board level. One stone for two birds.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
@Furquan, if you don't like NVS. I propose use Kconfig as well.. And can solve the APL issue in the same time. Add CONFIG_SUPPORT_GPIO_PM in soc level and we can override it in board level. One stone for two birds.
My concern is that by adding NVS or Kconfig, we are not really gaining much since it would be anyways used by all current boards. In the future, if we really need this for some mainboard, we can always add that config/NVS variable. Please see my comment here: https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/...
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Patch Set 3:
Patch Set 3:
@Furquan, if you don't like NVS. I propose use Kconfig as well.. And can solve the APL issue in the same time. Add CONFIG_SUPPORT_GPIO_PM in soc level and we can override it in board level. One stone for two birds.
My concern is that by adding NVS or Kconfig, we are not really gaining much since it would be anyways used by all current boards. In the future, if we really need this for some mainboard, we can always add that config/NVS variable. Please see my comment here: https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/...
Okay, we can discuss there :)
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37793 )
Change subject: soc/intel/common: Add GPIO PM override configuration into NVS ......................................................................
Abandoned
-
EricR Lai (Code Review) -
Furquan Shaikh (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)