diff mbox

mmc: sdhci: supporting PCI MSI

Message ID 1383299119-3952-1-git-send-email-Jackey.Shen@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shen, Jackey Nov. 1, 2013, 9:45 a.m. UTC 
Enable MSI support in sdhci-pci driver and provide the mechanism
to fall back to Legacy Pin-based Interrupt if MSI register fails.

Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
Reviewed-by: Huang Rui <ray.huang@amd.com>
---
 drivers/mmc/host/Kconfig  |  11 +++++
 drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
 include/linux/mmc/sdhci.h |   2 +
 3 files changed, 109 insertions(+), 12 deletions(-)

Comments

Shen, Jackey Nov. 6, 2013, 8:13 a.m. UTC | #1 
Hi,

This patch is tested on HW platform and work fine.

Thanks,
Jackey

> -----Original Message-----
> From: Jackey Shen [mailto:Jackey.Shen@amd.com]
> Sent: Friday, November 01, 2013 5:45 PM
> To: linux-mmc@vger.kernel.org
> Cc: mcuos.com@gmail.com; Huang, Ray; jackey.shen.bo@gmail.com; Shen, Jackey
> Subject: [PATCH] mmc: sdhci: supporting PCI MSI
> 
> Enable MSI support in sdhci-pci driver and provide the mechanism
> to fall back to Legacy Pin-based Interrupt if MSI register fails.
> 
> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> ---
>  drivers/mmc/host/Kconfig  |  11 +++++
>  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++-----
> -
>  include/linux/mmc/sdhci.h |   2 +
>  3 files changed, 109 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 7fc5099..a56cf27 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
> 
>  	  If unsure, say N.
> 
> +config MMC_SDHCI_PCI_MSI
> +	bool "SDHCI support with MSI on PCI bus"
> +	depends on MMC_SDHCI_PCI && PCI_MSI
> +	help
> +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
> +	  Digital Host Controller Interface.
> +
> +	  If you have a controller with this capability, say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_RICOH_MMC
>  	bool "Ricoh MMC Controller Disabler"
>  	depends on MMC_SDHCI_PCI
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6785fb1..de509bc 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -13,6 +13,10 @@
>   *     - JMicron (hardware and technical support)
>   */
> 
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +#include <linux/pci.h>
> +#endif
> +
>  #include <linux/delay.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> @@ -2520,6 +2524,64 @@ out:
>  	return result;
>  }
> 
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +static int sdhci_setup_msi(struct sdhci_host *host)
> +{
> +	int ret;
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	host->msi_enabled = false;
> +
> +	ret = pci_enable_msi(pdev);
> +	if (ret) {
> +		pr_warn("%s: Failed to allocate MSI entry\n",
> +			mmc_hostname(host->mmc));
> +		return ret;
> +	}
> +
> +	ret = request_irq(pdev->irq, sdhci_irq, 0,
> +		mmc_hostname(host->mmc), host);
> +	if (ret) {
> +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
> +		       mmc_hostname(host->mmc), pdev->irq, ret);
> +		pci_disable_msi(pdev);
> +		return ret;
> +	}
> +
> +	host->irq = pdev->irq;
> +	host->msi_enabled = true;
> +	return ret;
> +}
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return sdhci_setup_msi(host);
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	free_irq(host->irq, host);
> +	if (host->msi_enabled) {
> +		pci_disable_msi(pdev);
> +		host->msi_enabled = false;
> +	}
> +}
> +
> +#else
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	free_irq(host->irq, host);
> +}
> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> +
> 
> /*****************************************************************************
> \
>   *
> *
>   * Suspend/resume
> *
> @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
> 
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
>  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -		free_irq(host->irq, host);
> +		/* free any IRQ and disable MSI */
> +		sdhci_cleanup_msi(host);
>  	} else {
>  		sdhci_enable_irq_wakeups(host);
>  		enable_irq_wake(host->irq);
> @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	}
> 
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -				  mmc_hostname(host->mmc), host);
> +		/* try to enable MSI */
> +		ret = sdhci_try_enable_msi(host);
>  		if (ret)
> -			return ret;
> +			pr_warn("%s: Fall back to Legacy Pin-based
> Interrupt: %d\n",
> +				mmc_hostname(host->mmc), ret);
> +
> +		if (!host->msi_enabled) {
> +			/* fall back to legacy pin-based interrupt */
> +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +					mmc_hostname(host->mmc), host);
> +			if (ret) {
> +				pr_err("%s: Failed to request IRQ %d: %d\n",
> +					 mmc_hostname(host->mmc), host->irq, ret);
> +				return ret;
> +			}
> +		}
>  	} else {
>  		sdhci_disable_irq_wakeups(host);
>  		disable_irq_wake(host->irq);
> @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
> 
>  	sdhci_init(host, 0);
> 
> -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -		mmc_hostname(mmc), host);
> -	if (ret) {
> -		pr_err("%s: Failed to request IRQ %d: %d\n",
> -		       mmc_hostname(mmc), host->irq, ret);
> -		goto untasklet;
> +	/* try to enable MSI */
> +	ret = sdhci_try_enable_msi(host);
> +	if (ret)
> +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> +			mmc_hostname(host->mmc), ret);
> +
> +	if (!host->msi_enabled) {
> +		/* fall back to legacy pin-based interrupt */
> +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +				mmc_hostname(host->mmc), host);
> +		if (ret) {
> +			pr_err("%s: Failed to request IRQ %d: %d\n",
> +				 mmc_hostname(host->mmc), host->irq, ret);
> +			goto untasklet;
> +		}
>  	}
> 
>  #ifdef CONFIG_MMC_DEBUG
> @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  reset:
>  	sdhci_reset(host, SDHCI_RESET_ALL);
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
>  #endif
>  untasklet:
>  	tasklet_kill(&host->card_tasklet);
> @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  		sdhci_reset(host, SDHCI_RESET_ALL);
> 
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
> 
>  	del_timer_sync(&host->timer);
> 
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 3e781b8..3812479 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -99,6 +99,8 @@ struct sdhci_host {
>  /* Controller has a non-standard host control register */
>  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
> 
> +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
> +
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> 
> --
> 1.8.1.2


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Nov. 8, 2013, 6:58 a.m. UTC | #2 
On 11/01/2013 05:45 PM, Jackey Shen wrote:
> Enable MSI support in sdhci-pci driver and provide the mechanism
> to fall back to Legacy Pin-based Interrupt if MSI register fails.
> 
> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> ---
>  drivers/mmc/host/Kconfig  |  11 +++++
>  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/mmc/sdhci.h |   2 +
>  3 files changed, 109 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 7fc5099..a56cf27 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_PCI_MSI
> +	bool "SDHCI support with MSI on PCI bus"
> +	depends on MMC_SDHCI_PCI && PCI_MSI
> +	help
> +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
> +	  Digital Host Controller Interface.
> +
> +	  If you have a controller with this capability, say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_RICOH_MMC
>  	bool "Ricoh MMC Controller Disabler"
>  	depends on MMC_SDHCI_PCI
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6785fb1..de509bc 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c

I think PCI stuffs should be in sdhci-pci.c, not sdhci.c.

Thanks,
Aaron

> @@ -13,6 +13,10 @@
>   *     - JMicron (hardware and technical support)
>   */
>  
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +#include <linux/pci.h>
> +#endif
> +
>  #include <linux/delay.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> @@ -2520,6 +2524,64 @@ out:
>  	return result;
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +static int sdhci_setup_msi(struct sdhci_host *host)
> +{
> +	int ret;
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	host->msi_enabled = false;
> +
> +	ret = pci_enable_msi(pdev);
> +	if (ret) {
> +		pr_warn("%s: Failed to allocate MSI entry\n",
> +			mmc_hostname(host->mmc));
> +		return ret;
> +	}
> +
> +	ret = request_irq(pdev->irq, sdhci_irq, 0,
> +		mmc_hostname(host->mmc), host);
> +	if (ret) {
> +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
> +		       mmc_hostname(host->mmc), pdev->irq, ret);
> +		pci_disable_msi(pdev);
> +		return ret;
> +	}
> +
> +	host->irq = pdev->irq;
> +	host->msi_enabled = true;
> +	return ret;
> +}
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return sdhci_setup_msi(host);
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	free_irq(host->irq, host);
> +	if (host->msi_enabled) {
> +		pci_disable_msi(pdev);
> +		host->msi_enabled = false;
> +	}
> +}
> +
> +#else
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	free_irq(host->irq, host);
> +}
> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> +
>  /*****************************************************************************\
>   *                                                                           *
>   * Suspend/resume                                                            *
> @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
>  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -		free_irq(host->irq, host);
> +		/* free any IRQ and disable MSI */
> +		sdhci_cleanup_msi(host);
>  	} else {
>  		sdhci_enable_irq_wakeups(host);
>  		enable_irq_wake(host->irq);
> @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	}
>  
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -				  mmc_hostname(host->mmc), host);
> +		/* try to enable MSI */
> +		ret = sdhci_try_enable_msi(host);
>  		if (ret)
> -			return ret;
> +			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> +				mmc_hostname(host->mmc), ret);
> +
> +		if (!host->msi_enabled) {
> +			/* fall back to legacy pin-based interrupt */
> +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +					mmc_hostname(host->mmc), host);
> +			if (ret) {
> +				pr_err("%s: Failed to request IRQ %d: %d\n",
> +					 mmc_hostname(host->mmc), host->irq, ret);
> +				return ret;
> +			}
> +		}
>  	} else {
>  		sdhci_disable_irq_wakeups(host);
>  		disable_irq_wake(host->irq);
> @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	sdhci_init(host, 0);
>  
> -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -		mmc_hostname(mmc), host);
> -	if (ret) {
> -		pr_err("%s: Failed to request IRQ %d: %d\n",
> -		       mmc_hostname(mmc), host->irq, ret);
> -		goto untasklet;
> +	/* try to enable MSI */
> +	ret = sdhci_try_enable_msi(host);
> +	if (ret)
> +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> +			mmc_hostname(host->mmc), ret);
> +
> +	if (!host->msi_enabled) {
> +		/* fall back to legacy pin-based interrupt */
> +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +				mmc_hostname(host->mmc), host);
> +		if (ret) {
> +			pr_err("%s: Failed to request IRQ %d: %d\n",
> +				 mmc_hostname(host->mmc), host->irq, ret);
> +			goto untasklet;
> +		}
>  	}
>  
>  #ifdef CONFIG_MMC_DEBUG
> @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  reset:
>  	sdhci_reset(host, SDHCI_RESET_ALL);
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
>  #endif
>  untasklet:
>  	tasklet_kill(&host->card_tasklet);
> @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  		sdhci_reset(host, SDHCI_RESET_ALL);
>  
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
>  
>  	del_timer_sync(&host->timer);
>  
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 3e781b8..3812479 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -99,6 +99,8 @@ struct sdhci_host {
>  /* Controller has a non-standard host control register */
>  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
>  
> +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
> +
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shen, Jackey Nov. 8, 2013, 8:05 a.m. UTC | #3 
Hi Aaron,

On Fri, Nov 08, 2013 at 02:58:45PM +0800, Aaron Lu wrote:
> On 11/01/2013 05:45 PM, Jackey Shen wrote:
> > Enable MSI support in sdhci-pci driver and provide the mechanism
> > to fall back to Legacy Pin-based Interrupt if MSI register fails.
> > 
> > Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/mmc/host/Kconfig  |  11 +++++
> >  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
> >  include/linux/mmc/sdhci.h |   2 +
> >  3 files changed, 109 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 7fc5099..a56cf27 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
> >  
> >  	  If unsure, say N.
> >  
> > +config MMC_SDHCI_PCI_MSI
> > +	bool "SDHCI support with MSI on PCI bus"
> > +	depends on MMC_SDHCI_PCI && PCI_MSI
> > +	help
> > +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
> > +	  Digital Host Controller Interface.
> > +
> > +	  If you have a controller with this capability, say Y or M here.
> > +
> > +	  If unsure, say N.
> > +
> >  config MMC_RICOH_MMC
> >  	bool "Ricoh MMC Controller Disabler"
> >  	depends on MMC_SDHCI_PCI
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6785fb1..de509bc 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> 
> I think PCI stuffs should be in sdhci-pci.c, not sdhci.c.

But I ran into one issue as per your suggestion:
sdhci_add|remove_host() in sdhci.c calls back sdhci_try_enable|cleanup()
in_msi sdhci-pci.c if sdhci_try_enable|cleanup_msi() are moved into sdhci-pci.c.

Is it a best practice?

Thanks,
Jackey

> 
> Thanks,
> Aaron
> 
> > @@ -13,6 +13,10 @@
> >   *     - JMicron (hardware and technical support)
> >   */
> >  
> > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > +#include <linux/pci.h>
> > +#endif
> > +
> >  #include <linux/delay.h>
> >  #include <linux/highmem.h>
> >  #include <linux/io.h>
> > @@ -2520,6 +2524,64 @@ out:
> >  	return result;
> >  }
> >  
> > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > +static int sdhci_setup_msi(struct sdhci_host *host)
> > +{
> > +	int ret;
> > +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > +
> > +	host->msi_enabled = false;
> > +
> > +	ret = pci_enable_msi(pdev);
> > +	if (ret) {
> > +		pr_warn("%s: Failed to allocate MSI entry\n",
> > +			mmc_hostname(host->mmc));
> > +		return ret;
> > +	}
> > +
> > +	ret = request_irq(pdev->irq, sdhci_irq, 0,
> > +		mmc_hostname(host->mmc), host);
> > +	if (ret) {
> > +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
> > +		       mmc_hostname(host->mmc), pdev->irq, ret);
> > +		pci_disable_msi(pdev);
> > +		return ret;
> > +	}
> > +
> > +	host->irq = pdev->irq;
> > +	host->msi_enabled = true;
> > +	return ret;
> > +}
> > +
> > +static int sdhci_try_enable_msi(struct sdhci_host *host)
> > +{
> > +	return sdhci_setup_msi(host);
> > +}
> > +
> > +static void sdhci_cleanup_msi(struct sdhci_host *host)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > +
> > +	free_irq(host->irq, host);
> > +	if (host->msi_enabled) {
> > +		pci_disable_msi(pdev);
> > +		host->msi_enabled = false;
> > +	}
> > +}
> > +
> > +#else
> > +
> > +static int sdhci_try_enable_msi(struct sdhci_host *host)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void sdhci_cleanup_msi(struct sdhci_host *host)
> > +{
> > +	free_irq(host->irq, host);
> > +}
> > +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> > +
> >  /*****************************************************************************\
> >   *                                                                           *
> >   * Suspend/resume                                                            *
> > @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
> >  
> >  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> >  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -		free_irq(host->irq, host);
> > +		/* free any IRQ and disable MSI */
> > +		sdhci_cleanup_msi(host);
> >  	} else {
> >  		sdhci_enable_irq_wakeups(host);
> >  		enable_irq_wake(host->irq);
> > @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
> >  	}
> >  
> >  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> > -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > -				  mmc_hostname(host->mmc), host);
> > +		/* try to enable MSI */
> > +		ret = sdhci_try_enable_msi(host);
> >  		if (ret)
> > -			return ret;
> > +			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> > +				mmc_hostname(host->mmc), ret);
> > +
> > +		if (!host->msi_enabled) {
> > +			/* fall back to legacy pin-based interrupt */
> > +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > +					mmc_hostname(host->mmc), host);
> > +			if (ret) {
> > +				pr_err("%s: Failed to request IRQ %d: %d\n",
> > +					 mmc_hostname(host->mmc), host->irq, ret);
> > +				return ret;
> > +			}
> > +		}
> >  	} else {
> >  		sdhci_disable_irq_wakeups(host);
> >  		disable_irq_wake(host->irq);
> > @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
> >  
> >  	sdhci_init(host, 0);
> >  
> > -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > -		mmc_hostname(mmc), host);
> > -	if (ret) {
> > -		pr_err("%s: Failed to request IRQ %d: %d\n",
> > -		       mmc_hostname(mmc), host->irq, ret);
> > -		goto untasklet;
> > +	/* try to enable MSI */
> > +	ret = sdhci_try_enable_msi(host);
> > +	if (ret)
> > +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> > +			mmc_hostname(host->mmc), ret);
> > +
> > +	if (!host->msi_enabled) {
> > +		/* fall back to legacy pin-based interrupt */
> > +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > +				mmc_hostname(host->mmc), host);
> > +		if (ret) {
> > +			pr_err("%s: Failed to request IRQ %d: %d\n",
> > +				 mmc_hostname(host->mmc), host->irq, ret);
> > +			goto untasklet;
> > +		}
> >  	}
> >  
> >  #ifdef CONFIG_MMC_DEBUG
> > @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >  reset:
> >  	sdhci_reset(host, SDHCI_RESET_ALL);
> >  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -	free_irq(host->irq, host);
> > +	sdhci_cleanup_msi(host);
> >  #endif
> >  untasklet:
> >  	tasklet_kill(&host->card_tasklet);
> > @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >  		sdhci_reset(host, SDHCI_RESET_ALL);
> >  
> >  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -	free_irq(host->irq, host);
> > +	sdhci_cleanup_msi(host);
> >  
> >  	del_timer_sync(&host->timer);
> >  
> > diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> > index 3e781b8..3812479 100644
> > --- a/include/linux/mmc/sdhci.h
> > +++ b/include/linux/mmc/sdhci.h
> > @@ -99,6 +99,8 @@ struct sdhci_host {
> >  /* Controller has a non-standard host control register */
> >  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
> >  
> > +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
> > +
> >  	int irq;		/* Device IRQ */
> >  	void __iomem *ioaddr;	/* Mapped address */
> >  
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Nov. 8, 2013, 9 a.m. UTC | #4 
On 11/08/2013 04:05 PM, Jackey Shen wrote:
> Hi Aaron,
> 
> On Fri, Nov 08, 2013 at 02:58:45PM +0800, Aaron Lu wrote:
>> On 11/01/2013 05:45 PM, Jackey Shen wrote:
>>> Enable MSI support in sdhci-pci driver and provide the mechanism
>>> to fall back to Legacy Pin-based Interrupt if MSI register fails.
>>>
>>> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
>>> Reviewed-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>  drivers/mmc/host/Kconfig  |  11 +++++
>>>  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
>>>  include/linux/mmc/sdhci.h |   2 +
>>>  3 files changed, 109 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 7fc5099..a56cf27 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
>>>  
>>>  	  If unsure, say N.
>>>  
>>> +config MMC_SDHCI_PCI_MSI
>>> +	bool "SDHCI support with MSI on PCI bus"
>>> +	depends on MMC_SDHCI_PCI && PCI_MSI
>>> +	help
>>> +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
>>> +	  Digital Host Controller Interface.
>>> +
>>> +	  If you have a controller with this capability, say Y or M here.
>>> +
>>> +	  If unsure, say N.
>>> +
>>>  config MMC_RICOH_MMC
>>>  	bool "Ricoh MMC Controller Disabler"
>>>  	depends on MMC_SDHCI_PCI
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 6785fb1..de509bc 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>
>> I think PCI stuffs should be in sdhci-pci.c, not sdhci.c.
> 
> But I ran into one issue as per your suggestion:
> sdhci_add|remove_host() in sdhci.c calls back sdhci_try_enable|cleanup()
> in_msi sdhci-pci.c if sdhci_try_enable|cleanup_msi() are moved into sdhci-pci.c.

I don't know much about PCI and interrupt, but what is the problem of
doing enable/disable msi in sdhci_pci_probe_slot?

Thanks,
Aaron

> 
> Is it a best practice?
> 
> Thanks,
> Jackey
> 
>>
>> Thanks,
>> Aaron
>>
>>> @@ -13,6 +13,10 @@
>>>   *     - JMicron (hardware and technical support)
>>>   */
>>>  
>>> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
>>> +#include <linux/pci.h>
>>> +#endif
>>> +
>>>  #include <linux/delay.h>
>>>  #include <linux/highmem.h>
>>>  #include <linux/io.h>
>>> @@ -2520,6 +2524,64 @@ out:
>>>  	return result;
>>>  }
>>>  
>>> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
>>> +static int sdhci_setup_msi(struct sdhci_host *host)
>>> +{
>>> +	int ret;
>>> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
>>> +
>>> +	host->msi_enabled = false;
>>> +
>>> +	ret = pci_enable_msi(pdev);
>>> +	if (ret) {
>>> +		pr_warn("%s: Failed to allocate MSI entry\n",
>>> +			mmc_hostname(host->mmc));
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = request_irq(pdev->irq, sdhci_irq, 0,
>>> +		mmc_hostname(host->mmc), host);
>>> +	if (ret) {
>>> +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
>>> +		       mmc_hostname(host->mmc), pdev->irq, ret);
>>> +		pci_disable_msi(pdev);
>>> +		return ret;
>>> +	}
>>> +
>>> +	host->irq = pdev->irq;
>>> +	host->msi_enabled = true;
>>> +	return ret;
>>> +}
>>> +
>>> +static int sdhci_try_enable_msi(struct sdhci_host *host)
>>> +{
>>> +	return sdhci_setup_msi(host);
>>> +}
>>> +
>>> +static void sdhci_cleanup_msi(struct sdhci_host *host)
>>> +{
>>> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
>>> +
>>> +	free_irq(host->irq, host);
>>> +	if (host->msi_enabled) {
>>> +		pci_disable_msi(pdev);
>>> +		host->msi_enabled = false;
>>> +	}
>>> +}
>>> +
>>> +#else
>>> +
>>> +static int sdhci_try_enable_msi(struct sdhci_host *host)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static void sdhci_cleanup_msi(struct sdhci_host *host)
>>> +{
>>> +	free_irq(host->irq, host);
>>> +}
>>> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
>>> +
>>>  /*****************************************************************************\
>>>   *                                                                           *
>>>   * Suspend/resume                                                            *
>>> @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>>>  
>>>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
>>>  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>> -		free_irq(host->irq, host);
>>> +		/* free any IRQ and disable MSI */
>>> +		sdhci_cleanup_msi(host);
>>>  	} else {
>>>  		sdhci_enable_irq_wakeups(host);
>>>  		enable_irq_wake(host->irq);
>>> @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
>>>  	}
>>>  
>>>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
>>> -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
>>> -				  mmc_hostname(host->mmc), host);
>>> +		/* try to enable MSI */
>>> +		ret = sdhci_try_enable_msi(host);
>>>  		if (ret)
>>> -			return ret;
>>> +			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
>>> +				mmc_hostname(host->mmc), ret);
>>> +
>>> +		if (!host->msi_enabled) {
>>> +			/* fall back to legacy pin-based interrupt */
>>> +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
>>> +					mmc_hostname(host->mmc), host);
>>> +			if (ret) {
>>> +				pr_err("%s: Failed to request IRQ %d: %d\n",
>>> +					 mmc_hostname(host->mmc), host->irq, ret);
>>> +				return ret;
>>> +			}
>>> +		}
>>>  	} else {
>>>  		sdhci_disable_irq_wakeups(host);
>>>  		disable_irq_wake(host->irq);
>>> @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
>>>  
>>>  	sdhci_init(host, 0);
>>>  
>>> -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
>>> -		mmc_hostname(mmc), host);
>>> -	if (ret) {
>>> -		pr_err("%s: Failed to request IRQ %d: %d\n",
>>> -		       mmc_hostname(mmc), host->irq, ret);
>>> -		goto untasklet;
>>> +	/* try to enable MSI */
>>> +	ret = sdhci_try_enable_msi(host);
>>> +	if (ret)
>>> +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
>>> +			mmc_hostname(host->mmc), ret);
>>> +
>>> +	if (!host->msi_enabled) {
>>> +		/* fall back to legacy pin-based interrupt */
>>> +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
>>> +				mmc_hostname(host->mmc), host);
>>> +		if (ret) {
>>> +			pr_err("%s: Failed to request IRQ %d: %d\n",
>>> +				 mmc_hostname(host->mmc), host->irq, ret);
>>> +			goto untasklet;
>>> +		}
>>>  	}
>>>  
>>>  #ifdef CONFIG_MMC_DEBUG
>>> @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
>>>  reset:
>>>  	sdhci_reset(host, SDHCI_RESET_ALL);
>>>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>> -	free_irq(host->irq, host);
>>> +	sdhci_cleanup_msi(host);
>>>  #endif
>>>  untasklet:
>>>  	tasklet_kill(&host->card_tasklet);
>>> @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>>  		sdhci_reset(host, SDHCI_RESET_ALL);
>>>  
>>>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>> -	free_irq(host->irq, host);
>>> +	sdhci_cleanup_msi(host);
>>>  
>>>  	del_timer_sync(&host->timer);
>>>  
>>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>>> index 3e781b8..3812479 100644
>>> --- a/include/linux/mmc/sdhci.h
>>> +++ b/include/linux/mmc/sdhci.h
>>> @@ -99,6 +99,8 @@ struct sdhci_host {
>>>  /* Controller has a non-standard host control register */
>>>  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
>>>  
>>> +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
>>> +
>>>  	int irq;		/* Device IRQ */
>>>  	void __iomem *ioaddr;	/* Mapped address */
>>>  
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Nov. 11, 2013, 8:35 a.m. UTC | #5 
On 11/01/2013 05:45 PM, Jackey Shen wrote:
> Enable MSI support in sdhci-pci driver and provide the mechanism
> to fall back to Legacy Pin-based Interrupt if MSI register fails.

Also note the following thread:
http://marc.info/?l=linux-mmc&m=133346937412708&w=2

Thanks,
Aaron

> 
> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> ---
>  drivers/mmc/host/Kconfig  |  11 +++++
>  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/mmc/sdhci.h |   2 +
>  3 files changed, 109 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 7fc5099..a56cf27 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_PCI_MSI
> +	bool "SDHCI support with MSI on PCI bus"
> +	depends on MMC_SDHCI_PCI && PCI_MSI
> +	help
> +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
> +	  Digital Host Controller Interface.
> +
> +	  If you have a controller with this capability, say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_RICOH_MMC
>  	bool "Ricoh MMC Controller Disabler"
>  	depends on MMC_SDHCI_PCI
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6785fb1..de509bc 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -13,6 +13,10 @@
>   *     - JMicron (hardware and technical support)
>   */
>  
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +#include <linux/pci.h>
> +#endif
> +
>  #include <linux/delay.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> @@ -2520,6 +2524,64 @@ out:
>  	return result;
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +static int sdhci_setup_msi(struct sdhci_host *host)
> +{
> +	int ret;
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	host->msi_enabled = false;
> +
> +	ret = pci_enable_msi(pdev);
> +	if (ret) {
> +		pr_warn("%s: Failed to allocate MSI entry\n",
> +			mmc_hostname(host->mmc));
> +		return ret;
> +	}
> +
> +	ret = request_irq(pdev->irq, sdhci_irq, 0,
> +		mmc_hostname(host->mmc), host);
> +	if (ret) {
> +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
> +		       mmc_hostname(host->mmc), pdev->irq, ret);
> +		pci_disable_msi(pdev);
> +		return ret;
> +	}
> +
> +	host->irq = pdev->irq;
> +	host->msi_enabled = true;
> +	return ret;
> +}
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return sdhci_setup_msi(host);
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> +	free_irq(host->irq, host);
> +	if (host->msi_enabled) {
> +		pci_disable_msi(pdev);
> +		host->msi_enabled = false;
> +	}
> +}
> +
> +#else
> +
> +static int sdhci_try_enable_msi(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +
> +static void sdhci_cleanup_msi(struct sdhci_host *host)
> +{
> +	free_irq(host->irq, host);
> +}
> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> +
>  /*****************************************************************************\
>   *                                                                           *
>   * Suspend/resume                                                            *
> @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
>  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -		free_irq(host->irq, host);
> +		/* free any IRQ and disable MSI */
> +		sdhci_cleanup_msi(host);
>  	} else {
>  		sdhci_enable_irq_wakeups(host);
>  		enable_irq_wake(host->irq);
> @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	}
>  
>  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -				  mmc_hostname(host->mmc), host);
> +		/* try to enable MSI */
> +		ret = sdhci_try_enable_msi(host);
>  		if (ret)
> -			return ret;
> +			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> +				mmc_hostname(host->mmc), ret);
> +
> +		if (!host->msi_enabled) {
> +			/* fall back to legacy pin-based interrupt */
> +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +					mmc_hostname(host->mmc), host);
> +			if (ret) {
> +				pr_err("%s: Failed to request IRQ %d: %d\n",
> +					 mmc_hostname(host->mmc), host->irq, ret);
> +				return ret;
> +			}
> +		}
>  	} else {
>  		sdhci_disable_irq_wakeups(host);
>  		disable_irq_wake(host->irq);
> @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	sdhci_init(host, 0);
>  
> -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> -		mmc_hostname(mmc), host);
> -	if (ret) {
> -		pr_err("%s: Failed to request IRQ %d: %d\n",
> -		       mmc_hostname(mmc), host->irq, ret);
> -		goto untasklet;
> +	/* try to enable MSI */
> +	ret = sdhci_try_enable_msi(host);
> +	if (ret)
> +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> +			mmc_hostname(host->mmc), ret);
> +
> +	if (!host->msi_enabled) {
> +		/* fall back to legacy pin-based interrupt */
> +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> +				mmc_hostname(host->mmc), host);
> +		if (ret) {
> +			pr_err("%s: Failed to request IRQ %d: %d\n",
> +				 mmc_hostname(host->mmc), host->irq, ret);
> +			goto untasklet;
> +		}
>  	}
>  
>  #ifdef CONFIG_MMC_DEBUG
> @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  reset:
>  	sdhci_reset(host, SDHCI_RESET_ALL);
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
>  #endif
>  untasklet:
>  	tasklet_kill(&host->card_tasklet);
> @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  		sdhci_reset(host, SDHCI_RESET_ALL);
>  
>  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> -	free_irq(host->irq, host);
> +	sdhci_cleanup_msi(host);
>  
>  	del_timer_sync(&host->timer);
>  
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 3e781b8..3812479 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -99,6 +99,8 @@ struct sdhci_host {
>  /* Controller has a non-standard host control register */
>  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
>  
> +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
> +
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shen, Jackey Nov. 12, 2013, 6:37 a.m. UTC | #6 
On Mon, Nov 11, 2013 at 04:35:28PM +0800, Aaron Lu wrote:
> On 11/01/2013 05:45 PM, Jackey Shen wrote:
> > Enable MSI support in sdhci-pci driver and provide the mechanism
> > to fall back to Legacy Pin-based Interrupt if MSI register fails.
> 
> Also note the following thread:
> http://marc.info/?l=linux-mmc&m=133346937412708&w=2

Yes, I noticed it. This patch is tested on AMD platform, and works fine.
I will submit this patch v2 adopting your suggestion.

Thanks,
Jackey

> 
> Thanks,
> Aaron
> 
> > 
> > Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/mmc/host/Kconfig  |  11 +++++
> >  drivers/mmc/host/sdhci.c  | 108 ++++++++++++++++++++++++++++++++++++++++------
> >  include/linux/mmc/sdhci.h |   2 +
> >  3 files changed, 109 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 7fc5099..a56cf27 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -68,6 +68,17 @@ config MMC_SDHCI_PCI
> >  
> >  	  If unsure, say N.
> >  
> > +config MMC_SDHCI_PCI_MSI
> > +	bool "SDHCI support with MSI on PCI bus"
> > +	depends on MMC_SDHCI_PCI && PCI_MSI
> > +	help
> > +	  This enables PCI MSI (Message Signaled Interrupts) for Secure
> > +	  Digital Host Controller Interface.
> > +
> > +	  If you have a controller with this capability, say Y or M here.
> > +
> > +	  If unsure, say N.
> > +
> >  config MMC_RICOH_MMC
> >  	bool "Ricoh MMC Controller Disabler"
> >  	depends on MMC_SDHCI_PCI
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6785fb1..de509bc 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -13,6 +13,10 @@
> >   *     - JMicron (hardware and technical support)
> >   */
> >  
> > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > +#include <linux/pci.h>
> > +#endif
> > +
> >  #include <linux/delay.h>
> >  #include <linux/highmem.h>
> >  #include <linux/io.h>
> > @@ -2520,6 +2524,64 @@ out:
> >  	return result;
> >  }
> >  
> > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > +static int sdhci_setup_msi(struct sdhci_host *host)
> > +{
> > +	int ret;
> > +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > +
> > +	host->msi_enabled = false;
> > +
> > +	ret = pci_enable_msi(pdev);
> > +	if (ret) {
> > +		pr_warn("%s: Failed to allocate MSI entry\n",
> > +			mmc_hostname(host->mmc));
> > +		return ret;
> > +	}
> > +
> > +	ret = request_irq(pdev->irq, sdhci_irq, 0,
> > +		mmc_hostname(host->mmc), host);
> > +	if (ret) {
> > +		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
> > +		       mmc_hostname(host->mmc), pdev->irq, ret);
> > +		pci_disable_msi(pdev);
> > +		return ret;
> > +	}
> > +
> > +	host->irq = pdev->irq;
> > +	host->msi_enabled = true;
> > +	return ret;
> > +}
> > +
> > +static int sdhci_try_enable_msi(struct sdhci_host *host)
> > +{
> > +	return sdhci_setup_msi(host);
> > +}
> > +
> > +static void sdhci_cleanup_msi(struct sdhci_host *host)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > +
> > +	free_irq(host->irq, host);
> > +	if (host->msi_enabled) {
> > +		pci_disable_msi(pdev);
> > +		host->msi_enabled = false;
> > +	}
> > +}
> > +
> > +#else
> > +
> > +static int sdhci_try_enable_msi(struct sdhci_host *host)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void sdhci_cleanup_msi(struct sdhci_host *host)
> > +{
> > +	free_irq(host->irq, host);
> > +}
> > +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> > +
> >  /*****************************************************************************\
> >   *                                                                           *
> >   * Suspend/resume                                                            *
> > @@ -2569,7 +2631,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
> >  
> >  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> >  		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -		free_irq(host->irq, host);
> > +		/* free any IRQ and disable MSI */
> > +		sdhci_cleanup_msi(host);
> >  	} else {
> >  		sdhci_enable_irq_wakeups(host);
> >  		enable_irq_wake(host->irq);
> > @@ -2589,10 +2652,22 @@ int sdhci_resume_host(struct sdhci_host *host)
> >  	}
> >  
> >  	if (!device_may_wakeup(mmc_dev(host->mmc))) {
> > -		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > -				  mmc_hostname(host->mmc), host);
> > +		/* try to enable MSI */
> > +		ret = sdhci_try_enable_msi(host);
> >  		if (ret)
> > -			return ret;
> > +			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> > +				mmc_hostname(host->mmc), ret);
> > +
> > +		if (!host->msi_enabled) {
> > +			/* fall back to legacy pin-based interrupt */
> > +			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > +					mmc_hostname(host->mmc), host);
> > +			if (ret) {
> > +				pr_err("%s: Failed to request IRQ %d: %d\n",
> > +					 mmc_hostname(host->mmc), host->irq, ret);
> > +				return ret;
> > +			}
> > +		}
> >  	} else {
> >  		sdhci_disable_irq_wakeups(host);
> >  		disable_irq_wake(host->irq);
> > @@ -3212,12 +3287,21 @@ int sdhci_add_host(struct sdhci_host *host)
> >  
> >  	sdhci_init(host, 0);
> >  
> > -	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > -		mmc_hostname(mmc), host);
> > -	if (ret) {
> > -		pr_err("%s: Failed to request IRQ %d: %d\n",
> > -		       mmc_hostname(mmc), host->irq, ret);
> > -		goto untasklet;
> > +	/* try to enable MSI */
> > +	ret = sdhci_try_enable_msi(host);
> > +	if (ret)
> > +		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
> > +			mmc_hostname(host->mmc), ret);
> > +
> > +	if (!host->msi_enabled) {
> > +		/* fall back to legacy pin-based interrupt */
> > +		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
> > +				mmc_hostname(host->mmc), host);
> > +		if (ret) {
> > +			pr_err("%s: Failed to request IRQ %d: %d\n",
> > +				 mmc_hostname(host->mmc), host->irq, ret);
> > +			goto untasklet;
> > +		}
> >  	}
> >  
> >  #ifdef CONFIG_MMC_DEBUG
> > @@ -3257,7 +3341,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >  reset:
> >  	sdhci_reset(host, SDHCI_RESET_ALL);
> >  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -	free_irq(host->irq, host);
> > +	sdhci_cleanup_msi(host);
> >  #endif
> >  untasklet:
> >  	tasklet_kill(&host->card_tasklet);
> > @@ -3300,7 +3384,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >  		sdhci_reset(host, SDHCI_RESET_ALL);
> >  
> >  	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > -	free_irq(host->irq, host);
> > +	sdhci_cleanup_msi(host);
> >  
> >  	del_timer_sync(&host->timer);
> >  
> > diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> > index 3e781b8..3812479 100644
> > --- a/include/linux/mmc/sdhci.h
> > +++ b/include/linux/mmc/sdhci.h
> > @@ -99,6 +99,8 @@ struct sdhci_host {
> >  /* Controller has a non-standard host control register */
> >  #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
> >  
> > +	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
> > +
> >  	int irq;		/* Device IRQ */
> >  	void __iomem *ioaddr;	/* Mapped address */
> >  
> > 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..a56cf27 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -68,6 +68,17 @@  config MMC_SDHCI_PCI
 
 	  If unsure, say N.
 
+config MMC_SDHCI_PCI_MSI
+	bool "SDHCI support with MSI on PCI bus"
+	depends on MMC_SDHCI_PCI && PCI_MSI
+	help
+	  This enables PCI MSI (Message Signaled Interrupts) for Secure
+	  Digital Host Controller Interface.
+
+	  If you have a controller with this capability, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_RICOH_MMC
 	bool "Ricoh MMC Controller Disabler"
 	depends on MMC_SDHCI_PCI
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6785fb1..de509bc 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -13,6 +13,10 @@ 
  *     - JMicron (hardware and technical support)
  */
 
+#ifdef CONFIG_MMC_SDHCI_PCI_MSI
+#include <linux/pci.h>
+#endif
+
 #include <linux/delay.h>
 #include <linux/highmem.h>
 #include <linux/io.h>
@@ -2520,6 +2524,64 @@  out:
 	return result;
 }
 
