diff mbox series

mmc: sdhci-acpi: Fix HS400 tuning for AMDI0040

Message ID 20200818162900.1.Ie8f0689ec9f449203328b37409d1cf06b565f331@changeid (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-acpi: Fix HS400 tuning for AMDI0040 | expand

Commit Message

Raul Rangel Aug. 18, 2020, 10:31 p.m. UTC 
The AMD eMMC Controller can only use the tuned clock while in HS200 and
HS400 mode. If we switch to a different mode, we need to disable the
tuned clock. If we have previously performed tuning and switch back to
HS200 or HS400, we can re-enable the tuned clock.

Previously the tuned clock was not getting disabled when switching to
DDR52 which is part of the HS400 tuning sequence.

Fixes: 34597a3f60b1 ("mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400")
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/host/sdhci-acpi.c | 68 +++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 10 deletions(-)

Comments

kernel test robot Aug. 19, 2020, 6:01 a.m. UTC | #1 
Hi Raul,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc1 next-20200818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Raul-E-Rangel/mmc-sdhci-acpi-Fix-HS400-tuning-for-AMDI0040/20200819-063255
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 18445bf405cb331117bc98427b1ba6f12418ad17
config: x86_64-randconfig-a003-20200818 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project b34b1e38381fa4d1b1d9751a6b5233b68e734cfe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci-acpi.c:607:5: warning: no previous prototype for function 'amd_sdhci_execute_tuning' [-Wmissing-prototypes]
   int amd_sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
       ^
   drivers/mmc/host/sdhci-acpi.c:607:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int amd_sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
   ^
   static 
   1 warning generated.

# https://github.com/0day-ci/linux/commit/dcfd321141ed51cca9f8a3262895bd8df1739f41
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Raul-E-Rangel/mmc-sdhci-acpi-Fix-HS400-tuning-for-AMDI0040/20200819-063255
git checkout dcfd321141ed51cca9f8a3262895bd8df1739f41
vim +/amd_sdhci_execute_tuning +607 drivers/mmc/host/sdhci-acpi.c

   606	
 > 607	int amd_sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
   608	{
   609		int err;
   610		struct sdhci_host *host = mmc_priv(mmc);
   611		struct sdhci_acpi_host *acpi_host = sdhci_priv(host);
   612		struct amd_sdhci_host *amd_host = sdhci_acpi_priv(acpi_host);
   613	
   614		amd_host->tuned_clock = false;
   615	
   616		err = sdhci_execute_tuning(mmc, opcode);
   617	
   618		if (!err && !host->tuning_err)
   619			amd_host->tuned_clock = true;
   620	
   621		return err;
   622	}
   623	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Adrian Hunter Aug. 19, 2020, 9:04 a.m. UTC | #2 
On 19/08/20 1:31 am, Raul E Rangel wrote:
> The AMD eMMC Controller can only use the tuned clock while in HS200 and
> HS400 mode. If we switch to a different mode, we need to disable the
> tuned clock. If we have previously performed tuning and switch back to
> HS200 or HS400, we can re-enable the tuned clock.
> 
> Previously the tuned clock was not getting disabled when switching to
> DDR52 which is part of the HS400 tuning sequence.
> 
> Fixes: 34597a3f60b1 ("mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400")
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

If you make amd_sdhci_execute_tuning() static as the robot pointed out:

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

> ---
> 
>  drivers/mmc/host/sdhci-acpi.c | 68 +++++++++++++++++++++++++++++------
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 48ecbd0b180d8..5a30920ef595f 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -535,6 +535,11 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_qcom_sd = {
>  	.caps    = MMC_CAP_NONREMOVABLE,
>  };
>  
> +struct amd_sdhci_host {
> +	bool	tuned_clock;
> +	bool	dll_enabled;
> +};
> +
>  /* AMD sdhci reset dll register. */
>  #define SDHCI_AMD_RESET_DLL_REGISTER    0x908
>  
> @@ -555,26 +560,67 @@ static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
>  }
>  
>  /*
> - * For AMD Platform it is required to disable the tuning
> - * bit first controller to bring to HS Mode from HS200
> - * mode, later enable to tune to HS400 mode.
> + * The initialization sequence for HS400 is:
> + *     HS->HS200->Perform Tuning->HS->HS400
> + *
> + * The re-tuning sequence is:
> + *     HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400
> + *
> + * The AMD eMMC Controller can only use the tuned clock while in HS200 and HS400
> + * mode. If we switch to a different mode, we need to disable the tuned clock.
> + * If we have previously performed tuning and switch back to HS200 or
> + * HS400, we can re-enable the tuned clock.
> + *
>   */
>  static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>  	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_acpi_host *acpi_host = sdhci_priv(host);
> +	struct amd_sdhci_host *amd_host = sdhci_acpi_priv(acpi_host);
>  	unsigned int old_timing = host->timing;
> +	u16 val;
>  
>  	sdhci_set_ios(mmc, ios);
> -	if (old_timing == MMC_TIMING_MMC_HS200 &&
> -	    ios->timing == MMC_TIMING_MMC_HS)
> -		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
> -	if (old_timing != MMC_TIMING_MMC_HS400 &&
> -	    ios->timing == MMC_TIMING_MMC_HS400) {
> -		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
> -		sdhci_acpi_amd_hs400_dll(host);
> +
> +	if (old_timing != host->timing && amd_host->tuned_clock) {
> +		if (host->timing == MMC_TIMING_MMC_HS400 ||
> +		    host->timing == MMC_TIMING_MMC_HS200) {
> +			val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +			val |= SDHCI_CTRL_TUNED_CLK;
> +			sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +		} else {
> +			val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +			val &= ~SDHCI_CTRL_TUNED_CLK;
> +			sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +		}
> +
> +		/* DLL is only required for HS400 */
> +		if (host->timing == MMC_TIMING_MMC_HS400 &&
> +		    !amd_host->dll_enabled) {
> +			trace_printk("%s: Enabling DLL\n", __func__);
> +			sdhci_acpi_amd_hs400_dll(host);
> +			amd_host->dll_enabled = true;
> +		}
>  	}
>  }
>  
> +int amd_sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	int err;
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_acpi_host *acpi_host = sdhci_priv(host);
> +	struct amd_sdhci_host *amd_host = sdhci_acpi_priv(acpi_host);
> +
> +	amd_host->tuned_clock = false;
> +
> +	err = sdhci_execute_tuning(mmc, opcode);
> +
> +	if (!err && !host->tuning_err)
> +		amd_host->tuned_clock = true;
> +
> +	return err;
> +}
> +
>  static const struct sdhci_ops sdhci_acpi_ops_amd = {
>  	.set_clock	= sdhci_set_clock,
>  	.set_bus_width	= sdhci_set_bus_width,
> @@ -602,6 +648,7 @@ static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
>  
>  	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;
>  	return 0;
>  }
>  
> @@ -613,6 +660,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
>  			  SDHCI_QUIRK_32BIT_ADMA_SIZE,
>  	.quirks2	= SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
>  	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
> +	.priv_size	= sizeof(struct amd_sdhci_host),
>  };
>  
>  struct sdhci_acpi_uid_slot {
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 48ecbd0b180d8..5a30920ef595f 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -535,6 +535,11 @@  static const struct sdhci_acpi_slot sdhci_acpi_slot_qcom_sd = {
 	.caps    = MMC_CAP_NONREMOVABLE,
 };
 
+struct amd_sdhci_host {
+	bool	tuned_clock;
+	bool	dll_enabled;
+};
+
 /* AMD sdhci reset dll register. */
 #define SDHCI_AMD_RESET_DLL_REGISTER    0x908
 
@@ -555,26 +560,67 @@  static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
 }
 
 /*
- * For AMD Platform it is required to disable the tuning
- * bit first controller to bring to HS Mode from HS200
- * mode, later enable to tune to HS400 mode.
+ * The initialization sequence for HS400 is:
+ *     HS->HS200->Perform Tuning->HS->HS400
+ *
+ * The re-tuning sequence is:
+ *     HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400
+ *
+ * The AMD eMMC Controller can only use the tuned clock while in HS200 and HS400
+ * mode. If we switch to a different mode, we need to disable the tuned clock.
+ * If we have previously performed tuning and switch back to HS200 or
+ * HS400, we can re-enable the tuned clock.
+ *
  */
 static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_acpi_host *acpi_host = sdhci_priv(host);
