Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
* Andreas Fenkart [130531 01:05]: > Hi, > > this email adress will soon expire, > my alternate email adress is OK, noticed that the SDIO wake behaves in a different way between wl12xx and mwifiex, and there's an issue with the muxing still for legacy non-SDIO booting. So at least one more update coming from me for this patch. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
Hi,
this email adress will soon expire,
my alternate email adress is
afenkart at gmail.com
On Mon, May 20, 2013 at 01:58:16PM -0700, Tony Lindgren wrote:
> * Andreas Fenkart [130515 01:51]:
> > 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. When the fclk is present it uses the standard SDIO IRQ detection of
> > the module.
> >
> > The gpio irq is managed via the 'disable_depth' ref counter of the irq
> > subsystem, this driver simply calls enable_irq/disable_irq when needed.
> >
> > sdio irqsdio irq
> > unmasked masked
> >-
> > runtime default |1 | 2
> > runtime suspend |0 | 1
> >
> > irq disable depth
> >
> >
> > only when sdio irq is enabled AND the module is idle, the reference
> > count drops to zero and the gpio irq is effectively armed.
> >
> > Patch was tested on AM335x/Stream800. Test setup was two modules
> > with sdio wifi cards. Modules where connected to a dual-band AP, each
> > module using a different band. One of module was running iperf as server
> > the other as client connecting to the server in a while true loop. Test
> > was running for 4+ weeks. There were about 60 Mio. suspend/resume
> > transitions. Test was shut down regularly.
>
> Looks like this somehow breaks detecting of eMMC on mmc2 for me at least
> with the non-dt legacyboot. For a removable mmc1 still keeps working.
>
> For mmc2 I have .nonremovable = true and no gpio_cd or gpio_wp.
>
> Also few comments below.
>
> > --- a/drivers/mmc/host/omap_hsmmc.c
> > +++ b/drivers/mmc/host/omap_hsmmc.c
> > + 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;
> > + }
>
> This should be checked, we have systems with all muxing done statically
> in the bootloader. And we also have systems that provide the default
> pinctrl state only as they don't need remuxing. So at most a warning should
> be printed.
>
> > 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) {
> > + disable_irq(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;
> > + }
> > +
> > + spin_lock_irqsave(&host->irq_lock, flags);
> > + host->active_pinmux = true;
> > +
> > + if (host->sdio_irq_en) {
> > + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
> > + OMAP_HSMMC_WRITE(host->base, ISE, CIRQ_EN);
> > + OMAP_HSMMC_WRITE(host->base, IE, CIRQ_EN);
> > + }
> > + spin_unlock_irqrestore(&host->irq_lock, flags);
> > + }
> > + return ret;
> > }
>
> The check for sdio_irq && host->pinctrl looks broken too as we may
> have not dynamic muxing via pinctrl needed for let's say omap3 based
> systems.
>
> Other than that, looks good to me.
>
> Regards,
>
> Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
* Tony Lindgren [130520 19:55]: > * Tony Lindgren [130520 14:03]: > > * Andreas Fenkart [130515 01:51]: > > > 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. When the fclk is present it uses the standard SDIO IRQ detection of > > > the module. > > > > > > The gpio irq is managed via the 'disable_depth' ref counter of the irq > > > subsystem, this driver simply calls enable_irq/disable_irq when needed. > > > > > > sdio irqsdio irq > > > unmasked masked > > >- > > > runtime default |1 | 2 > > > runtime suspend |0 | 1 > > > > > > irq disable depth > > > > > > > > > only when sdio irq is enabled AND the module is idle, the reference > > > count drops to zero and the gpio irq is effectively armed. > > > > > > Patch was tested on AM335x/Stream800. Test setup was two modules > > > with sdio wifi cards. Modules where connected to a dual-band AP, each > > > module using a different band. One of module was running iperf as server > > > the other as client connecting to the server in a while true loop. Test > > > was running for 4+ weeks. There were about 60 Mio. suspend/resume > > > transitions. Test was shut down regularly. > > > > Looks like this somehow breaks detecting of eMMC on mmc2 for me at least > > with the non-dt legacyboot. For a removable mmc1 still keeps working. > > > > For mmc2 I have .nonremovable = true and no gpio_cd or gpio_wp. > > Looks like that's because gpio0 is considered valid. But for these pins > gpio0 is not valid, so let's cut the dependency to updating the pdata > and ignore gpio0. > > We should also have three pinmux states: default, active and idle to > avoid remuxing all the pins unnecessarily. The default pins should > contain the fixed pins, active mmc_dat1, and idle the gpio pin. > > I've attempted to fix up these issues, did not add the wake-up events > from Steve Sakoman's patch for the non-muxed case: > > http://www.sakoman.com/cgi-bin/gitweb.cgi?p=linux.git;a=commitdiff_plain;h=010810d22f6f49ac03da4ba384969432e0320453 > > Based on the patch above, sounds like the wake-up events won't work > for deeper idle states though. But we should probably still support > those as most people run without the deeper idle states anyways. Actually, let's go with your patch with the pinmuxing changed. Then the active state support can be added later on. And let's not add any pinmuxing callbacks to the legacy booting mode, now there's at least some reason for people to move on to device tree based booting. > Can you check if the following changes on top of your patch still > work for you? Note that you need to update your .dts file for the > new pinctrl states. Here's your original patch updated for the muxing for the default, active and idle states. Regards, Tony From: Andreas Fenkart Date: Wed, 15 May 2013 10:45:14 +0200 Subject: [PATCH] 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. When the fclk is present it uses the standard SDIO IRQ detection of the module. The gpio irq is managed via the 'disable_depth' ref counter of the irq subsystem, this driver simply calls enable_irq/disable_irq when needed. sdio irqsdio irq unmasked masked - runtime default |1 | 2 runtime suspend |0 | 1 irq disable depth only when sdio irq is enabled AND the module is idle, the reference count drops to zero and the gpio irq is effectively armed. Patch was tested on AM335x/Stream800. Test setup was two modules with sdio wifi cards. Modules where connected to a dual-band AP, each module using a different band. One of module was running iperf as server the other as client connecting to the server in a while true loop. Test was running for 4+ weeks. There were about 60 Mio. suspend/resume transitions. Test was shut down regularly. Signed-off-by: Andreas Fenkart Reviewed-by: Grant Likely [t...@atomide.com: updated pin muxing and some comments] Signed-off-by: Tony Lindgren diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt index ed271fc..5a3df37 100644 --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt @@ -20,6 +20,29 @@ ti,dual-volt: boolean, supports dual voltage cards ti,non-removable: non-removable slot (like eMMC) ti,needs-special-reset: Requires a special softreset sequence ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed +ti,cirq-gp
Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
* Tony Lindgren [130520 14:03]:
> * Andreas Fenkart [130515 01:51]:
> > 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. When the fclk is present it uses the standard SDIO IRQ detection of
> > the module.
> >
> > The gpio irq is managed via the 'disable_depth' ref counter of the irq
> > subsystem, this driver simply calls enable_irq/disable_irq when needed.
> >
> > sdio irqsdio irq
> > unmasked masked
> >-
> > runtime default |1 | 2
> > runtime suspend |0 | 1
> >
> > irq disable depth
> >
> >
> > only when sdio irq is enabled AND the module is idle, the reference
> > count drops to zero and the gpio irq is effectively armed.
> >
> > Patch was tested on AM335x/Stream800. Test setup was two modules
> > with sdio wifi cards. Modules where connected to a dual-band AP, each
> > module using a different band. One of module was running iperf as server
> > the other as client connecting to the server in a while true loop. Test
> > was running for 4+ weeks. There were about 60 Mio. suspend/resume
> > transitions. Test was shut down regularly.
>
> Looks like this somehow breaks detecting of eMMC on mmc2 for me at least
> with the non-dt legacyboot. For a removable mmc1 still keeps working.
>
> For mmc2 I have .nonremovable = true and no gpio_cd or gpio_wp.
Looks like that's because gpio0 is considered valid. But for these pins
gpio0 is not valid, so let's cut the dependency to updating the pdata
and ignore gpio0.
We should also have three pinmux states: default, active and idle to
avoid remuxing all the pins unnecessarily. The default pins should
contain the fixed pins, active mmc_dat1, and idle the gpio pin.
I've attempted to fix up these issues, did not add the wake-up events
from Steve Sakoman's patch for the non-muxed case:
http://www.sakoman.com/cgi-bin/gitweb.cgi?p=linux.git;a=commitdiff_plain;h=010810d22f6f49ac03da4ba384969432e0320453
Based on the patch above, sounds like the wake-up events won't work
for deeper idle states though. But we should probably still support
those as most people run without the deeper idle states anyways.
Can you check if the following changes on top of your patch still
work for you? Note that you need to update your .dts file for the
new pinctrl states.
Regards,
Tony
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -186,7 +186,7 @@ struct omap_hsmmc_host {
struct omap_hsmmc_next next_data;
boolsdio_irq_en;
struct pinctrl *pinctrl;
- struct pinctrl_state*active, *idle;
+ struct pinctrl_state*fixed, *active, *idle;
boolactive_pinmux;
struct omap_mmc_platform_data *pdata;
@@ -434,7 +434,8 @@ 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)) {
+ if (pdata->slots[0].gpio_cirq > 0 &&
+ gpio_is_valid(pdata->slots[0].gpio_cirq)) {
pdata->slots[0].sdio_irq =
gpio_to_irq(pdata->slots[0].gpio_cirq);
@@ -1635,7 +1636,7 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc,
struct mmc_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;
+ u32 irq_mask, con;
unsigned long flags;
spin_lock_irqsave(&host->irq_lock, flags);
@@ -2116,35 +2117,43 @@ static int omap_hsmmc_probe(struct platform_device
*pdev)
omap_hsmmc_disable_irq(host);
+ mmc->caps |= MMC_CAP_SDIO_IRQ;
+
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;
+ if (!IS_ERR(host->pinctrl)) {
+ host->fixed = pinctrl_lookup_state(host->pinctrl,
+
Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.
* Andreas Fenkart [130515 01:51]:
> 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. When the fclk is present it uses the standard SDIO IRQ detection of
> the module.
>
> The gpio irq is managed via the 'disable_depth' ref counter of the irq
> subsystem, this driver simply calls enable_irq/disable_irq when needed.
>
> sdio irqsdio irq
> unmasked masked
>-
> runtime default |1 | 2
> runtime suspend |0 | 1
>
> irq disable depth
>
>
> only when sdio irq is enabled AND the module is idle, the reference
> count drops to zero and the gpio irq is effectively armed.
>
> Patch was tested on AM335x/Stream800. Test setup was two modules
> with sdio wifi cards. Modules where connected to a dual-band AP, each
> module using a different band. One of module was running iperf as server
> the other as client connecting to the server in a while true loop. Test
> was running for 4+ weeks. There were about 60 Mio. suspend/resume
> transitions. Test was shut down regularly.
Looks like this somehow breaks detecting of eMMC on mmc2 for me at least
with the non-dt legacyboot. For a removable mmc1 still keeps working.
For mmc2 I have .nonremovable = true and no gpio_cd or gpio_wp.
Also few comments below.
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> + 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;
> + }
This should be checked, we have systems with all muxing done statically
in the bootloader. And we also have systems that provide the default
pinctrl state only as they don't need remuxing. So at most a warning should
be printed.
> 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) {
> + disable_irq(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;
> + }
> +
> + spin_lock_irqsave(&host->irq_lock, flags);
> + host->active_pinmux = true;
> +
> + if (host->sdio_irq_en) {
> + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
> + OMAP_HSMMC_WRITE(host->base, ISE, CIRQ_EN);
> + OMAP_HSMMC_WRITE(host->base, IE, CIRQ_EN);
> + }
> + spin_unlock_irqrestore(&host->irq_lock, flags);
> + }
> + return ret;
> }
The check for sdio_irq && host->pinctrl looks broken too as we may
have not dynamic muxing via pinctrl needed for let's say omap3 based
systems.
Other than that, looks good to me.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
