Change in ...coreboot[master]: soc/intel/common/block/gpio: Add gpio_pm_configure() function
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32788
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
Logic is to enable gpio pm registers as GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and GPSIDEDPCGEN for GPIO communities.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/Kconfig M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/1
diff --git a/src/soc/intel/common/block/gpio/Kconfig b/src/soc/intel/common/block/gpio/Kconfig index bdbc323..30e61d4 100644 --- a/src/soc/intel/common/block/gpio/Kconfig +++ b/src/soc/intel/common/block/gpio/Kconfig @@ -43,3 +43,8 @@ depends on SOC_INTEL_COMMON_BLOCK_GPIO bool default n + +config SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING + depends on SOC_INTEL_COMMON_BLOCK_GPIO + bool + default n diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c index 3a0594c..7fdf084 100644 --- a/src/soc/intel/common/block/gpio/gpio.c +++ b/src/soc/intel/common/block/gpio/gpio.c @@ -57,6 +57,11 @@ #define MISCCFG_GPE0_DW1_MASK (0xf << MISCCFG_GPE0_DW1_SHIFT) #define MISCCFG_GPE0_DW2_SHIFT 16 #define MISCCFG_GPE0_DW2_MASK (0xf << MISCCFG_GPE0_DW2_SHIFT) +#define MISCCFG_GPSIDEDPCGEN 0x20 +#define MISCCFG_GPRCOMPCDLCGEN 0x10 +#define MISCCFG_GPRTCDLCGEN 0x8 +#define MISCCFG_GPDPCGEN 0x2 +#define MISCCFG_GPDLCGEN 0x1
#define GPI_SMI_STS_OFFSET(comm, group) ((comm)->gpi_smi_sts_reg_0 + \ ((group) * sizeof(uint32_t))) @@ -609,3 +614,36 @@ } } } + +/* + * The function performs GPIO Power Management programming. + * + * Enable GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, + * GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and + * GPSIDEDPCGEN for GPIO communities. + */ +void gpio_pm_configure(void) +{ + int i; + uint32_t misccfg_mask; + uint32_t misccfg_value; + size_t gpio_communities; + const struct pad_community *comm; + + if (CONFIG(SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING)) + misccfg_mask = ~MISCCFG_GPRCOMPCDLCGEN; + else + misccfg_mask = ~0; + + if (!CONFIG(SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING)) + misccfg_value = MISCCFG_GPRCOMPCDLCGEN; + + misccfg_value |= (MISCCFG_GPSIDEDPCGEN | MISCCFG_GPRTCDLCGEN | + MISCCFG_GPDPCGEN | MISCCFG_GPDLCGEN); + + comm = soc_gpio_get_community(&gpio_communities); + /* Program GPIO_MISCCFG */ + for (i = 0; i < gpio_communities; i++, comm++) + pcr_rmw32(comm->port, GPIO_MISCCFG, + misccfg_mask, misccfg_value); +} diff --git a/src/soc/intel/common/block/include/intelblocks/gpio.h b/src/soc/intel/common/block/include/intelblocks/gpio.h index 4179293..1630e5d 100644 --- a/src/soc/intel/common/block/include/intelblocks/gpio.h +++ b/src/soc/intel/common/block/include/intelblocks/gpio.h @@ -215,5 +215,14 @@ */ void gpi_clear_int_cfg(void);
+/* + * The function performs GPIO Power Management programming. + * + * Enable GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, + * GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and + * GPSIDEDPCGEN for GPIO communities. + */ +void gpio_pm_configure(void); + #endif #endif /* _SOC_INTELBLOCKS_GPIO_H_ */
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/+/32788
to look at the new patch set (#2).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
Logic is to enable gpio pm registers as GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and GPSIDEDPCGEN for GPIO communities.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/Kconfig M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/Kcon... File src/soc/intel/common/block/gpio/Kconfig:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/Kcon... PS2, Line 47: SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING Can you please add some help text indicating what this option is used for?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 625: gpio_pm_configure Can this function just accept a mask and value that it should use to set in the misccfg registers rather than having a Kconfig defined? That would allow us in the future to set the pm bits differently for different communities and also to accept the values from mainboard itself if required.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 638: if (!CONFIG(SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING)) This condition is the same as the 'else' clause above. Please move this statement into the 'else' and eliminate the redundant 'if' here.
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#3).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
Logic is to enable gpio pm registers as GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and GPSIDEDPCGEN for GPIO communities.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/Kconfig M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 3 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/Kcon... File src/soc/intel/common/block/gpio/Kconfig:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/Kcon... PS2, Line 47: SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING
Can you please add some help text indicating what this option is used for?
Done
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 625: gpio_pm_configure
Can this function just accept a mask and value that it should use to set in the misccfg registers ra […]
I thought we need this solution for time being till cr50 FW actually updated with recommended IRQ pulse ?
I still prefer to make use of this functionality and allow complete GPIO PM programming for all community.
As i understand, you might wish to skip certain community to set dynamic clock local programming, in that case, we can make use of another Kconfig to tell which GPIO PIN you wish to skip ?
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 638: if (!CONFIG(SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING))
This condition is the same as the 'else' clause above. […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32788/3/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/3/src/soc/intel/common/block/gpio/gpio... PS3, Line 637: else else should follow close brace '}'
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 625: gpio_pm_configure
I thought we need this solution for time being till cr50 FW actually updated with recommended IRQ pu […]
What I meant is: 1. GPIO common block code (gpio_pm_configure) can set the misccfg registers provided by the SoC i.e. we don't need to implement the policy here.
2. Each SoC can configure this based on either its own Kconfig or mainboard configuration i.e. currently cr50 is one example and it affects only google boards. There could be other devices which might require this functionality to be disabled or non-google boards wanting to implement different policies.
In these cases having a Kconfig variable selected by default by SoC doesn't seem like the right thing to do.
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#4).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
Logic is to enable gpio pm registers as GPDLCGEN, GPDPCGEN, GPRTCDLCGEN, GPRCOMPCDLCGEN if !CONFIG_SOC_INTEL_DISABLE_DYNAMIC_CLOCK_LOCAL_GATING and GPSIDEDPCGEN for GPIO communities.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/Kconfig M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 3 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/4
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#5).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/2/src/soc/intel/common/block/gpio/gpio... PS2, Line 625: gpio_pm_configure
What I meant is: […]
can you please review patchset 5, if you like then i will add CML soc changes as well.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... PS5, Line 73: /* no-op */ no-op means `mask` and `value` stay unitialized and registers are filled with garbage? I think there is simply no need for a weak implementation here: If gpio_pm_configure() gets called and there is no proper soc_fill_gpio_pm_ configuration(), then it's called by error.
But actually, I would prefer not to have soc_fill_gpio_pm_configuration(), see below.
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... PS5, Line 633: soc_fill_gpio_pm_configuration(misccfg_pm_mask, misccfg_pm_value); I don't understand why you want to call back to soc code. As Furquan mentioned, we could pass all necessary information in the call to gpio_ pm_configure():
soc common | mask, value | |------------>| |<------------|
should be much simpler than
soc common |------------>| | | |<------------| | mask, value | |------------>| | | |<------------|
unless I miss why the latter would be necessary.
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#6).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... PS5, Line 73: /* no-op */
no-op means `mask` and `value` stay unitialized and registers are filled with […]
Done
https://review.coreboot.org/#/c/32788/5/src/soc/intel/common/block/gpio/gpio... PS5, Line 633: soc_fill_gpio_pm_configuration(misccfg_pm_mask, misccfg_pm_value);
I don't understand why you want to call back to soc code. As Furquan […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; : I don't see this used anywhere in the commit; what is this for?
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 231: /* The function performs GPIO Power Management programming. */ : void gpio_pm_configure(uint8_t *misccfg_pm_mask, uint8_t *misccfg_pm_value); It is not obvious that this function takes arrays of data, expecting them to contain 'number of GPIO communities' elements in length. Can it take a length parameter too? Also if the data is not changing, const pointers would be preferred, to indicate the arrays are not modified.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; :
I don't see this used anywhere in the commit; what is this for?
for chip.h to select required gpio dynamic local clock gating option
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 231: /* The function performs GPIO Power Management programming. */ : void gpio_pm_configure(uint8_t *misccfg_pm_mask, uint8_t *misccfg_pm_value);
It is not obvious that this function takes arrays of data, expecting them to contain 'number of GPIO […]
got it
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/gpio/gpio... PS6, Line 625: misccfg_pm_mask[i] Shouldn't the mask be always 0x3F so that you can actually set the value (0 or 1) that is requested by mainboard?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; :
for chip. […]
Should we just have macros here? GDLCGEN (1 << 0) GPDPCGEN (1 << 1) ...
Those can be then used directly in devicetree?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; :
Should we just have macros here? […]
Agree with Furquan here, the order of bitfields is compiler/implementation defined and should be avoided.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/gpio/gpio... PS6, Line 625: misccfg_pm_mask[i]
Shouldn't the mask be always 0x3F so that you can actually set the value (0 or 1) that is requested […]
Done
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; :
Agree with Furquan here, the order of bitfields is compiler/implementation defined and should be avo […]
Done
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#7).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 7: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 618: 0x3f Define this as a macro along with the rest of MISCCFG_* you are adding in gpio.h?
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 627: pcr_rmw8 GPIO_MISCCFG is a 32-bit register. Just to confirm: Doing a 8-bit rmw is okay here?
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 618: 0x3f
Define this as a macro along with the rest of MISCCFG_* you are adding in gpio. […]
Agree with Furquan's comment. I'd made it a the bitwise OR of the other named constants instead of just making it 0x3F.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 618: 0x3f
Agree with Furquan's comment. […]
Done
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 618: 0x3f
Define this as a macro along with the rest of MISCCFG_* you are adding in gpio. […]
Done
https://review.coreboot.org/#/c/32788/7/src/soc/intel/common/block/gpio/gpio... PS7, Line 627: pcr_rmw8
GPIO_MISCCFG is a 32-bit register. […]
i don't see any such strict recommendation about 32 bit read alone
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#8).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/8
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 8: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32788/8/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/8/src/soc/intel/common/block/gpio/gpio... PS8, Line 614: uint8_t *misccfg_pm_value Would prefer to see this const uint8_t *misccfg_pm_values
Hello Patrick Rudolph, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32788
to look at the new patch set (#9).
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32788/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32788/8/src/soc/intel/common/block/gpio/gpio... File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/#/c/32788/8/src/soc/intel/common/block/gpio/gpio... PS8, Line 614: uint8_t *misccfg_pm_value
Would prefer to see this const uint8_t *misccfg_pm_values
Done
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 9: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 9: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 9: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
soc/intel/common/block/gpio: Add gpio_pm_configure() function
This patch adds new function to perform gpio power management programming as per EDS.
BUG=b:130764684 TEST=Able to build and boot from fixed media on ICL and CML.
Change-Id: I816a70ad92595f013740a235a9799912ad51665e Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32788 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/gpio/gpio.c M src/soc/intel/common/block/include/intelblocks/gpio.h 2 files changed, 38 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c index 3a0594c..d37601c 100644 --- a/src/soc/intel/common/block/gpio/gpio.c +++ b/src/soc/intel/common/block/gpio/gpio.c @@ -609,3 +609,21 @@ } } } + +/* The function performs GPIO Power Management programming. */ +void gpio_pm_configure(const uint8_t *misccfg_pm_values, size_t num) +{ + int i; + size_t gpio_communities; + uint8_t misccfg_pm_mask = MISCCFG_ENABLE_GPIO_PM_CONFIG; + const struct pad_community *comm; + + comm = soc_gpio_get_community(&gpio_communities); + if (gpio_communities != num) + die("Incorrect GPIO community count!\n"); + + /* Program GPIO_MISCCFG */ + for (i = 0; i < num; i++, comm++) + pcr_rmw8(comm->port, GPIO_MISCCFG, + misccfg_pm_mask, misccfg_pm_values[i]); +} diff --git a/src/soc/intel/common/block/include/intelblocks/gpio.h b/src/soc/intel/common/block/include/intelblocks/gpio.h index 4179293..a2cccc7 100644 --- a/src/soc/intel/common/block/include/intelblocks/gpio.h +++ b/src/soc/intel/common/block/include/intelblocks/gpio.h @@ -23,6 +23,23 @@ #ifndef __ACPI__ #include <types.h>
+/* GPIO community IOSF sideband clock gating */ +#define MISCCFG_GPSIDEDPCGEN (1 << 5) +/* GPIO community RCOMP clock gating */ +#define MISCCFG_GPRCOMPCDLCGEN (1 << 4) +/* GPIO community RTC clock gating */ +#define MISCCFG_GPRTCDLCGEN (1 << 3) +/* GFX controller clock gating */ +#define MISCCFG_GSXSLCGEN (1 << 2) +/* GPIO community partition clock gating */ +#define MISCCFG_GPDPCGEN (1 << 1) +/* GPIO community local clock gating */ +#define MISCCFG_GPDLCGEN (1 << 0) +/* Enable GPIO community power management configuration */ +#define MISCCFG_ENABLE_GPIO_PM_CONFIG (MISCCFG_GPSIDEDPCGEN | \ + MISCCFG_GPRCOMPCDLCGEN | MISCCFG_GPRTCDLCGEN | MISCCFG_GSXSLCGEN \ + | MISCCFG_GPDPCGEN | MISCCFG_GPDLCGEN) + /* * GPIO numbers may not be contiguous and instead will have a different * starting pin number for each pad group. @@ -215,5 +232,8 @@ */ void gpi_clear_int_cfg(void);
+/* The function performs GPIO Power Management programming. */ +void gpio_pm_configure(const uint8_t *misccfg_pm_values, size_t num); + #endif #endif /* _SOC_INTELBLOCKS_GPIO_H_ */
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)