On Wed, 20 Oct 2021 at 18:01, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Oct 20, 2021 at 3:29 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >
> > For dw_mmc, the ->init_card() callback is being used to turn on/off
> > automatic internal clock gating for powersave, which is needed to properly
> > support SDIO irqs on DAT1.
> >
> > However, using the ->init_card() comes with a drawback in this case, as it
> > means that the powersave feature becomes disabled, no matter whether the
> > SDIO irqs becomes turned on or not. To improve the behaviour, let's change
> > into using the ->enable_sdio_irq() callback instead. This works fine,
> > because dw_mmc uses sdio_signal_irq() to signal the irqs, thus the
> > ->enable_sdio_irq() is never executed from within atomic context.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > ---
> > drivers/mmc/host/dw_mmc.c | 39 +++++++++++++++++----------------------
> > 1 file changed, 17 insertions(+), 22 deletions(-)
>
> So it was a really long time ago now, but I swear that I put it in
> init_card() for a reason. Sure enough, commit b24c8b260189 ("mmc:
> dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts") talks
> about this. Your patch is largely a revert of that one. Looking at
> that commit plus commit f8c58c113634 ("mmc: dw_mmc: Protect
> read-modify-write of INTMASK with a lock") makes me wonder whether
> it's indeed safe to do all the modifications that you're doing in
> dw_mci_enable_sdio_irq().
>
> I think that back in the day dw_mci_enable_sdio_irq() could be called
> in multiple places: directly as a result of the interrupt handler and
> also by other code that wanted the interrupt enabled.
>
> Oh, I think I see. Commit 32dba73772f8 ("mmc: dw_mmc: Convert to use
> MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs") is the key? After that
> commit then it makes sense. The place you've added the code is a place
> that is _not_ called from the interrupt handler.
>
> OK, so this looks right to me then. Feel free to add:
>
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
Thanks a lot for reviewing!
>
>
> I also wouldn't mind if you added some of the research above to the
> commit message
The commit message states:
"This works fine, because dw_mmc uses sdio_signal_irq() to signal the
irqs, thus the ->enable_sdio_irq() is never executed from within
atomic context."
But I can throw in some more background and refer to the commits you
pointed out above.
[...]
Kind regards
Uffe
