Change in ...coreboot[master]: Enable SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING if !SOC_INTEL_SKY...

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32789

Change subject: Enable SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING if !SOC_INTEL_SKYLAKE
......................................................................

Enable SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING if !SOC_INTEL_SKYLAKE

Additional gpio pm programming logic has been added since CNP PCH hence
selecting this feature for all PCH family beyond SPT PCH.

BUG=b:130764684
TEST=Able to build and boot ICL and CML.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/common/pch/Kconfig
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/1

diff --git a/src/soc/intel/common/pch/Kconfig b/src/soc/intel/common/pch/Kconfig
index a832742..0c654bc 100644
--- a/src/soc/intel/common/pch/Kconfig
+++ b/src/soc/intel/common/pch/Kconfig
@@ -44,5 +44,6 @@
    select SOC_INTEL_COMMON_BLOCK_XDCI
    select SOC_INTEL_COMMON_BLOCK_XHCI
    select SOC_INTEL_COMMON_PCH_LOCKDOWN
+	select SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING if !SOC_INTEL_SKYLAKE

endif

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#5).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/5

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#6).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
3 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/6

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 6:

(5 comments)

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h 
File src/soc/intel/icelake/chip.h:

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h@278 
PS6, Line 278: TOTAL_GPIO_COMM
Can we add an enum or macro in some header file for each community? Then it can be used in gpio.c for the communities:
icl_communities[TOTAL_GPIO_COMM] = {
 [COMM_0] = { 
             ...
            },
 [COMM_1] = {
              ...
            },
};

And same can be used in devicetree:
register "gpio_pm[COMM_0]" = "..."

That will ensure if there is any change in icl_communities, mainboards do not need to be updated.

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@113 
PS6, Line 113: PCH_DEV_SLOT_LPC
Why LPC?

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@120 
PS6, Line 120: config->gpio_pm[i].config
Can we treat 0 as special value or maybe have another flag that indicates mainboard has set these values? I am just concerned that some mainboard might forget to set this and end up with no PM enabled for any community. Thoughts?

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@121 
PS6, Line 121: mask[i] = ~value[i];
I don't think this is correct. It will never reset a bit that mainboard requests if it is already set in the config register.

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@142 
PS6, Line 142: TODO(subrata): Need to implement 4us IRQ width recommendation.
Where do you plan to implement this?

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 6:

(5 comments)

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h 
File src/soc/intel/icelake/chip.h:

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h@278 
PS6, Line 278: TOTAL_GPIO_COMM
> Can we add an enum or macro in some header file for each community? Then it can be used in gpio. […]

Done

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@113 
PS6, Line 113: PCH_DEV_SLOT_LPC
> Why LPC?

need any device to access device tree variable ?
I will use first device

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@120 
PS6, Line 120: config->gpio_pm[i].config
> Can we treat 0 as special value or maybe have another flag that indicates mainboard has set these va […]

add a print ?

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@121 
PS6, Line 121: mask[i] = ~value[i];
> I don't think this is correct. […]

Done

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@142 
PS6, Line 142: TODO(subrata): Need to implement 4us IRQ width recommendation.
> Where do you plan to implement this?

Done

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#7).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 150 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/7

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@142 
PS6, Line 142: TODO(subrata): Need to implement 4us IRQ width recommendation.
> Done

Sorry, I meant this is very specific to google boards. Boards might have other reasons to not enable the GPIO PM programming. I don't think we should get rid of this call. That is another reason why we should add the gpio_override_pm config. Not all mainboards have to change this and we can use the default value of enabling all PM programming bits if no override set in mainboard.

https://review.coreboot.org/#/c/32789/7/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/7/src/soc/intel/icelake/chip.c@121 
PS7, Line 121: !value[i]
Its easy to miss the print in the huge set of logs printed during boot. How about this:
1. Add another config gpio_override_pm that can be set by the mainboards that want to override the default GPIO PM config.
2. Boards that set gpio_override_pm are expected to set gpio_pm[] for all communities.
3. In this function, you can check if gpio_override_pm is set. If yes, then memcpy gpio_pm[] to value[]. Else memset 0x3F(its macro) to value[].

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#8).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 149 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/8

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/32789/7/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/7/src/soc/intel/icelake/chip.c@121 
PS7, Line 121: !value[i]
> Its easy to miss the print in the huge set of logs printed during boot. How about this: […]

