[PATCH v1 0/3] mmc: mediatek: add support for SDIO async int

[PATCH v1 0/3] mmc: mediatek: add support for SDIO async int

[Axe Yang]

Axe Yang (3):
  dt-bindings: mmc: add cap-sdio-async-int flag
  mmc: core: Add support for SDIO async interrupt
  mmc: mediatek: add support for SDIO eint irq

 .../bindings/mmc/mmc-controller.yaml          |   5 +
 drivers/mmc/core/host.c                       |   2 +
 drivers/mmc/core/sdio.c                       |  17 +++
 drivers/mmc/host/mtk-sd.c                     | 113 +++++++++++++++++-
 include/linux/mmc/card.h                      |   3 +-
 include/linux/mmc/host.h                      |   1 +
 include/linux/mmc/sdio.h                      |   5 +
 7 files changed, 139 insertions(+), 7 deletions(-)

--
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v1 1/3] dt-bindings: mmc: add cap-sdio-async-int flag

[Axe Yang]

Asynchronous interrupt is a mechanism that allow SDIO devices alarm
interrupt when host stop providing clock to card. Add a DT flag to
enable this feature if it is supported by SDIO card.

Signed-off-by: Axe Yang <axe.yang@mediatek.com>
---
 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index 25ac8e200970..7230421583c6 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -165,6 +165,11 @@ properties:
     description:
       eMMC hardware reset is supported
 
+  cap-sdio-async-int:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      SDIO async interrupt is supported.
+
   cap-sdio-irq:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/3] dt-bindings: mmc: add cap-sdio-async-int flag

[Rob Herring]

On Mon, Dec 27, 2021 at 04:36:39PM +0800, Axe Yang wrote:
> Asynchronous interrupt is a mechanism that allow SDIO devices alarm
> interrupt when host stop providing clock to card. Add a DT flag to
> enable this feature if it is supported by SDIO card.

A card property should be in the card node. Is this not discoverable?

> 
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> index 25ac8e200970..7230421583c6 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> @@ -165,6 +165,11 @@ properties:
>      description:
>        eMMC hardware reset is supported
>  
> +  cap-sdio-async-int:

Perhaps be consistent with the next property and use 'irq'.

> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      SDIO async interrupt is supported.
> +
>    cap-sdio-irq:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:
> -- 
> 2.25.1
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/3] dt-bindings: mmc: add cap-sdio-async-int flag

[Axe Yang]

On Tue, 2022-01-04 at 16:33 -0600, Rob Herring wrote:
> On Mon, Dec 27, 2021 at 04:36:39PM +0800, Axe Yang wrote:
> > Asynchronous interrupt is a mechanism that allow SDIO devices alarm
> > interrupt when host stop providing clock to card. Add a DT flag to
> > enable this feature if it is supported by SDIO card.
> 
> A card property should be in the card node. Is this not discoverable?

Thank you for your comment.
Async interrupt is not a 'card property', but more like a protocol.
The intention of this flag is to decide whether to support this feature
on SDIO host side.
Before that, host need to confirm that async interrupt is supported on
card side(by read and parse Support Async Interrupt segment in CCCR
from card).

> 
> > 
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 5
> > +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc-
> > controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-
> > controller.yaml
> > index 25ac8e200970..7230421583c6 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> > @@ -165,6 +165,11 @@ properties:
> >      description:
> >        eMMC hardware reset is supported
> >  
> > +  cap-sdio-async-int:
> 
> Perhaps be consistent with the next property and use 'irq'.

Will fix it in next version.

> 
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      SDIO async interrupt is supported.
> > +
> >    cap-sdio-irq:
> >      $ref: /schemas/types.yaml#/definitions/flag
> >      description:
> > -- 
> > 2.25.1
> > 
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v1 2/3] mmc: core: Add support for SDIO async interrupt

[Axe Yang]

If cap-sdio-async-int flag is set in host dts node, parse EAI
information from SDIO CCCR interrupt externsion segment. If async
interrupt is supported by SDIO card then send command to card to
enable it and set enable_async_int flag in sdio_cccr structure to 1.
The parse flow is implemented in sdio_read_cccr().

Signed-off-by: Axe Yang <axe.yang@mediatek.com>
---
 drivers/mmc/core/host.c  |  2 ++
 drivers/mmc/core/sdio.c  | 17 +++++++++++++++++
 include/linux/mmc/card.h |  3 ++-
 include/linux/mmc/host.h |  1 +
 include/linux/mmc/sdio.h |  5 +++++
 5 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index d4683b1d263f..7ad60fab099a 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -401,6 +401,8 @@ int mmc_of_parse(struct mmc_host *host)
 	if (device_property_read_bool(dev, "no-mmc-hs400"))
 		host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
 				 MMC_CAP2_HS400_ES);