+	struct amd_sdhci_host *amd_host = sdhci_acpi_priv(acpi_host);
 	unsigned int old_timing = host->timing;
+	u16 val;
 
 	sdhci_set_ios(mmc, ios);
-	if (old_timing == MMC_TIMING_MMC_HS200 &&
-	    ios->timing == MMC_TIMING_MMC_HS)
-		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
-	if (old_timing != MMC_TIMING_MMC_HS400 &&
-	    ios->timing == MMC_TIMING_MMC_HS400) {
-		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
-		sdhci_acpi_amd_hs400_dll(host);
+
+	if (old_timing != host->timing && amd_host->tuned_clock) {
+		if (host->timing == MMC_TIMING_MMC_HS400 ||
+		    host->timing == MMC_TIMING_MMC_HS200) {
+			val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+			val |= SDHCI_CTRL_TUNED_CLK;
+			sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+		} else {
+			val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+			val &= ~SDHCI_CTRL_TUNED_CLK;
+			sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+		}
+
+		/* DLL is only required for HS400 */
+		if (host->timing == MMC_TIMING_MMC_HS400 &&
+		    !amd_host->dll_enabled) {
+			trace_printk("%s: Enabling DLL\n", __func__);
+			sdhci_acpi_amd_hs400_dll(host);
+			amd_host->dll_enabled = true;
+		}
 	}
 }
 
+int amd_sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	int err;
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_acpi_host *acpi_host = sdhci_priv(host);
+	struct amd_sdhci_host *amd_host = sdhci_acpi_priv(acpi_host);
+
+	amd_host->tuned_clock = false;
+
+	err = sdhci_execute_tuning(mmc, opcode);
+
+	if (!err && !host->tuning_err)
+		amd_host->tuned_clock = true;
+
+	return err;
+}
+
 static const struct sdhci_ops sdhci_acpi_ops_amd = {
 	.set_clock	= sdhci_set_clock,
 	.set_bus_width	= sdhci_set_bus_width,
@@ -602,6 +648,7 @@  static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
 
 	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;
 	return 0;
 }
 
@@ -613,6 +660,7 @@  static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
 			  SDHCI_QUIRK_32BIT_ADMA_SIZE,
 	.quirks2	= SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
 	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
+	.priv_size	= sizeof(struct amd_sdhci_host),
 };
 
 struct sdhci_acpi_uid_slot {