diff mbox series

[v2] mmc: sdhci-msm: Disable CDR function on TX

Message ID 1542881887-21296-1-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series [v2] mmc: sdhci-msm: Disable CDR function on TX | expand

Commit Message

Loic Poulain Nov. 22, 2018, 10:18 a.m. UTC 
The Clock Data Recovery (CDR) circuit allows to automatically adjust
the RX sampling-point/phase for high frequency cards (SDR104, HS200...).
CDR is automatically enabled during DLL configuration.
However, according to the APQ8016 reference manual, this function
must be disabled during TX and tuning phase in order to prevent any
interferences during tuning challenges and unexpected phase alteration
during TX transfers.

This patch enables/disables CDR according to the current transfer mode.

This fixes sporadic write transfer issues observed with some SDR104 and
HS200 cards.

Inspired by sdhci-msm downstream patch:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/432516/

Reported-by: Leonid Segal <leonid.s@variscite.com>
Reported-by: Manabu Igusa <migusa@arrowjapan.com>
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 v2: reword: previous commit message was not the good version

 drivers/mmc/host/sdhci-msm.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Adrian Hunter Nov. 27, 2018, 7:12 a.m. UTC | #1 
On 22/11/18 12:18 PM, Loic Poulain wrote:
> The Clock Data Recovery (CDR) circuit allows to automatically adjust

> the RX sampling-point/phase for high frequency cards (SDR104, HS200...).

> CDR is automatically enabled during DLL configuration.

> However, according to the APQ8016 reference manual, this function

> must be disabled during TX and tuning phase in order to prevent any

> interferences during tuning challenges and unexpected phase alteration

> during TX transfers.

> 

> This patch enables/disables CDR according to the current transfer mode.

> 

> This fixes sporadic write transfer issues observed with some SDR104 and

> HS200 cards.

> 

> Inspired by sdhci-msm downstream patch:

> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/432516/

> 

> Reported-by: Leonid Segal <leonid.s@variscite.com>

> Reported-by: Manabu Igusa <migusa@arrowjapan.com>

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>


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


> ---

>  v2: reword: previous commit message was not the good version

> 

>  drivers/mmc/host/sdhci-msm.c | 44 +++++++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 43 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c

> index a28f5fe..7495854 100644

> --- a/drivers/mmc/host/sdhci-msm.c

> +++ b/drivers/mmc/host/sdhci-msm.c

> @@ -260,6 +260,8 @@ struct sdhci_msm_host {

>  	const struct sdhci_msm_variant_ops *var_ops;

>  	const struct sdhci_msm_offset *offset;

>  	struct icc_path *path;

> +	bool use_cdr;

> +	u32 transfer_mode;

>  };

>  

>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)

> @@ -1027,6 +1029,27 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)

>  	return ret;

>  }

>  

> +static void sdhci_msm_set_cdr(struct sdhci_host *host, bool enable)

> +{

> +	const struct sdhci_msm_offset *msm_offset = sdhci_priv_msm_offset(host);

> +	u32 config, oldconfig = readl_relaxed(host->ioaddr +

> +					      msm_offset->core_dll_config);

> +

> +	config = oldconfig;

> +	if (enable) {

> +		config |= CORE_CDR_EN;

> +		config &= ~CORE_CDR_EXT_EN;

> +	} else {

> +		config &= ~CORE_CDR_EN;

> +		config |= CORE_CDR_EXT_EN;

> +	}

> +

> +	if (config != oldconfig) {

> +		writel_relaxed(config, host->ioaddr +

> +			       msm_offset->core_dll_config);

> +	}

> +}

> +

>  static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)

>  {

>  	struct sdhci_host *host = mmc_priv(mmc);

> @@ -1044,8 +1067,14 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)

>  	if (host->clock <= CORE_FREQ_100MHZ ||

>  	    !(ios.timing == MMC_TIMING_MMC_HS400 ||

>  	    ios.timing == MMC_TIMING_MMC_HS200 ||

> -	    ios.timing == MMC_TIMING_UHS_SDR104))

> +	    ios.timing == MMC_TIMING_UHS_SDR104)) {

> +		msm_host->use_cdr = false;

> +		sdhci_msm_set_cdr(host, false);

>  		return 0;

> +	}

> +

> +	/* Clock-Data-Recovery used to dynamically adjust RX sampling point */

> +	msm_host->use_cdr = true;

>  

