omap_hsmmc: SDIO IRQ on AM335x family

omap_hsmmc: SDIO IRQ on AM335x family

[Andreas Fenkart]

Hi,

I submitted following patches a while back,
http://www.spinics.net/lists/linux-mmc/msg17624.html. Since
there was no feedback I'm taking a step back, documenting:

1. why is it needed, missing swakeup line
2. transition, enable workaround by default
3. device tree configuration


[why is it needed, problem outline]

The omap_hsmmc module is suspended whenever it is idle, its
functional clock being turned off. In this mode it is not able to
forwared IRQs to the system. For that to happen, it needs to tell
the PRCM to restore it's fclk.

                   ------
                  | PRCM |
                   ------
                    | ^
               fclk | | swakeup
                    v |
                  -------               ------
      <-- IRQ -- | hsmmc | <-- CIRQ -- | card |
                  -------               ------

This is done through the swakeup line, which can be configured to
trigger for various events, among others; CIRQ. The problem is
that on the AM335x family the swakeup line is missing, it has not
been routed from the module to the PRCM.

https://www.google.com/search?q=spruh73c
see 18.2 compare with 20.2.2, or just search the whole doc for
swakeup

One solution is to keep the fclk enabled while waiting
for CIRQ. This is the approach taken by existing patches:

http://www.spinics.net/lists/linux-omap/msg63363.html
http://marc.info/?l=linux-mmc&m=126580279403824&w=2
http://thread.gmane.org/gmane.linux.kernel.mmc/1107/focus=1109

authors, contributors
David Vrabel (based my work on it)
John Rigby

The alternative was to configure dat1 line as a GPIO, while
waiting for an IRQ. Then configuring it back as dat1 when the
SDIO card is signalling an IRQ. Or the host starts a transfer. I
guess this will perform poorly, hence not considering it really.

[transition, enable workaround by default?]

Up to now the driver uses polling, which works for all SOCs.
Once we enable the IRQ, all chips missing an swakeup will fail.
But I don't know all of them, so how to transition without
breaking existing platforms?

Enable IRQ only for AM335x, use polling for others?
Keep clock running for all, only disable workaround for known
good platforms?

[device tree configuration]

My previous patch, see link at top, uses a new property
'ti,swakeup-not-implemented'. Then in the DT files I enabled the
workaround for my platform.

Now I learned that I should rather use a new 'compatible'
section. Actually Daniel talked me out of it:
'Since this feature should always be enabled on one given
processor, the IP block is actually "compatible" to something
else than a processor that doesn't support that feature. So the
"compatible" line alone should be enough of an information for
the one who provides the DT. On the other hand, if enabling a
certain option depends on the system design of an application, it
deserves a special binding in DT.'

I'd prefer the latter now.


Thanks for your attention,
/Andi

Re: omap_hsmmc: SDIO IRQ on AM335x family

[Tony Lindgren]

* Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121130 03:21]:
> 
> The alternative was to configure dat1 line as a GPIO, while
> waiting for an IRQ. Then configuring it back as dat1 when the
> SDIO card is signalling an IRQ. Or the host starts a transfer. I
> guess this will perform poorly, hence not considering it really.

This might work for SDIO cards. It should be disabled for data
cards naturally to avoid potential data corruption.

The way to implement this is set named states in the .dts file
for the pins using pinctrl-single.c, then have the MMC driver
request states "default" "active" and "idle" during the probe,
then toggle between active and idle during the runtime.

As far as I remember the GPIO functionality does not need to
be enabled, just muxing the pin to GPIO mode for the wake-up
is enough.

Regards,

Tony

Re: omap_hsmmc: SDIO IRQ on AM335x family

[Daniel Mack]

Hi Tony,
Hi Andreas,

On 30.11.2012 18:40, Tony Lindgren wrote:
> * Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121130 03:21]:
>>
>> The alternative was to configure dat1 line as a GPIO, while
>> waiting for an IRQ. Then configuring it back as dat1 when the
>> SDIO card is signalling an IRQ. Or the host starts a transfer. I
>> guess this will perform poorly, hence not considering it really.
> 
> This might work for SDIO cards. It should be disabled for data
> cards naturally to avoid potential data corruption.
> 
> The way to implement this is set named states in the .dts file
> for the pins using pinctrl-single.c, then have the MMC driver
> request states "default" "active" and "idle" during the probe,
> then toggle between active and idle during the runtime.
> 
> As far as I remember the GPIO functionality does not need to
> be enabled, just muxing the pin to GPIO mode for the wake-up
> is enough.

