Hi Laurent,

On Fri, Jun 15, 2012 at 4:12 AM, Laurent Pinchart
[off-list ref] wrote:

Hi Magnus,

On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:

quoted

On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:

quoted

The tmio_mmc_set_ios() function configures the MMC power, clock and bus
width. When the MMC controller gets powered off, runtime PM switches the
MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
fails and prints an error message to the kernel log.

As configuring the bus width is pointless when the interface gets
powered down, skip the operation when power is off.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

First of all, thanks for reporting this issue and coming up with a fix!

You're welcome. You can expect more of them ;-)

quoted

Can you please explain a bit more about when this triggers? Is this
related to suspend-to-ram perhaps? Which hardware platform? Is
CONFIG_PM_RUNTIME=y set?

I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y. The
driver spits out "timeout waiting for SD bus idle" error messages more or less
continuously.

I see, thanks for explaining.

quoted

I suspect that this may be a side effect of the current PM code used
on the A1 SoC (which is hooked up on the armadillo board).

In short, the A1 SoC does not yet make use of PM domains, but AP4 and
the mackerel board (sh7372 based) is using PM domains.

Does this mean that runtime PM is a no-op on A1 ? That would surprise me, as
turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM is
somehow involved. Even if the power domain does not get turned off, can't the
MSTP clock be turned off ?

The behavior may be different depending on how the pm domains are tied in.

quoted

This difference may result in different state of clocks at suspend-to-ram
timing. So the SDHI driver may work just fine on the mackerel board, but not
on the armadillo board. If that's the case then perhaps we shouldn't fix the
driver, but instead look at the platform level.

I still believe there's a bug in the driver, please see below.

quoted

quoted

diff --git a/drivers/mmc/host/tmio_mmc_pio.c

b/drivers/mmc/host/tmio_mmc_pio.c index 9a7996a..de1226d 100644

--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c

@@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc,

struct mmc_ios *ios) tmio_mmc_clk_stop(host);
       }

-       switch (ios->bus_width) {
-       case MMC_BUS_WIDTH_1:
-               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
-       break;
-       case MMC_BUS_WIDTH_4:
-               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
-       break;
+       if (host->power) {
+               switch (ios->bus_width) {
+               case MMC_BUS_WIDTH_1:
+                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
0x80e0);
+               break;
+               case MMC_BUS_WIDTH_4:
+                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
0x00e0);
+               break;
+               }
       }

       /* Let things settle. delay taken from winCE driver */
--
1.7.3.4

Is it possible that you get here through tmio_mmc_host_suspend() and
mmc_suspend_host()?

Just guessing. =)

I haven't checked the complete call stack, but I don't think it matters that
much.

I suppose that depends on what you are trying to fix. =)

What happens here is that the driver tried to write to a 16-bit hardware
register after calling pm_runtime_put(). At that point runtime PM may have
turned to power domain and/or the MSTP clock off for the device. Writing to a
16-bit register first polls for the bus idle bit in the CTL_STATUS2 to be set.
This never happens in this case (probably because the clock has been turned
off), so the write operation fails.

Speaking generally, I think we should avoid writing to the device after
calling pm_runtime_put(), it just doesn't make much sense. If we release the
device from a PM point of view, it means we don't need to touch it anymore, so
we should prevent writes until the next pm_runtime_get_sync() call.

I totally agree with you on that. I'm just surprised that it triggers
at this point.

Guennadi, does this trigger on sh7372 as well? If not, why is that?

Thanks for your help!

/ magnus