Change in ...coreboot[master]: soc/intel/cannonlake: Add a power control workaround for SD controller
Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31445
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
soc/intel/cannonlake: Add a power control workaround for SD controller
SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted. For platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native funtion in _PS0.
Also add a Kconfig option to allow a mainboard to choose if this workaround is required, based on how the SD_VDD1_PWR_EN is implemented on it.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 3 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index bacbe7b..dce116b 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -236,6 +236,14 @@ hex default 0x200000
+config MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE + bool "Activate workaround in ASL for keeping SD_PWR_ENABLE low in D3" + default n + help + Select this if the board has a SD_PWR_ENABLE pin connected to a + active high sensing load switch to turn on power to the card reader. + This will enable a workaround in ASL _PS3 and _PS0 methods to force + SD_PWR_ENABLE to stay low in D3. choice prompt "Cache-as-ram implementation" default USE_CANNONLAKE_CAR_NEM_ENHANCED if MAINBOARD_HAS_CHROMEOS diff --git a/src/soc/intel/cannonlake/acpi/scs.asl b/src/soc/intel/cannonlake/acpi/scs.asl index 896fd77..558d538 100644 --- a/src/soc/intel/cannonlake/acpi/scs.asl +++ b/src/soc/intel/cannonlake/acpi/scs.asl @@ -111,6 +111,11 @@ /* Set Power State to D0 */ And (PMCR, 0xFFFC, PMCR) Store (PMCR, ^TEMP) + +#if IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE) + /* Change pad mode to Native*/ + GPMO(SD_PWR_EN_PIN, 0x1) +#endif }
Method (_PS3, 0, Serialized) @@ -120,6 +125,14 @@ /* Set Power State to D3 */ Or (PMCR, 0x0003, PMCR) Store (PMCR, ^TEMP) + +#if IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE) + /* Change pad mode to GPIO control */ + GPMO(SD_PWR_EN_PIN, 0x0) + + /* Drive TX to zero */ + CTXS(SD_PWR_EN_PIN) +#endif }
Device (CARD) diff --git a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h index cb184c9..380ec66 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h @@ -352,4 +352,6 @@ #define NUM_GPIO_COM3_PADS (GPIO_RSVD_78 - HDA_BCLK + 1)
#define TOTAL_PADS 275 + +#define SD_PWR_EN_PIN GPP_A17 #endif
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
I agree to the ASL code beside my inline question. Two remarks, though:
Why is it a workaround? Is it documented somewhere that it should behave differently?
Why the additional Kconfig? Can't we simply read the pad mode in _PS3 and do what needs to be done if it was set to native1?
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN) What about other bits like enabling the output buffer? Are they all guaranteed to have the correct value?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review+1
(1 comment)
I agree to the ASL code beside my inline question. Two remarks, though:
Why is it a workaround? Is it documented somewhere that it should behave differently?
errta is https://cdrdv2.intel.com/v1/dl/getContent/596808. Not sure if you have access. The issue is what is mentioned in the commit message. SDXC controller can operate the SD_PWR_EN as either an active high signal or an active low signal. This issue only happens when the SD_PW_EN is used as an active high signal.
Why the additional Kconfig? Can't we simply read the pad mode in _PS3 and do what needs to be done if it was set to native1?
Pad mode will be set to Native1 irrespective. This WA has to be applied only when SD_PW_EN is used as an active high signal.
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN)
What about other bits like enabling the output buffer? Are they […]
the method CTXS is initializing the fields by setting the "UpdateRule" as preserve, which should preserve all the bits which are not being modified.
Hello Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
soc/intel/cannonlake: Add a power control workaround for SD controller
SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted. For platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native funtion in _PS0.
Also add a Kconfig option to allow a mainboard to choose if this workaround is required, based on how the SD_VDD1_PWR_EN is implemented on it.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 3 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31445/3/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/3/src/soc/intel/cannonlake/Kconfig@240 PS3, Line 240: "Activate workaround in ASL for keeping SD_PWR_ENABLE low in D3" Why is this a user-visible option?
Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
soc/intel/cannonlake: Add a power control workaround for SD controller
SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted. For platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native funtion in _PS0.
Also add a Kconfig option to allow a mainboard to choose if this workaround is required, based on how the SD_VDD1_PWR_EN is implemented on it.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 3 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/7
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31445/3/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/3/src/soc/intel/cannonlake/Kconfig@240 PS3, Line 240: "Activate workaround in ASL for keeping SD_PWR_ENABLE low in D3"
Why is this a user-visible option?
not required
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 7: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 8:
(1 comment)
Why the additional Kconfig? Can't we simply read the pad mode in _PS3 and do what needs to be done if it was set to native1?
Pad mode will be set to Native1 irrespective. This WA has to be applied only when SD_PW_EN is used as an active high signal.
What I meant is, that we can detect that case by looking at the register, even the active high setting. Or is that con- figured somewhere else?
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN)
the method CTXS is initializing the fields by setting the "UpdateRule" as preserve, which should pre […]
That's neither what I meant nor how CTXS does it. It does preserve the bits (but not by the UpdateRule but rather by manually masking) and that is my concern. It means the state of the other bits before calling GPMO has to be the state that we need them to be after cal- ling CTXS. And the question is, is that guaranteed or does this only work by chance (e.g. depend on bits set in the board's initial GPIO config)?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 8: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig@238 PS8, Line 238: config MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE This adds a redundant setting. If I'm not complete confused, we have that information already somewhere in the mainboard code.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig@238 PS8, Line 238: config MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE
This adds a redundant setting. If I'm not complete confused, […]
The Active High/Low is configurable, and is a SoC setting that can be provided to FSP silicon init. i.e., SdCardPowerEnableActiveHigh. I assume that this setting cannot be accessed in the ASL code. Hence the need for config.
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN)
That's neither what I meant nor how CTXS does it. It does preserve […]
ok I get, was leaving it upto chance (earlier programming) for now :). I will create another function to update the TX buffer enable/disable.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 8:
(2 comments)
Please note that neither does hatch set `SdCardPowerEnableActiveHigh` nor is the UPD ever set (the chip.h field is simply ignored). Which means that at the end of this patch train, both redundant options are already out of sync.
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/8/src/soc/intel/cannonlake/Kconfig@238 PS8, Line 238: config MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE
The Active High/Low is configurable, and is a SoC setting that can be provided to FSP silicon init. […]
There are many possible solutions, even without a Kconfig: * export the value of `SdCardPowerEnableActiveHigh` via GNVS * export the value of `SdCardPowerEnableActiveHigh` via runtime generated ASL * check what register settings it changes and act based on the register values
Also, with a Kconfig you can avoid the redundancy, too: * set `SdCardPowerEnableActiveHigh` based on the Kconfig
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN)
ok I get, was leaving it upto chance (earlier programming) for now :). […]
The output buffer was just an example. I haven't looked into what exactly is necessary.
Furquan Shaikh has removed a vote on this change.
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a power control workaround for SD controller ......................................................................
Patch Set 8:
Patch Set 8:
(2 comments)
Please note that neither does hatch set `SdCardPowerEnableActiveHigh` nor is the UPD ever set (the chip.h field is simply ignored). Which means that at the end of this patch train, both redundant options are already out of sync.
it was set to 1 as a default, i think that was reason it was not added. Will update the params.
Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN#
SD controller in CNL-PCH provides a ability to configure the behavior of SD_VDD1_PWR_EN# as an active high or low signal. FSP provides an UPD "SdCardPowerEnableActiveHigh" to control the same.
However, for platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power. This is because SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native function in _PS0.
Hence add a Kconfig option to update the UPD, which the board can select based on how the SD_VDD1_PWR_EN is implemented on it. And, the workaround gets applied based on this config.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 4 files changed, 31 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/9
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 134: CTXS(SD_PWR_EN_PIN)
The output buffer was just an example. I haven't looked into […]
Tx buffer should be sufficient in this case.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 10:
https://qa.coreboot.org/job/coreboot-gerrit/89687/ : UNSTABLE
Looks like the RVP11 build is failing, however the same board is able to build fine in my machine. Also the error is not very clear.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 10:
Patch Set 10:
https://qa.coreboot.org/job/coreboot-gerrit/89687/ : UNSTABLE
Looks like the RVP11 build is failing, however the same board is able to build fine in my machine. Also the error is not very clear.
I think I know why it is failing.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 11:
(1 comment)
Please note that neither does hatch set `SdCardPowerEnableActiveHigh` nor is the UPD ever set (the chip.h field is simply ignored). Which means that at the end of this patch train, both redundant options are already out of sync.
it was set to 1 as a default, i think that was reason it was not added. Will update the params.
Let me rephrase. There is a setting in coreboot's `chip.h` that is ignored. There is no "reason" :)
https://review.coreboot.org/#/c/31445/10/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31445/10/src/soc/intel/cannonlake/fsp_params... PS10, Line 216: CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE; thanks
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 12: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/Kconfig@24... PS12, Line 245: SD_PWR_ENABLE to stay low in D3. nit, missing empty line below
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/acpi/scs.a... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/acpi/scs.a... PS12, Line 116: Native*/ nit, missing space
Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#13).
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN#
SD controller in CNL-PCH provides a ability to configure the behavior of SD_VDD1_PWR_EN# as an active high or low signal. FSP provides an UPD "SdCardPowerEnableActiveHigh" to control the same.
However, for platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power. This is because SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native function in _PS0.
Hence add a Kconfig option to update the UPD, which the board can select based on how the SD_VDD1_PWR_EN is implemented on it. And, the workaround gets applied based on this config.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 4 files changed, 32 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/13
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/Kconfig@24... PS12, Line 245: SD_PWR_ENABLE to stay low in D3.
nit, missing empty line below
Done
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/acpi/scs.a... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/acpi/scs.a... PS12, Line 116: Native*/
nit, missing space
Done
Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN#
SD controller in CNL-PCH provides a ability to configure the behavior of SD_VDD1_PWR_EN# as an active high or low signal. FSP provides an UPD "SdCardPowerEnableActiveHigh" to control the same.
However, for platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power. This is because SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native function in _PS0.
Hence add a Kconfig option to update the UPD, which the board can select based on how the SD_VDD1_PWR_EN is implemented on it. And, the workaround gets applied based on this config.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 4 files changed, 32 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/14
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 14: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 216: CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE)
Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31445
to look at the new patch set (#15).
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN#
SD controller in CNL-PCH provides a ability to configure the behavior of SD_VDD1_PWR_EN# as an active high or low signal. FSP provides an UPD "SdCardPowerEnableActiveHigh" to control the same.
However, for platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power. This is because SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native function in _PS0.
Hence add a Kconfig option to update the UPD, which the board can select based on how the SD_VDD1_PWR_EN is implemented on it. And, the workaround gets applied based on this config.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 4 files changed, 32 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31445/15
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 216: CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE
IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE)
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 15: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 216: CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE
Done
I'm curious, any reasoning behind this or just preference?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31445/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 216: CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE
I'm curious, any reasoning behind this or just preference?
Just preference.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31445 )
Change subject: soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN# ......................................................................
soc/intel/cannonlake: Add a config for configuring SD_VDD1_PWR_EN#
SD controller in CNL-PCH provides a ability to configure the behavior of SD_VDD1_PWR_EN# as an active high or low signal. FSP provides an UPD "SdCardPowerEnableActiveHigh" to control the same.
However, for platforms using SD_VDD1_PWR_EN# as active high, the SDXC card connector is always powered and may impact system power. This is because SD_VDD1_PWR_EN# does not de-assert during SDXC D3 or when SD card is not inserted.
Workaround is to change the pad ownership of SD_VDD1_PWR_EN to GPIO and force the TX buffer to low in _PS3. And restore the pad mode to native function in _PS0.
Hence add a Kconfig option to update the UPD, which the board can select based on how the SD_VDD1_PWR_EN is implemented on it. And, the workaround gets applied based on this config.
BUG=b:123350329
Change-Id: Iee262d7ecdf8c31362aec3d95dd9b3e8359e0c25 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Reviewed-on: https://review.coreboot.org/c/31445 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h 4 files changed, 32 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index cd8819d..927409d 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -235,6 +235,15 @@ hex default 0x200000
+config MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE + bool + default n + help + Select this if the board has a SD_PWR_ENABLE pin connected to a + active high sensing load switch to turn on power to the card reader. + This will enable a workaround in ASL _PS3 and _PS0 methods to force + SD_PWR_ENABLE to stay low in D3. + choice prompt "Cache-as-ram implementation" default USE_CANNONLAKE_CAR_NEM_ENHANCED if MAINBOARD_HAS_CHROMEOS diff --git a/src/soc/intel/cannonlake/acpi/scs.asl b/src/soc/intel/cannonlake/acpi/scs.asl index 896fd77..1806e75 100644 --- a/src/soc/intel/cannonlake/acpi/scs.asl +++ b/src/soc/intel/cannonlake/acpi/scs.asl @@ -111,6 +111,11 @@ /* Set Power State to D0 */ And (PMCR, 0xFFFC, PMCR) Store (PMCR, ^TEMP) + +#if IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE) + /* Change pad mode to Native */ + GPMO(SD_PWR_EN_PIN, 0x1) +#endif }
Method (_PS3, 0, Serialized) @@ -120,6 +125,17 @@ /* Set Power State to D3 */ Or (PMCR, 0x0003, PMCR) Store (PMCR, ^TEMP) + +#if IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE) + /* Change pad mode to GPIO control */ + GPMO(SD_PWR_EN_PIN, 0x0) + + /* Enable Tx Buffer */ + GTXE(SD_PWR_EN_PIN, 0x1) + + /* Drive TX to zero */ + CTXS(SD_PWR_EN_PIN) +#endif }
Device (CARD) diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 17a014b..1ebde35 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -208,10 +208,13 @@ }
dev = dev_find_slot(0, PCH_DEVFN_SDCARD); - if (!dev) + if (!dev) { params->ScsSdCardEnabled = 0; - else + } else { params->ScsSdCardEnabled = dev->enabled; + params->SdCardPowerEnableActiveHigh = + IS_ENABLED(CONFIG_MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE); + }
dev = dev_find_slot(0, PCH_DEVFN_UFS); if (!dev) diff --git a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h index 5d7c0e8..03f4314 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs.h @@ -352,4 +352,6 @@ #define NUM_GPIO_COM3_PADS (GPIO_RSVD_38 - HDA_BCLK + 1)
#define TOTAL_PADS 275 + +#define SD_PWR_EN_PIN GPP_A17 #endif
-
Furquan Shaikh (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)