Wouldn't that be racy, given that an interrupt which occurs at beween
the point in time when the driver decides to wait for IRQs again until
the mux has finished switching over, could potentially be lost?

If there is another way to solve that cleanly on at least some hardware
derivats, I'd say that should be preferred and be supported by the driver.


Daniel

Re: omap_hsmmc: SDIO IRQ on AM335x family

[Andreas Fenkart]

Hi,

On Fri, Nov 30, 2012 at 07:57:35PM +0100, Daniel Mack wrote:
> 
> On 30.11.2012 18:40, Tony Lindgren wrote:
> > * Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121130 03:21]:
> >>
> >> The alternative was to configure dat1 line as a GPIO, while
> >> waiting for an IRQ. Then configuring it back as dat1 when the
> >> SDIO card is signalling an IRQ. Or the host starts a transfer. I
> >> guess this will perform poorly, hence not considering it really.
> > 
> > This might work for SDIO cards. It should be disabled for data
> > cards naturally to avoid potential data corruption.
I don't understand your concern here, could you explain

> > The way to implement this is set named states in the .dts file
> > for the pins using pinctrl-single.c, then have the MMC driver
> > request states "default" "active" and "idle" during the probe,
> > then toggle between active and idle during the runtime.
> > 
> > As far as I remember the GPIO functionality does not need to
> > be enabled, just muxing the pin to GPIO mode for the wake-up
> > is enough.
> 
> Wouldn't that be racy, given that an interrupt which occurs at beween
> the point in time when the driver decides to wait for IRQs again until
> the mux has finished switching over, could potentially be lost?

The IRQ is level triggered, so can't be lost. I implemented it as
suggested and surprisingly performance is pretty good. Actually not
worse than keeping the fclk enabled all times.

module: 88W8787 / mwifiex
tx bitrate: 150.0 MBit/s MCS 7 40Mhz short GI

                   |   tcp tx         |  signal  | cpu idle
---------------------------------------------------------------
keep fclk enabled  |   50.3 Mbits/sec | -23 dBm  | 15 %
suspend/resume     |   49.7 Mbits/sec | -22 dBM  | 13 %

patch follows

/Andi

[PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Andreas Fenkart]

Without functional clock the omap_hsmmc module can't forward
SDIO IRQs to the system. This patch reconfigures dat1 line
as a gpio while the fclk is off. And uses SDIO IRQ detection of
the module, while fclk is present.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 .../devicetree/bindings/mmc/ti-omap-hsmmc.txt      |   42 ++++
 arch/arm/plat-omap/include/plat/mmc.h              |    4 +
 drivers/mmc/host/omap_hsmmc.c                      |  219 ++++++++++++++++++--
 3 files changed, 247 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index d1b8932..4d57637 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -24,6 +24,29 @@ One tx and one rx pair is required.
 dma-names: DMA request names. These strings correspond 1:1 with
 the ordered pairs in dmas. The RX request must be "rx" and the
 TX request must be "tx".
+ti,cirq-gpio: When omap_hsmmc module is suspended, its functional
+clock is turned off. Without fclk it can't forward SDIO IRQs to the
+system. For that to happen, it needs to tell the PRCM to restore
+its fclk, which is done through the swakeup line.
+
+                   ------
+                  | PRCM |
+                   ------
+                    | ^
+               fclk | | swakeup
+                    v |
+                  -------               ------
+      <-- IRQ -- | hsmmc | <-- CIRQ -- | card |
+                  -------               ------
+
+The problem is, that on the AM335x family the swakeup line is
+missing, it has not been routed from the module to the PRCM.
+The way to work around this, is to reconfigure the dat1 line as a
+GPIO upon suspend. Beyond this option you also need to set named
+states "default" and "idle "in the .dts file for the pins, using
+pinctrl-single.c. The MMC driver will then then toggle between
+default and idle during the runtime.
+
 
 Examples:
 
@@ -53,3 +76,22 @@ Examples:
 			&edma 25>;
 		dma-names = "tx", "rx";
 	};
