omap_hsmmc: SDIO IRQ on AM335x family
Subsystems (5)
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
Quoted 5 lines · Andreas Fenkart
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
Hi Tony,
Hi Andreas,
Quoted 10 lines · Tony Lindgren
Quoted 5 lines · Andreas Fenkart
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.
Quoted 8 lines
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
Hi,
Quoted 11 lines · Daniel Mack
Quoted 9 lines · Tony Lindgren
Quoted 5 lines · Andreas Fenkart
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
Quoted 5 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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 1/1 mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
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.
--- .../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(-)
Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +42 -0
@@ -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>; + };
arch/arm/plat-omap/include/plat/mmc.h +4 -0
@@ -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 ];
drivers/mmc/host/omap_hsmmc.c +201 -18
361 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Re: [PATCH] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
Quoted 4 lines · Andreas Fenkart
Without functional clock the
omap_hsmmcmodule 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?
Quoted 14 lines
drivers/mmc/host/omap_hsmmc.c +14
-0+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 ;
drivers/mmc/host/omap_hsmmc.c static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) · line 1649
Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +10
-0+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.
Quoted 29 lines · Tony Lindgren
Quoted 4 lines · Andreas Fenkart
Without functional clock the
omap_hsmmcmodule 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?
Quoted 19 lines
drivers/mmc/host/omap_hsmmc.c +19
-0+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)
Quoted 8 lines
drivers/mmc/host/omap_hsmmc.c static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) · line 1649
Quoted 5 lines
drivers/mmc/host/omap_hsmmc.c +5
-0+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.
Hi,
Quoted 39 lines · Daniel Mack
Quoted 29 lines · Tony Lindgren
Quoted 4 lines · Andreas Fenkart
Without functional clock the
omap_hsmmcmodule 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?
Quoted 19 lines
(quoted diff) +19
-0+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)Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ OMAP_HSMMC_WRITE(host->base, ISE, irq_mask); + +#if 0 + OMAP_HSMMC_READ(host->base, IE); /* flush posted write */ +#endifMaybe just replace #if 0 with just a comment in case it turns out to be needed for some cases?
Quoted 1 line
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.
drivers/mmc/host/omap_hsmmc.c +1 -1
@@ -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.
In reply to Andreas FenkartQuoted 59 lines · Andreas Fenkart
Without functional clock the
omap_hsmmcmodule 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 <[email protected]>--- .../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(-)Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +32
-0@@ -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 */ + >; + }; +
Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt Examples: · line 97
Quoted 10 lines
Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +10
-0+ 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.
Quoted 36 lines · Andreas Fenkart
Hi,
Quoted 11 lines · Daniel Mack
Quoted 9 lines · Tony Lindgren
Quoted 5 lines · Andreas Fenkart
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
Quoted 5 lines
Quoted 5 lines
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.Quoted 7 lines
Quoted 3 lines
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
Quoted 6 lines
| 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