[PATCH] mmc: dw_mmc: Drop use of ->init_card() callback

[PATCH] mmc: dw_mmc: Drop use of ->init_card() callback

[Ulf Hansson]

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@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1e8f1bb3cad7..d977f34f6b55 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1611,37 +1611,32 @@ static void dw_mci_hw_reset(struct mmc_host *mmc)
 	usleep_range(200, 300);
 }
 
-static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
+static void dw_mci_prepare_sdio_irq(struct dw_mci_slot *slot, bool prepare)
 {
-	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
+	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+	u32 clk_en_a_old;
+	u32 clk_en_a;
 
 	/*
 	 * Low power mode will stop the card clock when idle.  According to the
 	 * description of the CLKENA register we should disable low power mode
 	 * for SDIO cards if we need SDIO interrupts to work.
 	 */
-	if (mmc->caps & MMC_CAP_SDIO_IRQ) {
-		const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
-		u32 clk_en_a_old;
-		u32 clk_en_a;
 
-		clk_en_a_old = mci_readl(host, CLKENA);
-
-		if (card->type == MMC_TYPE_SDIO ||
-		    card->type == MMC_TYPE_SD_COMBO) {
-			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			clk_en_a = clk_en_a_old & ~clken_low_pwr;
-		} else {
-			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			clk_en_a = clk_en_a_old | clken_low_pwr;
-		}
+	clk_en_a_old = mci_readl(host, CLKENA);
+	if (prepare) {
+		set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+		clk_en_a = clk_en_a_old & ~clken_low_pwr;
+	} else {
+		clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+		clk_en_a = clk_en_a_old | clken_low_pwr;
+	}
 
-		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);
-		}
+	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);
 	}
 }
 
@@ -1669,6 +1664,7 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 
+	dw_mci_prepare_sdio_irq(slot, enb);
 	__dw_mci_enable_sdio_irq(slot, enb);
 
 	/* Avoid runtime suspending the device when SDIO IRQ is enabled */
@@ -1790,7 +1786,6 @@ static const struct mmc_host_ops dw_mci_ops = {
 	.execute_tuning		= dw_mci_execute_tuning,
 	.card_busy		= dw_mci_card_busy,
 	.start_signal_voltage_switch = dw_mci_switch_voltage,
-	.init_card		= dw_mci_init_card,
 	.prepare_hs400_tuning	= dw_mci_prepare_hs400_tuning,
 };
 
-- 
2.25.1

Re: [PATCH] mmc: dw_mmc: Drop use of ->init_card() callback

[Doug Anderson]

Hi,

On Wed, Oct 20, 2021 at 3:29 AM Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
> ---
>  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@chromium.org>


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. :(

Re: [PATCH] mmc: dw_mmc: Drop use of ->init_card() callback

[Ulf Hansson]

On Wed, 20 Oct 2021 at 18:01, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Oct 20, 2021 at 3:29 AM Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
> > ---
> >  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@chromium.org>

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

Re: [PATCH] mmc: dw_mmc: Drop use of ->init_card() callback

[Doug Anderson]

Hi,

On Thu, Oct 21, 2021 at 12:52 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> > 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.

Ah, you are correct. I must have skimmed that over too fast. I think
it's fine. Thanks! :-)

-Doug