Re: [RESEND PATCH v2 1/3] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode.

[Andreas Fenkart <andreas.fenkart@streamunlimited.com>]

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 <andreas.fenkart@streamunlimited.com> [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 irq    sdio 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