+	if (device_property_read_bool(dev, "cap-sdio-async-int"))
+		host->caps2 |= MMC_CAP2_SDIO_ASYNC_INT;
 
 	/* Must be after "non-removable" check */
 	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 68edf7a615be..335e4ada733a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -225,6 +225,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->caps2 & MMC_CAP2_SDIO_ASYNC_INT) {
+				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_int = 1;
+				}
+			}
 		}
 
 		/* if no uhs mode ensure we check for high speed */
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 37f975875102..b0deb8ca8eeb 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -219,7 +219,8 @@ struct sdio_cccr {
 				wide_bus:1,
 				high_power:1,
 				high_speed:1,
-				disable_cd:1;
+				disable_cd:1,
+				enable_async_int:1;
 };
 
 struct sdio_cis {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c0c9a0fdf57..60c8ebe1a5e8 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -399,6 +399,7 @@ struct mmc_host {
 #define MMC_CAP2_CRYPTO		0
 #endif
 #define MMC_CAP2_ALT_GPT_TEGRA	(1 << 28)	/* Host with eMMC that has GPT entry at a non-standard location */
+#define MMC_CAP2_SDIO_ASYNC_INT	(1 << 29)	/* SDIO host supports asynchronous interrupt */
 
 	int			fixed_drv_type;	/* fixed driver type for non-removable media */
 
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index 2a05d1ac4f0e..1ef400f28642 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -159,6 +159,11 @@
 #define  SDIO_DTSx_SET_TYPE_A	(1 << SDIO_DRIVE_DTSx_SHIFT)
 #define  SDIO_DTSx_SET_TYPE_C	(2 << SDIO_DRIVE_DTSx_SHIFT)
 #define  SDIO_DTSx_SET_TYPE_D	(3 << SDIO_DRIVE_DTSx_SHIFT)
+
+#define SDIO_CCCR_INTERRUPT_EXT	0x16
+#define SDIO_INTERRUPT_EXT_SAI	(1 << 0)
+#define SDIO_INTERRUPT_EXT_EAI	(1 << 1)
+
 /*
  * Function Basic Registers (FBR)
  */
-- 
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 2/3] mmc: core: Add support for SDIO async interrupt

[AngeloGioacchino Del Regno]

Il 27/12/21 09:36, Axe Yang ha scritto:
> If cap-sdio-async-int flag is set in host dts node, parse EAI
> information from SDIO CCCR interrupt externsion segment. If async
> interrupt is supported by SDIO card then send command to card to
> enable it and set enable_async_int flag in sdio_cccr structure to 1.
> The parse flow is implemented in sdio_read_cccr().
> 
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>

Acked-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   drivers/mmc/core/host.c  |  2 ++
>   drivers/mmc/core/sdio.c  | 17 +++++++++++++++++
>   include/linux/mmc/card.h |  3 ++-
>   include/linux/mmc/host.h |  1 +
>   include/linux/mmc/sdio.h |  5 +++++
>   5 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index d4683b1d263f..7ad60fab099a 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -401,6 +401,8 @@ int mmc_of_parse(struct mmc_host *host)
>   	if (device_property_read_bool(dev, "no-mmc-hs400"))
>   		host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
>   				 MMC_CAP2_HS400_ES);
> +	if (device_property_read_bool(dev, "cap-sdio-async-int"))
> +		host->caps2 |= MMC_CAP2_SDIO_ASYNC_INT;
>   
>   	/* Must be after "non-removable" check */
>   	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 68edf7a615be..335e4ada733a 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -225,6 +225,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->caps2 & MMC_CAP2_SDIO_ASYNC_INT) {
> +				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_int = 1;
> +				}
> +			}
>   		}
>   
>   		/* if no uhs mode ensure we check for high speed */
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 37f975875102..b0deb8ca8eeb 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -219,7 +219,8 @@ struct sdio_cccr {
>   				wide_bus:1,
>   				high_power:1,
>   				high_speed:1,
> -				disable_cd:1;
> +				disable_cd:1,
> +				enable_async_int:1;
>   };
>   
>   struct sdio_cis {
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c0c9a0fdf57..60c8ebe1a5e8 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -399,6 +399,7 @@ struct mmc_host {
>   #define MMC_CAP2_CRYPTO		0
>   #endif
>   #define MMC_CAP2_ALT_GPT_TEGRA	(1 << 28)	/* Host with eMMC that has GPT entry at a non-standard location */
> +#define MMC_CAP2_SDIO_ASYNC_INT	(1 << 29)	/* SDIO host supports asynchronous interrupt */
>   
>   	int			fixed_drv_type;	/* fixed driver type for non-removable media */
>   
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index 2a05d1ac4f0e..1ef400f28642 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -159,6 +159,11 @@
>   #define  SDIO_DTSx_SET_TYPE_A	(1 << SDIO_DRIVE_DTSx_SHIFT)
>   #define  SDIO_DTSx_SET_TYPE_C	(2 << SDIO_DRIVE_DTSx_SHIFT)
>   #define  SDIO_DTSx_SET_TYPE_D	(3 << SDIO_DRIVE_DTSx_SHIFT)
> +
> +#define SDIO_CCCR_INTERRUPT_EXT	0x16
> +#define SDIO_INTERRUPT_EXT_SAI	(1 << 0)
> +#define SDIO_INTERRUPT_EXT_EAI	(1 << 1)
> +
>   /*
>    * Function Basic Registers (FBR)
>    */
> 



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v1 3/3] mmc: mediatek: add support for SDIO eint irq

[Axe Yang]

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 named state_dat1 and state_eint
are mandatory. And cap-sdio-async-int flag is necessary since this
feature depends on asynchronous interrupt:
	&mmcX {
		...
		pinctrl-names = "default", "state_uhs", "state_eint",
						"state_dat1";
		...
		pinctrl-2 = <&mmc2_pins_eint>;
		pinctrl-3 = <&mmc2_pins_dat1>;
		...
		cap-sdio-async-int;
		...
	};

Signed-off-by: Axe Yang <axe.yang@mediatek.com>
---
 drivers/mmc/host/mtk-sd.c | 113 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 4dfc246c5f95..8f23349f2963 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-2021 MediaTek Inc.
  * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
  */
 
@@ -432,9 +432,13 @@ struct msdc_host {
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pins_default;
 	struct pinctrl_state *pins_uhs;
+	struct pinctrl_state *pins_eint;
+	struct pinctrl_state *pins_dat1;
 	struct delayed_work req_timeout;
 	int irq;		/* host interrupt */
 	struct reset_control *reset;
+	int eint_irq;		/* device interrupt */
+	int sdio_irq_cnt;	/* irq enable cnt */
 
 	struct clk *src_clk;	/* msdc source clock */
 	struct clk *h_clk;      /* msdc h_clk */
@@ -1519,10 +1523,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->cccr.enable_async_int) {
+		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)
@@ -2380,6 +2386,49 @@ static const struct mmc_host_ops mt_msdc_ops = {
 	.hw_reset = msdc_hw_reset,
 };
 
+static irqreturn_t msdc_sdio_eint_irq(int irq, void *dev_id)
+{
+	unsigned long flags;
+	struct msdc_host *host = (struct msdc_host *)dev_id;
+	struct mmc_host *mmc = mmc_from_priv(host);
+
+	spin_lock_irqsave(&host->lock, flags);
+	if (likely(host->sdio_irq_cnt > 0)) {
+		disable_irq_nosync(host->eint_irq);
+		disable_irq_wake(host->eint_irq);
+		host->sdio_irq_cnt--;
+	}
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	sdio_signal_irq(mmc);
+
+	return IRQ_HANDLED;
+}
+
+static int msdc_request_dat1_eint_irq(struct msdc_host *host)
+{
+	struct gpio_desc *desc;
+	int ret = 0;
+	int irq;
+
+	desc = devm_gpiod_get_index(host->dev, "eint", 0, GPIOD_IN);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	irq = gpiod_to_irq(desc);
+	if (irq >= 0) {
+		irq_set_status_flags(irq, IRQ_NOAUTOEN);
+		ret = devm_request_threaded_irq(host->dev, irq, NULL, msdc_sdio_eint_irq,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"sdio-eint", host);
+	} else {
+		ret = irq;
+	}
+
+	host->eint_irq = irq;
+	return ret;
+}
+
 static const struct cqhci_host_ops msdc_cmdq_ops = {
 	.enable         = msdc_cqe_enable,
 	.disable        = msdc_cqe_disable,
@@ -2534,6 +2583,19 @@ static int msdc_drv_probe(struct platform_device *pdev)
 		goto host_free;
 	}
 
+	/* Support for SDIO eint irq */
+	host->pins_eint = pinctrl_lookup_state(host->pinctrl, "state_eint");
+	if (IS_ERR(host->pins_eint)) {
+		dev_dbg(&pdev->dev, "Cannot find pinctrl eint!\n");
+	} else {
+		host->pins_dat1 = pinctrl_lookup_state(host->pinctrl, "state_dat1");
+		if (IS_ERR(host->pins_dat1)) {
+			ret = PTR_ERR(host->pins_dat1);
+			dev_err(&pdev->dev, "Cannot find pinctrl dat1!\n");
+			goto host_free;
+		}
+	}
+
 	msdc_of_property_parse(pdev, host);
 
 	host->dev = &pdev->dev;
@@ -2621,6 +2683,16 @@ static int msdc_drv_probe(struct platform_device *pdev)
 	if (ret)
 		goto release;
 
+	if (!IS_ERR(host->pins_eint) && !IS_ERR(host->pins_dat1)) {
+		ret = msdc_request_dat1_eint_irq(host);
+		if (ret) {
+			dev_err(host->dev, "Failed to register data1 eint irq!\n");
+			goto release;
+		}
+
+		pinctrl_select_state(host->pinctrl, host->pins_dat1);
+	}
+
 	pm_runtime_set_active(host->dev);
 	pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
 	pm_runtime_use_autosuspend(host->dev);
@@ -2740,21 +2812,50 @@ static void msdc_restore_reg(struct msdc_host *host)
 
 static int __maybe_unused msdc_runtime_suspend(struct device *dev)
 {
+	unsigned long flags;
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct msdc_host *host = mmc_priv(mmc);
 
 	msdc_save_reg(host);
+
+	if (!IS_ERR(host->pins_eint)) {
+		disable_irq(host->irq);
+		pinctrl_select_state(host->pinctrl, host->pins_eint);
+		spin_lock_irqsave(&host->lock, flags);
+		if (host->sdio_irq_cnt == 0) {
+			enable_irq(host->eint_irq);
+			enable_irq_wake(host->eint_irq);
+			host->sdio_irq_cnt++;
+		}
+		sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
 	msdc_gate_clock(host);
 	return 0;
 }
 
 static int __maybe_unused msdc_runtime_resume(struct device *dev)
 {
+	unsigned long flags;
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct msdc_host *host = mmc_priv(mmc);
 
 	msdc_ungate_clock(host);
 	msdc_restore_reg(host);
+	if (!IS_ERR(host->pins_eint)) {
+		spin_lock_irqsave(&host->lock, flags);
+		if (host->sdio_irq_cnt > 0) {
+			disable_irq_nosync(host->eint_irq);
+			disable_irq_wake(host->eint_irq);
+			host->sdio_irq_cnt--;
+			sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+		} else {
+			sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
+		}
+		spin_unlock_irqrestore(&host->lock, flags);
+		pinctrl_select_state(host->pinctrl, host->pins_uhs);
+		enable_irq(host->irq);
+	}
 	return 0;
 }
 
@@ -2778,7 +2879,7 @@ static int __maybe_unused msdc_resume(struct device *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, msdc_resume)
 	SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume, NULL)
 };
 
