[PATCH v2 1/2] mmc: sdhci: Add PLL Enable support to internal clock setup

[PATCH v2 1/2] mmc: sdhci: Add PLL Enable support to internal clock setup

[Michael K. Johnson]

The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
setup as part of the internal clock setup as described in 3.2.1 Internal
Clock Setup Sequence of SD Host Controller Simplified Specification
Version 4.20.  This changes the timeouts to the new specification of
150ms for each step and is documented as safe for "prior versions which
do not support PLL Enable."

Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
Signed-off-by: Michael K Johnson <johnsonm@danlj.org>

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 59acf8e3331e..14957578bf2e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1636,8 +1636,8 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 	clk |= SDHCI_CLOCK_INT_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
-	/* Wait max 20 ms */
-	timeout = ktime_add_ms(ktime_get(), 20);
+	/* Wait max 150 ms */
+	timeout = ktime_add_ms(ktime_get(), 150);
 	while (1) {
 		bool timedout = ktime_after(ktime_get(), timeout);
 
@@ -1650,7 +1650,28 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 			sdhci_dumpregs(host);
 			return;
 		}
-		udelay(10);
+		usleep_range(10,15);
+	}
+
+	clk |= SDHCI_CLOCK_PLL_EN;
+	clk &= ~SDHCI_CLOCK_INT_STABLE;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	/* Wait max 150 ms */
+	timeout = ktime_add_ms(ktime_get(), 150);
+	while (1) {
+		bool timedout = ktime_after(ktime_get(), timeout);
+
+		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		if (clk & SDHCI_CLOCK_INT_STABLE)
+			break;
+		if (timedout) {
+			pr_err("%s: PLL clock never stabilised.\n",
+			       mmc_hostname(host->mmc));
+			sdhci_dumpregs(host);
+			return;
+		}
+		usleep_range(10,15);
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 199712e7adbb..72601a4d2e95 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -114,6 +114,7 @@
 #define  SDHCI_DIV_HI_MASK	0x300
 #define  SDHCI_PROG_CLOCK_MODE	0x0020
 #define  SDHCI_CLOCK_CARD_EN	0x0004
+#define  SDHCI_CLOCK_PLL_EN	0x0008
 #define  SDHCI_CLOCK_INT_STABLE	0x0002
 #define  SDHCI_CLOCK_INT_EN	0x0001
 

Re: [PATCH v2 1/2] mmc: sdhci: Add PLL Enable support to internal clock setup

[Adrian Hunter]

On 26/07/19 5:07 AM, Michael K. Johnson wrote:
> The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
> setup as part of the internal clock setup as described in 3.2.1 Internal
> Clock Setup Sequence of SD Host Controller Simplified Specification
> Version 4.20.  This changes the timeouts to the new specification of
> 150ms for each step and is documented as safe for "prior versions which
> do not support PLL Enable."
> 
> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
> Signed-off-by: Michael K Johnson <johnsonm@danlj.org>
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 59acf8e3331e..14957578bf2e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1636,8 +1636,8 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  	clk |= SDHCI_CLOCK_INT_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  
> -	/* Wait max 20 ms */
> -	timeout = ktime_add_ms(ktime_get(), 20);
> +	/* Wait max 150 ms */
> +	timeout = ktime_add_ms(ktime_get(), 150);
>  	while (1) {
>  		bool timedout = ktime_after(ktime_get(), timeout);
>  
> @@ -1650,7 +1650,28 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  			sdhci_dumpregs(host);
>  			return;
>  		}
> -		udelay(10);
> +		usleep_range(10,15);

There is still one place that calls ->set_clock() under spinlock, so that
needs to stay udelay(10) for now.

> +	}
> +

PLL Enable is only valid from v4.1 i.e.

	if (host->version >= SDHCI_SPEC_410 && host->v4_mode)