Done

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/#/c/32789/8/src/soc/intel/icelake/chip.h 
File src/soc/intel/icelake/chip.h:

https://review.coreboot.org/#/c/32789/8/src/soc/intel/icelake/chip.h@273 
PS8, Line 273:         uint8_t gpio_override_pm;
code indent should use tabs where possible

https://review.coreboot.org/#/c/32789/8/src/soc/intel/icelake/chip.h@273 
PS8, Line 273:         uint8_t gpio_override_pm;
please, no spaces at the start of a line

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#9).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 149 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/9

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 10: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32789/10/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/10/src/soc/intel/icelake/chip.c@118 
PS10, Line 118: TOTAL_GPIO_COMM
sizeof(value)? I know its the same right now. But protects against change in data type of value.

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#11).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 151 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/11

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/#/c/32789/10/src/soc/intel/icelake/chip.c 
File src/soc/intel/icelake/chip.c:

https://review.coreboot.org/#/c/32789/10/src/soc/intel/icelake/chip.c@118 
PS10, Line 118: TOTAL_GPIO_COMM
> sizeof(value)? I know its the same right now. But protects against change in data type of value.

Done

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32789

to look at the new patch set (#12).

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 66 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32789/12

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

Patch Set 12: Code-Review+2

Change in ...coreboot[master]: soc/intel/icelake: Make use of gpio_pm_configure()

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32789 )

Change subject: soc/intel/icelake: Make use of gpio_pm_configure()
......................................................................

soc/intel/icelake: Make use of gpio_pm_configure()

Provide option in chip.h to set dynamic local clock gating
setting.

BUG=b:130764684
TEST=Able to build and boot ICL.

Change-Id: Ic30a490aadb8cc9c05a19a05533ab0196c69b7f1
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32789
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/icelake/chip.c
M src/soc/intel/icelake/chip.h
M src/soc/intel/icelake/gpio.c
M src/soc/intel/icelake/include/soc/gpio_soc_defs.h
4 files changed, 66 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/intel/icelake/chip.c b/src/soc/intel/icelake/chip.c
index 11d14de..2616db1 100644
--- a/src/soc/intel/icelake/chip.c
+++ b/src/soc/intel/icelake/chip.c
@@ -103,6 +103,27 @@
 }
 #endif

+/* SoC rotine to fill GPIO PM mask and value for GPIO_MISCCFG register */
+static void soc_fill_gpio_pm_configuration(void)
+{
+	uint8_t value[TOTAL_GPIO_COMM];
+	const struct device *dev;
+	dev = pcidev_on_root(SA_DEV_SLOT_ROOT, 0);
+	if (!dev || !dev->chip_info)
+		return;
+
+	const config_t *config = dev->chip_info;
+
+	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);
+
+	gpio_pm_configure(value, TOTAL_GPIO_COMM);
+}
+
 void soc_init_pre_device(void *chip_info)
 {
    /* Snapshot the current GPIO IRQ polarities. FSP is setting a
@@ -117,6 +138,8 @@

/* Restore GPIO IRQ polarities back to previous settings. */
    itss_restore_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);
+
+	soc_fill_gpio_pm_configuration();
 }

static void pci_domain_set_resources(struct device *dev)
diff --git a/src/soc/intel/icelake/chip.h b/src/soc/intel/icelake/chip.h
index 3e2b78a..7761126 100644
--- a/src/soc/intel/icelake/chip.h
+++ b/src/soc/intel/icelake/chip.h
@@ -18,6 +18,7 @@

#include <intelblocks/chip.h>
 #include <drivers/i2c/designware/dw_i2c.h>
+#include <intelblocks/gpio.h>
 #include <intelblocks/gspi.h>
 #include <stdint.h>
 #include <soc/gpe.h>
@@ -263,6 +264,25 @@
    	FORCE_ENABLE,
    	FORCE_DISABLE,
    } CnviBtAudioOffload;
