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

[Adrian Hunter <adrian.hunter@intel.com>]

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@chromium.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 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;
>