-- 
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 3/3] mmc: mediatek: add support for SDIO eint irq

[Andy Shevchenko]

On Mon, Dec 27, 2021 at 6:46 PM Axe Yang <axe.yang@mediatek.com> wrote:

...

> +       if (mmc->card && !mmc->card->cccr.enable_async_int) {
> +               if (enb)

Spell it fully, i.e. enable.


> +                       pm_runtime_get_noresume(host->dev);
> +               else
> +                       pm_runtime_put_noidle(host->dev);
> +       }

...

> +       int ret = 0;

Redundant assignment, see below.

...

> +       desc = devm_gpiod_get_index(host->dev, "eint", 0, GPIOD_IN);

Why _index variant? By default devm_gpiod_get() uses 0 for index.

> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);

...

> +       irq = gpiod_to_irq(desc);

ret = ...
if (ret < 0)
  ...handle error...

> +       if (irq >= 0) {

(for the record, 0 is never returned by gpiod_to_irq() according to
all its versions).

> +               irq_set_status_flags(irq, IRQ_NOAUTOEN);

Use corresponding flag:
https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L83

> +               ret = devm_request_threaded_irq(host->dev, irq, NULL, msdc_sdio_eint_irq,
> +                                               IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +                                               "sdio-eint", host);
> +       } else {
> +               ret = irq;
> +       }
> +
> +       host->eint_irq = irq;

