Hi, On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ, > the SDIO IRQs are processed from a dedicated kernel thread. For these > cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new > SDIO IRQ. > > Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be > invoked to temporary disable the IRQs, before the kernel thread is woken up > to process it. When processing of the IRQs are completed, they are > re-enabled by the kernel thread, again via invoking the host's > ->enable_sdio_irq(). > > The observation from this, is that the execution path is being unnecessary > complex, as the host driver already knows that it needs to temporary > disable the IRQs before signaling a new one. Moreover, replacing the kernel > thread with a work/workqueue would greatly simplify the code. > > To address the above problems, let's continue to build upon the support for > MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be > processed without using the clumsy kernel thread, but it also avoids the > ping-ponging calls of the host's ->enable_sdio_irq() callback for each > processed IRQ. > > Therefore, let's add new API sdio_signal_irq(), which enables hosts to > signal/process SDIO IRQs by using a work/workqueue, rather than using the > kernel thread. > > Add also a new host callback ->ack_sdio_irq(), which the work invokes when > the SDIO IRQs are processed. This informs the host about when it can > re-enable the SDIO IRQs. Potentially, we could re-use the existing > ->enable_sdio_irq() callback for this matter, however it has turned out > that it's more convenient for hosts to get this information via a separate > callback. > > Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use this new feature, > however the feature is optional for already existing hosts suppporting > MMC_CAP2_SDIO_IRQ_NOTHREAD. > > It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and > benefit from this feature. Further changes will have to tell. Until then > the old path using the kernel thread remains possible. So one other subtle problem with the new approach is that you totally lose all of the polling logic in sdio_irq_thread(). ...so if I take your series and then comment out "cap-sdio-irq;" in the veyron "dtsi" then things stop working. Right now dw_mmc only enables SDIO Interrupts if that bit is set and relies on polling otherwise. Presumably there's not a _huge_ reason why you would need to make dw_mmc work without actual SDIO IRQ signaling, but the way the code is structured right now things will probably break for some users out there. One note is that I remember on exynos5250-snow that we needed to enable a hybrid interrupt/polling mechanism. The problem we ran into was terribly rare and it was never root caused if there was just some subtle bug or if certain versions of dw_mmc sometimes just dropped interrupts (and the patch was never upstreamed), so possibly we don't care. ...but having the polling code there as a fallback seems like it could have a benefit. -- 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