+
+[am335x with with gpio for sdio irq]
+
+	mmc1_cirq_pin: pinmux_cirq_pin {
+		pinctrl-single,pins = <
+			0x0f8 0x3f	/* MMC0_DAT1 as GPIO2_28 */
+		>;
+	};
+
+	mmc1: mmc@48060000 {
+		pinctrl-names = "default", "idle";
+		pinctrl-0 = <&mmc1_pins>;
+		pinctrl-1 = <&mmc1_cirq_pin>;
+		ti,cirq-gpio = <&gpio3 28 0>;
+		ti,non-removable;
+		bus-width = <4>;
+		vmmc-supply = <&ldo2_reg>;
+		vmmc_aux-supply = <&vmmc>;
+	};
diff --git a/arch/arm/plat-omap/include/plat/mmc.h b/arch/arm/plat-omap/include/plat/mmc.h
index 8b4e4f2..807676a 100644
--- a/arch/arm/plat-omap/include/plat/mmc.h
+++ b/arch/arm/plat-omap/include/plat/mmc.h
@@ -130,6 +130,7 @@ struct omap_mmc_platform_data {
 
 		int switch_pin;			/* gpio (card detect) */
 		int gpio_wp;			/* gpio (write protect) */
+		int gpio_cirq;			/* gpio (card irq) */
 
 		int (*set_bus_mode)(struct device *dev, int slot, int bus_mode);
 		int (*set_power)(struct device *dev, int slot,
@@ -160,6 +161,9 @@ struct omap_mmc_platform_data {
 		int card_detect_irq;
 		int (*card_detect)(struct device *dev, int slot);
 
+		/* SDIO IRQs */
+		int sdio_irq;
+
 		unsigned int ban_openended:1;
 
 	} slots[OMAP_MMC_MAX_SLOTS];
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 53703c6..1a48cb1 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -108,6 +108,7 @@ static void apply_clk_hack(void)
 #define INT_EN_MASK		0x307F0033
 #define BWR_ENABLE		(1 << 4)
 #define BRR_ENABLE		(1 << 5)
+#define CIRQ_ENABLE		(1 << 8)
 #define DTO_ENABLE		(1 << 20)
 #define INIT_STREAM		(1 << 1)
 #define DP_SELECT		(1 << 21)
@@ -121,6 +122,7 @@ static void apply_clk_hack(void)
 #define CC			0x1
 #define TC			0x02
 #define OD			0x1
+#define CIRQ			(1 << 8)
 #define ERR			(1 << 15)
 #define CMD_TIMEOUT		(1 << 16)
 #define DATA_TIMEOUT		(1 << 20)
@@ -198,11 +200,25 @@ struct omap_hsmmc_host {
 	int			reqs_blocked;
 	int			use_reg;
 	int			req_in_progress;
+	bool			active_pinmux;
+	bool			sdio_irq_en;
 	struct omap_hsmmc_next	next_data;
 
 	struct	omap_mmc_platform_data	*pdata;
+
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *active, *idle;
+
 };
 
+static irqreturn_t omap_hsmmc_cirq(int irq, void *dev_id)
+{
+	struct omap_hsmmc_host *host = dev_id;
+
+	mmc_signal_sdio_irq(host->mmc);
+	return IRQ_HANDLED;
+}
+
 static int omap_hsmmc_card_detect(struct device *dev, int slot)
 {
 	struct omap_hsmmc_host *host = dev_get_drvdata(dev);
@@ -437,10 +453,30 @@ static int omap_hsmmc_gpio_init(struct omap_mmc_platform_data *pdata)
 	} else
 		pdata->slots[0].gpio_wp = -EINVAL;
 
+	if (gpio_is_valid(pdata->slots[0].gpio_cirq)) {
+		pdata->slots[0].sdio_irq =
+				gpio_to_irq(pdata->slots[0].gpio_cirq);
+
+		ret = gpio_request(pdata->slots[0].gpio_cirq, "sdio_cirq");
+		if (ret)
+			goto err_free_ro;
+		ret = gpio_direction_input(pdata->slots[0].gpio_cirq);
+		if (ret)
+			goto err_free_cirq;
+
+	} else {
+		pdata->slots[0].gpio_cirq = -EINVAL;
+	}
+
+
 	return 0;
 
+err_free_cirq:
+	gpio_free(pdata->slots[0].gpio_cirq);
+err_free_ro:
+	if (gpio_is_valid(pdata->slots[0].gpio_wp))
 err_free_wp:
-	gpio_free(pdata->slots[0].gpio_wp);
+		gpio_free(pdata->slots[0].gpio_wp);
 err_free_cd:
 	if (gpio_is_valid(pdata->slots[0].switch_pin))
 err_free_sp:
@@ -454,6 +490,8 @@ static void omap_hsmmc_gpio_free(struct omap_mmc_platform_data *pdata)
 		gpio_free(pdata->slots[0].gpio_wp);
 	if (gpio_is_valid(pdata->slots[0].switch_pin))
 		gpio_free(pdata->slots[0].switch_pin);
+	if (gpio_is_valid(pdata->slots[0].gpio_cirq))
+		gpio_free(pdata->slots[0].gpio_cirq);
 }
 
 /*
@@ -479,27 +517,46 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host)
 static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
 				  struct mmc_command *cmd)
 {
-	unsigned int irq_mask;
+	u32 irq_mask = INT_EN_MASK;
+	unsigned long flags;
 
 	if (host->use_dma)
-		irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
-	else
-		irq_mask = INT_EN_MASK;
+		irq_mask &= ~(BRR_ENABLE | BWR_ENABLE);
 
 	/* Disable timeout for erases */
 	if (cmd->opcode == MMC_ERASE)
 		irq_mask &= ~DTO_ENABLE;
 
+	spin_lock_irqsave(&host->irq_lock, flags);
+
 	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
 	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
+
+	/* latch pending CIRQ, but don't signal */
+	if (host->sdio_irq_en)
+		irq_mask |= CIRQ_ENABLE;
+
 	OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
+
+	spin_unlock_irqrestore(&host->irq_lock, flags);
 }
 
 static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host)
 {
-	OMAP_HSMMC_WRITE(host->base, ISE, 0);
-	OMAP_HSMMC_WRITE(host->base, IE, 0);
+	u32 irq_mask = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->irq_lock, flags);
+
+	/* no transfer running, need to signal cirq */
+	if (host->sdio_irq_en && host->active_pinmux)
+		irq_mask |= CIRQ_ENABLE;
+
+	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
+	OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
 	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