+#ifdef CONFIG_MMC_SDHCI_PCI_MSI
+static int sdhci_setup_msi(struct sdhci_host *host)
+{
+	int ret;
+	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
+
+	host->msi_enabled = false;
+
+	ret = pci_enable_msi(pdev);
+	if (ret) {
+		pr_warn("%s: Failed to allocate MSI entry\n",
+			mmc_hostname(host->mmc));
+		return ret;
+	}
+
+	ret = request_irq(pdev->irq, sdhci_irq, 0,
+		mmc_hostname(host->mmc), host);
+	if (ret) {
+		pr_warn("%s: Failed to request MSI IRQ %d: %d\n",
+		       mmc_hostname(host->mmc), pdev->irq, ret);
+		pci_disable_msi(pdev);
+		return ret;
+	}
+
+	host->irq = pdev->irq;
+	host->msi_enabled = true;
+	return ret;
+}
+
+static int sdhci_try_enable_msi(struct sdhci_host *host)
+{
+	return sdhci_setup_msi(host);
+}
+
+static void sdhci_cleanup_msi(struct sdhci_host *host)
+{
+	struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
+
+	free_irq(host->irq, host);
+	if (host->msi_enabled) {
+		pci_disable_msi(pdev);
+		host->msi_enabled = false;
+	}
+}
+
+#else
+
+static int sdhci_try_enable_msi(struct sdhci_host *host)
+{
+	return 0;
+}
+
+static void sdhci_cleanup_msi(struct sdhci_host *host)
+{
+	free_irq(host->irq, host);
+}
+#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
+
 /*****************************************************************************\
  *                                                                           *
  * Suspend/resume                                                            *
@@ -2569,7 +2631,8 @@  int sdhci_suspend_host(struct sdhci_host *host)
 
 	if (!device_may_wakeup(mmc_dev(host->mmc))) {
 		sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
-		free_irq(host->irq, host);
+		/* free any IRQ and disable MSI */
+		sdhci_cleanup_msi(host);
 	} else {
 		sdhci_enable_irq_wakeups(host);
 		enable_irq_wake(host->irq);
@@ -2589,10 +2652,22 @@  int sdhci_resume_host(struct sdhci_host *host)
 	}
 
 	if (!device_may_wakeup(mmc_dev(host->mmc))) {
-		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
-				  mmc_hostname(host->mmc), host);
+		/* try to enable MSI */
+		ret = sdhci_try_enable_msi(host);
 		if (ret)
