diff mbox

[v6,05/14] mmc: sdhci-msm: Update DLL reset sequence

Message ID 1478517877-23733-6-git-send-email-riteshh@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ritesh Harjani Nov. 7, 2016, 11:24 a.m. UTC 
From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

SDCC core with minor version >= 0x42 introduced new 14lpp
DLL. This has additional requirements in the reset sequence
for DLL tuning. Make necessary changes as needed.

Without this patch we see below errors on such SDHC controllers
	sdhci_msm 7464900.sdhci: mmc0: DLL failed to LOCK
	mmc0: tuning execution failed: -110

Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-msm.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Stephen Boyd Nov. 8, 2016, 11:06 p.m. UTC | #1 
On 11/07, Ritesh Harjani wrote:
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 42f42aa..32b0b79 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -58,11 +58,17 @@
>  #define CORE_DLL_CONFIG		0x100
>  #define CORE_DLL_STATUS		0x108
>  
> +#define CORE_DLL_CONFIG_2	0x1b4
> +#define CORE_FLL_CYCLE_CNT	BIT(18)
> +#define CORE_DLL_CLOCK_DISABLE	BIT(21)
> +
>  #define CORE_VENDOR_SPEC	0x10c
>  #define CORE_CLK_PWRSAVE	BIT(1)
>  
>  #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
>  
> +#define TCXO_FREQ		19200000

TCXO_FREQ could change based on the board. For example, IPQ has
it as 25 MHz.

> +
>  #define CDR_SELEXT_SHIFT	20
>  #define CDR_SELEXT_MASK		(0xf << CDR_SELEXT_SHIFT)
>  #define CMUX_SHIFT_PHASE_SHIFT	24
> @@ -330,6 +349,24 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>  	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>  	msm_cm_dll_set_freq(host);
>  
> +	if (msm_host->use_14lpp_dll_reset) {
> +		u32 mclk_freq = 0;
> +
> +		if ((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2)
> +					& CORE_FLL_CYCLE_CNT))

I suggest you grow a local variable.

> +			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 8);

Is the cast necessary?

> +		else
> +			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 4);

Ditto

> +
> +		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
> +		config &= ~(0xFF << 10);
> +		config |= mclk_freq << 10;
> +
> +		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
> +		/* wait for 5us before enabling DLL clock */

Usually there's a barrier between writel_relaxed() and delay
because we don't know when the writel will be posted out and the
delay is there to wait for the operation to happen. Probably
should change this to be a writel() instead.

> +		udelay(5);
> +	}
> +
>  	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
>  	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>  	config &= ~CORE_DLL_RST;
> @@ -340,6 +377,14 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>  	config &= ~CORE_DLL_PDN;
>  	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>  
> +	if (msm_host->use_14lpp_dll_reset) {
> +		msm_cm_dll_set_freq(host);
> +		/* Enable the DLL clock */
> +		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
> +		config &= ~CORE_DLL_CLOCK_DISABLE;
> +		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
> +	}
> +
>  	/* Set DLL_EN bit to 1. */
>  	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>  	config |= CORE_DLL_EN;
> @@ -641,6 +686,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
>  		core_version, core_major, core_minor);
>  
> +	if ((core_major == 1) && (core_minor >= 0x42))

Why so many parenthesis?

> +		msm_host->use_14lpp_dll_reset = true;
> +
>  	/*
Arnd Bergmann Nov. 8, 2016, 11:14 p.m. UTC | #2 
On Tuesday, November 8, 2016 3:06:22 PM CET Stephen Boyd wrote:
> > +
> > +             config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
> > +             config &= ~(0xFF << 10);
> > +             config |= mclk_freq << 10;
> > +
> > +             writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
> > +             /* wait for 5us before enabling DLL clock */
> 
> Usually there's a barrier between writel_relaxed() and delay
> because we don't know when the writel will be posted out and the
> delay is there to wait for the operation to happen. Probably
> should change this to be a writel() instead.
> 

The barrier in writel() is not for posted writes, it is to synchronize
with memory accesses *before* the write.

In general, if you want to ensure that a write has made it to the
device, you need to read back from the same address (the specific
behavior may depend on the bus).

While in general, using the non-relaxed accessors should be the
default (and there should be a comment for each *_relaxed access),
but I don't think using writel() would let you skip the delay here.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ritesh Harjani Nov. 9, 2016, 12:06 p.m. UTC | #3 
Hi Stephen,

On 11/9/2016 4:36 AM, Stephen Boyd wrote:
> On 11/07, Ritesh Harjani wrote:
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 42f42aa..32b0b79 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -58,11 +58,17 @@
>>  #define CORE_DLL_CONFIG		0x100
>>  #define CORE_DLL_STATUS		0x108
>>
>> +#define CORE_DLL_CONFIG_2	0x1b4
>> +#define CORE_FLL_CYCLE_CNT	BIT(18)
>> +#define CORE_DLL_CLOCK_DISABLE	BIT(21)
>> +
>>  #define CORE_VENDOR_SPEC	0x10c
>>  #define CORE_CLK_PWRSAVE	BIT(1)
>>
>>  #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
>>
>> +#define TCXO_FREQ		19200000
>
> TCXO_FREQ could change based on the board. For example, IPQ has
> it as 25 MHz.
Actually not sure of the proper way on how to get this freq in driver
today. We may use xo_board clock but, it is not available for all boards
except 8996/8916 I guess.