> +	clk |= SDHCI_CLOCK_PLL_EN;
> +	clk &= ~SDHCI_CLOCK_INT_STABLE;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +	/* Wait max 150 ms */
> +	timeout = ktime_add_ms(ktime_get(), 150);
> +	while (1) {
> +		bool timedout = ktime_after(ktime_get(), timeout);
> +
> +		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +		if (clk & SDHCI_CLOCK_INT_STABLE)
> +			break;
> +		if (timedout) {
> +			pr_err("%s: PLL clock never stabilised.\n",
> +			       mmc_hostname(host->mmc));
> +			sdhci_dumpregs(host);
> +			return;
> +		}
> +		usleep_range(10,15);
>  	}
>  
>  	clk |= SDHCI_CLOCK_CARD_EN;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 199712e7adbb..72601a4d2e95 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -114,6 +114,7 @@
>  #define  SDHCI_DIV_HI_MASK	0x300
>  #define  SDHCI_PROG_CLOCK_MODE	0x0020
>  #define  SDHCI_CLOCK_CARD_EN	0x0004
> +#define  SDHCI_CLOCK_PLL_EN	0x0008
>  #define  SDHCI_CLOCK_INT_STABLE	0x0002
>  #define  SDHCI_CLOCK_INT_EN	0x0001
>  
> 

[PATCH v3 1/3] mmc: sdhci: Add PLL Enable support to internal clock setup

[Michael K. Johnson]

The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
setup as part of the internal clock setup as described in 3.2.1 Internal
Clock Setup Sequence of SD Host Controller Simplified Specification
Version 4.20.  This changes the timeouts to the new specification of
150ms for each step and is documented as safe for "prior versions which
do not support PLL Enable."

Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
Signed-off-by: Michael K Johnson <johnsonm@danlj.org>

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 59acf8e3331e..9106ebc7a422 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1636,8 +1636,8 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 	clk |= SDHCI_CLOCK_INT_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
-	/* Wait max 20 ms */
-	timeout = ktime_add_ms(ktime_get(), 20);
+	/* Wait max 150 ms */
+	timeout = ktime_add_ms(ktime_get(), 150);
 	while (1) {
 		bool timedout = ktime_after(ktime_get(), timeout);
 
@@ -1653,6 +1653,29 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 		udelay(10);
 	}
 
+	if (host->version >= SDHCI_SPEC_410 && host->v4_mode) {
+		clk |= SDHCI_CLOCK_PLL_EN;
+		clk &= ~SDHCI_CLOCK_INT_STABLE;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+		/* Wait max 150 ms */
+		timeout = ktime_add_ms(ktime_get(), 150);
+		while (1) {
+			bool timedout = ktime_after(ktime_get(), timeout);
+
+			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+			if (clk & SDHCI_CLOCK_INT_STABLE)
+				break;
+			if (timedout) {
+				pr_err("%s: PLL clock never stabilised.\n",
+				       mmc_hostname(host->mmc));
+				sdhci_dumpregs(host);
+				return;
+			}
+			udelay(10);
+		}
+	}
+
 	clk |= SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 199712e7adbb..72601a4d2e95 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -114,6 +114,7 @@
 #define  SDHCI_DIV_HI_MASK	0x300
 #define  SDHCI_PROG_CLOCK_MODE	0x0020
 #define  SDHCI_CLOCK_CARD_EN	0x0004
+#define  SDHCI_CLOCK_PLL_EN	0x0008
 #define  SDHCI_CLOCK_INT_STABLE	0x0002
 #define  SDHCI_CLOCK_INT_EN	0x0001
 

Re: [PATCH v3 1/3] mmc: sdhci: Add PLL Enable support to internal clock setup

[Adrian Hunter]

On 13/08/19 1:56 AM, Michael K. Johnson wrote:
> The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
> setup as part of the internal clock setup as described in 3.2.1 Internal
> Clock Setup Sequence of SD Host Controller Simplified Specification
> Version 4.20.  This changes the timeouts to the new specification of
> 150ms for each step and is documented as safe for "prior versions which
> do not support PLL Enable."
> 
> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> Co-developed-by: Michael K Johnson <johnsonm@danlj.org>

Did you mean for this patch to be "From:" Ben Chuang because otherwise
"Co-developed-by" the author is redundant.

> Signed-off-by: Michael K Johnson <johnsonm@danlj.org>
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 59acf8e3331e..9106ebc7a422 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1636,8 +1636,8 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  	clk |= SDHCI_CLOCK_INT_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  
> -	/* Wait max 20 ms */
> -	timeout = ktime_add_ms(ktime_get(), 20);
> +	/* Wait max 150 ms */
> +	timeout = ktime_add_ms(ktime_get(), 150);

Should be a separate patch.

