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>
I also wouldn't mind if you added some of the research above to the
commit message
> + if (clk_en_a != clk_en_a_old) {
> + mci_writel(host, CLKENA, clk_en_a);
> + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT,
> + 0);
nit: that 0 looks lonely on its own line now. :(
