Re: [RESEND v12 3/3] mmc: mediatek: add support for SDIO eint wakup IRQ

[Ulf Hansson]

On Wed, 25 May 2022 at 03:51, Axe Yang <axe.yang@xxxxxxxxxxxx> wrote:
>
> Add support for eint IRQ when MSDC is used as an SDIO host. This
> feature requires SDIO device support async IRQ function. With this
> feature, SDIO host can be awakened by SDIO card in suspend state,
> without additional pin.
>
> MSDC driver will time-share the SDIO DAT1 pin. During suspend, MSDC
> turn off clock and switch SDIO DAT1 pin to GPIO mode. And during
> resume, switch GPIO function back to DAT1 mode then turn on clock.
>
> Some device tree property should be added or modified in MSDC node
> to support SDIO eint IRQ. Pinctrls "state_eint" is mandatory. Since
> this feature depends on asynchronous interrupts, "wakeup-source",
> "keep-power-in-suspend" and "cap-sdio-irq" flags are necessary, and
> the interrupts list should be extended(the interrupt named with
> sdio_wakeup):
>         &mmcX {
>                 ...
>                 interrupt-names = "msdc", "sdio_wakeup";
>                 interrupts-extended = <...>,
>                                       <&pio xxx IRQ_TYPE_LEVEL_LOW>;
>                 ...
>                 pinctrl-names = "default", "state_uhs", "state_eint";
>                 ...
>                 pinctrl-2 = <&mmc2_pins_eint>;
>                 ...
>                 cap-sdio-irq;
>                 keep-power-in-suspend;
>                 wakeup-source;
>                 ...
>         };
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> Co-developed-by: Yong Mao <yong.mao@xxxxxxxxxxxx>
> Signed-off-by: Yong Mao <yong.mao@xxxxxxxxxxxx>
> Signed-off-by: Axe Yang <axe.yang@xxxxxxxxxxxx>
> ---
>  drivers/mmc/host/mtk-sd.c | 80 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 195dc897188b..2d5b23616df6 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2014-2015 MediaTek Inc.
> + * Copyright (c) 2014-2015, 2022 MediaTek Inc.
>   * Author: Chaotian.Jing <chaotian.jing@xxxxxxxxxxxx>
>   */
>
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> @@ -440,8 +441,10 @@ struct msdc_host {
>         struct pinctrl *pinctrl;
>         struct pinctrl_state *pins_default;
>         struct pinctrl_state *pins_uhs;
> +       struct pinctrl_state *pins_eint;
>         struct delayed_work req_timeout;
>         int irq;                /* host interrupt */
> +       int eint_irq;           /* interrupt from sdio device for waking up system */
>         struct reset_control *reset;
>
>         struct clk *src_clk;    /* msdc source clock */
> @@ -1520,17 +1523,41 @@ static void __msdc_enable_sdio_irq(struct msdc_host *host, int enb)
>
>  static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  {
> -       unsigned long flags;
>         struct msdc_host *host = mmc_priv(mmc);
> +       unsigned long flags;
> +       int ret;
>
>         spin_lock_irqsave(&host->lock, flags);
>         __msdc_enable_sdio_irq(host, enb);
>         spin_unlock_irqrestore(&host->lock, flags);
>
> -       if (enb)
> -               pm_runtime_get_noresume(host->dev);
> -       else
> -               pm_runtime_put_noidle(host->dev);
> +       if (mmc_card_enable_async_irq(mmc->card) && host->pins_eint) {
> +               if (enb) {
> +                       pinctrl_select_state(host->pinctrl, host->pins_eint);

This looks a bit odd to me. The pins are not supposed to be configured
for wakeirq at this point, right?

As I understand it, the pin state for wakeirq should be set from the
->runtime_suspend() callback, no?

> +                       ret = dev_pm_set_dedicated_wake_irq_reverse(host->dev, host->eint_irq);
> +
> +                       if (ret) {
> +                               dev_err(host->dev, "Failed to register SDIO wakeup irq!\n");
> +                               host->pins_eint = NULL;
> +                               pm_runtime_get_noresume(host->dev);
> +                       } else {
> +                               dev_info(host->dev, "SDIO eint irq: %d!\n", host->eint_irq);

If you want to log a message, please use a dev_dbg for this instead.

> +                               device_init_wakeup(host->dev, true);

To me, it looks like this is better called from ->probe(), once.

> +                       }
> +
> +                       pinctrl_select_state(host->pinctrl, host->pins_uhs);

Assuming that we can drop the earlier call to pinctrl_select_state()
to set "host->pins_eint", this call can be dropped too.

> +               } else {
> +                       dev_pm_clear_wake_irq(host->dev);
> +               }
> +       } else {
> +               if (enb) {
> +                       /* Ensure host->pins_eint is NULL */
> +                       host->pins_eint = NULL;
> +                       pm_runtime_get_noresume(host->dev);
> +               } else {
> +                       pm_runtime_put_noidle(host->dev);
> +               }
> +       }
>  }
>
>  static irqreturn_t msdc_cmdq_irq(struct msdc_host *host, u32 intsts)
> @@ -2631,6 +2658,19 @@ static int msdc_drv_probe(struct platform_device *pdev)
>                 goto host_free;
>         }
>
> +       /* Support for SDIO eint irq ? */
> +       if ((mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ) && (mmc->pm_caps & MMC_PM_KEEP_POWER)) {
> +               host->eint_irq = platform_get_irq_byname(pdev, "sdio_wakeup");
> +               if (host->eint_irq > 0) {
> +                       host->pins_eint = pinctrl_lookup_state(host->pinctrl, "state_eint");
> +                       if (IS_ERR(host->pins_eint)) {
> +                               dev_err_probe(&pdev->dev, PTR_ERR(host->pins_eint),
> +                                             "Cannot find pinctrl eint!\n");

We can probably use dev_err() instead of dev_err_probe() as
pinctrl_lookup_state() should never return -EPROBE_DEFER, I think.

> +                               host->pins_eint = NULL;
> +                       }
> +               }
> +       }
> +
>         msdc_of_property_parse(pdev, host);
>
>         host->dev = &pdev->dev;
> @@ -2845,6 +2885,12 @@ static int __maybe_unused msdc_runtime_suspend(struct device *dev)
>         struct msdc_host *host = mmc_priv(mmc);
>
>         msdc_save_reg(host);
> +
> +       if (sdio_irq_claimed(mmc) && host->pins_eint) {
> +               disable_irq(host->irq);
> +               pinctrl_select_state(host->pinctrl, host->pins_eint);
> +               sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);

