On Wed, Mar 9, 2011 at 4:00 PM, Grant Grundler <grundler@xxxxxxxxxx> wrote: > >> 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. > Ok, let me rephrase my original statement. I think I might see. SDIO interrupts are received as Card Interrupts on the SDHCI side. MMC_CAP_SDIO_IRQ is set by sdhci.c if SDHCI_QUIRK_NO_SDIO_IRQ is not present. MMC_CAP_SDIO_IRQ means that ops->enable_sdio_irq will be invoked by code in sdio_irq.c. enable_sdio_irq in the sdhci case enables SDHCI_INT_CARD_INT. When you get an SDHCI interrupt with SDHCI_INT_CARD_INT as a reason, the SDIO irq handling thread is woken up. That thread processes SDIO IRQs. If the host doesn't have SDHCI_QUIRK_NO_SDIO_IRQ, the thread will sleep for MAX_SCHEDULE_TIMEOUT. Otherwise it will periodcally awake to scan for pending IRQs. If you have SDHCI_QUIRK_NO_SDIO_IRQ, then you have periodic scanning for SDIO IRQs for each card. Otherwise, you need to actually get an interrupt from SDHCI. If you suspend+resume, and SDHCI_INT_CARD_INT is lost, then you just lost your SDIO interrupts forever. > 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? It just means that your SDHCI controller doesn't support SDIO interrupts and allows you to use SDIO cards in slots which technically don't support SDIO cards. I believe. It means you need to poll for card IRQs. > > >> 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? But you don't have it set? If you have it set, then your patch shouldn't be necessary? It's necessary for cases where you are expecting SDHCI host to signal you on a card interrupt. > >> 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; > I'd verify that the card interrupt is really the reason (I believe it is as far as I can tell), but this is probably self documenting enough. > Does kernel.org still need something to deal with state of > SDHCI_QUIRK_NO_SDIO_IRQ quirk bit? I'm confused. I see this quirk added as part of Tegra sdhci support in our K36 tree. I suppose with this patch the should be no need for this quirk. I'll apply your change today and see what I get. Thanks, A -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