>  	/*

>  	 * For HS400 tuning in HS200 timing requires:

> @@ -1527,6 +1556,19 @@ static int __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)

>  	case SDHCI_POWER_CONTROL:

>  		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;

>  		break;

> +	case SDHCI_TRANSFER_MODE:

> +		msm_host->transfer_mode = val;

> +		break;

> +	case SDHCI_COMMAND:

> +		if (!msm_host->use_cdr)

> +			break;

> +		if ((msm_host->transfer_mode & SDHCI_TRNS_READ) &&

> +		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK_HS200) &&

> +		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK))

> +			sdhci_msm_set_cdr(host, true);

> +		else

> +			sdhci_msm_set_cdr(host, false);

> +		break;

>  	}

>  

>  	if (req_type) {

>
Jeffrey Hugo Nov. 27, 2018, 3:36 p.m. UTC | #2 
On 11/22/2018 3:18 AM, Loic Poulain wrote:
> The Clock Data Recovery (CDR) circuit allows to automatically adjust

> the RX sampling-point/phase for high frequency cards (SDR104, HS200...).

> CDR is automatically enabled during DLL configuration.

> However, according to the APQ8016 reference manual, this function


Is this "errata" also mentioned in the reference manuals for other 
devices?  It looks like the below patch gets applied to every device 
that the driver is used for.

> must be disabled during TX and tuning phase in order to prevent any

> interferences during tuning challenges and unexpected phase alteration

> during TX transfers.

> 

> This patch enables/disables CDR according to the current transfer mode.

> 

> This fixes sporadic write transfer issues observed with some SDR104 and

> HS200 cards.

> 

> Inspired by sdhci-msm downstream patch:

> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/432516/

> 

> Reported-by: Leonid Segal <leonid.s@variscite.com>

> Reported-by: Manabu Igusa <migusa@arrowjapan.com>

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> ---

>   v2: reword: previous commit message was not the good version

> 

>   drivers/mmc/host/sdhci-msm.c | 44 +++++++++++++++++++++++++++++++++++++++++++-

>   1 file changed, 43 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c

> index a28f5fe..7495854 100644

> --- a/drivers/mmc/host/sdhci-msm.c

> +++ b/drivers/mmc/host/sdhci-msm.c

> @@ -260,6 +260,8 @@ struct sdhci_msm_host {

>   	const struct sdhci_msm_variant_ops *var_ops;

>   	const struct sdhci_msm_offset *offset;

>   	struct icc_path *path;

> +	bool use_cdr;

> +	u32 transfer_mode;

>   };

>   

>   static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)

> @@ -1027,6 +1029,27 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)

>   	return ret;

>   }

>   

> +static void sdhci_msm_set_cdr(struct sdhci_host *host, bool enable)

> +{

> +	const struct sdhci_msm_offset *msm_offset = sdhci_priv_msm_offset(host);

> +	u32 config, oldconfig = readl_relaxed(host->ioaddr +

> +					      msm_offset->core_dll_config);

> +

> +	config = oldconfig;

> +	if (enable) {

> +		config |= CORE_CDR_EN;

> +		config &= ~CORE_CDR_EXT_EN;

> +	} else {

> +		config &= ~CORE_CDR_EN;

> +		config |= CORE_CDR_EXT_EN;

> +	}

> +

> +	if (config != oldconfig) {

> +		writel_relaxed(config, host->ioaddr +

> +			       msm_offset->core_dll_config);

> +	}

> +}

> +

>   static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)

>   {

>   	struct sdhci_host *host = mmc_priv(mmc);

> @@ -1044,8 +1067,14 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)

>   	if (host->clock <= CORE_FREQ_100MHZ ||

>   	    !(ios.timing == MMC_TIMING_MMC_HS400 ||

>   	    ios.timing == MMC_TIMING_MMC_HS200 ||

> -	    ios.timing == MMC_TIMING_UHS_SDR104))

> +	    ios.timing == MMC_TIMING_UHS_SDR104)) {

> +		msm_host->use_cdr = false;

> +		sdhci_msm_set_cdr(host, false);

>   		return 0;

> +	}

> +

> +	/* Clock-Data-Recovery used to dynamically adjust RX sampling point */

> +	msm_host->use_cdr = true;

>   

