mbox series

[v9,0/3] mmc: mediatek: add support for SDIO async IRQ

Message ID 20220329032913.8750-1-axe.yang@mediatek.com
Headers show
Series mmc: mediatek: add support for SDIO async IRQ | expand

Message

Axe Yang March 29, 2022, 3:29 a.m. UTC
Changes in v9:
- remove pinctrl "state_dat1"

Changes in v8:
- remove maxItems property under pinctrl-names property

Changes in v7:
- add device_init_wakeup() to register SDIO host as wakeup source

Changes in v6:
- abandon cap-sdio-async-irq flag, use wakeup-source flag instead
- extend interrupts and pinctrls in mediatek mmc host controller DT documents
- add mmc_card_enable_async_irq() to access enable_async_irq flag
- simplify wakeup irq implementation with dedicate wake up irq related interface

Changes in v5:
- resort variables to reversed xmas tree order
- restore old copyright year range and add current year back

Changes in v4:
- add MMC_CAP2_SDIO_ASYNC_IRQ judge before lookup eint pinctrl
- replace spin_lock_irqsave() variant with spin_lock() in eint irq handler

Changes in v3:
- correct abbreviations with capital letters in commit message
- replace copyright year with 2022 in mtk-sd.c
- remove unnessary pointer casting
- adjust variable order to reversed xmas tree
- remove a redundant blank line
- refine if statement, following standard pattern

Changes in v2:
- change flag name from 'cap-sdio-async-int' to 'cap-sdio-async-irq'
- change corresponding macro names from xxx_INT to xxx_IRQ
- resort new member in msdc_host structure
- refine function msdc_request_dat1_eint_irq()
- rename msdc_{suspend,resume} function names, add suffix '_noirq'
- add MMC_CAP2_NO_SDIO judgement before parse eint related pin setting

Axe Yang (3):
  dt-bindings: mmc: mtk-sd: extend interrupts and pinctrls properties
  mmc: core: Add support for SDIO wakeup interrupt
  mmc: mediatek: add support for SDIO eint wakup IRQ

 .../devicetree/bindings/mmc/mtk-sd.yaml       | 17 +++-
 drivers/mmc/core/sdio.c                       | 17 ++++
 drivers/mmc/host/mtk-sd.c                     | 94 +++++++++++++++++--
 include/linux/mmc/card.h                      |  8 +-
 include/linux/mmc/sdio.h                      |  5 +
 5 files changed, 131 insertions(+), 10 deletions(-)

Comments

AngeloGioacchino Del Regno March 29, 2022, 10:33 a.m. UTC | #1
Il 29/03/22 05:29, Axe Yang ha scritto:
> If wakeup-source flag is set in host dts node, parse EAI information
> from SDIO CCCR interrupt externsion segment for in-band wakeup. If
> async interrupt is supported by SDIO card then enable it and set
> enable_async_irq flag in sdio_cccr structure to 1. The parse flow is
> implemented in sdio_read_cccr().
> 
> Acked-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>

Hello Axe,
there was a mistake about my tag (my fault, sorry), can you please change it from
Acked-by to Reviewed-by?

Thanks,
Angelo
Axe Yang March 30, 2022, 8:03 a.m. UTC | #2
On Tue, 2022-03-29 at 12:33 +0200, AngeloGioacchino Del Regno wrote:
> Il 29/03/22 05:29, Axe Yang ha scritto:
> > If wakeup-source flag is set in host dts node, parse EAI
> > information
> > from SDIO CCCR interrupt externsion segment for in-band wakeup. If
> > async interrupt is supported by SDIO card then enable it and set
> > enable_async_irq flag in sdio_cccr structure to 1. The parse flow
> > is
> > implemented in sdio_read_cccr().
> > 
> > Acked-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> 
> Hello Axe,
> there was a mistake about my tag (my fault, sorry), can you please
> change it from
> Acked-by to Reviewed-by?
> 

Sure, will update that in next version.

