Hi,
I have recently observed that the quirk
SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST I have introduced doesn't fix all
the bugs relative to the internal clock disabling while configuring the SD
clock. This delay was introduced because of a re-synchronisation done when
disabling the internal clock.
Unfortunately, we can still have clock stabilization bug even if it occurs
rarely. Moreover, trying to use presets, disabling the internal clock causes an
unexpected switch to the base clock. It should be solved on next revision of
the chip.
For those reasons plus the non acceptance of new quirks, I have decided to
remove it and to implement my own set_clock() function. In ordrer to reduce
code duplication with the sdhci set_clock function, I moved some of the
code in a new sdhci_compute_clock_config() function.
Regards
Ludovic Desroches (3):
mmc: sdhci: introduce sdhci_compute_clock_config
mmc: sdhci-of-at91: implement specific set_clock function
mmc: sdhci: removal of SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST
drivers/mmc/host/sdhci-of-at91.c | 48 ++++++++++++++++++++++++++++++++++++++--
drivers/mmc/host/sdhci.c | 41 ++++++++++++++++++++--------------
drivers/mmc/host/sdhci.h | 7 ++----
3 files changed, 72 insertions(+), 24 deletions(-)
--
2.5.0
In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
reduce code duplication, put the code relative to the SD clock
configuration in a function which can be used by hosts for the
implementation of the set_clock() callback.
Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/sdhci.c | 43 ++++++++++++++++++++++++++-----------------
drivers/mmc/host/sdhci.h | 2 ++
2 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6bd3d17..ee095df 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1091,34 +1091,24 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
return preset;
}
-void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+void sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock,
+ u16 *clk_reg)
{
int div = 0; /* Initialized for compiler warning */
int real_div = div, clk_mul = 1;
- u16 clk = 0;
- unsigned long timeout;
bool switch_base_clk = false;
- host->mmc->actual_clock = 0;
-
- sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
- if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
- mdelay(1);
-
- if (clock == 0)
- return;
-
if (host->version >= SDHCI_SPEC_300) {
if (host->preset_enabled) {
u16 pre_val;
- clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ *clk_reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
pre_val = sdhci_get_preset_value(host);
div = (pre_val & SDHCI_PRESET_SDCLK_FREQ_MASK)
>> SDHCI_PRESET_SDCLK_FREQ_SHIFT;
if (host->clk_mul &&
(pre_val & SDHCI_PRESET_CLKGEN_SEL_MASK)) {
- clk = SDHCI_PROG_CLOCK_MODE;
+ *clk_reg = SDHCI_PROG_CLOCK_MODE;
real_div = div + 1;
clk_mul = host->clk_mul;
} else {
@@ -1142,7 +1132,7 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
* Set Programmable Clock Mode in the Clock
* Control register.
*/
- clk = SDHCI_PROG_CLOCK_MODE;
+ *clk_reg = SDHCI_PROG_CLOCK_MODE;
real_div = div;
clk_mul = host->clk_mul;
div--;
@@ -1185,9 +1175,28 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
clock_set:
if (real_div)
host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
- clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
- clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
+ *clk_reg |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
+ *clk_reg |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
<< SDHCI_DIVIDER_HI_SHIFT;
+}
+EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
+
+void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+ u16 clk = 0;
+ unsigned long timeout;
+
+ host->mmc->actual_clock = 0;
+
+ sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+ if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
+ mdelay(1);
+
+ if (clock == 0)
+ return;
+
+ sdhci_compute_clock_config(host, clock, &clk);
+
clk |= SDHCI_CLOCK_INT_EN;
sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0f39f4f..4215eb7 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
}
+void sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock,
+ u16 *clk_reg);
void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
unsigned short vdd);
--
2.5.0
Disabling the internal clock while configuring the SD card clock can
lead to internal clock stabilization issue and/or unexpected switch to
the base clock when using presets.
A quirk SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST was introduced to fix
these bugs. The cause was assumed to be a too long internal
re-synchronisation but it seems in some cases the delay (even if longer)
doesn't fix this bug. The safest workaround is to not disable/enable the
internal clock during the SD card clock configuration.
Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/sdhci-of-at91.c | 48 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 46 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..5b345371 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -15,6 +15,7 @@
*/
#include <linux/clk.h>
+#include <linux/delay.h>
#include <linux/err.h>
#include <linux/io.h>
#include <linux/mmc/host.h>
@@ -37,8 +38,52 @@ struct sdhci_at91_priv {
struct clk *mainck;
};
+static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+ u16 clk;
+ unsigned long timeout;
+
+ host->mmc->actual_clock = 0;
+
+ /*
+ * There is no requirement to disable the internal clock before
+ * changing the SD clock configuration. Moreover, disabling the
+ * internal clock, changing the configuration and re-enabling the
+ * internal clock causes some bugs. It can prevent to get the internal
+ * clock stable flag ready and an unexpected switch to the base clock
+ * when using presets.
+ */
+ clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ clk &= SDHCI_CLOCK_INT_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+ if (clock == 0)
+ return;
+
+ sdhci_compute_clock_config(host, clock, &clk);
+
+ clk |= SDHCI_CLOCK_INT_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+ /* Wait max 20 ms */
+ timeout = 20;
+ while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
+ & SDHCI_CLOCK_INT_STABLE)) {
+ if (timeout == 0) {
+ pr_err("%s: Internal clock never stabilised.\n",
+ mmc_hostname(host->mmc));
+ return;
+ }
+ timeout--;
+ mdelay(1);
+ }
+
+ clk |= SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+}
+
static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
- .set_clock = sdhci_set_clock,
+ .set_clock = sdhci_at91_set_clock,
.set_bus_width = sdhci_set_bus_width,
.reset = sdhci_reset,
.set_uhs_signaling = sdhci_set_uhs_signaling,
@@ -46,7 +91,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
static const struct sdhci_pltfm_data soc_data_sama5d2 = {
.ops = &sdhci_at91_sama5d2_ops,
- .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
};
static const struct of_device_id sdhci_at91_dt_match[] = {
--
2.5.0
SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST quirk is not used anymore so
remove it.
Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/sdhci.c | 2 --
drivers/mmc/host/sdhci.h | 5 -----
2 files changed, 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ee095df..002c9f8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1189,8 +1189,6 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
host->mmc->actual_clock = 0;
sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
- if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
- mdelay(1);
if (clock == 0)
return;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 4215eb7..8302e4c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -417,11 +417,6 @@ struct sdhci_host {
#define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)
/* Broken Clock divider zero in controller */
#define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)
-/*
- * When internal clock is disabled, a delay is needed before modifying the
- * SD clock frequency or enabling back the internal clock.
- */
-#define SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST (1<<16)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
2.5.0
On 4 April 2016 at 16:00, Ludovic Desroches <[email protected]> wrote:
> In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
> reduce code duplication, put the code relative to the SD clock
> configuration in a function which can be used by hosts for the
> implementation of the set_clock() callback.
I like this as it moves the code into the "library direction".
>
> Signed-off-by: Ludovic Desroches <[email protected]>
> ---
> drivers/mmc/host/sdhci.c | 43 ++++++++++++++++++++++++++-----------------
> drivers/mmc/host/sdhci.h | 2 ++
> 2 files changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6bd3d17..ee095df 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1091,34 +1091,24 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
> return preset;
> }
>
> -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +void sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock,
> + u16 *clk_reg)
I would prefer that the value is returned as the result, instead of
providing it as an out-parameter.
> {
> int div = 0; /* Initialized for compiler warning */
> int real_div = div, clk_mul = 1;
> - u16 clk = 0;
> - unsigned long timeout;
> bool switch_base_clk = false;
>
> - host->mmc->actual_clock = 0;
> -
> - sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> - if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> - mdelay(1);
> -
> - if (clock == 0)
> - return;
> -
> if (host->version >= SDHCI_SPEC_300) {
> if (host->preset_enabled) {
> u16 pre_val;
>
> - clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> + *clk_reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> pre_val = sdhci_get_preset_value(host);
> div = (pre_val & SDHCI_PRESET_SDCLK_FREQ_MASK)
> >> SDHCI_PRESET_SDCLK_FREQ_SHIFT;
> if (host->clk_mul &&
> (pre_val & SDHCI_PRESET_CLKGEN_SEL_MASK)) {
> - clk = SDHCI_PROG_CLOCK_MODE;
> + *clk_reg = SDHCI_PROG_CLOCK_MODE;
> real_div = div + 1;
> clk_mul = host->clk_mul;
> } else {
> @@ -1142,7 +1132,7 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> * Set Programmable Clock Mode in the Clock
> * Control register.
> */
> - clk = SDHCI_PROG_CLOCK_MODE;
> + *clk_reg = SDHCI_PROG_CLOCK_MODE;
> real_div = div;
> clk_mul = host->clk_mul;
> div--;
> @@ -1185,9 +1175,28 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> clock_set:
> if (real_div)
> host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
> - clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
> - clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
> + *clk_reg |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
> + *clk_reg |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
> << SDHCI_DIVIDER_HI_SHIFT;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
> +
> +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> + u16 clk = 0;
> + unsigned long timeout;
> +
> + host->mmc->actual_clock = 0;
> +
> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> + if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> + mdelay(1);
> +
> + if (clock == 0)
> + return;
> +
> + sdhci_compute_clock_config(host, clock, &clk);
> +
> clk |= SDHCI_CLOCK_INT_EN;
> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0f39f4f..4215eb7 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
> }
>
> +void sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock,
> + u16 *clk_reg);
> void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> unsigned short vdd);
> --
> 2.5.0
>
Kind regards
Uffe