__msdc_enable_sdio_irq() also calls "sdr_clr_bits(host->base +
MSDC_INTEN, MSDC_INTEN_SDIOIRQ);"

Perhaps we should call __msdc_enable_sdio_irq() here instead? To be consistent.

> +       }
>         msdc_gate_clock(host);
>         return 0;
>  }
> @@ -2860,12 +2906,19 @@ static int __maybe_unused msdc_runtime_resume(struct device *dev)
>                 return ret;
>
>         msdc_restore_reg(host);
> +
> +       if (sdio_irq_claimed(mmc) && host->pins_eint) {

msdc_restore_reg() already calls __msdc_enable_sdio_irq(), but based
only upon whether sdio_irq_claimed() returns true.

It looks like we should align the code in
msdc_runtime_resume|suspend(). Perhaps sdio_irq_claimed() should
indicate in both cases that __msdc_enable_sdio_irq() needs to be
called, while "host->pins_eint" means that we have also additional
wakeup configurations (pins and irqs) to handle.

> +               sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +               pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +               enable_irq(host->irq);
> +       }
>         return 0;
>  }
>
>  static int __maybe_unused msdc_suspend(struct device *dev)
>  {
>         struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
>         int ret;
>
>         if (mmc->caps2 & MMC_CAP2_CQE) {
> @@ -2874,11 +2927,26 @@ static int __maybe_unused msdc_suspend(struct device *dev)
>                         return ret;
>         }
>
> +       if (sdio_irq_claimed(mmc) && host->pins_eint) {
> +               pm_runtime_put_sync_suspend(dev);
> +
> +               return 0;
> +       }
> +

I assume the point with the above is to trigger rpm_suspend() to be
called for the device, so that the wakeirq can be enabled, correctly?

However, this isn't the correct way to do it (for various reasons I
can explain, if you want). Instead I think there are two options going
forward:
1. Deal with the wakeirq from the system suspend/resume callbacks,
locally in the driver.
2. Extend pm_runtime_force_suspend|resume() to let it deal with the
wakeirq for us. Similar to what rpm_suspend|resume() do.

I am inclined to try with option 2) first, as this would prevent the
boilerplate code that otherwise gets introduced by option 1). To help
out, I have prepared a patch that I am about to send, I will keep you
posted.

>         return pm_runtime_force_suspend(dev);
>  }
>
>  static int __maybe_unused msdc_resume(struct device *dev)
>  {
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       if (sdio_irq_claimed(mmc) && host->pins_eint) {
> +               pm_runtime_get_sync(dev);

Similar comments apply to this as for msdc_suspend().

> +
> +               return 0;
> +       }
> +
>         return pm_runtime_force_resume(dev);
>  }
>

Kind regards
Uffe