+
+	/*
+	 * Override GPIO PM configuration:
+	 * 0: Use FSP default GPIO PM program,
+	 * 1: coreboot to override GPIO PM program
+	 */
+	uint8_t gpio_override_pm;
+
+	/*
+	 * GPIO PM configuration: 0 to disable, 1 to enable power gating
+	 * Bit 6-7: Reserved
+	 * Bit 5: MISCCFG_GPSIDEDPCGEN
+	 * Bit 4: MISCCFG_GPRCOMPCDLCGEN
+	 * Bit 3: MISCCFG_GPRTCDLCGEN
+	 * Bit 2: MISCCFG_GSXLCGEN
+	 * Bit 1: MISCCFG_GPDPCGEN
+	 * Bit 0: MISCCFG_GPDLCGEN
+	 */
+	uint8_t gpio_pm[TOTAL_GPIO_COMM];
 };

typedef struct soc_intel_icelake_config config_t;
diff --git a/src/soc/intel/icelake/gpio.c b/src/soc/intel/icelake/gpio.c
index b1c46ab..96c5c83 100644
--- a/src/soc/intel/icelake/gpio.c
+++ b/src/soc/intel/icelake/gpio.c
@@ -76,8 +76,9 @@
    INTEL_GPP_BASE(GPP_C0, GPP_S0, GPP_S7, 320),		/* GPP_S */
 };

-static const struct pad_community icl_communities[] = {
-	{ /* GPP G, B, A */
+static const struct pad_community icl_communities[TOTAL_GPIO_COMM] = {
+	/* GPP G, B, A */
+	[COMM_0] = {
    	.port = PID_GPIOCOM0,
    	.first_pad = GPP_G0,
    	.last_pad = GPP_A23,
@@ -95,7 +96,9 @@
    	.num_reset_vals = ARRAY_SIZE(rst_map_com0),
    	.groups = icl_community0_groups,
    	.num_groups = ARRAY_SIZE(icl_community0_groups),
-	}, { /* GPP H, D, F */
+	},
+	/* GPP H, D, F */
+	[COMM_1] = {
    	.port = PID_GPIOCOM1,
    	.first_pad = GPP_H0,
    	.last_pad = GPP_F19,
@@ -113,7 +116,9 @@
    	.num_reset_vals = ARRAY_SIZE(rst_map),
    	.groups = icl_community1_groups,
    	.num_groups = ARRAY_SIZE(icl_community1_groups),
-	}, { /* GPD */
+	},
+	 /* GPD */
+	[COMM_2] = {
    	.port = PID_GPIOCOM2,
    	.first_pad = GPD0,
    	.last_pad = GPD11,
@@ -131,7 +136,9 @@
    	.num_reset_vals = ARRAY_SIZE(rst_map),
    	.groups = icl_community2_groups,
    	.num_groups = ARRAY_SIZE(icl_community2_groups),
-	}, { /* GPP C, E */
+	},
+	/* GPP C, E */
+	[COMM_3] = {
    	.port = PID_GPIOCOM4,
    	.first_pad = GPP_C0,
    	.last_pad = GPP_E23,
@@ -149,7 +156,9 @@
    	.num_reset_vals = ARRAY_SIZE(rst_map),
    	.groups = icl_community4_groups,
    	.num_groups = ARRAY_SIZE(icl_community4_groups),
-	}, { /* GPP R, S */
+	},
+	/* GPP R, S */
+	[COMM_4] = {
    	.port = PID_GPIOCOM5,
    	.first_pad = GPP_R0,
    	.last_pad = GPP_S7,
diff --git a/src/soc/intel/icelake/include/soc/gpio_soc_defs.h b/src/soc/intel/icelake/include/soc/gpio_soc_defs.h
index 5a27a15..887e378 100644
--- a/src/soc/intel/icelake/include/soc/gpio_soc_defs.h
+++ b/src/soc/intel/icelake/include/soc/gpio_soc_defs.h
@@ -281,4 +281,12 @@
 #define NUM_GPIO_COM5_PADS	(GPP_S7 - GPP_R0 + 1)

#define TOTAL_PADS	205
+
+#define COMM_0		0
+#define COMM_1		1
+#define COMM_2		2
+#define COMM_3		3
+#define COMM_4		4
+#define TOTAL_GPIO_COMM	5
+
 #endif