Regards,
Axe Yang
Chaotian Jing (井朝天) March 30, 2022, 8:30 a.m. UTC | #3
On Tue, 2022-03-29 at 11:29 +0800, Axe Yang 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:
>         &mmcX {
> 		...
> 		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@mediatek.com>
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
  Acked-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 94 +++++++++++++++++++++++++++++++++++
> ----
>  1 file changed, 86 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 65037e1d7723..2905d7134243 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@mediatek.com>
>   */
>  
> @@ -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,11 @@ 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 */
> +	int sdio_wake_irq_depth;
>  	struct reset_control *reset;
>  
>  	struct clk *src_clk;	/* msdc source clock */
> @@ -465,6 +469,7 @@ struct msdc_host {
>  	bool hs400_tuning;	/* hs400 mode online tuning */
>  	bool internal_cd;	/* Use internal card-detect logic */
>  	bool cqhci;		/* support eMMC hw cmdq */
> +	bool sdio_eint_ready;	/* Ready to support SDIO eint
> interrupt */
>  	struct msdc_save_para save_para; /* used when gate HCLK */
>  	struct msdc_tune_para def_tune_para; /* default tune setting */
>  	struct msdc_tune_para saved_tune_para; /* tune result of
> CMD21/CMD19 */
> @@ -1527,10 +1532,12 @@ static void msdc_enable_sdio_irq(struct
> mmc_host *mmc, int enb)
>  	__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 && !mmc_card_enable_async_irq(mmc->card)) {
> +		if (enb)
> +			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 +2638,18 @@ 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) {
> +		host->pins_eint = pinctrl_lookup_state(host->pinctrl,
> "state_eint");
> +		if (IS_ERR(host->pins_eint)) {
> +			ret = dev_err_probe(&pdev->dev, PTR_ERR(host-
> >pins_eint),
> +					    "Cannot find pinctrl
> eint!\n");
> +			goto host_free;
> +		}
> +
> +		host->sdio_eint_ready = true;
> +	}
> +
>  	msdc_of_property_parse(pdev, host);
>  
>  	host->dev = &pdev->dev;
> @@ -2722,6 +2741,21 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>  	if (ret)
>  		goto release;
>  
> +	if (host->sdio_eint_ready) {
> +		host->eint_irq = irq_of_parse_and_map(host->dev-
> >of_node, 1);
> +		ret = host->eint_irq ?
> dev_pm_set_dedicated_wake_irq(host->dev, host->eint_irq) :
> +		      -ENODEV;
> +
> +		if (ret) {
> +			dev_err(host->dev, "Failed to register data1
> eint irq!\n");
> +			goto release;
> +		}
> +
> +		dev_pm_disable_wake_irq(host->dev);
> +		pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +	}
> +
> +	device_init_wakeup(host->dev, true);
>  	pm_runtime_set_active(host->dev);
>  	pm_runtime_set_autosuspend_delay(host->dev,
> MTK_MMC_AUTOSUSPEND_DELAY);
>  	pm_runtime_use_autosuspend(host->dev);
> @@ -2734,6 +2768,7 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>  	return 0;
>  end:
>  	pm_runtime_disable(host->dev);
> +	dev_pm_clear_wake_irq(host->dev);
>  release:
>  	platform_set_drvdata(pdev, NULL);
>  	msdc_deinit_hw(host);
> @@ -2845,6 +2880,16 @@ static int __maybe_unused
> msdc_runtime_suspend(struct device *dev)
>  	struct msdc_host *host = mmc_priv(mmc);
>  
>  	msdc_save_reg(host);
> +
> +	if (host->sdio_eint_ready) {
> +		disable_irq(host->irq);
> +		pinctrl_select_state(host->pinctrl, host->pins_eint);
> +		if (host->sdio_wake_irq_depth == 0) {
> +			dev_pm_enable_wake_irq(dev);
> +			host->sdio_wake_irq_depth++;
> +		}
> +		sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +	}
>  	msdc_gate_clock(host);
>  	return 0;
>  }
> @@ -2860,12 +2905,25 @@ static int __maybe_unused
> msdc_runtime_resume(struct device *dev)
>  		return ret;
>  
>  	msdc_restore_reg(host);
> +
> +	if (host->sdio_eint_ready) {
> +		if (host->sdio_wake_irq_depth > 0) {
> +			dev_pm_disable_wake_irq(dev);
> +			host->sdio_wake_irq_depth--;
> +			sdr_set_bits(host->base + SDC_CFG,
> SDC_CFG_SDIOIDE);
> +		} else {
> +			sdr_clr_bits(host->base + MSDC_INTEN,
> MSDC_INTEN_SDIOIRQ);
> +		}
> +		pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +		enable_irq(host->irq);
> +	}
>  	return 0;
>  }
>  
> -static int __maybe_unused msdc_suspend(struct device *dev)
> +static int __maybe_unused msdc_suspend_noirq(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,16 +2932,36 @@ static int __maybe_unused msdc_suspend(struct
> device *dev)
>  			return ret;
>  	}
>  
> +	if (host->sdio_eint_ready)
> +		enable_irq_wake(host->eint_irq);
> +
>  	return pm_runtime_force_suspend(dev);
>  }
>  
> -static int __maybe_unused msdc_resume(struct device *dev)
> +static int __maybe_unused msdc_resume_noirq(struct device *dev)
>  {
> +	struct mmc_host *mmc = dev_get_drvdata(dev);
> +	struct msdc_host *host = mmc_priv(mmc);
> +
> +	if (host->sdio_eint_ready) {
> +		disable_irq_wake(host->eint_irq);
> +
> +		/*
> +		 * In noirq resume stage, msdc_runtime_resume()
> +		 * won't be called, so disalbe wake irq here
> +		 * to block dedicated wake irq handler callback.
> +		 */
> +		if (likely(host->sdio_wake_irq_depth > 0)) {
> +			dev_pm_disable_wake_irq(dev);
> +			host->sdio_wake_irq_depth--;
> +		}
> +	}
> +
>  	return pm_runtime_force_resume(dev);
>  }
>  
>  static const struct dev_pm_ops msdc_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(msdc_suspend, msdc_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(msdc_suspend_noirq,
> msdc_resume_noirq)
>  	SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume,
> NULL)
>  };
>
Ulf Hansson April 1, 2022, 12:42 p.m. UTC | #4
On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> wrote:
>
> If wakeup-source flag is set in host dts node, parse EAI information
> from SDIO CCCR interrupt externsion segment for in-band wakeup. If
> async interrupt is supported by SDIO card then enable it and set
> enable_async_irq flag in sdio_cccr structure to 1. The parse flow is
> implemented in sdio_read_cccr().
>
> Acked-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> ---
>  drivers/mmc/core/sdio.c  | 17 +++++++++++++++++
>  include/linux/mmc/card.h |  8 +++++++-
>  include/linux/mmc/sdio.h |  5 +++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 25799accf8a0..4898c5e9a299 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -226,6 +226,23 @@ static int sdio_read_cccr(struct mmc_card *card, u32 ocr)
>                                 card->sw_caps.sd3_drv_type |= SD_DRIVER_TYPE_C;
>                         if (data & SDIO_DRIVE_SDTD)
>                                 card->sw_caps.sd3_drv_type |= SD_DRIVER_TYPE_D;
> +
> +                       if (card->host->pm_caps & MMC_PM_WAKE_SDIO_IRQ) {

After a second thought, I think we can just skip this check. The
MMC_PM_WAKE_SDIO_IRQ indicates that the host supports SDIO IRQs as
*system wakeups*.

But, in fact, I think we want this feature to be enabled to allow
waking up for runtime_suspend (RPM_SUSPENDED) too.

> +                               ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTERRUPT_EXT, 0,
> +                                                      &data);
> +                               if (ret)
> +                                       goto out;
> +
> +                               if (data & SDIO_INTERRUPT_EXT_SAI) {
> +                                       data |= SDIO_INTERRUPT_EXT_EAI;
> +                                       ret = mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_INTERRUPT_EXT,
> +                                                              data, NULL);
> +                                       if (ret)
> +                                               goto out;
> +
> +                                       card->cccr.enable_async_irq = 1;
> +                               }
> +                       }
>                 }