>  	while (1) {
>  		bool timedout = ktime_after(ktime_get(), timeout);
>  
> @@ -1653,6 +1653,29 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  		udelay(10);
>  	}
>  
> +	if (host->version >= SDHCI_SPEC_410 && host->v4_mode) {
> +		clk |= SDHCI_CLOCK_PLL_EN;
> +		clk &= ~SDHCI_CLOCK_INT_STABLE;
> +		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +		/* Wait max 150 ms */
> +		timeout = ktime_add_ms(ktime_get(), 150);
> +		while (1) {
> +			bool timedout = ktime_after(ktime_get(), timeout);
> +
> +			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +			if (clk & SDHCI_CLOCK_INT_STABLE)
> +				break;
> +			if (timedout) {
> +				pr_err("%s: PLL clock never stabilised.\n",
> +				       mmc_hostname(host->mmc));
> +				sdhci_dumpregs(host);
> +				return;
> +			}
> +			udelay(10);
> +		}
> +	}
> +
>  	clk |= SDHCI_CLOCK_CARD_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 199712e7adbb..72601a4d2e95 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -114,6 +114,7 @@
>  #define  SDHCI_DIV_HI_MASK	0x300
>  #define  SDHCI_PROG_CLOCK_MODE	0x0020
>  #define  SDHCI_CLOCK_CARD_EN	0x0004
> +#define  SDHCI_CLOCK_PLL_EN	0x0008
>  #define  SDHCI_CLOCK_INT_STABLE	0x0002
>  #define  SDHCI_CLOCK_INT_EN	0x0001
>  
> 

Re: [PATCH v3 1/3] mmc: sdhci: Add PLL Enable support to internal clock setup

[Michael K. Johnson]

On Thu, Aug 15, 2019 at 02:27:44PM +0300, Adrian Hunter wrote:
> On 13/08/19 1:56 AM, Michael K. Johnson wrote:
> > The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
> > setup as part of the internal clock setup as described in 3.2.1 Internal
> > Clock Setup Sequence of SD Host Controller Simplified Specification
> > Version 4.20.  This changes the timeouts to the new specification of
> > 150ms for each step and is documented as safe for "prior versions which
> > do not support PLL Enable."
> > 
> > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
> 
> Did you mean for this patch to be "From:" Ben Chuang because otherwise
> "Co-developed-by" the author is redundant.

Ben wrote the code and is the primary author. I helped with some
changes to bring it closer to normal style, so I have definitely
been a secondary co-developer. Ben's corporate email server adds a
generic confidentiality notice outside his control, and we were
informed that with that header on the email the patches could not
be accepted.  We developed it in a git repository, so that I have
not been "tainted" by the automatic confidentiality notice, and
at Ben's request I have posted the work. To credit me as primary
author would be fundamentally incorrect.

Are you saying that this work cannot be accepted until Ben chooses
an alternative email provider besides his corporate email in order
to avoid the spurious confidentiality notice, such that he is the
sender of the email?

Re: [PATCH v3 1/3] mmc: sdhci: Add PLL Enable support to internal clock setup

[Adrian Hunter]

On 15/08/19 3:04 PM, Michael K. Johnson wrote:
> On Thu, Aug 15, 2019 at 02:27:44PM +0300, Adrian Hunter wrote:
>> On 13/08/19 1:56 AM, Michael K. Johnson wrote:
>>> The GL9750 and GL9755 chipsets, and possibly others, require PLL Enable
>>> setup as part of the internal clock setup as described in 3.2.1 Internal
>>> Clock Setup Sequence of SD Host Controller Simplified Specification
>>> Version 4.20.  This changes the timeouts to the new specification of
>>> 150ms for each step and is documented as safe for "prior versions which
>>> do not support PLL Enable."
>>>
>>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>>> Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
>>
>> Did you mean for this patch to be "From:" Ben Chuang because otherwise
>> "Co-developed-by" the author is redundant.
> 
> Ben wrote the code and is the primary author. I helped with some
> changes to bring it closer to normal style, so I have definitely
> been a secondary co-developer. Ben's corporate email server adds a
> generic confidentiality notice outside his control, and we were
> informed that with that header on the email the patches could not
> be accepted.  We developed it in a git repository, so that I have
> not been "tainted" by the automatic confidentiality notice, and
> at Ben's request I have posted the work. To credit me as primary
> author would be fundamentally incorrect.
> 
> Are you saying that this work cannot be accepted until Ben chooses
> an alternative email provider besides his corporate email in order
> to avoid the spurious confidentiality notice, such that he is the
> sender of the email?
> 

No, there should be a "From:" line.  Here's an example:

	https://marc.info/?l=linux-mmc&m=155966548112475&q=raw

If you had committed your changes with Ben as the author (git commit
--author) then "git format-patch" would have added the correct "From:" line.