On Fri, 2015-05-22 at 14:51 +0200, Ulf Hansson wrote:

[...]

quoted

quoted

You are invoking msdc_gate_clock() and msdc_ungate_clock() in a
balanced manner, thus hclk_enabled is redundant. Please remove it.

on drv->probe(), already invoke the msdc_ungate_clock(), so, when the
runtime pm resume invoke the msdc_ungate_clock(), the hclk already
enabled.

That's why you invoke pm_runtime_set_active() during ->probe() when
deploying PM support in patch3. It's not an issue then.

OK, then I can remove the hclk_enabled and sclk_enabled.

[...]

quoted

quoted

I assume it's possible to gate the clock by updating a MSDC register
instead!? That would be prefereable since then you can leave clock
gating/ungating via the clk API, to be dealt from runtime PM. That
would also make "sclk_enabled" in the struct msdc_host redundant.

Adopting to above, obviously requires MSDC to be able to ungate the
clock by also updating a MSDC register. I assume that's possible as
well!?

We can set the bit1 of MSDC_CFG, when this bit is 0, the bus clock was
gated to 0 if no command or data is transmitted.
And, from our designer, when we operate the MSDC register, we need make
sure both HCLK and source are enabled, if source clock was disabled,
write some MSDC registers will have no effect(eg. send CMD, without
source clock, not only cannot send CMD, but also cannot get CMD timeout
interrupt.)

Thanks, that answered my question. As I understand it you should be
able to adopt to my propsual.

[...]

quoted

quoted

quoted

+{
+       unsigned long tmo = jiffies + msecs_to_jiffies(20);
+
+       while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
+                       && time_before(jiffies, tmo))
+               continue;
+
+       if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
+               dev_err(host->dev, "CMD bus busy detected\n");
+               host->error |= REQ_CMD_BUSY;
+               msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
+               return false;
+       }
+
+       if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
+               /* R1B or with data, should check SDCBUSY */
+               while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
+                       cpu_relax();
+       }

MSDC seems to be handling card busy detection in HW, right?

Do not have this ability, HW only know if CMD/DAT is low, but do not
have any interrupt for it,

I see, but doesn't the above polling mean that msdc will not propagate
the response until the card have stopped signal busy? That's what
MMC_CAP_WAIT_WHILE_BUSY shall be used for.

As you see, we only check the "busy state" BEFORE issue a R1B command or
with data command, but do not check if AFTER the request was done, that
would do not match the "MMC_CAP_WAIT_WHILE_BUSY"(eg. CMD5 to sleep)
In addition, about CMD5, I find that the suspend/resume flow of EMMC is
stranger in new kernel version, when suspend, it may issue CMD5 to enter
sleep mode, then power off MMC, but when resume, it will
re-initialization, So that why need do the redundant CMD5 in suspend ?

Perhaps you should remove the above polling, and rely on the MMC core
to poll with CMD13 instead?

before any read/write command, core will issue CMD13 to confirm card
status, here is just only do double confirm to avoid HW issue.

quoted

quoted

If so, you should enable MMC_CAP_WAIT_WHILE_BUSY and set
"max_busy_timeout" to DAT_TIMEOUT to inform the mmc core about it.

[...]

Kind regards
Uffe