Is it okay if you assign garbage here in case of error?

> +       return ret;

...

> +       host->pins_eint = pinctrl_lookup_state(host->pinctrl, "state_eint");
> +       if (IS_ERR(host->pins_eint)) {
> +               dev_dbg(&pdev->dev, "Cannot find pinctrl eint!\n");
> +       } else {
> +               host->pins_dat1 = pinctrl_lookup_state(host->pinctrl, "state_dat1");
> +               if (IS_ERR(host->pins_dat1)) {

> +                       ret = PTR_ERR(host->pins_dat1);
> +                       dev_err(&pdev->dev, "Cannot find pinctrl dat1!\n");

ret = dev_err_probe(...); ?

> +                       goto host_free;
> +               }
> +       }

...

> +       if (!IS_ERR(host->pins_eint)) {

I'm wondering if you can use a pattern "error check first"?

> +               disable_irq(host->irq);
> +               pinctrl_select_state(host->pinctrl, host->pins_eint);
> +               spin_lock_irqsave(&host->lock, flags);
> +               if (host->sdio_irq_cnt == 0) {
> +                       enable_irq(host->eint_irq);
> +                       enable_irq_wake(host->eint_irq);
> +                       host->sdio_irq_cnt++;
> +               }
> +               sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +               spin_unlock_irqrestore(&host->lock, flags);
> +       }

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 3/3] mmc: mediatek: add support for SDIO eint irq

[Axe Yang]

On Mon, 2021-12-27 at 19:27 +0200, Andy Shevchenko wrote:
> On Mon, Dec 27, 2021 at 6:46 PM Axe Yang <axe.yang@mediatek.com>
> wrote:
> 
> ...
> 
> > +       if (mmc->card && !mmc->card->cccr.enable_async_int) {
> > +               if (enb)
> 
> Spell it fully, i.e. enable.

Will fix it in next version.

> 
> 
> > +                       pm_runtime_get_noresume(host->dev);
> > +               else
> > +                       pm_runtime_put_noidle(host->dev);
> > +       }
> 
> ...
> 
> > +       int ret = 0;
> 
> Redundant assignment, see below.

