* [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500
@ 2013-09-03 9:29 Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling Ulf Hansson
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 9:29 UTC (permalink / raw)
To: linux-arm-kernel
To be able to enter ap_sleep state in cpuidle were the APE power domain
regulator will be cut, a register save and restore mechanism needs to be
implemented. This patchset adapts to these restrictions.
Changes in v3:
- Fold in a new patch, mmc: mmci: Adapt to new pinctrl handling
- Updated patch "mmc: mmci: Use optional sleep pinctrl state" to
use the new pinctrl API.
- Added Acks from Rickard Andersson.
Ulf Hansson (4):
mmc: mmci: Adapt to new pinctrl handling
mmc: mmci: Use optional sleep pinctrl state
mmc: mmci: Adapt to register write restrictions
mmc: mmci: Save and restore register context
drivers/mmc/host/mmci.c | 78 ++++++++++++++++++++++++++++++++++++-----------
drivers/mmc/host/mmci.h | 4 ---
2 files changed, 61 insertions(+), 21 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling
2013-09-03 9:29 [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500 Ulf Hansson
@ 2013-09-03 9:29 ` Ulf Hansson
2013-09-04 5:42 ` Linus Walleij
2013-09-03 9:29 ` [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state Ulf Hansson
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 9:29 UTC (permalink / raw)
To: linux-arm-kernel
There is no need for every driver to fetch a pinctrl handle and to
select the default state. Instead this is handled by the device driver
core, thus we can remove this piece of code from mmci.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/host/mmci.c | 17 -----------------
drivers/mmc/host/mmci.h | 4 ----
2 files changed, 21 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c3785ed..5dffbbd 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1510,23 +1510,6 @@ static int mmci_probe(struct amba_device *dev,
mmc->f_max = min(host->mclk, fmax);
dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
- host->pinctrl = devm_pinctrl_get(&dev->dev);
- if (IS_ERR(host->pinctrl)) {
- ret = PTR_ERR(host->pinctrl);
- goto clk_disable;
- }
-
- host->pins_default = pinctrl_lookup_state(host->pinctrl,
- PINCTRL_STATE_DEFAULT);
-
- /* enable pins to be muxed in and configured */
- if (!IS_ERR(host->pins_default)) {
- ret = pinctrl_select_state(host->pinctrl, host->pins_default);
- if (ret)
- dev_warn(&dev->dev, "could not set default pins\n");
- } else
- dev_warn(&dev->dev, "could not get default pinstate\n");
-
/* Get regulators and the supported OCR mask */
mmc_regulator_get_supply(mmc);
if (!mmc->ocr_avail)
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 69080fa..168bc72 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -200,10 +200,6 @@ struct mmci_host {
struct sg_mapping_iter sg_miter;
unsigned int size;
- /* pinctrl handles */
- struct pinctrl *pinctrl;
- struct pinctrl_state *pins_default;
-
#ifdef CONFIG_DMA_ENGINE
/* DMA stuff */
struct dma_chan *dma_current;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state
2013-09-03 9:29 [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500 Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling Ulf Hansson
@ 2013-09-03 9:29 ` Ulf Hansson
2013-09-04 5:43 ` Linus Walleij
2013-09-03 9:29 ` [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 4/4] mmc: mmci: Save and restore register context Ulf Hansson
3 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 9:29 UTC (permalink / raw)
To: linux-arm-kernel
By optionally putting the pins into sleep state in the .runtime_suspend
callback we can accomplish two things. One is to minimize current leakage
from pins and thus save power, second we can prevent the IP from driving
pins output in an uncontrolled manner, which may happen if the power domain
drops the domain regulator.
When returning from idle, entering .runtime_resume callback, the pins
are restored to default state.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/host/mmci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 5dffbbd..c550b3e 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1750,6 +1750,7 @@ static int mmci_runtime_suspend(struct device *dev)
if (mmc) {
struct mmci_host *host = mmc_priv(mmc);
+ pinctrl_pm_select_sleep_state(dev);
clk_disable_unprepare(host->clk);
}
@@ -1764,6 +1765,7 @@ static int mmci_runtime_resume(struct device *dev)
if (mmc) {
struct mmci_host *host = mmc_priv(mmc);
clk_prepare_enable(host->clk);
+ pinctrl_pm_select_default_state(dev);
}
return 0;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions
2013-09-03 9:29 [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500 Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state Ulf Hansson
@ 2013-09-03 9:29 ` Ulf Hansson
2013-09-03 9:50 ` Daniel Lezcano
2013-09-03 9:29 ` [PATCH V3 4/4] mmc: mmci: Save and restore register context Ulf Hansson
3 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 9:29 UTC (permalink / raw)
To: linux-arm-kernel
After a write to the MMCICLOCK register data cannot be written to this
register for three feedback clock cycles. Writes to the MMCIPOWER
register must be separated by three MCLK cycles. Previously no issues
has been observered, but using higher ARM clock frequencies on STE-
platforms has triggered this problem.
The MMCICLOCK register is written to in .set_ios and for some data
transmissions for SDIO. We do not need a delay at the data transmission
path, because sending and receiving data will require more than three
clock cycles. Then we use a simple logic to only delay in .set_ios and
thus we don't affect throughput performance.
Signed-off-by: Johan Rudholm <jrudholm@gmail.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
---
drivers/mmc/host/mmci.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c550b3e..82afcd3 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -189,6 +189,21 @@ static int mmci_validate_data(struct mmci_host *host,
return 0;
}
+static void mmci_reg_delay(struct mmci_host *host)
+{
+ /*
+ * According to the spec, at least three feedback clock cycles
+ * of max 52 MHz must pass between two writes to the MMCICLOCK reg.
+ * Three MCLK clock cycles must pass between two MMCIPOWER reg writes.
+ * Worst delay time during card init is at 100 kHz => 30 us.
+ * Worst delay time when up and running is at 25 MHz => 120 ns.
+ */
+ if (host->cclk < 20000000)
+ udelay(30);
+ else
+ ndelay(120);
+}
+
/*
* This must be called with host->lock held
*/
@@ -1264,6 +1279,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
mmci_set_clkreg(host, ios->clock);
mmci_write_pwrreg(host, pwr);
+ mmci_reg_delay(host);
spin_unlock_irqrestore(&host->lock, flags);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V3 4/4] mmc: mmci: Save and restore register context
2013-09-03 9:29 [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500 Ulf Hansson
` (2 preceding siblings ...)
2013-09-03 9:29 ` [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions Ulf Hansson
@ 2013-09-03 9:29 ` Ulf Hansson
2013-09-03 9:53 ` Daniel Lezcano
3 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 9:29 UTC (permalink / raw)
To: linux-arm-kernel
If a corresponding power domain exists for the device and it manages
to cut the domain regulator while the device is runtime suspended,
the IP loses it's registers context. We restore the context in the
.runtime_resume callback from the existing register caches to adapt
to this siutuation.
We also want to make sure the registers are in a known state while
restoring context in the case when the power domain did not drop the
power, since there are restrictions for the order of writing to these
registers. To handle this, we clear the registers in the
.runtime_suspend callback.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/mmc/host/mmci.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 82afcd3..97e541b 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -62,6 +62,7 @@ static unsigned int fmax = 515633;
* @signal_direction: input/out direction of bus signals can be indicated
* @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
* @busy_detect: true if busy detection on dat0 is supported
+ * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
*/
struct variant_data {
unsigned int clkreg;
@@ -76,6 +77,7 @@ struct variant_data {
bool signal_direction;
bool pwrreg_clkgate;
bool busy_detect;
+ bool pwrreg_nopower;
};
static struct variant_data variant_arm = {
@@ -109,6 +111,7 @@ static struct variant_data variant_u300 = {
.pwrreg_powerup = MCI_PWR_ON,
.signal_direction = true,
.pwrreg_clkgate = true,
+ .pwrreg_nopower = true,
};
static struct variant_data variant_nomadik = {
@@ -121,6 +124,7 @@ static struct variant_data variant_nomadik = {
.pwrreg_powerup = MCI_PWR_ON,
.signal_direction = true,
.pwrreg_clkgate = true,
+ .pwrreg_nopower = true,
};
static struct variant_data variant_ux500 = {
@@ -135,6 +139,7 @@ static struct variant_data variant_ux500 = {
.signal_direction = true,
.pwrreg_clkgate = true,
.busy_detect = true,
+ .pwrreg_nopower = true,
};
static struct variant_data variant_ux500v2 = {
@@ -150,6 +155,7 @@ static struct variant_data variant_ux500v2 = {
.signal_direction = true,
.pwrreg_clkgate = true,
.busy_detect = true,
+ .pwrreg_nopower = true,
};
static int mmci_card_busy(struct mmc_host *mmc)
@@ -1759,6 +1765,41 @@ static int mmci_resume(struct device *dev)
#endif
#ifdef CONFIG_PM_RUNTIME
+static void mmci_save(struct mmci_host *host)
+{
+ unsigned long flags;
+
+ if (host->variant->pwrreg_nopower) {
+ spin_lock_irqsave(&host->lock, flags);
+
+ writel(0, host->base + MMCIMASK0);
+ writel(0, host->base + MMCIDATACTRL);
+ writel(0, host->base + MMCIPOWER);
+ writel(0, host->base + MMCICLOCK);
+ mmci_reg_delay(host);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+ }
+
+}
+
+static void mmci_restore(struct mmci_host *host)
+{
+ unsigned long flags;
+
+ if (host->variant->pwrreg_nopower) {
+ spin_lock_irqsave(&host->lock, flags);
+
+ writel(host->clk_reg, host->base + MMCICLOCK);
+ writel(host->datactrl_reg, host->base + MMCIDATACTRL);
+ writel(host->pwr_reg, host->base + MMCIPOWER);
+ writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+ mmci_reg_delay(host);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+ }
+}
+
static int mmci_runtime_suspend(struct device *dev)
{
struct amba_device *adev = to_amba_device(dev);
@@ -1767,6 +1808,7 @@ static int mmci_runtime_suspend(struct device *dev)
if (mmc) {
struct mmci_host *host = mmc_priv(mmc);
pinctrl_pm_select_sleep_state(dev);
+ mmci_save(host);
clk_disable_unprepare(host->clk);
}
@@ -1781,6 +1823,7 @@ static int mmci_runtime_resume(struct device *dev)
if (mmc) {
struct mmci_host *host = mmc_priv(mmc);
clk_prepare_enable(host->clk);
+ mmci_restore(host);
pinctrl_pm_select_default_state(dev);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions
2013-09-03 9:29 ` [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions Ulf Hansson
@ 2013-09-03 9:50 ` Daniel Lezcano
2013-09-03 11:56 ` Ulf Hansson
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2013-09-03 9:50 UTC (permalink / raw)
To: linux-arm-kernel
On 09/03/2013 11:29 AM, Ulf Hansson wrote:
> After a write to the MMCICLOCK register data cannot be written to this
> register for three feedback clock cycles. Writes to the MMCIPOWER
> register must be separated by three MCLK cycles. Previously no issues
> has been observered, but using higher ARM clock frequencies on STE-
> platforms has triggered this problem.
>
> The MMCICLOCK register is written to in .set_ios and for some data
> transmissions for SDIO. We do not need a delay at the data transmission
> path, because sending and receiving data will require more than three
> clock cycles. Then we use a simple logic to only delay in .set_ios and
> thus we don't affect throughput performance.
>
> Signed-off-by: Johan Rudholm <jrudholm@gmail.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
> ---
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
+ two questions below.
> drivers/mmc/host/mmci.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index c550b3e..82afcd3 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -189,6 +189,21 @@ static int mmci_validate_data(struct mmci_host *host,
> return 0;
> }
>
> +static void mmci_reg_delay(struct mmci_host *host)
> +{
> + /*
> + * According to the spec, at least three feedback clock cycles
> + * of max 52 MHz must pass between two writes to the MMCICLOCK reg.
> + * Three MCLK clock cycles must pass between two MMCIPOWER reg writes.
> + * Worst delay time during card init is at 100 kHz => 30 us.
> + * Worst delay time when up and running is at 25 MHz => 120 ns.
> + */
> + if (host->cclk < 20000000)
Shouldn't it be 25000000 ?
What about using macros ?
#define MMC_CLOCK 25000000
#define MMCINIT_DELAY 30
#define MMCUP_DELAY 120
> + udelay(30);
> + else
> + ndelay(120);
> +}
> +
> /*
> * This must be called with host->lock held
> */
> @@ -1264,6 +1279,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> mmci_set_clkreg(host, ios->clock);
> mmci_write_pwrreg(host, pwr);
> + mmci_reg_delay(host);
>
> spin_unlock_irqrestore(&host->lock, flags);
>
>
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 4/4] mmc: mmci: Save and restore register context
2013-09-03 9:29 ` [PATCH V3 4/4] mmc: mmci: Save and restore register context Ulf Hansson
@ 2013-09-03 9:53 ` Daniel Lezcano
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Lezcano @ 2013-09-03 9:53 UTC (permalink / raw)
To: linux-arm-kernel
On 09/03/2013 11:29 AM, Ulf Hansson wrote:
> If a corresponding power domain exists for the device and it manages
> to cut the domain regulator while the device is runtime suspended,
> the IP loses it's registers context. We restore the context in the
> .runtime_resume callback from the existing register caches to adapt
> to this siutuation.
>
> We also want to make sure the registers are in a known state while
> restoring context in the case when the power domain did not drop the
> power, since there are restrictions for the order of writing to these
> registers. To handle this, we clear the registers in the
> .runtime_suspend callback.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> drivers/mmc/host/mmci.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 82afcd3..97e541b 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -62,6 +62,7 @@ static unsigned int fmax = 515633;
> * @signal_direction: input/out direction of bus signals can be indicated
> * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
> * @busy_detect: true if busy detection on dat0 is supported
> + * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
> */
> struct variant_data {
> unsigned int clkreg;
> @@ -76,6 +77,7 @@ struct variant_data {
> bool signal_direction;
> bool pwrreg_clkgate;
> bool busy_detect;
> + bool pwrreg_nopower;
> };
>
> static struct variant_data variant_arm = {
> @@ -109,6 +111,7 @@ static struct variant_data variant_u300 = {
> .pwrreg_powerup = MCI_PWR_ON,
> .signal_direction = true,
> .pwrreg_clkgate = true,
> + .pwrreg_nopower = true,
> };
>
> static struct variant_data variant_nomadik = {
> @@ -121,6 +124,7 @@ static struct variant_data variant_nomadik = {
> .pwrreg_powerup = MCI_PWR_ON,
> .signal_direction = true,
> .pwrreg_clkgate = true,
> + .pwrreg_nopower = true,
> };
>
> static struct variant_data variant_ux500 = {
> @@ -135,6 +139,7 @@ static struct variant_data variant_ux500 = {
> .signal_direction = true,
> .pwrreg_clkgate = true,
> .busy_detect = true,
> + .pwrreg_nopower = true,
> };
>
> static struct variant_data variant_ux500v2 = {
> @@ -150,6 +155,7 @@ static struct variant_data variant_ux500v2 = {
> .signal_direction = true,
> .pwrreg_clkgate = true,
> .busy_detect = true,
> + .pwrreg_nopower = true,
> };
>
> static int mmci_card_busy(struct mmc_host *mmc)
> @@ -1759,6 +1765,41 @@ static int mmci_resume(struct device *dev)
> #endif
>
> #ifdef CONFIG_PM_RUNTIME
> +static void mmci_save(struct mmci_host *host)
> +{
> + unsigned long flags;
> +
> + if (host->variant->pwrreg_nopower) {
> + spin_lock_irqsave(&host->lock, flags);
> +
> + writel(0, host->base + MMCIMASK0);
> + writel(0, host->base + MMCIDATACTRL);
> + writel(0, host->base + MMCIPOWER);
> + writel(0, host->base + MMCICLOCK);
> + mmci_reg_delay(host);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> + }
> +
> +}
> +
> +static void mmci_restore(struct mmci_host *host)
> +{
> + unsigned long flags;
> +
> + if (host->variant->pwrreg_nopower) {
> + spin_lock_irqsave(&host->lock, flags);
> +
> + writel(host->clk_reg, host->base + MMCICLOCK);
> + writel(host->datactrl_reg, host->base + MMCIDATACTRL);
> + writel(host->pwr_reg, host->base + MMCIPOWER);
> + writel(MCI_IRQENABLE, host->base + MMCIMASK0);
> + mmci_reg_delay(host);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> + }
> +}
> +
> static int mmci_runtime_suspend(struct device *dev)
> {
> struct amba_device *adev = to_amba_device(dev);
> @@ -1767,6 +1808,7 @@ static int mmci_runtime_suspend(struct device *dev)
> if (mmc) {
> struct mmci_host *host = mmc_priv(mmc);
> pinctrl_pm_select_sleep_state(dev);
> + mmci_save(host);
> clk_disable_unprepare(host->clk);
> }
>
> @@ -1781,6 +1823,7 @@ static int mmci_runtime_resume(struct device *dev)
> if (mmc) {
> struct mmci_host *host = mmc_priv(mmc);
> clk_prepare_enable(host->clk);
> + mmci_restore(host);
> pinctrl_pm_select_default_state(dev);
> }
>
>
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions
2013-09-03 9:50 ` Daniel Lezcano
@ 2013-09-03 11:56 ` Ulf Hansson
0 siblings, 0 replies; 10+ messages in thread
From: Ulf Hansson @ 2013-09-03 11:56 UTC (permalink / raw)
To: linux-arm-kernel
On 3 September 2013 11:50, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 09/03/2013 11:29 AM, Ulf Hansson wrote:
>> After a write to the MMCICLOCK register data cannot be written to this
>> register for three feedback clock cycles. Writes to the MMCIPOWER
>> register must be separated by three MCLK cycles. Previously no issues
>> has been observered, but using higher ARM clock frequencies on STE-
>> platforms has triggered this problem.
>>
>> The MMCICLOCK register is written to in .set_ios and for some data
>> transmissions for SDIO. We do not need a delay at the data transmission
>> path, because sending and receiving data will require more than three
>> clock cycles. Then we use a simple logic to only delay in .set_ios and
>> thus we don't affect throughput performance.
>>
>> Signed-off-by: Johan Rudholm <jrudholm@gmail.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
>> ---
>
> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> + two questions below.
>
>> drivers/mmc/host/mmci.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index c550b3e..82afcd3 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -189,6 +189,21 @@ static int mmci_validate_data(struct mmci_host *host,
>> return 0;
>> }
>>
>> +static void mmci_reg_delay(struct mmci_host *host)
>> +{
>> + /*
>> + * According to the spec, at least three feedback clock cycles
>> + * of max 52 MHz must pass between two writes to the MMCICLOCK reg.
>> + * Three MCLK clock cycles must pass between two MMCIPOWER reg writes.
>> + * Worst delay time during card init is at 100 kHz => 30 us.
>> + * Worst delay time when up and running is at 25 MHz => 120 ns.
>> + */
>> + if (host->cclk < 20000000)
>
> Shouldn't it be 25000000 ?
You are right! Thanks for spotting this! I resend a new version.
>
> What about using macros ?
>
> #define MMC_CLOCK 25000000
> #define MMCINIT_DELAY 30
> #define MMCUP_DELAY 120
These values wont be used anywhere else but here. Additionally, mmci
is already having other hard-coded values around. Unless you feel it
is important too you, I would prefer keeping this as is.
Thanks for review Daniel!
Kind regards
Ulf Hansson
>
>> + udelay(30);
>> + else
>> + ndelay(120);
>> +}
>> +
>> /*
>> * This must be called with host->lock held
>> */
>> @@ -1264,6 +1279,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>
>> mmci_set_clkreg(host, ios->clock);
>> mmci_write_pwrreg(host, pwr);
>> + mmci_reg_delay(host);
>>
>> spin_unlock_irqrestore(&host->lock, flags);
>>
>>
>
>
> --
> <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling
2013-09-03 9:29 ` [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling Ulf Hansson
@ 2013-09-04 5:42 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2013-09-04 5:42 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 3, 2013 at 11:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> There is no need for every driver to fetch a pinctrl handle and to
> select the default state. Instead this is handled by the device driver
> core, thus we can remove this piece of code from mmci.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state
2013-09-03 9:29 ` [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state Ulf Hansson
@ 2013-09-04 5:43 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2013-09-04 5:43 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 3, 2013 at 11:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> By optionally putting the pins into sleep state in the .runtime_suspend
> callback we can accomplish two things. One is to minimize current leakage
> from pins and thus save power, second we can prevent the IP from driving
> pins output in an uncontrolled manner, which may happen if the power domain
> drops the domain regulator.
>
> When returning from idle, entering .runtime_resume callback, the pins
> are restored to default state.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Rickard Andersson <rickard.andersson@stericsson.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-09-04 5:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 9:29 [PATCH V3 0/4] mmc: mmci: Support ap_sleep in cpuidle for ux500 Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 1/4] mmc: mmci: Adapt to new pinctrl handling Ulf Hansson
2013-09-04 5:42 ` Linus Walleij
2013-09-03 9:29 ` [PATCH V3 2/4] mmc: mmci: Use optional sleep pinctrl state Ulf Hansson
2013-09-04 5:43 ` Linus Walleij
2013-09-03 9:29 ` [PATCH V3 3/4] mmc: mmci: Adapt to register write restrictions Ulf Hansson
2013-09-03 9:50 ` Daniel Lezcano
2013-09-03 11:56 ` Ulf Hansson
2013-09-03 9:29 ` [PATCH V3 4/4] mmc: mmci: Save and restore register context Ulf Hansson
2013-09-03 9:53 ` Daniel Lezcano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).