Also, there is no sdhc for IPQ board and for all other boards TCXO_FREQ 
is same where sdhci-msm driver is used. For that purpose this was 
defined here for sdhci-msm driver.

Do you think in that case we should keep it this way for now and later 
change if a need arise to change the TCXO_FREQ ?

>
>> +
>>  #define CDR_SELEXT_SHIFT	20
>>  #define CDR_SELEXT_MASK		(0xf << CDR_SELEXT_SHIFT)
>>  #define CMUX_SHIFT_PHASE_SHIFT	24
>> @@ -330,6 +349,24 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>  	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>>  	msm_cm_dll_set_freq(host);
>>
>> +	if (msm_host->use_14lpp_dll_reset) {
>> +		u32 mclk_freq = 0;
>> +
>> +		if ((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2)
>> +					& CORE_FLL_CYCLE_CNT))
>
> I suggest you grow a local variable.
Ok.

>
>> +			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 8);
>
> Is the cast necessary?
Will remove it.

>
>> +		else
>> +			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 4);
>
> Ditto
Will remove it.

>
>> +
>> +		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +		config &= ~(0xFF << 10);
>> +		config |= mclk_freq << 10;
>> +
>> +		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> +		/* wait for 5us before enabling DLL clock */
>
> Usually there's a barrier between writel_relaxed() and delay
> because we don't know when the writel will be posted out and the
> delay is there to wait for the operation to happen. Probably
> should change this to be a writel() instead.

Arnd, already explained here.
We do need the udelay here as per the HW sequence itself.


>
>> +		udelay(5);
>> +	}
>> +
>>  	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
>>  	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>>  	config &= ~CORE_DLL_RST;
>> @@ -340,6 +377,14 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>  	config &= ~CORE_DLL_PDN;
>>  	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>>
>> +	if (msm_host->use_14lpp_dll_reset) {
>> +		msm_cm_dll_set_freq(host);
>> +		/* Enable the DLL clock */
>> +		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +		config &= ~CORE_DLL_CLOCK_DISABLE;
>> +		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> +	}
>> +
>>  	/* Set DLL_EN bit to 1. */
>>  	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>>  	config |= CORE_DLL_EN;
>> @@ -641,6 +686,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>  	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
>>  		core_version, core_major, core_minor);
>>
>> +	if ((core_major == 1) && (core_minor >= 0x42))
>
> Why so many parenthesis?
Sure, will remove it.

