diff mbox series

[2/2] mmc: sdhci-acpi: AMDI0040: Allow changing HS200/HS400 driver strength

Message ID 20200928154718.2.Ic6b6031366f090393d00a53fd69e1ada31ceb29e@changeid
State New
Headers show
Series [1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN | expand

Commit Message

Raul Rangel Sept. 28, 2020, 9:59 p.m. UTC
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(-)

Comments

Adrian Hunter Sept. 30, 2020, 7:26 a.m. UTC | #1
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)
>
Ulf Hansson Oct. 5, 2020, 12:50 p.m. UTC | #2
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

>
Ulf Hansson Oct. 6, 2020, 9:40 a.m. UTC | #3
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
diff mbox series

Patch

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)