[...]

Other than the above, this looks good to me!

Kind regards
Uffe
Ulf Hansson April 4, 2022, 3:01 p.m. UTC | #5
On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> 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:
>         &mmcX {
>                 ...
>                 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@mediatek.com>
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 94 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 65037e1d7723..2905d7134243 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@mediatek.com>
>   */
>
> @@ -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,11 @@ 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 */
> +       int sdio_wake_irq_depth;
>         struct reset_control *reset;
>
>         struct clk *src_clk;    /* msdc source clock */
> @@ -465,6 +469,7 @@ struct msdc_host {
>         bool hs400_tuning;      /* hs400 mode online tuning */
>         bool internal_cd;       /* Use internal card-detect logic */
>         bool cqhci;             /* support eMMC hw cmdq */
> +       bool sdio_eint_ready;   /* Ready to support SDIO eint interrupt */
>         struct msdc_save_para save_para; /* used when gate HCLK */
>         struct msdc_tune_para def_tune_para; /* default tune setting */
>         struct msdc_tune_para saved_tune_para; /* tune result of CMD21/CMD19 */
> @@ -1527,10 +1532,12 @@ static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
>         __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 && !mmc_card_enable_async_irq(mmc->card)) {

It might not be obvious, but in fact mmc->card always exists in this
case, hence there is no need to check the pointer before using it.

> +               if (enb)
> +                       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 +2638,18 @@ 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) {
> +               host->pins_eint = pinctrl_lookup_state(host->pinctrl, "state_eint");
> +               if (IS_ERR(host->pins_eint)) {
> +                       ret = dev_err_probe(&pdev->dev, PTR_ERR(host->pins_eint),
> +                                           "Cannot find pinctrl eint!\n");
> +                       goto host_free;
> +               }
> +
> +               host->sdio_eint_ready = true;

Rather than using a separate variable, couldn't we just check "if
(host->pins_eint)" to know whether wakeup settings shall be managed?

> +       }
> +
>         msdc_of_property_parse(pdev, host);
>
>         host->dev = &pdev->dev;
> @@ -2722,6 +2741,21 @@ static int msdc_drv_probe(struct platform_device *pdev)
>         if (ret)
>                 goto release;
>
> +       if (host->sdio_eint_ready) {
> +               host->eint_irq = irq_of_parse_and_map(host->dev->of_node, 1);
> +               ret = host->eint_irq ? dev_pm_set_dedicated_wake_irq(host->dev, host->eint_irq) :
> +                     -ENODEV;

I think it would make better sense to call
dev_pm_set_dedicated_wake_irq() from msdc_enable_sdio_irq(). In this
way, you can still support SDIO irqs, even if there are no GPIO irq
for available for re-routing.

In other words, if the GPIO irq is available, then that's the best
option and we should use that - otherwise we need to keep preventing
runtime-suspend by bumping the runtime PM usage count.

> +
> +               if (ret) {
> +                       dev_err(host->dev, "Failed to register data1 eint irq!\n");
> +                       goto release;
> +               }
> +
> +               dev_pm_disable_wake_irq(host->dev);
> +               pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +       }
> +
> +       device_init_wakeup(host->dev, true);

I think this better belongs under the condition of whether
MMC_PM_WAKE_SDIO_IRQ and MMC_PM_KEEP_POWER caps are set.

>         pm_runtime_set_active(host->dev);
>         pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
>         pm_runtime_use_autosuspend(host->dev);
> @@ -2734,6 +2768,7 @@ static int msdc_drv_probe(struct platform_device *pdev)
>         return 0;
>  end:
>         pm_runtime_disable(host->dev);
> +       dev_pm_clear_wake_irq(host->dev);
>  release:
>         platform_set_drvdata(pdev, NULL);
>         msdc_deinit_hw(host);
> @@ -2845,6 +2880,16 @@ static int __maybe_unused msdc_runtime_suspend(struct device *dev)
>         struct msdc_host *host = mmc_priv(mmc);
>
>         msdc_save_reg(host);
> +
> +       if (host->sdio_eint_ready) {

The core provides a helper function, sdio_irq_claimed(), that can be
used to know whether there is an SDIO irq registered. It looks like
that certainly can be useful here.

> +               disable_irq(host->irq);
> +               pinctrl_select_state(host->pinctrl, host->pins_eint);
> +               if (host->sdio_wake_irq_depth == 0) {
> +                       dev_pm_enable_wake_irq(dev);

This should be managed from the runtime PM core. See rpm_suspend().

Or perhaps there is a reason that doesn't work for you?

> +                       host->sdio_wake_irq_depth++;

Again, sdio_irq_claimed() is probably better to use.

> +               }
> +               sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +       }
>         msdc_gate_clock(host);
>         return 0;
>  }
> @@ -2860,12 +2905,25 @@ static int __maybe_unused msdc_runtime_resume(struct device *dev)
>                 return ret;
>
>         msdc_restore_reg(host);
> +
> +       if (host->sdio_eint_ready) {
> +               if (host->sdio_wake_irq_depth > 0) {
> +                       dev_pm_disable_wake_irq(dev);
> +                       host->sdio_wake_irq_depth--;
> +                       sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +               } else {
> +                       sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> +               }
> +               pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +               enable_irq(host->irq);
> +       }
>         return 0;
>  }
>
> -static int __maybe_unused msdc_suspend(struct device *dev)
> +static int __maybe_unused msdc_suspend_noirq(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,16 +2932,36 @@ static int __maybe_unused msdc_suspend(struct device *dev)
>                         return ret;
>         }
>
> +       if (host->sdio_eint_ready)
> +               enable_irq_wake(host->eint_irq);

The PM core should manage this for us through dpm_suspend_noirq(), I
think. We just need to make sure that the runtime PM status of the
device has been set to RPM_SUSPENDED before dpm_suspend_noirq()  gets
called. Have a look at dpm_suspend_noirq() ->
device_wakeup_arm_wake_irqs() -> dev_pm_arm_wake_irq().

In other words (and unless I am missing something), if we continue to
call pm_runtime_force_suspend() from a ->suspend() callback this
should work just fine.

> +
>         return pm_runtime_force_suspend(dev);
>  }
>
> -static int __maybe_unused msdc_resume(struct device *dev)
> +static int __maybe_unused msdc_resume_noirq(struct device *dev)
>  {
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       if (host->sdio_eint_ready) {
> +               disable_irq_wake(host->eint_irq);
> +
> +               /*
> +                * In noirq resume stage, msdc_runtime_resume()
> +                * won't be called, so disalbe wake irq here
> +                * to block dedicated wake irq handler callback.
> +                */
> +               if (likely(host->sdio_wake_irq_depth > 0)) {
> +                       dev_pm_disable_wake_irq(dev);
> +                       host->sdio_wake_irq_depth--;
> +               }

Similar comment as from the system suspend case. I don't think we need
to move to the noirq phase.

Or maybe I am missing something here?

> +       }
> +
>         return pm_runtime_force_resume(dev);
>  }
>
>  static const struct dev_pm_ops msdc_dev_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(msdc_suspend, msdc_resume)
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(msdc_suspend_noirq, msdc_resume_noirq)
>         SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume, NULL)
>  };
>
> --
> 2.25.1
>