+
+	spin_unlock_irqrestore(&host->irq_lock, flags);
 }
 
 /* Calculate divisor for the given clock frequency */
@@ -1044,8 +1101,13 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
 	int status;
 
 	status = OMAP_HSMMC_READ(host->base, STAT);
-	while (status & INT_EN_MASK && host->req_in_progress) {
-		omap_hsmmc_do_irq(host, status);
+	while (status & (INT_EN_MASK | CIRQ)) {
+
+		if (host->req_in_progress)
+			omap_hsmmc_do_irq(host, status);
+
+		if (status & CIRQ)
+			mmc_signal_sdio_irq(host->mmc);
 
 		/* Flush posted write */
 		OMAP_HSMMC_WRITE(host->base, STAT, status);
@@ -1561,6 +1623,44 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card)
 		mmc_slot(host).init_card(card);
 }
 
+static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+	struct omap_hsmmc_host *host = mmc_priv(mmc);
+	u32 irq_mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->irq_lock, flags);
+
+	host->sdio_irq_en = (enable != 0) ? true : false;
+
+	if (host->active_pinmux) {
+		irq_mask = OMAP_HSMMC_READ(host->base, ISE);
+		if (enable)
+			irq_mask |= CIRQ_ENABLE;
+		else
+			irq_mask &= ~CIRQ_ENABLE;
+		OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
+
+		if (!host->req_in_progress)
+			OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
+
+#if 0
+		OMAP_HSMMC_READ(host->base, IE); /* flush posted write */
+#endif
+	}
+
+	if ((mmc_slot(host).sdio_irq)) {
+		/* enable/disable-irq uses depth counter, this allows
+		 * it to drop to zero in runtime suspend */
+		if (enable)
+			enable_irq(mmc_slot(host).sdio_irq);
+		else
+			disable_irq_nosync(mmc_slot(host).sdio_irq);
+	}
+
+	spin_unlock_irqrestore(&host->irq_lock, flags);
+}
+
 static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 {
 	u32 hctl, capa, value;
@@ -1613,7 +1713,7 @@ static const struct mmc_host_ops omap_hsmmc_ops = {
 	.get_cd = omap_hsmmc_get_cd,
 	.get_ro = omap_hsmmc_get_ro,
 	.init_card = omap_hsmmc_init_card,
-	/* NYET -- enable_sdio_irq */
+	.enable_sdio_irq = omap_hsmmc_enable_sdio_irq,
 };
 
 #ifdef CONFIG_DEBUG_FS
@@ -1729,6 +1829,7 @@ static struct omap_mmc_platform_data *of_get_hsmmc_pdata(struct device *dev)
 	pdata->nr_slots = 1;
 	pdata->slots[0].switch_pin = of_get_named_gpio(np, "cd-gpios", 0);
 	pdata->slots[0].gpio_wp = of_get_named_gpio(np, "wp-gpios", 0);
+	pdata->slots[0].gpio_cirq = of_get_named_gpio(np, "ti,cirq-gpio", 0);
 
 	if (of_find_property(np, "ti,non-removable", NULL)) {
 		pdata->slots[0].nonremovable = true;
@@ -1766,7 +1867,6 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	dma_cap_mask_t mask;
 	unsigned tx_req, rx_req;
-	struct pinctrl *pinctrl;
 
 	apply_clk_hack();
 
@@ -1818,6 +1918,8 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 	host->dma_ch	= -1;
 	host->irq	= irq;
 	host->slot_id	= 0;
+	host->sdio_irq_en = false;
+	host->active_pinmux = true;
 	host->mapbase	= res->start + pdata->reg_offset;
 	host->base	= ioremap(host->mapbase, SZ_4K);
 	host->power_mode = MMC_POWER_OFF;
@@ -1988,12 +2090,51 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 		pdata->resume = omap_hsmmc_resume_cdirq;
 	}
 
+	if ((mmc_slot(host).sdio_irq)) {
+		ret = request_irq(mmc_slot(host).sdio_irq, omap_hsmmc_cirq,
+				  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+				  mmc_hostname(mmc), host);
+		if (ret) {
+			dev_dbg(mmc_dev(host->mmc),
+				"Unable to grab MMC SDIO IRQ\n");
+			goto err_irq_sdio;
+		}
+		disable_irq(mmc_slot(host).sdio_irq); /* active pinmux */
+		disable_irq(mmc_slot(host).sdio_irq); /* SDIO IRQ disabled */
+	}
+
 	omap_hsmmc_disable_irq(host);
 
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev,
-			"pins are not configured from the driver\n");
+	host->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(host->pinctrl)) {
+		ret = PTR_ERR(host->pinctrl);
+		goto err_pinctrl;
+	}
+
+	host->active = pinctrl_lookup_state(host->pinctrl,
+					    PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(host->active)) {
+		dev_warn(mmc_dev(host->mmc), "Unable to lookup active pinmux\n");
+		ret = PTR_ERR(host->active);
+		goto err_pinctrl_state;
+	}
+
+	if (mmc_slot(host).sdio_irq) {
+		host->idle = pinctrl_lookup_state(host->pinctrl,
+						  PINCTRL_STATE_IDLE);
+		if (IS_ERR(host->idle)) {
+			dev_warn(mmc_dev(host->mmc), "Unable to lookup idle pinmux\n");
+			ret = PTR_ERR(host->idle);
+			goto err_pinctrl_state;
+		}
+		mmc->caps |= MMC_CAP_SDIO_IRQ;
+	}
+
+	ret = pinctrl_select_state(host->pinctrl, host->active);
+	if (ret < 0) {
+		dev_warn(mmc_dev(host->mmc), "Unable to select idle pinmux\n");
+		goto err_pinctrl_state;
+	}
 
 	omap_hsmmc_protect_card(host);
 