Will fix it in next version.

> 
> ...
> 
> > +       desc = devm_gpiod_get_index(host->dev, "eint", 0,
> > GPIOD_IN);
> 
> Why _index variant? By default devm_gpiod_get() uses 0 for index.

Will fix it in next version.

> 
> > +       if (IS_ERR(desc))
> > +               return PTR_ERR(desc);
> 
> ...
> 
> > +       irq = gpiod_to_irq(desc);
> 
> ret = ...
> if (ret < 0)
>   ...handle error...

Will fix it in next version.

> 
> > +       if (irq >= 0) {
> 
> (for the record, 0 is never returned by gpiod_to_irq() according to
> all its versions).

Will fix it in next version.

> 
> > +               irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
> Use corresponding flag:
> 
https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L83

I think IRQ_NOAUTOEN is the correct parameter I should use:

https://elixir.bootli.com/linux/latest/source/include/linux/irq.h#L800

IRQF_XXX defined in interrupt.h is only for xxx_request_xxx_irq().
Can you confirm that?

> 
> > +               ret = devm_request_threaded_irq(host->dev, irq,
> > NULL, msdc_sdio_eint_irq,
> > +                                               IRQF_TRIGGER_LOW |
> > IRQF_ONESHOT,
> > +                                               "sdio-eint", host);
> > +       } else {
> > +               ret = irq;
> > +       }
> > +
> > +       host->eint_irq = irq;
> 
> Is it okay if you assign garbage here in case of error?

Will refine this part in next version.