Kind regards
Uffe
Axe Yang April 6, 2022, 9:41 a.m. UTC | #6
On Fri, 2022-04-01 at 14:42 +0200, Ulf Hansson wrote:
> On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> wrote:
> > 
> > If wakeup-source flag is set in host dts node, parse EAI
> > information
> > from SDIO CCCR interrupt externsion segment for in-band wakeup. If
> > async interrupt is supported by SDIO card then enable it and set
> > enable_async_irq flag in sdio_cccr structure to 1. The parse flow
> > is
> > implemented in sdio_read_cccr().
> > 
> > Acked-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > ---
> >  drivers/mmc/core/sdio.c  | 17 +++++++++++++++++
> >  include/linux/mmc/card.h |  8 +++++++-
> >  include/linux/mmc/sdio.h |  5 +++++
> >  3 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > index 25799accf8a0..4898c5e9a299 100644
> > --- a/drivers/mmc/core/sdio.c
> > +++ b/drivers/mmc/core/sdio.c
> > @@ -226,6 +226,23 @@ static int sdio_read_cccr(struct mmc_card
> > *card, u32 ocr)
> >                                 card->sw_caps.sd3_drv_type |=
> > SD_DRIVER_TYPE_C;
> >                         if (data & SDIO_DRIVE_SDTD)
> >                                 card->sw_caps.sd3_drv_type |=
> > SD_DRIVER_TYPE_D;
> > +
> > +                       if (card->host->pm_caps &
> > MMC_PM_WAKE_SDIO_IRQ) {
> 
> After a second thought, I think we can just skip this check. The
> MMC_PM_WAKE_SDIO_IRQ indicates that the host supports SDIO IRQs as
> *system wakeups*.
> 
> But, in fact, I think we want this feature to be enabled to allow
> waking up for runtime_suspend (RPM_SUSPENDED) too.
> 
> > +                               ret = mmc_io_rw_direct(card, 0, 0,
> > SDIO_CCCR_INTERRUPT_EXT, 0,
> > +                                                      &data);
> > +                               if (ret)
> > +                                       goto out;
> > +
> > +                               if (data & SDIO_INTERRUPT_EXT_SAI)
> > {
> > +                                       data |=
> > SDIO_INTERRUPT_EXT_EAI;
> > +                                       ret =
> > mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_INTERRUPT_EXT,
> > +                                                              data
> > , NULL);
> > +                                       if (ret)
> > +                                               goto out;
> > +
> > +                                       card->cccr.enable_async_irq 
> > = 1;
> > +                               }
> > +                       }
> >                 }
> 
> 
Will skip this check in next version. Thanks.