@@ -2019,6 +2160,12 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 
 err_slot_name:
 	mmc_remove_host(mmc);
+err_pinctrl_state:
+	devm_pinctrl_put(host->pinctrl);
+err_pinctrl:
+	if ((mmc_slot(host).sdio_irq))
+		free_irq(mmc_slot(host).sdio_irq, host);
+err_irq_sdio:
 	free_irq(mmc_slot(host).card_detect_irq, host);
 err_irq_cd:
 	if (host->use_reg)
@@ -2065,13 +2212,15 @@ static int __devexit omap_hsmmc_remove(struct platform_device *pdev)
 	if (host->pdata->cleanup)
 		host->pdata->cleanup(&pdev->dev);
 	free_irq(host->irq, host);
+	if ((mmc_slot(host).sdio_irq))
+		free_irq(mmc_slot(host).sdio_irq, host);
 	if (mmc_slot(host).card_detect_irq)
 		free_irq(mmc_slot(host).card_detect_irq, host);
-
 	if (host->tx_chan)
 		dma_release_channel(host->tx_chan);
 	if (host->rx_chan)
 		dma_release_channel(host->rx_chan);
+	devm_pinctrl_put(host->pinctrl);
 
 	pm_runtime_put_sync(host->dev);
 	pm_runtime_disable(host->dev);