> 
> > +       return ret;
> 
> ...
> 
> > +       host->pins_eint = pinctrl_lookup_state(host->pinctrl,
> > "state_eint");
> > +       if (IS_ERR(host->pins_eint)) {
> > +               dev_dbg(&pdev->dev, "Cannot find pinctrl eint!\n");
> > +       } else {
> > +               host->pins_dat1 = pinctrl_lookup_state(host-
> > >pinctrl, "state_dat1");
> > +               if (IS_ERR(host->pins_dat1)) {
> > +                       ret = PTR_ERR(host->pins_dat1);
> > +                       dev_err(&pdev->dev, "Cannot find pinctrl
> > dat1!\n");
> 
> ret = dev_err_probe(...); ?

Will fix it in next version.

> 
> > +                       goto host_free;
> > +               }
> > +       }
> 
> ...
> 
> > +       if (!IS_ERR(host->pins_eint)) {
> 
> I'm wondering if you can use a pattern "error check first"?

The intention of this line is to determine whether current mmc device
is a SDIO card which supports eint, not for error check. But, since it
may bring ambiguity, I will implement it in another way. Thanks for
your advice.

...

--
Regards,
Axe Yang

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 3/3] mmc: mediatek: add support for SDIO eint irq

[AngeloGioacchino Del Regno]

Il 27/12/21 09:36, Axe Yang ha scritto:
> 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 named state_dat1 and state_eint
> are mandatory. And cap-sdio-async-int flag is necessary since this
> feature depends on asynchronous interrupt:
> 	&mmcX {
> 		...
> 		pinctrl-names = "default", "state_uhs", "state_eint",
> 						"state_dat1";
> 		...
> 		pinctrl-2 = <&mmc2_pins_eint>;
> 		pinctrl-3 = <&mmc2_pins_dat1>;
> 		...
> 		cap-sdio-async-int;
> 		...
> 	};
> 
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>

Hello Axe,
I agree on Andy's review, and I have some more comments...

> ---
>   drivers/mmc/host/mtk-sd.c | 113 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 107 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 4dfc246c5f95..8f23349f2963 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-2021 MediaTek Inc.
>    * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
>    */
>   
> @@ -432,9 +432,13 @@ struct msdc_host {
>   	struct pinctrl *pinctrl;
>   	struct pinctrl_state *pins_default;
>   	struct pinctrl_state *pins_uhs;
> +	struct pinctrl_state *pins_eint;
> +	struct pinctrl_state *pins_dat1;
>   	struct delayed_work req_timeout;
>   	int irq;		/* host interrupt */

int eint_irq; should be here, under the host interrupt, for the sake of perfection.

>   	struct reset_control *reset;
> +	int eint_irq;		/* device interrupt */
> +	int sdio_irq_cnt;	/* irq enable cnt */
>   
>   	struct clk *src_clk;	/* msdc source clock */
>   	struct clk *h_clk;      /* msdc h_clk */
> @@ -1519,10 +1523,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->cccr.enable_async_int) {
> +		if (enb)

If you are going to rename `enb` to `enable, this means that you're changing the
function signature... so, please, do that as a separated commit.

> +			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)
> @@ -2380,6 +2386,49 @@ static const struct mmc_host_ops mt_msdc_ops = {
>   	.hw_reset = msdc_hw_reset,
>   };
>   
> +static irqreturn_t msdc_sdio_eint_irq(int irq, void *dev_id)
> +{
> +	unsigned long flags;
> +	struct msdc_host *host = (struct msdc_host *)dev_id;
> +	struct mmc_host *mmc = mmc_from_priv(host);
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	if (likely(host->sdio_irq_cnt > 0)) {
> +		disable_irq_nosync(host->eint_irq);
> +		disable_irq_wake(host->eint_irq);
> +		host->sdio_irq_cnt--;
> +	}
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	sdio_signal_irq(mmc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int msdc_request_dat1_eint_irq(struct msdc_host *host)
> +{
> +	struct gpio_desc *desc;
> +	int ret = 0;
> +	int irq;
> +
> +	desc = devm_gpiod_get_index(host->dev, "eint", 0, GPIOD_IN);
> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);
> +
> +	irq = gpiod_to_irq(desc);
> +	if (irq >= 0) {
> +		irq_set_status_flags(irq, IRQ_NOAUTOEN);
> +		ret = devm_request_threaded_irq(host->dev, irq, NULL, msdc_sdio_eint_irq,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						"sdio-eint", host);

I think you may have misunderstood what Andy tried to say in his review: your call
to the function irq_set_status_flags() is using the right flag -- the suggestion
here is to add the IRQF_NO_AUTOEN flag to the call to devm_request_threaded_irq()
(ex: IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_NO_AUTOEN) and remove the now redundant
call to irq_set_status_flags().


> +	} else {
> +		ret = irq;
> +	}
> +
> +	host->eint_irq = irq;
> +	return ret;
> +}
> +
>   static const struct cqhci_host_ops msdc_cmdq_ops = {
>   	.enable         = msdc_cqe_enable,
>   	.disable        = msdc_cqe_disable,
> @@ -2534,6 +2583,19 @@ static int msdc_drv_probe(struct platform_device *pdev)
>   		goto host_free;
>   	}
>   
> +	/* Support for SDIO eint irq */
> +	host->pins_eint = pinctrl_lookup_state(host->pinctrl, "state_eint");
> +	if (IS_ERR(host->pins_eint)) {
> +		dev_dbg(&pdev->dev, "Cannot find pinctrl eint!\n");
> +	} else {
> +		host->pins_dat1 = pinctrl_lookup_state(host->pinctrl, "state_dat1");
> +		if (IS_ERR(host->pins_dat1)) {
> +			ret = PTR_ERR(host->pins_dat1);
> +			dev_err(&pdev->dev, "Cannot find pinctrl dat1!\n");
> +			goto host_free;
> +		}
> +	}
> +
>   	msdc_of_property_parse(pdev, host);
>   
>   	host->dev = &pdev->dev;
> @@ -2621,6 +2683,16 @@ static int msdc_drv_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto release;
>   
> +	if (!IS_ERR(host->pins_eint) && !IS_ERR(host->pins_dat1)) {
> +		ret = msdc_request_dat1_eint_irq(host);
> +		if (ret) {
> +			dev_err(host->dev, "Failed to register data1 eint irq!\n");
> +			goto release;
> +		}
> +
> +		pinctrl_select_state(host->pinctrl, host->pins_dat1);
> +	}
> +
>   	pm_runtime_set_active(host->dev);
>   	pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
>   	pm_runtime_use_autosuspend(host->dev);
> @@ -2740,21 +2812,50 @@ static void msdc_restore_reg(struct msdc_host *host)
>   
>   static int __maybe_unused msdc_runtime_suspend(struct device *dev)
>   {
> +	unsigned long flags;
>   	struct mmc_host *mmc = dev_get_drvdata(dev);
>   	struct msdc_host *host = mmc_priv(mmc);
>   
>   	msdc_save_reg(host);
> +
> +	if (!IS_ERR(host->pins_eint)) {
> +		disable_irq(host->irq);
> +		pinctrl_select_state(host->pinctrl, host->pins_eint);
> +		spin_lock_irqsave(&host->lock, flags);
> +		if (host->sdio_irq_cnt == 0) {
> +			enable_irq(host->eint_irq);
> +			enable_irq_wake(host->eint_irq);
> +			host->sdio_irq_cnt++;
> +		}
> +		sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +		spin_unlock_irqrestore(&host->lock, flags);
> +	}
>   	msdc_gate_clock(host);
>   	return 0;
>   }
>   
>   static int __maybe_unused msdc_runtime_resume(struct device *dev)
>   {
> +	unsigned long flags;
>   	struct mmc_host *mmc = dev_get_drvdata(dev);
>   	struct msdc_host *host = mmc_priv(mmc);
>   
>   	msdc_ungate_clock(host);
>   	msdc_restore_reg(host);
> +	if (!IS_ERR(host->pins_eint)) {
> +		spin_lock_irqsave(&host->lock, flags);
> +		if (host->sdio_irq_cnt > 0) {
> +			disable_irq_nosync(host->eint_irq);
> +			disable_irq_wake(host->eint_irq);
> +			host->sdio_irq_cnt--;
> +			sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +		} else {
> +			sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> +		}
> +		spin_unlock_irqrestore(&host->lock, flags);
> +		pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +		enable_irq(host->irq);
> +	}
>   	return 0;
>   }
>   
> @@ -2778,7 +2879,7 @@ static int __maybe_unused msdc_resume(struct device *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, msdc_resume)

Just a nitpick: msdc_{suspend,resume} are now noirq, so you should change the
function names to msdc_suspend_noirq, msdc_resume_noirq.

>   	SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume, NULL)
>   };
>   
> 

Regards,
- Angelo


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 3/3] mmc: mediatek: add support for SDIO eint irq

[Axe Yang]

On Tue, 2022-01-04 at 11:44 +0100, AngeloGioacchino Del Regno wrote:
> Il 27/12/21 09:36, Axe Yang ha scritto:
> > 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 named state_dat1 and state_eint
> > are mandatory. And cap-sdio-async-int flag is necessary since this
> > feature depends on asynchronous interrupt:
> > 	&mmcX {
> > 		...
> > 		pinctrl-names = "default", "state_uhs", "state_eint",
> > 						"state_dat1";
> > 		...
> > 		pinctrl-2 = <&mmc2_pins_eint>;
> > 		pinctrl-3 = <&mmc2_pins_dat1>;
> > 		...
> > 		cap-sdio-async-int;
> > 		...
> > 	};
> > 
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> 
> Hello Axe,
> I agree on Andy's review, and I have some more comments...
> 
> > ---
> >   drivers/mmc/host/mtk-sd.c | 113
> > ++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 107 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 4dfc246c5f95..8f23349f2963 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-2021 MediaTek Inc.
> >    * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
> >    */
> >   
> > @@ -432,9 +432,13 @@ struct msdc_host {
> >   	struct pinctrl *pinctrl;
> >   	struct pinctrl_state *pins_default;
> >   	struct pinctrl_state *pins_uhs;
> > +	struct pinctrl_state *pins_eint;
> > +	struct pinctrl_state *pins_dat1;
> >   	struct delayed_work req_timeout;
> >   	int irq;		/* host interrupt */
> 
> int eint_irq; should be here, under the host interrupt, for the sake
> of perfection.

Will fix it in next version.

> 
> >   	struct reset_control *reset;
> > +	int eint_irq;		/* device interrupt */
> > +	int sdio_irq_cnt;	/* irq enable cnt */
> >   
> >   	struct clk *src_clk;	/* msdc source clock */
> >   	struct clk *h_clk;      /* msdc h_clk */
> > @@ -1519,10 +1523,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->cccr.enable_async_int) {
> > +		if (enb)
> 
> If you are going to rename `enb` to `enable, this means that you're
> changing the
> function signature... so, please, do that as a separated commit.

Okay, will keep 'enb' in this commit.

> 
> > +			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)
> > @@ -2380,6 +2386,49 @@ static const struct mmc_host_ops mt_msdc_ops
> > = {
> >   	.hw_reset = msdc_hw_reset,
> >   };
> >   
> > +static irqreturn_t msdc_sdio_eint_irq(int irq, void *dev_id)
> > +{
> > +	unsigned long flags;
> > +	struct msdc_host *host = (struct msdc_host *)dev_id;
> > +	struct mmc_host *mmc = mmc_from_priv(host);
> > +
> > +	spin_lock_irqsave(&host->lock, flags);
> > +	if (likely(host->sdio_irq_cnt > 0)) {
> > +		disable_irq_nosync(host->eint_irq);
> > +		disable_irq_wake(host->eint_irq);
> > +		host->sdio_irq_cnt--;
> > +	}
> > +	spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +	sdio_signal_irq(mmc);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int msdc_request_dat1_eint_irq(struct msdc_host *host)
> > +{
> > +	struct gpio_desc *desc;
> > +	int ret = 0;
> > +	int irq;
> > +
> > +	desc = devm_gpiod_get_index(host->dev, "eint", 0, GPIOD_IN);
> > +	if (IS_ERR(desc))
> > +		return PTR_ERR(desc);
> > +
> > +	irq = gpiod_to_irq(desc);
> > +	if (irq >= 0) {
> > +		irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > +		ret = devm_request_threaded_irq(host->dev, irq, NULL,
> > msdc_sdio_eint_irq,
> > +						IRQF_TRIGGER_LOW |
> > IRQF_ONESHOT,
> > +						"sdio-eint", host);
> 
> I think you may have misunderstood what Andy tried to say in his
> review: your call
> to the function irq_set_status_flags() is using the right flag -- the
> suggestion
> here is to add the IRQF_NO_AUTOEN flag to the call to
> devm_request_threaded_irq()
> (ex: IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_NO_AUTOEN) and remove the
> now redundant
> call to irq_set_status_flags().

Thank you for explanation. Will fix it in next version.

> 
> 
> > +	} else {
> > +		ret = irq;
> > +	}
> > +
> > +	host->eint_irq = irq;
> > +	return ret;
> > +}
> > +
> >   static const struct cqhci_host_ops msdc_cmdq_ops = {
> >   	.enable         = msdc_cqe_enable,
> >   	.disable        = msdc_cqe_disable,
> > @@ -2534,6 +2583,19 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >   		goto host_free;
> >   	}
> >   
> > +	/* Support for SDIO eint irq */
> > +	host->pins_eint = pinctrl_lookup_state(host->pinctrl,
> > "state_eint");
> > +	if (IS_ERR(host->pins_eint)) {
> > +		dev_dbg(&pdev->dev, "Cannot find pinctrl eint!\n");
> > +	} else {
> > +		host->pins_dat1 = pinctrl_lookup_state(host->pinctrl,
> > "state_dat1");
> > +		if (IS_ERR(host->pins_dat1)) {
> > +			ret = PTR_ERR(host->pins_dat1);
> > +			dev_err(&pdev->dev, "Cannot find pinctrl
> > dat1!\n");
> > +			goto host_free;
> > +		}
> > +	}
> > +
> >   	msdc_of_property_parse(pdev, host);
> >   
> >   	host->dev = &pdev->dev;
> > @@ -2621,6 +2683,16 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >   	if (ret)
> >   		goto release;
> >   
> > +	if (!IS_ERR(host->pins_eint) && !IS_ERR(host->pins_dat1)) {
> > +		ret = msdc_request_dat1_eint_irq(host);
> > +		if (ret) {
> > +			dev_err(host->dev, "Failed to register data1
> > eint irq!\n");
> > +			goto release;
> > +		}
> > +
> > +		pinctrl_select_state(host->pinctrl, host->pins_dat1);
> > +	}
> > +
> >   	pm_runtime_set_active(host->dev);
> >   	pm_runtime_set_autosuspend_delay(host->dev,
> > MTK_MMC_AUTOSUSPEND_DELAY);
> >   	pm_runtime_use_autosuspend(host->dev);
> > @@ -2740,21 +2812,50 @@ static void msdc_restore_reg(struct
> > msdc_host *host)
> >   
> >   static int __maybe_unused msdc_runtime_suspend(struct device
> > *dev)
> >   {
> > +	unsigned long flags;
> >   	struct mmc_host *mmc = dev_get_drvdata(dev);
> >   	struct msdc_host *host = mmc_priv(mmc);
> >   
> >   	msdc_save_reg(host);
> > +
> > +	if (!IS_ERR(host->pins_eint)) {
> > +		disable_irq(host->irq);
> > +		pinctrl_select_state(host->pinctrl, host->pins_eint);
> > +		spin_lock_irqsave(&host->lock, flags);
> > +		if (host->sdio_irq_cnt == 0) {
> > +			enable_irq(host->eint_irq);
> > +			enable_irq_wake(host->eint_irq);
> > +			host->sdio_irq_cnt++;
> > +		}
> > +		sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> > +		spin_unlock_irqrestore(&host->lock, flags);
> > +	}
> >   	msdc_gate_clock(host);
> >   	return 0;
> >   }
> >   
> >   static int __maybe_unused msdc_runtime_resume(struct device *dev)
> >   {
> > +	unsigned long flags;
> >   	struct mmc_host *mmc = dev_get_drvdata(dev);
> >   	struct msdc_host *host = mmc_priv(mmc);
> >   
> >   	msdc_ungate_clock(host);
> >   	msdc_restore_reg(host);
> > +	if (!IS_ERR(host->pins_eint)) {
> > +		spin_lock_irqsave(&host->lock, flags);
> > +		if (host->sdio_irq_cnt > 0) {
> > +			disable_irq_nosync(host->eint_irq);
> > +			disable_irq_wake(host->eint_irq);
> > +			host->sdio_irq_cnt--;
> > +			sdr_set_bits(host->base + SDC_CFG,
> > SDC_CFG_SDIOIDE);
> > +		} else {
> > +			sdr_clr_bits(host->base + MSDC_INTEN,
> > MSDC_INTEN_SDIOIRQ);
> > +		}
> > +		spin_unlock_irqrestore(&host->lock, flags);
> > +		pinctrl_select_state(host->pinctrl, host->pins_uhs);
> > +		enable_irq(host->irq);
> > +	}
> >   	return 0;
> >   }
> >   
> > @@ -2778,7 +2879,7 @@ static int __maybe_unused msdc_resume(struct
> > device *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, msdc_resume)
> 
> Just a nitpick: msdc_{suspend,resume} are now noirq, so you should
> change the
> function names to msdc_suspend_noirq, msdc_resume_noirq.

Will fix it in next version. Thank you for your suggestion.

> 
> >   	SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume,
> > NULL)
> >   };
> >   
> > 
> 
> Regards,
> - Angelo
> 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek