All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN
@ 2020-09-28 21:59 Raul E Rangel
  2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Raul E Rangel @ 2020-09-28 21:59 UTC (permalink / raw)
  To: linux-mmc, adrian.hunter
  Cc: Shirish.S, Akshu.Agrawal, Nehal-bakulchandra.Shah, chris.wang,
	Raul E Rangel, Ulf Hansson, linux-kernel

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength
  2020-09-28 21:59 [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Raul E Rangel
@ 2020-09-28 21:59 ` Raul E Rangel
  2020-09-30  7:26   ` Adrian Hunter
  2020-10-05 12:50   ` Ulf Hansson
  2020-09-30  7:25 ` [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Adrian Hunter
  2020-10-05 12:50 ` Ulf Hansson
  2 siblings, 2 replies; 7+ messages in thread
From: Raul E Rangel @ 2020-09-28 21:59 UTC (permalink / raw)
  To: linux-mmc, adrian.hunter
  Cc: Shirish.S, Akshu.Agrawal, Nehal-bakulchandra.Shah, chris.wang,
	Raul E Rangel, Ulf Hansson, linux-kernel

This change will allow platform designers better control over signal
integrity by allowing them to tune the HS200 and HS400 driver strengths.

The driver strength was previously hard coded to A to solve boot
problems with certain platforms. This driver strength does not
universally apply to all platforms so we need a knob to adjust it.

All older platforms currently have the SDR104 preset hard coded to A in
the firmware. This means that switching from the hard coded value in
the kernel to reading the SDR104 preset is a no-op for these platforms.
Newer platforms will have properly set presets. So this change will
support both new and old platforms.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index d335a34ad05b3..5c9a041af5b4b 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -545,10 +545,43 @@ struct amd_sdhci_host {
 
 static int amd_select_drive_strength(struct mmc_card *card,
 				     unsigned int max_dtr, int host_drv,
-				     int card_drv, int *drv_type)
+				     int card_drv, int *host_driver_strength)
 {
-	*drv_type = MMC_SET_DRIVER_TYPE_A;
-	return MMC_SET_DRIVER_TYPE_A;
+	struct sdhci_host *host = mmc_priv(card->host);
+	u16 preset, preset_driver_strength;
+
+	/*
+	 * This method is only called by mmc_select_hs200 so we only need to
+	 * read from the HS200 (SDR104) preset register.
+	 *
+	 * Firmware that has "invalid/default" presets return a driver strength
+	 * of A. This matches the previously hard coded value.
+	 */
+	preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104);
+	preset_driver_strength =
+		(preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT;
+
+	/*
+	 * We want the controller driver strength to match the card's driver
+	 * strength so they have similar rise/fall times.
+	 *
+	 * The controller driver strength set by this method is sticky for all
+	 * timings after this method is called. This unfortunately means that
+	 * while HS400 tuning is in progress we end up with mismatched driver
+	 * strengths between the controller and the card. HS400 tuning requires
+	 * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch
+	 * happens while in DDR52 and HS modes. This has not been observed to
+	 * cause problems. Enabling presets would fix this issue.
+	 */
+	*host_driver_strength = preset_driver_strength;
+
+	/*
+	 * The resulting card driver strength is only set when switching the
+	 * card's timing to HS200 or HS400. The card will use the default driver
+	 * strength (B) for any other mode.
+	 */
+	return preset_driver_strength;
+
 }
 
 static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable)
-- 
2.28.0.709.gb0816b6eb0-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN
  2020-09-28 21:59 [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Raul E Rangel
  2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
@ 2020-09-30  7:25 ` Adrian Hunter
  2020-10-05 12:50 ` Ulf Hansson
  2 siblings, 0 replies; 7+ messages in thread
From: Adrian Hunter @ 2020-09-30  7:25 UTC (permalink / raw)
  To: Raul E Rangel, linux-mmc
  Cc: Shirish.S, Akshu.Agrawal, Nehal-bakulchandra.Shah, chris.wang,
	Ulf Hansson, linux-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength
  2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
@ 2020-09-30  7:26   ` Adrian Hunter
  2020-10-05 12:50   ` Ulf Hansson
  1 sibling, 0 replies; 7+ messages in thread
From: Adrian Hunter @ 2020-09-30  7:26 UTC (permalink / raw)
  To: Raul E Rangel, linux-mmc
  Cc: Shirish.S, Akshu.Agrawal, Nehal-bakulchandra.Shah, chris.wang,
	Ulf Hansson, linux-kernel

On 29/09/20 12:59 am, Raul E Rangel wrote:
> This change will allow platform designers better control over signal
> integrity by allowing them to tune the HS200 and HS400 driver strengths.
> 
> The driver strength was previously hard coded to A to solve boot
> problems with certain platforms. This driver strength does not
> universally apply to all platforms so we need a knob to adjust it.
> 
> All older platforms currently have the SDR104 preset hard coded to A in
> the firmware. This means that switching from the hard coded value in
> the kernel to reading the SDR104 preset is a no-op for these platforms.
> Newer platforms will have properly set presets. So this change will
> support both new and old platforms.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

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

> ---
> 
>  drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index d335a34ad05b3..5c9a041af5b4b 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -545,10 +545,43 @@ struct amd_sdhci_host {
>  
>  static int amd_select_drive_strength(struct mmc_card *card,
>  				     unsigned int max_dtr, int host_drv,
> -				     int card_drv, int *drv_type)
> +				     int card_drv, int *host_driver_strength)
>  {
> -	*drv_type = MMC_SET_DRIVER_TYPE_A;
> -	return MMC_SET_DRIVER_TYPE_A;
> +	struct sdhci_host *host = mmc_priv(card->host);
> +	u16 preset, preset_driver_strength;
> +
> +	/*
> +	 * This method is only called by mmc_select_hs200 so we only need to
> +	 * read from the HS200 (SDR104) preset register.
> +	 *
> +	 * Firmware that has "invalid/default" presets return a driver strength
> +	 * of A. This matches the previously hard coded value.
> +	 */
> +	preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104);
> +	preset_driver_strength =
> +		(preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT;
> +
> +	/*
> +	 * We want the controller driver strength to match the card's driver
> +	 * strength so they have similar rise/fall times.
> +	 *
> +	 * The controller driver strength set by this method is sticky for all
> +	 * timings after this method is called. This unfortunately means that
> +	 * while HS400 tuning is in progress we end up with mismatched driver
> +	 * strengths between the controller and the card. HS400 tuning requires
> +	 * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch
> +	 * happens while in DDR52 and HS modes. This has not been observed to
> +	 * cause problems. Enabling presets would fix this issue.
> +	 */
> +	*host_driver_strength = preset_driver_strength;
> +
> +	/*
> +	 * The resulting card driver strength is only set when switching the
> +	 * card's timing to HS200 or HS400. The card will use the default driver
> +	 * strength (B) for any other mode.
> +	 */
> +	return preset_driver_strength;
> +
>  }
>  
>  static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable)
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN
  2020-09-28 21:59 [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Raul E Rangel
  2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
  2020-09-30  7:25 ` [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Adrian Hunter
@ 2020-10-05 12:50 ` Ulf Hansson
  2 siblings, 0 replies; 7+ messages in thread
From: Ulf Hansson @ 2020-10-05 12:50 UTC (permalink / raw)
  To: Raul E Rangel
  Cc: linux-mmc, Adrian Hunter, Shirish.S, Akshu Agrawal, Shah,
	Nehal-bakulchandra, Wang, Chris, Linux Kernel Mailing List

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength
  2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
  2020-09-30  7:26   ` Adrian Hunter
@ 2020-10-05 12:50   ` Ulf Hansson
  2020-10-06  9:40     ` Ulf Hansson
  1 sibling, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2020-10-05 12:50 UTC (permalink / raw)
  To: Raul E Rangel
  Cc: linux-mmc, Adrian Hunter, Shirish.S, Akshu Agrawal, Shah,
	Nehal-bakulchandra, Wang, Chris, Linux Kernel Mailing List

On Mon, 28 Sep 2020 at 23:59, Raul E Rangel <rrangel@chromium.org> wrote:
>
> This change will allow platform designers better control over signal
> integrity by allowing them to tune the HS200 and HS400 driver strengths.
>
> The driver strength was previously hard coded to A to solve boot
> problems with certain platforms. This driver strength does not
> universally apply to all platforms so we need a knob to adjust it.
>
> All older platforms currently have the SDR104 preset hard coded to A in
> the firmware. This means that switching from the hard coded value in
> the kernel to reading the SDR104 preset is a no-op for these platforms.
> Newer platforms will have properly set presets. So this change will
> support both new and old platforms.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Applied for next, thanks!

Kind regards
Uffe


> ---
>
>  drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index d335a34ad05b3..5c9a041af5b4b 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -545,10 +545,43 @@ struct amd_sdhci_host {
>
>  static int amd_select_drive_strength(struct mmc_card *card,
>                                      unsigned int max_dtr, int host_drv,
> -                                    int card_drv, int *drv_type)
> +                                    int card_drv, int *host_driver_strength)
>  {
> -       *drv_type = MMC_SET_DRIVER_TYPE_A;
> -       return MMC_SET_DRIVER_TYPE_A;
> +       struct sdhci_host *host = mmc_priv(card->host);
> +       u16 preset, preset_driver_strength;
> +
> +       /*
> +        * This method is only called by mmc_select_hs200 so we only need to
> +        * read from the HS200 (SDR104) preset register.
> +        *
> +        * Firmware that has "invalid/default" presets return a driver strength
> +        * of A. This matches the previously hard coded value.
> +        */
> +       preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104);
> +       preset_driver_strength =
> +               (preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT;
> +
> +       /*
> +        * We want the controller driver strength to match the card's driver
> +        * strength so they have similar rise/fall times.
> +        *
> +        * The controller driver strength set by this method is sticky for all
> +        * timings after this method is called. This unfortunately means that
> +        * while HS400 tuning is in progress we end up with mismatched driver
> +        * strengths between the controller and the card. HS400 tuning requires
> +        * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch
> +        * happens while in DDR52 and HS modes. This has not been observed to
> +        * cause problems. Enabling presets would fix this issue.
> +        */
> +       *host_driver_strength = preset_driver_strength;
> +
> +       /*
> +        * The resulting card driver strength is only set when switching the
> +        * card's timing to HS200 or HS400. The card will use the default driver
> +        * strength (B) for any other mode.
> +        */
> +       return preset_driver_strength;
> +
>  }
>
>  static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable)
> --
> 2.28.0.709.gb0816b6eb0-goog
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength
  2020-10-05 12:50   ` Ulf Hansson
@ 2020-10-06  9:40     ` Ulf Hansson
  0 siblings, 0 replies; 7+ messages in thread
From: Ulf Hansson @ 2020-10-06  9:40 UTC (permalink / raw)
  To: Raul E Rangel
  Cc: linux-mmc, Adrian Hunter, Shirish.S, Akshu Agrawal, Shah,
	Nehal-bakulchandra, Wang, Chris, Linux Kernel Mailing List

On Mon, 5 Oct 2020 at 14:50, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 28 Sep 2020 at 23:59, Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > This change will allow platform designers better control over signal
> > integrity by allowing them to tune the HS200 and HS400 driver strengths.
> >
> > The driver strength was previously hard coded to A to solve boot
> > problems with certain platforms. This driver strength does not
> > universally apply to all platforms so we need a knob to adjust it.
> >
> > All older platforms currently have the SDR104 preset hard coded to A in
> > the firmware. This means that switching from the hard coded value in
> > the kernel to reading the SDR104 preset is a no-op for these platforms.
> > Newer platforms will have properly set presets. So this change will
> > support both new and old platforms.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>
> Applied for next, thanks!

Turned out that this doesn't build, hence I am dropping it from my
next branch. Please submit a new version.

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-06  9:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 21:59 [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Raul E Rangel
2020-09-28 21:59 ` [PATCH 2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength Raul E Rangel
2020-09-30  7:26   ` Adrian Hunter
2020-10-05 12:50   ` Ulf Hansson
2020-10-06  9:40     ` Ulf Hansson
2020-09-30  7:25 ` [PATCH 1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN Adrian Hunter
2020-10-05 12:50 ` Ulf Hansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.