>
>> +		msm_host->use_14lpp_dll_reset = true;
>> +
>>  	/*
>
Stephen Boyd Nov. 9, 2016, 8:43 p.m. UTC | #4 
On 11/09, Ritesh Harjani wrote:
> Hi Stephen,
> 
> On 11/9/2016 4:36 AM, Stephen Boyd wrote:
> >On 11/07, Ritesh Harjani wrote:
> >>
> >>diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >>index 42f42aa..32b0b79 100644
> >>--- a/drivers/mmc/host/sdhci-msm.c
> >>+++ b/drivers/mmc/host/sdhci-msm.c
> >>@@ -58,11 +58,17 @@
> >> #define CORE_DLL_CONFIG		0x100
> >> #define CORE_DLL_STATUS		0x108
> >>
> >>+#define CORE_DLL_CONFIG_2	0x1b4
> >>+#define CORE_FLL_CYCLE_CNT	BIT(18)
> >>+#define CORE_DLL_CLOCK_DISABLE	BIT(21)
> >>+
> >> #define CORE_VENDOR_SPEC	0x10c
> >> #define CORE_CLK_PWRSAVE	BIT(1)
> >>
> >> #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
> >>
> >>+#define TCXO_FREQ		19200000
> >
> >TCXO_FREQ could change based on the board. For example, IPQ has
> >it as 25 MHz.
> Actually not sure of the proper way on how to get this freq in driver
> today. We may use xo_board clock but, it is not available for all boards
> except 8996/8916 I guess.
> 
> Also, there is no sdhc for IPQ board and for all other boards
> TCXO_FREQ is same where sdhci-msm driver is used. For that purpose
> this was defined here for sdhci-msm driver.
> 
> Do you think in that case we should keep it this way for now and
> later change if a need arise to change the TCXO_FREQ ?

We've added xo_board (or cxo_board/pxo_board) to all the qcom
platforms upstream, so there should always be something to
reference in the dts and call clk_get_rate() on. So I would add
it to the binding as another clock and then use that instead of
hardcoding the value. That's much more flexible in case this
changes in the future.
Ritesh Harjani Nov. 14, 2016, 6:03 a.m. UTC | #5 
Hi Stephen,

On 11/10/2016 2:13 AM, Stephen Boyd wrote:
> On 11/09, Ritesh Harjani wrote:
>> Hi Stephen,
>>
>> On 11/9/2016 4:36 AM, Stephen Boyd wrote:
>>> On 11/07, Ritesh Harjani wrote:
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 42f42aa..32b0b79 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -58,11 +58,17 @@
>>>> #define CORE_DLL_CONFIG		0x100
>>>> #define CORE_DLL_STATUS		0x108
>>>>
>>>> +#define CORE_DLL_CONFIG_2	0x1b4
>>>> +#define CORE_FLL_CYCLE_CNT	BIT(18)
>>>> +#define CORE_DLL_CLOCK_DISABLE	BIT(21)
>>>> +
>>>> #define CORE_VENDOR_SPEC	0x10c
>>>> #define CORE_CLK_PWRSAVE	BIT(1)
>>>>
>>>> #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
>>>>
>>>> +#define TCXO_FREQ		19200000
>>>
>>> TCXO_FREQ could change based on the board. For example, IPQ has
>>> it as 25 MHz.
>> Actually not sure of the proper way on how to get this freq in driver
>> today. We may use xo_board clock but, it is not available for all boards
>> except 8996/8916 I guess.
>>
>> Also, there is no sdhc for IPQ board and for all other boards
>> TCXO_FREQ is same where sdhci-msm driver is used. For that purpose
>> this was defined here for sdhci-msm driver.
>>
>> Do you think in that case we should keep it this way for now and
>> later change if a need arise to change the TCXO_FREQ ?
>
> We've added xo_board (or cxo_board/pxo_board) to all the qcom
> platforms upstream, so there should always be something to
> reference in the dts and call clk_get_rate() on. So I would add
> it to the binding as another clock and then use that instead of
> hardcoding the value. That's much more flexible in case this
> changes in the future.
>
Sure, I have addressed this in v7 as per your above comment.
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 42f42aa..32b0b79 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -58,11 +58,17 @@ 
 #define CORE_DLL_CONFIG		0x100
 #define CORE_DLL_STATUS		0x108
 
+#define CORE_DLL_CONFIG_2	0x1b4
+#define CORE_FLL_CYCLE_CNT	BIT(18)
+#define CORE_DLL_CLOCK_DISABLE	BIT(21)
+
 #define CORE_VENDOR_SPEC	0x10c
 #define CORE_CLK_PWRSAVE	BIT(1)
 
 #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
 
+#define TCXO_FREQ		19200000
+
 #define CDR_SELEXT_SHIFT	20
 #define CDR_SELEXT_MASK		(0xf << CDR_SELEXT_SHIFT)
 #define CMUX_SHIFT_PHASE_SHIFT	24
@@ -76,6 +82,7 @@  struct sdhci_msm_host {
 	struct clk *pclk;	/* SDHC peripheral bus clock */
 	struct clk *bus_clk;	/* SDHC bus voter clock */
 	struct mmc_host *mmc;
+	bool use_14lpp_dll_reset;
 };
 
 /* Platform specific tuning */
@@ -304,6 +311,8 @@  static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
 static int msm_init_cm_dll(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	int wait_cnt = 50;
 	unsigned long flags;
 	u32 config = 0;
@@ -319,6 +328,16 @@  static int msm_init_cm_dll(struct sdhci_host *host)
 	config &= ~CORE_CLK_PWRSAVE;
 	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config &= ~CORE_CK_OUT_EN;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config |= CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+	}
+
 	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config |= CORE_DLL_RST;
@@ -330,6 +349,24 @@  static int msm_init_cm_dll(struct sdhci_host *host)
 	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 	msm_cm_dll_set_freq(host);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		u32 mclk_freq = 0;
+
+		if ((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2)
+					& CORE_FLL_CYCLE_CNT))
+			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 8);
+		else
+			mclk_freq = (u32)((host->clock / TCXO_FREQ) * 4);
+
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config &= ~(0xFF << 10);
+		config |= mclk_freq << 10;
+
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+		/* wait for 5us before enabling DLL clock */
+		udelay(5);
+	}
+
 	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config &= ~CORE_DLL_RST;
@@ -340,6 +377,14 @@  static int msm_init_cm_dll(struct sdhci_host *host)
 	config &= ~CORE_DLL_PDN;
 	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		msm_cm_dll_set_freq(host);
+		/* Enable the DLL clock */
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config &= ~CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+	}
+
 	/* Set DLL_EN bit to 1. */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config |= CORE_DLL_EN;
@@ -641,6 +686,9 @@  static int sdhci_msm_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
 		core_version, core_major, core_minor);
 
+	if ((core_major == 1) && (core_minor >= 0x42))
+		msm_host->use_14lpp_dll_reset = true;
+
 	/*
 	 * Support for some capabilities is not advertised by newer
 	 * controller versions and must be explicitly enabled.