@@ -2188,23 +2337,57 @@ static int omap_hsmmc_resume(struct device *dev)
 static int omap_hsmmc_runtime_suspend(struct device *dev)
 {
 	struct omap_hsmmc_host *host;
+	unsigned long flags;
+	int ret = 0;
 
 	host = platform_get_drvdata(to_platform_device(dev));
 	omap_hsmmc_context_save(host);
 	dev_dbg(dev, "disabled\n");
 
-	return 0;
+	if (mmc_slot(host).sdio_irq && host->pinctrl) {
+
+		spin_lock_irqsave(&host->irq_lock, flags);
+		host->active_pinmux = false;
+		spin_unlock_irqrestore(&host->irq_lock, flags);
+
+		omap_hsmmc_disable_irq(host);
+
+		ret = pinctrl_select_state(host->pinctrl, host->idle);
+		if (ret < 0) {
+			dev_warn(mmc_dev(host->mmc), "Unable to select idle pinmux\n");
+			return ret;
+		}
+
+		enable_irq(mmc_slot(host).sdio_irq);
+	}
+
+	return ret;
 }
 
 static int omap_hsmmc_runtime_resume(struct device *dev)
 {
 	struct omap_hsmmc_host *host;
+	unsigned long flags;
+	int ret = 0;
 
 	host = platform_get_drvdata(to_platform_device(dev));
 	omap_hsmmc_context_restore(host);
 	dev_dbg(dev, "enabled\n");
 
-	return 0;
+	if (mmc_slot(host).sdio_irq && host->pinctrl) {
+		spin_lock_irqsave(&host->irq_lock, flags);
+		host->active_pinmux = true;
+		spin_unlock_irqrestore(&host->irq_lock, flags);
+
+		disable_irq_nosync(mmc_slot(host).sdio_irq);
+
+		ret = pinctrl_select_state(host->pinctrl, host->active);
+		if (ret < 0) {
+			dev_warn(mmc_dev(host->mmc), "Unable to select active pinmux\n");
+			return ret;
+		}
+	}
+	return ret;
 }
 
 static struct dev_pm_ops omap_hsmmc_dev_pm_ops = {
-- 
1.7.10.4

Re: [PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Tony Lindgren]

* Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121220 14:15]:
> Without functional clock the omap_hsmmc module can't forward
> SDIO IRQs to the system. This patch reconfigures dat1 line
> as a gpio while the fclk is off. And uses SDIO IRQ detection of
> the module, while fclk is present.

Looks pretty good to me, however I could not figure out what
to apply this on for testing. It fails to apply at least against
current linux next, can you please update against that?
 
> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +{
> +	struct omap_hsmmc_host *host = mmc_priv(mmc);
> +	u32 irq_mask;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&host->irq_lock, flags);
> +
> +	host->sdio_irq_en = (enable != 0) ? true : false;
> +
> +	if (host->active_pinmux) {
> +		irq_mask = OMAP_HSMMC_READ(host->base, ISE);
> +		if (enable)
> +			irq_mask |= CIRQ_ENABLE;
> +		else
> +			irq_mask &= ~CIRQ_ENABLE;
> +		OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
> +
> +		if (!host->req_in_progress)
> +			OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
> +
> +#if 0
> +		OMAP_HSMMC_READ(host->base, IE); /* flush posted write */
> +#endif

Maybe just replace #if 0 with just a comment in case it turns out to be
needed for some cases?

Regards,

Tony

Re: [PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Daniel Mack]

On 10.01.2013 21:22, Tony Lindgren wrote:
> * Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121220 14:15]:
>> Without functional clock the omap_hsmmc module can't forward
>> SDIO IRQs to the system. This patch reconfigures dat1 line
>> as a gpio while the fclk is off. And uses SDIO IRQ detection of
>> the module, while fclk is present.
> 
> Looks pretty good to me, however I could not figure out what
> to apply this on for testing. It fails to apply at least against
> current linux next, can you please update against that?
>  
>> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +{
>> +	struct omap_hsmmc_host *host = mmc_priv(mmc);
>> +	u32 irq_mask;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&host->irq_lock, flags);
>> +
>> +	host->sdio_irq_en = (enable != 0) ? true : false;
>> +
>> +	if (host->active_pinmux) {
>> +		irq_mask = OMAP_HSMMC_READ(host->base, ISE);
>> +		if (enable)
>> +			irq_mask |= CIRQ_ENABLE;
>> +		else
>> +			irq_mask &= ~CIRQ_ENABLE;
>> +		OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
>> +
>> +		if (!host->req_in_progress)
>> +			OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>> +
>> +#if 0
>> +		OMAP_HSMMC_READ(host->base, IE); /* flush posted write */
>> +#endif
> 
> Maybe just replace #if 0 with just a comment in case it turns out to be
> needed for some cases?

Is there any update on this series? Andreas, did you do more tests?


Thanks and best regards,
Daniel

Re: [PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Andreas Fenkart]

Hi,

On Mon, Feb 18, 2013 at 11:26:38AM +0100, Daniel Mack wrote:
> On 10.01.2013 21:22, Tony Lindgren wrote:
> > * Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121220 14:15]:
> >> Without functional clock the omap_hsmmc module can't forward
> >> SDIO IRQs to the system. This patch reconfigures dat1 line
> >> as a gpio while the fclk is off. And uses SDIO IRQ detection of
> >> the module, while fclk is present.
> > 
> > Looks pretty good to me, however I could not figure out what
> > to apply this on for testing. It fails to apply at least against
> > current linux next, can you please update against that?
> >  
> >> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >> +{
> >> +	struct omap_hsmmc_host *host = mmc_priv(mmc);
> >> +	u32 irq_mask;
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&host->irq_lock, flags);
> >> +
> >> +	host->sdio_irq_en = (enable != 0) ? true : false;
> >> +
> >> +	if (host->active_pinmux) {
> >> +		irq_mask = OMAP_HSMMC_READ(host->base, ISE);
> >> +		if (enable)
> >> +			irq_mask |= CIRQ_ENABLE;
> >> +		else
> >> +			irq_mask &= ~CIRQ_ENABLE;
> >> +		OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
> >> +
> >> +		if (!host->req_in_progress)
> >> +			OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
> >> +
> >> +#if 0
> >> +		OMAP_HSMMC_READ(host->base, IE); /* flush posted write */
> >> +#endif
> > 
> > Maybe just replace #if 0 with just a comment in case it turns out to be
> > needed for some cases?
> 
> Is there any update on this series? Andreas, did you do more tests?

Thanks for all the feedback so far. Yes I did more testing. After
reducing the autosuspend delay, I found two more bugs.

--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -138,7 +138,7 @@ static void apply_clk_hack(void)
 #define SOFTRESET              (1 << 1)
 #define RESETDONE              (1 << 0)

-#define MMC_AUTOSUSPEND_DELAY  100
+#define MMC_AUTOSUSPEND_DELAY  1
 #define MMC_TIMEOUT_MS         20

One is already fixed[1], the other I'm still working on.
Hope to progress this week, then need to redo the testing.
Plan is to resubmit latest next week.

Andi

Re: [PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Grant Likely]

On Thu, 20 Dec 2012 23:12:12 +0100, Andreas Fenkart <andreas.fenkart@streamunlimited.com> wrote:
> Without functional clock the omap_hsmmc module can't forward
> SDIO IRQs to the system. This patch reconfigures dat1 line
> as a gpio while the fclk is off. And uses SDIO IRQ detection of
> the module, while fclk is present.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  .../devicetree/bindings/mmc/ti-omap-hsmmc.txt      |   42 ++++
>  arch/arm/plat-omap/include/plat/mmc.h              |    4 +
>  drivers/mmc/host/omap_hsmmc.c                      |  219 ++++++++++++++++++--
>  3 files changed, 247 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index d1b8932..4d57637 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -24,6 +24,29 @@ One tx and one rx pair is required.
>  dma-names: DMA request names. These strings correspond 1:1 with
>  the ordered pairs in dmas. The RX request must be "rx" and the
>  TX request must be "tx".
> +ti,cirq-gpio: When omap_hsmmc module is suspended, its functional
> +clock is turned off. Without fclk it can't forward SDIO IRQs to the
> +system. For that to happen, it needs to tell the PRCM to restore
> +its fclk, which is done through the swakeup line.
> +
> +                   ------
> +                  | PRCM |
> +                   ------
> +                    | ^
> +               fclk | | swakeup
> +                    v |
> +                  -------               ------
> +      <-- IRQ -- | hsmmc | <-- CIRQ -- | card |
> +                  -------               ------
> +
> +The problem is, that on the AM335x family the swakeup line is
> +missing, it has not been routed from the module to the PRCM.
> +The way to work around this, is to reconfigure the dat1 line as a
> +GPIO upon suspend. Beyond this option you also need to set named
> +states "default" and "idle "in the .dts file for the pins, using
> +pinctrl-single.c. The MMC driver will then then toggle between
> +default and idle during the runtime.
> +
>  
>  Examples:
>  
> @@ -53,3 +76,22 @@ Examples:
>  			&edma 25>;
>  		dma-names = "tx", "rx";
>  	};
> +
> +[am335x with with gpio for sdio irq]
> +
> +	mmc1_cirq_pin: pinmux_cirq_pin {
> +		pinctrl-single,pins = <
> +			0x0f8 0x3f	/* MMC0_DAT1 as GPIO2_28 */
> +		>;
> +	};
> +
> +	mmc1: mmc@48060000 {
> +		pinctrl-names = "default", "idle";
> +		pinctrl-0 = <&mmc1_pins>;
> +		pinctrl-1 = <&mmc1_cirq_pin>;
> +		ti,cirq-gpio = <&gpio3 28 0>;
> +		ti,non-removable;
> +		bus-width = <4>;
> +		vmmc-supply = <&ldo2_reg>;
> +		vmmc_aux-supply = <&vmmc>;
> +	};

