Re: [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN

[Ulf Hansson <ulf.hansson@linaro.org>]

On Mon, 28 Sep 2020 at 23:59, Raul E Rangel <rrangel@chromium.org> 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@chromium.org>

Applied for next and by adding a stable tag, thanks!

Kind regards
Uffe


> ---
> 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;
> --
> 2.28.0.709.gb0816b6eb0-goog
>