On Wed, Mar 9, 2011 at 1:10 PM, Andrei Warkentin <andr...@motorola.com> wrote:
> On Wed, Mar 9, 2011 at 3:01 PM, Grant Grundler <grund...@google.com> wrote:
>> On Wed, Mar 9, 2011 at 12:24 PM, Andrei Warkentin <andr...@motorola.com>
>> wrote:
>>> On Tue, Mar 8, 2011 at 3:39 PM, Grant Grundler <grund...@google.com> wrote:
>>>>
>>>> Save and restore SDHCI interrupt mask during suspend/resume.
>>>> Enables ARM Tegra2 board to suspend/resume.
>>>>
>>>> Signed-off-by: Venkat Rao <v...@broadcom.com>
>>>> Reviewed-by: Olof Johansson <ol...@chromium.org>
>>>> Reviewed-by: Grant Grundler <grund...@chromium.org>
>>>> ---
>>>> drivers/mmc/host/sdhci.c | 5 +++++
>>>> include/linux/mmc/sdhci.h | 2 ++
>>>> 2 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 3c6358f..68d2e36 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1654,6 +1654,8 @@ int sdhci_suspend_host(struct sdhci_host *host,
>>>> pm_message_t state)
>>>> if (mmc->card && (mmc->card->type != MMC_TYPE_SDIO))
>>>> ret = mmc_suspend_host(host->mmc);
>>>>
>>>> + /* Save the original intmask to restore later */
>>>> + host->save_intmask = sdhci_readl(host, SDHCI_INT_ENABLE);
>>>> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>>>
>>>> if (host->vmmc)
>>>> @@ -1695,6 +1697,9 @@ int sdhci_resume_host(struct sdhci_host *host)
>>>>
>>>> sdhci_enable_card_detection(host);
>>>>
>>>> + /* Restore the original intmask */
>>>> + sdhci_unmask_irqs(host, host->save_intmask);
>>>> +
>>>> return ret;
>>>> }
>>>>
>>>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>>>> index dfb2106..5b9f6db 100644
>>>> --- a/include/linux/mmc/sdhci.h
>>>> +++ b/include/linux/mmc/sdhci.h
>>>> @@ -141,6 +141,8 @@ struct sdhci_host {
>>>>
>>>> unsigned int caps; /* Alternative capabilities */
>>>>
>>>> + unsigned int save_intmask; /* Store original intmask */
>>>> +
>>>> unsigned long private[0] ____cacheline_aligned;
>>>> };
>>>> #endif /* __SDHCI_H */
>>>> --
>>>> 1.7.2.3
>>>>
>>>> --
>>>
>>> Hi Grant,
>>>
>>> Why is this necessary?
>>
>> Hi Andrei,
>> Sorry - I didn't CC the original author of the patch...(now CC'd) and
>> hopefully he or Sandeep can explain.
>>
>>> If I understand correctly, the sdhci resume
>>> sequence sets some base needed bits inside SDHCI_INT_ENABLE inside
>>> sdhci_clear_set_irqs. Only if you have some outstanding DMA, but
>>> surely all DMA must be completed prior to your suspending
>>> (mmc_queue_suspend ensures nothing is outstanding)?
>>
>> That's a good question. Given this is a WIFI device, my initial
>> assumption is we could receive packets or state/event changes any
>> time. But the device might in fact be quiesced during the suspend
>> operation. I have to check what the brcm80211/brcmfmac driver is doing
>> in this case.
>>
>>> I'm asking since we're on Tegra 2 and have suspend/resume working fine
>>> with eMMC.
>>
>> We do too - but only if the brcmfmac driver isn't loaded.
>>
>> I'll look up if there are any side effects of writing SDHCI_INT_ENABLE
>> as well. It's possible this is unique to having BRCM4329 connected to
>> the eMMC controller instead of a flash device.
>>
>> thanks,
>> grant
>>
>
> +Dmitry Shmidt
>
> Ah, interesting. So this is an SDIO device. We also have a BRCM4329
> :-).
Yes. :)
> Different driver, though, although I'm sure it's the same
> Broadcom DHD stuff...
It might be. I've been pulling brcm80211 from gregkh's
staging-2.6:drivers/staging tree and that's been evolving. I expect
the driver used by Android is functionally equivalent modulo any
"sleep lock" (or whatever it's called now) changes.
> I took a look (yay web archives,
> http://groups.google.com/a/chromium.org/group/chromium-os-reviews/browse_thread/thread/bfe4ece948222f96?pli=1),
> and it seems you have SDHCI_QUIRK_NO_SDIO_IRQ disabled in your
> sdhci-tegra.
Indeed - this patch was proposed shortly after Olof submitted the
change to remove SDHCI_QUIRK_NO_SDIO_IRQ. My role here is to get
patches we were handed over a month ago upstream.
> I see why above patch is necessary now.
I don't understand the relationship between the two. Can you
elaborate? Or point at docs?
I'd be happy to add comments to the code.
In a followup you wrote
The dependency bothers me since it suggests that if
SDHCI_QUIRK_NO_SDIO_IRQ flag is set, we should not be touching this
register. Is that correct?
> This is bit OT, but are the LP2 idle gains significant here?
I don't know if CPU pwer is reduced or if that was intentional or just
a side effect. Venkat or Sandeep (Broadcom) might be able to answer
that.
In a follow up, you also wrote:
> As an additional comment, I think the way this patch is made it hides
> the reason for why this is necessary.
TBH, even after this conversation, it's still not clear to me why.
The dependency bothers me since it suggests that if
SDHCI_QUIRK_NO_SDIO_IRQ flag is set, we should not be touching this
register. Is that correct?
> The patch should explicitly record if SDHCI_INT_CARD_INT is
> enabled on suspend and set it back again on resume.
Would this be better?
host->save_intmask = sdhci_readl(host, SDHCI_INT_ENABLE) &
SDHCI_INT_CARD_INT;
Does kernel.org still need something to deal with state of
SDHCI_QUIRK_NO_SDIO_IRQ quirk bit?
thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