Binding looks reasonable.

Reviewed-by: Grant Likely <grant.likely@secretlab.ca>

Re: omap_hsmmc: SDIO IRQ on AM335x family

[Tony Lindgren]

* Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121220 14:07]:
> Hi,
> 
> On Fri, Nov 30, 2012 at 07:57:35PM +0100, Daniel Mack wrote:
> > 
> > On 30.11.2012 18:40, Tony Lindgren wrote:
> > > * Andreas Fenkart <andreas.fenkart@streamunlimited.com> [121130 03:21]:
> > >>
> > >> The alternative was to configure dat1 line as a GPIO, while
> > >> waiting for an IRQ. Then configuring it back as dat1 when the
> > >> SDIO card is signalling an IRQ. Or the host starts a transfer. I
> > >> guess this will perform poorly, hence not considering it really.
> > > 
> > > This might work for SDIO cards. It should be disabled for data
> > > cards naturally to avoid potential data corruption.
> I don't understand your concern here, could you explain
> 
> > > The way to implement this is set named states in the .dts file
> > > for the pins using pinctrl-single.c, then have the MMC driver
> > > request states "default" "active" and "idle" during the probe,
> > > then toggle between active and idle during the runtime.
> > > 
> > > As far as I remember the GPIO functionality does not need to
> > > be enabled, just muxing the pin to GPIO mode for the wake-up
> > > is enough.
> > 
> > Wouldn't that be racy, given that an interrupt which occurs at beween
> > the point in time when the driver decides to wait for IRQs again until
> > the mux has finished switching over, could potentially be lost?
> 
> The IRQ is level triggered, so can't be lost. I implemented it as
> suggested and surprisingly performance is pretty good. Actually not
> worse than keeping the fclk enabled all times.
> 
> module: 88W8787 / mwifiex
> tx bitrate: 150.0 MBit/s MCS 7 40Mhz short GI
> 
>                    |   tcp tx         |  signal  | cpu idle
> ---------------------------------------------------------------
> keep fclk enabled  |   50.3 Mbits/sec | -23 dBm  | 15 %
> suspend/resume     |   49.7 Mbits/sec | -22 dBM  | 13 %
> 
> patch follows

Hey that's cool :) Will take a look at the patch.

Tony