>   	/*

>   	 * For HS400 tuning in HS200 timing requires:

> @@ -1527,6 +1556,19 @@ static int __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)

>   	case SDHCI_POWER_CONTROL:

>   		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;

>   		break;

> +	case SDHCI_TRANSFER_MODE:

> +		msm_host->transfer_mode = val;

> +		break;

> +	case SDHCI_COMMAND:

> +		if (!msm_host->use_cdr)

> +			break;

> +		if ((msm_host->transfer_mode & SDHCI_TRNS_READ) &&

> +		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK_HS200) &&

> +		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK))

> +			sdhci_msm_set_cdr(host, true);

> +		else

> +			sdhci_msm_set_cdr(host, false);

> +		break;

>   	}

>   

>   	if (req_type) {

> 



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Jeffrey Hugo Nov. 28, 2018, 5:48 p.m. UTC | #3 
On 11/28/2018 2:48 AM, Loic Poulain wrote:
> Hi Jeffrey,

> 

> On Tue, 27 Nov 2018 at 16:36, Jeffrey Hugo <jhugo@codeaurora.org 

> <mailto:jhugo@codeaurora.org>> wrote:

> 

>     On 11/22/2018 3:18 AM, Loic Poulain wrote:

>      > The Clock Data Recovery (CDR) circuit allows to automatically adjust

>      > the RX sampling-point/phase for high frequency cards (SDR104,

>     HS200...).

>      > CDR is automatically enabled during DLL configuration.

>      > However, according to the APQ8016 reference manual, this function

> 

>     Is this "errata" also mentioned in the reference manuals for other

>     devices?  It looks like the below patch gets applied to every device

>     that the driver is used for.

> 

> 

> This is not really an errata but a normal step descibed in the SDCC 

> Write data sequence

> (cf chapter 9.4.8 of APQ8016E Technical Reference Manual [1]).

> 

> I tested the patch with apq8096 as well (dragonboard-820c), but since I 

> have only access to

> the apq8016 documentation, I can not confirm it 100% applies to all 

> other platforms.

> 

> However, I noted this mechanism is also present in littlekernel(LK) as 

> msm **shared** sdhci code [2].

> I concluded it was a common msm sdhci configuration.

> 

> Let me know if you have more information but it does not look like a 

> specific apq8016 quirk for me.


The downstream Qualcomm driver appears to do this, and I see similar 
text in the hardware programming guide, so it appears that this is not a 
apq8016 specific quirk.  Thanks for the clarification.

I'm going to give this change a go on 8998.  I do notice a minor issue 
(see below) which I need to workaround when applying the change.

> 

> [1] 

> https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf

> [2] 

> https://git.linaro.org/landing-teams/working/qualcomm/lk.git/tree/platform/msm_shared/sdhci.c?h=release/LA.HB.1.3.2-19600-8x96.0#n854

> 

> 

> 

>      > must be disabled during TX and tuning phase in order to prevent any

>      > interferences during tuning challenges and unexpected phase

>     alteration

>      > during TX transfers.

>      >

>      > This patch enables/disables CDR according to the current transfer

>     mode.

>      >

>      > This fixes sporadic write transfer issues observed with some

>     SDR104 and

>      > HS200 cards.

>      >

>      > Inspired by sdhci-msm downstream patch:

>      >

>     https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/432516/

>      >

>      > Reported-by: Leonid Segal <leonid.s@variscite.com

>     <mailto:leonid.s@variscite.com>>

>      > Reported-by: Manabu Igusa <migusa@arrowjapan.com

>     <mailto:migusa@arrowjapan.com>>

>      > Signed-off-by: Loic Poulain <loic.poulain@linaro.org

>     <mailto:loic.poulain@linaro.org>>

>      > ---

>      >   v2: reword: previous commit message was not the good version

>      >

>      >   drivers/mmc/host/sdhci-msm.c | 44

>     +++++++++++++++++++++++++++++++++++++++++++-

>      >   1 file changed, 43 insertions(+), 1 deletion(-)

>      >

>      > diff --git a/drivers/mmc/host/sdhci-msm.c

>     b/drivers/mmc/host/sdhci-msm.c

>      > index a28f5fe..7495854 100644

>      > --- a/drivers/mmc/host/sdhci-msm.c

>      > +++ b/drivers/mmc/host/sdhci-msm.c

>      > @@ -260,6 +260,8 @@ struct sdhci_msm_host {

>      >       const struct sdhci_msm_variant_ops *var_ops;

>      >       const struct sdhci_msm_offset *offset;

>      >       struct icc_path *path;


I don't see "struct icc_path *path;" in mainline or -next.  Is there 
some dependency I'm missing?

>      > +     bool use_cdr;

>      > +     u32 transfer_mode;

>      >   };

>      >

>      >   static const struct sdhci_msm_offset

>     *sdhci_priv_msm_offset(struct sdhci_host *host)

>      > @@ -1027,6 +1029,27 @@ static int

>     sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)

>      >       return ret;

>      >   }

>      >

>      > +static void sdhci_msm_set_cdr(struct sdhci_host *host, bool enable)

>      > +{

>      > +     const struct sdhci_msm_offset *msm_offset =

>     sdhci_priv_msm_offset(host);

>      > +     u32 config, oldconfig = readl_relaxed(host->ioaddr +

>      > +                                         

>       msm_offset->core_dll_config);

>      > +

>      > +     config = oldconfig;

>      > +     if (enable) {

>      > +             config |= CORE_CDR_EN;

>      > +             config &= ~CORE_CDR_EXT_EN;

>      > +     } else {

>      > +             config &= ~CORE_CDR_EN;

>      > +             config |= CORE_CDR_EXT_EN;

>      > +     }

>      > +

>      > +     if (config != oldconfig) {

>      > +             writel_relaxed(config, host->ioaddr +

>      > +                            msm_offset->core_dll_config);

>      > +     }

>      > +}

>      > +

>      >   static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32

>     opcode)

>      >   {

>      >       struct sdhci_host *host = mmc_priv(mmc);

>      > @@ -1044,8 +1067,14 @@ static int sdhci_msm_execute_tuning(struct

>     mmc_host *mmc, u32 opcode)

>      >       if (host->clock <= CORE_FREQ_100MHZ ||

>      >           !(ios.timing == MMC_TIMING_MMC_HS400 ||

>      >           ios.timing == MMC_TIMING_MMC_HS200 ||

>      > -         ios.timing == MMC_TIMING_UHS_SDR104))

>      > +         ios.timing == MMC_TIMING_UHS_SDR104)) {

>      > +             msm_host->use_cdr = false;

>      > +             sdhci_msm_set_cdr(host, false);

>      >               return 0;

>      > +     }

>      > +

>      > +     /* Clock-Data-Recovery used to dynamically adjust RX

>     sampling point */