Regards,
Axe
Axe Yang May 19, 2022, 11:53 a.m. UTC | #7
On Mon, 2022-04-04 at 17:01 +0200, Ulf Hansson wrote:
> On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> 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:
> >         &mmcX {
> >                 ...
> >                 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@mediatek.com>
> > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 94
> > +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 86 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 65037e1d7723..2905d7134243 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@mediatek.com>
> >   */
> > 
> > @@ -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,11 @@ 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 */
> > +       int sdio_wake_irq_depth;
> >         struct reset_control *reset;
> > 
> >         struct clk *src_clk;    /* msdc source clock */
> > @@ -465,6 +469,7 @@ struct msdc_host {
> >         bool hs400_tuning;      /* hs400 mode online tuning */
> >         bool internal_cd;       /* Use internal card-detect logic
> > */
> >         bool cqhci;             /* support eMMC hw cmdq */
> > +       bool sdio_eint_ready;   /* Ready to support SDIO eint
> > interrupt */
> >         struct msdc_save_para save_para; /* used when gate HCLK */
> >         struct msdc_tune_para def_tune_para; /* default tune
> > setting */
> >         struct msdc_tune_para saved_tune_para; /* tune result of
> > CMD21/CMD19 */
> > @@ -1527,10 +1532,12 @@ static void msdc_enable_sdio_irq(struct
> > mmc_host *mmc, int enb)
> >         __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 && !mmc_card_enable_async_irq(mmc->card)) {
> 
> It might not be obvious, but in fact mmc->card always exists in this
> case, hence there is no need to check the pointer before using it.

this check is removed in v10.

> 
> > +               if (enb)
> > +                       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 +2638,18 @@ 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) {
> > +               host->pins_eint = pinctrl_lookup_state(host-
> > >pinctrl, "state_eint");
> > +               if (IS_ERR(host->pins_eint)) {
> > +                       ret = dev_err_probe(&pdev->dev,
> > PTR_ERR(host->pins_eint),
> > +                                           "Cannot find pinctrl
> > eint!\n");
> > +                       goto host_free;
> > +               }
> > +
> > +               host->sdio_eint_ready = true;
> 
> Rather than using a separate variable, couldn't we just check "if
> (host->pins_eint)" to know whether wakeup settings shall be managed?

This variable is removed in v10, use host->pins_eint instead. 

> 
> > +       }
> > +
> >         msdc_of_property_parse(pdev, host);
> > 
> >         host->dev = &pdev->dev;
> > @@ -2722,6 +2741,21 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >         if (ret)
> >                 goto release;
> > 
> > +       if (host->sdio_eint_ready) {
> > +               host->eint_irq = irq_of_parse_and_map(host->dev-
> > >of_node, 1);
> > +               ret = host->eint_irq ?
> > dev_pm_set_dedicated_wake_irq(host->dev, host->eint_irq) :
> > +                     -ENODEV;
> 
> I think it would make better sense to call
> dev_pm_set_dedicated_wake_irq() from msdc_enable_sdio_irq(). In this
> way, you can still support SDIO irqs, even if there are no GPIO irq
> for available for re-routing.
> 
> In other words, if the GPIO irq is available, then that's the best
> option and we should use that - otherwise we need to keep preventing
> runtime-suspend by bumping the runtime PM usage count.

dev_pm_set_dedicated_wake_irq() is replaced with
dev_pm_set_dedicated_wake_irq_reverse() to assure msdc irq is disabled
earlier than enable wake irq in rpm_suspend(), and it is moved to
msdc_enable_sdio_irq() phase.

> 
> > +
> > +               if (ret) {
> > +                       dev_err(host->dev, "Failed to register
> > data1 eint irq!\n");
> > +                       goto release;
> > +               }
> > +
> > +               dev_pm_disable_wake_irq(host->dev);
> > +               pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +       }
> > +
> > +       device_init_wakeup(host->dev, true);
> 
> I think this better belongs under the condition of whether
> MMC_PM_WAKE_SDIO_IRQ and MMC_PM_KEEP_POWER caps are set.

Both of them are checked before parsing eint pinstate in v10.

> 
> >         pm_runtime_set_active(host->dev);
> >         pm_runtime_set_autosuspend_delay(host->dev,
> > MTK_MMC_AUTOSUSPEND_DELAY);
> >         pm_runtime_use_autosuspend(host->dev);
> > @@ -2734,6 +2768,7 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >         return 0;
> >  end:
> >         pm_runtime_disable(host->dev);
> > +       dev_pm_clear_wake_irq(host->dev);
> >  release:
> >         platform_set_drvdata(pdev, NULL);
> >         msdc_deinit_hw(host);
> > @@ -2845,6 +2880,16 @@ static int __maybe_unused
> > msdc_runtime_suspend(struct device *dev)
> >         struct msdc_host *host = mmc_priv(mmc);
> > 
> >         msdc_save_reg(host);
> > +
> > +       if (host->sdio_eint_ready) {
> 
> The core provides a helper function, sdio_irq_claimed(), that can be
> used to know whether there is an SDIO irq registered. It looks like
> that certainly can be useful here.

Yes. We need sdio_irq_claimed() check since set dedicated wake irq is
been moved to claim irq stage.

> 
> > +               disable_irq(host->irq);
> > +               pinctrl_select_state(host->pinctrl, host-
> > >pins_eint);
> > +               if (host->sdio_wake_irq_depth == 0) {
> > +                       dev_pm_enable_wake_irq(dev);
> 
> This should be managed from the runtime PM core. See rpm_suspend().
> 
> Or perhaps there is a reason that doesn't work for you?
> 
> > +                       host->sdio_wake_irq_depth++;
> 
> Again, sdio_irq_claimed() is probably better to use.
> 
> > +               }
> > +               sdr_clr_bits(host->base + SDC_CFG,
> > SDC_CFG_SDIOIDE);
> > +       }
> >         msdc_gate_clock(host);
> >         return 0;
> >  }
> > @@ -2860,12 +2905,25 @@ static int __maybe_unused
> > msdc_runtime_resume(struct device *dev)
> >                 return ret;
> > 
> >         msdc_restore_reg(host);
> > +
> > +       if (host->sdio_eint_ready) {
> > +               if (host->sdio_wake_irq_depth > 0) {
> > +                       dev_pm_disable_wake_irq(dev);
> > +                       host->sdio_wake_irq_depth--;
> > +                       sdr_set_bits(host->base + SDC_CFG,
> > SDC_CFG_SDIOIDE);
> > +               } else {
> > +                       sdr_clr_bits(host->base + MSDC_INTEN,
> > MSDC_INTEN_SDIOIRQ);
> > +               }
> > +               pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +               enable_irq(host->irq);
> > +       }
> >         return 0;
> >  }
> > 
> > -static int __maybe_unused msdc_suspend(struct device *dev)
> > +static int __maybe_unused msdc_suspend_noirq(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,16 +2932,36 @@ static int __maybe_unused
> > msdc_suspend(struct device *dev)
> >                         return ret;
> >         }
> > 
> > +       if (host->sdio_eint_ready)
> > +               enable_irq_wake(host->eint_irq);
> 
> The PM core should manage this for us through dpm_suspend_noirq(), I
> think. We just need to make sure that the runtime PM status of the
> device has been set to RPM_SUSPENDED before dpm_suspend_noirq()  gets
> called. Have a look at dpm_suspend_noirq() ->
> device_wakeup_arm_wake_irqs() -> dev_pm_arm_wake_irq().
> 
> In other words (and unless I am missing something), if we continue to
> call pm_runtime_force_suspend() from a ->suspend() callback this
> should work just fine.

This part is simplified in v10. dpm/rpm system can manage wake irq
well.

> 
> > +
> >         return pm_runtime_force_suspend(dev);
> >  }
> > 
> > -static int __maybe_unused msdc_resume(struct device *dev)
> > +static int __maybe_unused msdc_resume_noirq(struct device *dev)
> >  {
> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +
> > +       if (host->sdio_eint_ready) {
> > +               disable_irq_wake(host->eint_irq);
> > +
> > +               /*
> > +                * In noirq resume stage, msdc_runtime_resume()
> > +                * won't be called, so disalbe wake irq here
> > +                * to block dedicated wake irq handler callback.
> > +                */
> > +               if (likely(host->sdio_wake_irq_depth > 0)) {
> > +                       dev_pm_disable_wake_irq(dev);
> > +                       host->sdio_wake_irq_depth--;
> > +               }
> 
> Similar comment as from the system suspend case. I don't think we
> need
> to move to the noirq phase.
> 
> Or maybe I am missing something here?

They are restored to system suspend stage in V10.
But I have to use rpm_suspend()/rpm_resueme() instead of 
pm_runtime_force_suspend()/pm_runtime_force_resmue(). 
pm_runtime_force_resume() will go out directly because rpm state is
RPM_SUSPENDED in system resume stage.

> 
> > +       }
> > +
> >         return pm_runtime_force_resume(dev);
> >  }
> > 
> >  static const struct dev_pm_ops msdc_dev_pm_ops = {
> > -       SET_SYSTEM_SLEEP_PM_OPS(msdc_suspend, msdc_resume)
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(msdc_suspend_noirq,
> > msdc_resume_noirq)
> >         SET_RUNTIME_PM_OPS(msdc_runtime_suspend,
> > msdc_runtime_resume, NULL)
> >  };
> > 
> > --
> > 2.25.1
> > 
> 

Regards,
Axe