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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 26 Aug 2013 09:05:48 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Jaehoon Chung <jh80.chung@...sung.com>
Cc:	Seungwon Jeon <tgih.jun@...sung.com>, Chris Ball <cjb@...top.org>,
	James Hogan <james.hogan@...tec.com>,
	Grant Grundler <grundler@...omium.org>,
	Alim Akhtar <alim.akhtar@...sung.com>,
	Abhilash Kesavan <a.kesavan@...sung.com>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Olof Johansson <olof@...om.net>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0
 (turn off clock)

Jaehoon / Seungwon,

On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <jh80.chung@...sung.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@...omium.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,
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.

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/