-			return ret;
+			pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
+				mmc_hostname(host->mmc), ret);
+
+		if (!host->msi_enabled) {
+			/* fall back to legacy pin-based interrupt */
+			ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
+					mmc_hostname(host->mmc), host);
+			if (ret) {
+				pr_err("%s: Failed to request IRQ %d: %d\n",
+					 mmc_hostname(host->mmc), host->irq, ret);
+				return ret;
+			}
+		}
 	} else {
 		sdhci_disable_irq_wakeups(host);
 		disable_irq_wake(host->irq);
@@ -3212,12 +3287,21 @@  int sdhci_add_host(struct sdhci_host *host)
 
 	sdhci_init(host, 0);
 
-	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
-		mmc_hostname(mmc), host);
-	if (ret) {
-		pr_err("%s: Failed to request IRQ %d: %d\n",
-		       mmc_hostname(mmc), host->irq, ret);
-		goto untasklet;
+	/* try to enable MSI */
+	ret = sdhci_try_enable_msi(host);
+	if (ret)
+		pr_warn("%s: Fall back to Legacy Pin-based Interrupt: %d\n",
+			mmc_hostname(host->mmc), ret);
+
+	if (!host->msi_enabled) {
+		/* fall back to legacy pin-based interrupt */
+		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
+				mmc_hostname(host->mmc), host);
+		if (ret) {
+			pr_err("%s: Failed to request IRQ %d: %d\n",
+				 mmc_hostname(host->mmc), host->irq, ret);
+			goto untasklet;
+		}
 	}
 
 #ifdef CONFIG_MMC_DEBUG
@@ -3257,7 +3341,7 @@  int sdhci_add_host(struct sdhci_host *host)
 reset:
 	sdhci_reset(host, SDHCI_RESET_ALL);
 	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
-	free_irq(host->irq, host);
+	sdhci_cleanup_msi(host);
 #endif
 untasklet:
 	tasklet_kill(&host->card_tasklet);
@@ -3300,7 +3384,7 @@  void sdhci_remove_host(struct sdhci_host *host, int dead)
 		sdhci_reset(host, SDHCI_RESET_ALL);
 
 	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
-	free_irq(host->irq, host);
+	sdhci_cleanup_msi(host);
 
 	del_timer_sync(&host->timer);
 
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 3e781b8..3812479 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -99,6 +99,8 @@  struct sdhci_host {
 /* Controller has a non-standard host control register */
 #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL		(1<<5)
 
+	bool msi_enabled;	/* SD host controller with PCI MSI enabled */
+
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */