On 29/09/20 12:59 am, Raul E Rangel wrote:
> This change fixes HS400 tuning for devices with invalid presets.
>
> SDHCI presets are not currently used for eMMC HS/HS200/HS400, but are
> used for DDR52. The HS400 retuning sequence is:
>
> HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400
>
> This means that when HS400 tuning happens, we transition through DDR52
> for a very brief period. This causes presets to be enabled
> unintentionally and stay enabled when transitioning back to HS200 or
> HS400. Some firmware has invalid presets, so we end up with driver
> strengths that can cause I/O problems.
>
> Fixes: 34597a3f60b1 ("mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400")
> Signed-off-by: Raul E Rangel <rrangel@xxxxxxxxxxxx>
Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
> I decided to abandon adding the preset_value_support for now. Enabling
> presets for the AMD controller currently results in using invalid
> presets for HS400. This is because sdhci_get_preset_value is using a
> non-standard HS400 register that the AMD controller does not support.
>
> I think preset_value_support also needs more thought. Since HS400
> re-tuning requires switching to HS, DDR52, and HS200, if one of those
> timings is not in the list, we would need to disable presets.
>
> I chose this approach to avoid any additional complexity.
>
> drivers/mmc/host/sdhci-acpi.c | 37 +++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 284cba11e2795..d335a34ad05b3 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -662,6 +662,43 @@ static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
> (host->mmc->caps & MMC_CAP_1_8V_DDR))
> host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
>
> + /*
> + * There are two types of presets out in the wild:
> + * 1) Default/broken presets.
> + * These presets have two sets of problems:
> + * a) The clock divisor for SDR12, SDR25, and SDR50 is too small.
> + * This results in clock frequencies that are 2x higher than
> + * acceptable. i.e., SDR12 = 25 MHz, SDR25 = 50 MHz, SDR50 =
> + * 100 MHz.x
> + * b) The HS200 and HS400 driver strengths don't match.
> + * By default, the SDR104 preset register has a driver strength of
> + * A, but the (internal) HS400 preset register has a driver
> + * strength of B. As part of initializing HS400, HS200 tuning
> + * needs to be performed. Having different driver strengths
> + * between tuning and operation is wrong. It results in different
> + * rise/fall times that lead to incorrect sampling.
> + * 2) Firmware with properly initialized presets.
> + * These presets have proper clock divisors. i.e., SDR12 => 12MHz,
> + * SDR25 => 25 MHz, SDR50 => 50 MHz. Additionally the HS200 and
> + * HS400 preset driver strengths match.
> + *
> + * Enabling presets for HS400 doesn't work for the following reasons:
> + * 1) sdhci_set_ios has a hard coded list of timings that are used
> + * to determine if presets should be enabled.
> + * 2) sdhci_get_preset_value is using a non-standard register to
> + * read out HS400 presets. The AMD controller doesn't support this
> + * non-standard register. In fact, it doesn't expose the HS400
> + * preset register anywhere in the SDHCI memory map. This results
> + * in reading a garbage value and using the wrong presets.
> + *
> + * Since HS400 and HS200 presets must be identical, we could
> + * instead use the the SDR104 preset register.
> + *
> + * If the above issues are resolved we could remove this quirk for
> + * firmware that that has valid presets (i.e., SDR12 <= 12 MHz).
> + */
> + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> +
> host->mmc_host_ops.select_drive_strength = amd_select_drive_strength;
> host->mmc_host_ops.set_ios = amd_set_ios;
> host->mmc_host_ops.execute_tuning = amd_sdhci_execute_tuning;
>
