RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

[Seungwon Jeon <tgih.jun@samsung.com>]

On Tuesday, August 27, 2013, Doug Anderson wrote:
> Jaehoon / Seungwon,
> 
> On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> > On 08/26/2013 01:34 PM, Seungwon Jeon wrote:
> >> On Fri, August 23, 2013, Doug Anderson wrote:
> >>> Previously the dw_mmc driver would ignore any requests to disable the
> >>> card's clock.  This doesn't seem like a good thing in general, but had
> >>> one extra bad side effect in the following situtation:
> >>> * mmc core would set clk to 400kHz at boot time while initting
> >>> * mmc core would set clk to 0 since no card, but it would be ignored.
> >>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
> >>>   but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
> >>> * insert card
> >>> * mmc core would set clk to 400kHz which would be considered a no-op.
> >>>
> >>> Note that if there is no card in the slot and we do a suspend/resume
> >>> cycle, we _do_ still end up with differences in a dw_mmc register
> >>> dump, but the differences are clock related and we've got the clock
> >>> disabled both before and after, so this should be OK.
> >>>
> >>> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >>> ---
> >>> Changes in v6:
> >>> - Replaces previous pathes that ensured saving/restoring clocks.
> >>>
> >>>  drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
> >>>  1 file changed, 11 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >>> index ee5f167..f6c7545 100644
> >>> --- a/drivers/mmc/host/dw_mmc.c
> >>> +++ b/drivers/mmc/host/dw_mmc.c
> >>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> >>>      u32 div;
> >>>      u32 clk_en_a;
> >>>
> >>> -    if (slot->clock != host->current_speed || force_clkinit) {
> >>> +    if (slot->clock == 0) {
> >>> +            mci_writel(host, CLKENA, 0);
> >>> +            mci_send_cmd(slot,
> >>> +                         SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> >> Basically dw_mmc driver uses host's low power mode(auto clock gating)
> >> So, how about keeping origin code rather than programming clock setting to '0'?
> > Right, Dw-mmc controller can use the Low-power mode.
> > This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0.
> 
> Ah, right.  ...so it's unlikely that we'd save any power because we're
> already gating the clock.
> 
> I'd really still rather honor the MMC subsystem's request.  It
> shouldn't _hurt_ to turn the clock off when the subsystem requests it,
Even though turning off by clock programming doesn't hurt, 
it is costly behavior when considering low power mode of host's own support.
Just now, how about focusing on the problem clock isn't updated properly after suspend/resume?

> right?  One reason to honor the mmc core is that it will make things
> cleaner if/when we support a voltage change operation.  The MMC core
> has the logic for the voltage change, and part of that involves
> turning off the clock.  We'll already need a bunch of special case
> code in dw_mmc for voltage change, but it would be nice to avoid one
> extra bit.
Turning off clock during voltage switching would be another procedure.
I guess it could be discussed later.
I want to fix some minor change to prevent frequent message that Jaehoon pointed.

Thanks,
Seungwon Jeon

> 
> Another option would be to add a core MMC quirk to disable MMC_CLKGATE
> on a per-driver basis.  In the "single zImage" world that seems like
> the right thing to do, since the MMC_CLKGATE description seems to
> indicate that enabling that won't really work on all drivers anyway.
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html