Change in coreboot[master]: soc/amd/picasso: Move sd_emmc_config into emmc_config struct
Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
soc/amd/picasso: Move sd_emmc_config into emmc_config struct
I plan on adding another eMMC parameter. This refactor keeps the config contained in a single struct.
BUG=b:159823235 TEST=Build test
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b57d651ab44d6c1cad661d620bffd4207dfebd4 --- M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/mainboard/google/zork/variants/dalboz/variant.c M src/mainboard/google/zork/variants/ezkinil/variant.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/fsp_params.c 8 files changed, 42 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/45095/1
diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index c22ed34..247616c 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -112,7 +112,7 @@ struct soc_amd_picasso_config *cfg = config_of_soc();
if (!CONFIG(PICASSO_LPC_IOMUX)) - cfg->sd_emmc_config = SD_EMMC_EMMC_HS400; + cfg->emmc_config.timing = SD_EMMC_EMMC_HS400;
mainboard_program_gpios();
diff --git a/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb b/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb index c603130..7ccec27 100644 --- a/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb +++ b/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb @@ -7,7 +7,9 @@ register "fadt_boot_arch" = "ACPI_FADT_LEGACY_DEVICES | ACPI_FADT_8042" register "fadt_flags" = "ACPI_FADT_SLEEP_BUTTON" # See table 5-34 ACPI 6.3 spec
- register "sd_emmc_config" = "SD_EMMC_DISABLE" + register "emmc_config" = "{ + .timing = SD_EMMC_DISABLE, + }"
register "has_usb2_phy_tune_params" = "1"
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb index 42219d7..4edbfa4 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb @@ -40,7 +40,9 @@
# End : OPN Performance Configuration
- register "sd_emmc_config" = "SD_EMMC_EMMC_HS400" + register "emmc_config" = "{ + .timing = SD_EMMC_EMMC_HS400, + }"
register "xhci0_force_gen1" = "0"
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb index 1620643..05bee6b 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb @@ -40,7 +40,9 @@
# End : OPN Performance Configuration
- register "sd_emmc_config" = "SD_EMMC_EMMC_HS400" + register "emmc_config" = "{ + .timing = SD_EMMC_EMMC_HS400, + }"
register "xhci0_force_gen1" = "0"
diff --git a/src/mainboard/google/zork/variants/dalboz/variant.c b/src/mainboard/google/zork/variants/dalboz/variant.c index 5138782..21aaec8 100644 --- a/src/mainboard/google/zork/variants/dalboz/variant.c +++ b/src/mainboard/google/zork/variants/dalboz/variant.c @@ -134,10 +134,10 @@ * So we keep the speed low here, with the intent that * other variants implement these corrections. */ - cfg->sd_emmc_config = SD_EMMC_EMMC_HS200; + cfg->emmc_config.timing = SD_EMMC_EMMC_HS200; } } else { - cfg->sd_emmc_config = SD_EMMC_DISABLE; + cfg->emmc_config.timing = SD_EMMC_DISABLE; }
update_audio_configuration(); diff --git a/src/mainboard/google/zork/variants/ezkinil/variant.c b/src/mainboard/google/zork/variants/ezkinil/variant.c index 29a50bc..f76ab62 100644 --- a/src/mainboard/google/zork/variants/ezkinil/variant.c +++ b/src/mainboard/google/zork/variants/ezkinil/variant.c @@ -15,5 +15,5 @@ * Enable eMMC if eMMC bit is set in FW_CONFIG or device is unprovisioned. */ if (!(variant_has_emmc() || boot_is_factory_unprovisioned())) - cfg->sd_emmc_config = SD_EMMC_DISABLE; + cfg->emmc_config.timing = SD_EMMC_DISABLE; } diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index ac1a12c..efac418 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -126,20 +126,34 @@ uint32_t telemetry_vddcr_soc_slope; uint32_t telemetry_vddcr_soc_offset;
- enum { - SD_EMMC_DISABLE, - SD_EMMC_SD_LOW_SPEED, - SD_EMMC_SD_HIGH_SPEED, - SD_EMMC_SD_UHS_I_SDR_50, - SD_EMMC_SD_UHS_I_DDR_50, - SD_EMMC_SD_UHS_I_SDR_104, - SD_EMMC_EMMC_SDR_26, - SD_EMMC_EMMC_SDR_52, - SD_EMMC_EMMC_DDR_52, - SD_EMMC_EMMC_HS200, - SD_EMMC_EMMC_HS400, - SD_EMMC_EMMC_HS300, - } sd_emmc_config; + struct { + /* + * SDHCI doesn't directly support eMMC. There is an implicit mapping between + * eMMC timing modes and SDHCI UHS-I timing modes defined in the linux + * kernel. + * + * HS -> UHS_SDR12 (0x00) + * DDR52 -> UHS_DDR50 (0x04) + * HS200 -> UHS_SDR104 (0x03) + * HS400 -> NONE (0x05) + * + * The kernel driver uses a heuristic to determine if HS400 is supported. + */ + enum { + SD_EMMC_DISABLE, + SD_EMMC_SD_LOW_SPEED, + SD_EMMC_SD_HIGH_SPEED, + SD_EMMC_SD_UHS_I_SDR_50, + SD_EMMC_SD_UHS_I_DDR_50, + SD_EMMC_SD_UHS_I_SDR_104, + SD_EMMC_EMMC_SDR_26, + SD_EMMC_EMMC_SDR_52, + SD_EMMC_EMMC_DDR_52, + SD_EMMC_EMMC_HS200, + SD_EMMC_EMMC_HS400, + SD_EMMC_EMMC_HS300, + } timing; + } emmc_config;
uint8_t xhci0_force_gen1;
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c index 1dbb8e5..1e4f2a5 100644 --- a/src/soc/amd/picasso/fsp_params.c +++ b/src/soc/amd/picasso/fsp_params.c @@ -12,7 +12,7 @@ { int val = SD_DISABLE;
- switch (cfg->sd_emmc_config) { + switch (cfg->emmc_config.timing) { case SD_EMMC_DISABLE: val = SD_DISABLE; break;
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45095/1/src/soc/amd/picasso/fsp_par... File src/soc/amd/picasso/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45095/1/src/soc/amd/picasso/fsp_par... PS1, Line 17: SD_DISABLE Outside the scope of this patch but it looks like we should move these definitions from soc/ to vc//platform_descriptors.h. Also FWIW I don't see much point in translating an enum value to an identical #define value but maybe I'm overlooking something.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45095/1/src/soc/amd/picasso/fsp_par... File src/soc/amd/picasso/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45095/1/src/soc/amd/picasso/fsp_par... PS1, Line 17: SD_DISABLE
Outside the scope of this patch but it looks like we should move these definitions from soc/ to vc// […]
Isolating oneself from vc and its churn. The compiler is happy enough to collapse things when they line up.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
soc/amd/picasso: Move sd_emmc_config into emmc_config struct
I plan on adding another eMMC parameter. This refactor keeps the config contained in a single struct.
BUG=b:159823235 TEST=Build test
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b57d651ab44d6c1cad661d620bffd4207dfebd4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45095 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/mainboard/google/zork/variants/dalboz/variant.c M src/mainboard/google/zork/variants/ezkinil/variant.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/fsp_params.c 8 files changed, 42 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Marshall Dawson: Looks good to me, approved
diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index c22ed34..247616c 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -112,7 +112,7 @@ struct soc_amd_picasso_config *cfg = config_of_soc();
if (!CONFIG(PICASSO_LPC_IOMUX)) - cfg->sd_emmc_config = SD_EMMC_EMMC_HS400; + cfg->emmc_config.timing = SD_EMMC_EMMC_HS400;
mainboard_program_gpios();
diff --git a/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb b/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb index c603130..7ccec27 100644 --- a/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb +++ b/src/mainboard/amd/mandolin/variants/mandolin/devicetree.cb @@ -7,7 +7,9 @@ register "fadt_boot_arch" = "ACPI_FADT_LEGACY_DEVICES | ACPI_FADT_8042" register "fadt_flags" = "ACPI_FADT_SLEEP_BUTTON" # See table 5-34 ACPI 6.3 spec
- register "sd_emmc_config" = "SD_EMMC_DISABLE" + register "emmc_config" = "{ + .timing = SD_EMMC_DISABLE, + }"
register "has_usb2_phy_tune_params" = "1"
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb index a0f6636..4004243 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb @@ -40,7 +40,9 @@
# End : OPN Performance Configuration
- register "sd_emmc_config" = "SD_EMMC_EMMC_HS400" + register "emmc_config" = "{ + .timing = SD_EMMC_EMMC_HS400, + }"
register "xhci0_force_gen1" = "0"
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb index 1694519..8d475e9 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb @@ -40,7 +40,9 @@
# End : OPN Performance Configuration
- register "sd_emmc_config" = "SD_EMMC_EMMC_HS400" + register "emmc_config" = "{ + .timing = SD_EMMC_EMMC_HS400, + }"
register "xhci0_force_gen1" = "0"
diff --git a/src/mainboard/google/zork/variants/dalboz/variant.c b/src/mainboard/google/zork/variants/dalboz/variant.c index 5138782..21aaec8 100644 --- a/src/mainboard/google/zork/variants/dalboz/variant.c +++ b/src/mainboard/google/zork/variants/dalboz/variant.c @@ -134,10 +134,10 @@ * So we keep the speed low here, with the intent that * other variants implement these corrections. */ - cfg->sd_emmc_config = SD_EMMC_EMMC_HS200; + cfg->emmc_config.timing = SD_EMMC_EMMC_HS200; } } else { - cfg->sd_emmc_config = SD_EMMC_DISABLE; + cfg->emmc_config.timing = SD_EMMC_DISABLE; }
update_audio_configuration(); diff --git a/src/mainboard/google/zork/variants/ezkinil/variant.c b/src/mainboard/google/zork/variants/ezkinil/variant.c index 29a50bc..f76ab62 100644 --- a/src/mainboard/google/zork/variants/ezkinil/variant.c +++ b/src/mainboard/google/zork/variants/ezkinil/variant.c @@ -15,5 +15,5 @@ * Enable eMMC if eMMC bit is set in FW_CONFIG or device is unprovisioned. */ if (!(variant_has_emmc() || boot_is_factory_unprovisioned())) - cfg->sd_emmc_config = SD_EMMC_DISABLE; + cfg->emmc_config.timing = SD_EMMC_DISABLE; } diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index ad492e0..e3da255 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -126,20 +126,34 @@ uint32_t telemetry_vddcr_soc_slope; uint32_t telemetry_vddcr_soc_offset;
- enum { - SD_EMMC_DISABLE, - SD_EMMC_SD_LOW_SPEED, - SD_EMMC_SD_HIGH_SPEED, - SD_EMMC_SD_UHS_I_SDR_50, - SD_EMMC_SD_UHS_I_DDR_50, - SD_EMMC_SD_UHS_I_SDR_104, - SD_EMMC_EMMC_SDR_26, - SD_EMMC_EMMC_SDR_52, - SD_EMMC_EMMC_DDR_52, - SD_EMMC_EMMC_HS200, - SD_EMMC_EMMC_HS400, - SD_EMMC_EMMC_HS300, - } sd_emmc_config; + struct { + /* + * SDHCI doesn't directly support eMMC. There is an implicit mapping between + * eMMC timing modes and SDHCI UHS-I timing modes defined in the linux + * kernel. + * + * HS -> UHS_SDR12 (0x00) + * DDR52 -> UHS_DDR50 (0x04) + * HS200 -> UHS_SDR104 (0x03) + * HS400 -> NONE (0x05) + * + * The kernel driver uses a heuristic to determine if HS400 is supported. + */ + enum { + SD_EMMC_DISABLE, + SD_EMMC_SD_LOW_SPEED, + SD_EMMC_SD_HIGH_SPEED, + SD_EMMC_SD_UHS_I_SDR_50, + SD_EMMC_SD_UHS_I_DDR_50, + SD_EMMC_SD_UHS_I_SDR_104, + SD_EMMC_EMMC_SDR_26, + SD_EMMC_EMMC_SDR_52, + SD_EMMC_EMMC_DDR_52, + SD_EMMC_EMMC_HS200, + SD_EMMC_EMMC_HS400, + SD_EMMC_EMMC_HS300, + } timing; + } emmc_config;
uint8_t xhci0_force_gen1;
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c index f7f23b5..b21f237 100644 --- a/src/soc/amd/picasso/fsp_params.c +++ b/src/soc/amd/picasso/fsp_params.c @@ -13,7 +13,7 @@ { int val = SD_DISABLE;
- switch (cfg->sd_emmc_config) { + switch (cfg->emmc_config.timing) { case SD_EMMC_DISABLE: val = SD_DISABLE; break;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45095 )
Change subject: soc/amd/picasso: Move sd_emmc_config into emmc_config struct ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19202 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19201 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19200 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19199 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19198 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19206 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19205 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19204 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19203
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)