>      > +     msm_host->use_cdr = true;

>      >

>      >       /*

>      >        * For HS400 tuning in HS200 timing requires:

>      > @@ -1527,6 +1556,19 @@ static int __sdhci_msm_check_write(struct

>     sdhci_host *host, u16 val, int reg)

>      >       case SDHCI_POWER_CONTROL:

>      >               req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;

>      >               break;

>      > +     case SDHCI_TRANSFER_MODE:

>      > +             msm_host->transfer_mode = val;

>      > +             break;

>      > +     case SDHCI_COMMAND:

>      > +             if (!msm_host->use_cdr)

>      > +                     break;

>      > +             if ((msm_host->transfer_mode & SDHCI_TRNS_READ) &&

>      > +                 (SDHCI_GET_CMD(val) !=

>     MMC_SEND_TUNING_BLOCK_HS200) &&

>      > +                 (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK))

>      > +                     sdhci_msm_set_cdr(host, true);

>      > +             else

>      > +                     sdhci_msm_set_cdr(host, false);

>      > +             break;

>      >       }

>      >

>      >       if (req_type) {

>      >

> 

> 

> Regards,

> Loic



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index a28f5fe..7495854 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -260,6 +260,8 @@  struct sdhci_msm_host {
 	const struct sdhci_msm_variant_ops *var_ops;
 	const struct sdhci_msm_offset *offset;
 	struct icc_path *path;
+	bool use_cdr;
+	u32 transfer_mode;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1027,6 +1029,27 @@  static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	return ret;
 }
 
+static void sdhci_msm_set_cdr(struct sdhci_host *host, bool enable)
+{
+	const struct sdhci_msm_offset *msm_offset = sdhci_priv_msm_offset(host);
+	u32 config, oldconfig = readl_relaxed(host->ioaddr +
+					      msm_offset->core_dll_config);
+
+	config = oldconfig;
+	if (enable) {
+		config |= CORE_CDR_EN;
+		config &= ~CORE_CDR_EXT_EN;
+	} else {
+		config &= ~CORE_CDR_EN;
+		config |= CORE_CDR_EXT_EN;
+	}
+
+	if (config != oldconfig) {
+		writel_relaxed(config, host->ioaddr +
+			       msm_offset->core_dll_config);
+	}
+}
+
 static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -1044,8 +1067,14 @@  static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	if (host->clock <= CORE_FREQ_100MHZ ||
 	    !(ios.timing == MMC_TIMING_MMC_HS400 ||
 	    ios.timing == MMC_TIMING_MMC_HS200 ||
-	    ios.timing == MMC_TIMING_UHS_SDR104))
+	    ios.timing == MMC_TIMING_UHS_SDR104)) {
+		msm_host->use_cdr = false;
+		sdhci_msm_set_cdr(host, false);
 		return 0;
+	}
+
+	/* Clock-Data-Recovery used to dynamically adjust RX sampling point */
+	msm_host->use_cdr = true;
 
 	/*
 	 * For HS400 tuning in HS200 timing requires:
@@ -1527,6 +1556,19 @@  static int __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
 	case SDHCI_POWER_CONTROL:
 		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
 		break;
+	case SDHCI_TRANSFER_MODE:
+		msm_host->transfer_mode = val;
+		break;
+	case SDHCI_COMMAND:
+		if (!msm_host->use_cdr)
+			break;
+		if ((msm_host->transfer_mode & SDHCI_TRNS_READ) &&
+		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK_HS200) &&
+		    (SDHCI_GET_CMD(val) != MMC_SEND_TUNING_BLOCK))
+			sdhci_msm_set_cdr(host, true);
+		else
+			sdhci_msm_set_cdr(host, false);
+		break;
 	}
 
 	if (req_type) {