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.