On Thu, 23 Jun 2022 at 11:05, 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;
> ...
> };
>
> Co-developed-by: Yong Mao <yong.mao@xxxxxxxxxxxx>
> Signed-off-by: Yong Mao <yong.mao@xxxxxxxxxxxx>
> Signed-off-by: Axe Yang <axe.yang@xxxxxxxxxxxx>
My apologies for the delay in reviewing this.
> ---
> drivers/mmc/host/mtk-sd.c | 84 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 195dc897188b..f907b96cfd87 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,46 @@ 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) {
> + /*
> + * In dev_pm_set_dedicated_wake_irq_reverse(), eint pin will be set to
> + * GPIO mode. We need to restore it to SDIO DAT1 mode after that.
> + * Since the current pinstate is pins_uhs, to ensure pinctrl select take
> + * affect successfully, we change the pinstate to pins_eint firstly.
> + */
> + pinctrl_select_state(host->pinctrl, host->pins_eint);
I am sorry, but I don't understand what goes on here. Why do you need
to change the pinctrl setting to "pins_eint" here?
The bellow call to dev_pm_set_dedicated_wake_irq_reverse() doesn't
change the pinctrl setting as the comment suggests above.
dev_pm_set_dedicated_wake_irq_reverse() will register the wakeirq, but
more importantly, it should also leave the wakeirq disabled, right?
> + 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_dbg(host->dev, "SDIO eint irq: %d!\n", host->eint_irq);
> + }
> +
> + pinctrl_select_state(host->pinctrl, host->pins_uhs);
According to my comment above, I also don't understand why you need
this. Why can't you just leave the pinctrl in the "pins_uhs" state?
> + } 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 +2663,20 @@ 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(&pdev->dev, "Cannot find pinctrl eint!\n");
> + host->pins_eint = NULL;
> + } else {
> + device_init_wakeup(&pdev->dev, true);
> + }
> + }
> + }
> +
> msdc_of_property_parse(pdev, host);
>
> host->dev = &pdev->dev;
> @@ -2845,6 +2891,13 @@ static int __maybe_unused msdc_runtime_suspend(struct device *dev)
> struct msdc_host *host = mmc_priv(mmc);
>
> msdc_save_reg(host);
> +
> + if (host->pins_eint) {
> + disable_irq(host->irq);
> + pinctrl_select_state(host->pinctrl, host->pins_eint);
> + if (sdio_irq_claimed(mmc))
> + __msdc_enable_sdio_irq(host, 0);
> + }
This above code isn't entirely correct. Just because you have
"pins_eint", doesn't mean that you have a wakeirq being registered, as
that also depends on whether there is an SDIO irq claimed.
So, I think this should rather be:
if (sdio_irq_claimed(mmc)) {
if (host->pins_eint) {
disable_irq(host->irq);
pinctrl_select_state(host->pinctrl, host->pins_eint);
}
__msdc_enable_sdio_irq(host, 0);
}
> msdc_gate_clock(host);
> return 0;
> }
> @@ -2860,12 +2913,18 @@ static int __maybe_unused msdc_runtime_resume(struct device *dev)
> return ret;
>
> msdc_restore_reg(host);
> +
> + if (host->pins_eint) {
Similar comment as above. You need to check sdio_irq_claimed() here too.
> + 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 +2933,24 @@ static int __maybe_unused msdc_suspend(struct device *dev)
> return ret;
> }
>
> + /*
> + * Bump up runtime PM usage counter otherwise dev->power.needs_force_resume will
> + * not be marked as 1, pm_runtime_force_resume() will go out directly.
> + */
> + if (host->pins_eint)
This works, but can be improved by checking sdio_irq_claimed() too.
> + pm_runtime_get_noresume(dev);
> +
> 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 (host->pins_eint)
> + pm_runtime_put_noidle(dev);
Ditto.
> +
> return pm_runtime_force_resume(dev);
> }
>
Kind regards
Uffe
